Merge pull request #42944 from liggitt/patch-defaulting

Automatic merge from submit-queue

Ensure patched objects are defaulted correctly

Restores defaulting behavior for patch API calls removed in e34e1ab#diff-517d1b81963bbc7c9b0a16e6eb3c0e2f

Restores the unit test that ensures we get a defaulted result after applying a patch

Fixes #42764
Fixes #42834

pkg/master/thirdparty/thirdparty.go

@@ -318,6 +318,7 @@ func (m *ThirdPartyResourceServer) thirdpartyapi(group, kind, version, pluralRes
 		Creater:   thirdpartyresourcedata.NewObjectCreator(group, version, api.Scheme),
 		Convertor: api.Scheme,
 		Copier:    api.Scheme,
+		Defaulter: api.Scheme,
 		Typer:     api.Scheme,
 
 		Mapper:                 thirdpartyresourcedata.NewMapper(api.Registry.GroupOrDie(extensions.GroupName).RESTMapper, kind, version, group),

staging/src/k8s.io/apiserver/pkg/endpoints/apiserver_test.go

@@ -284,6 +284,7 @@ func handleInternal(storage map[string]rest.Storage, admissionControl admission.
 		Creater:   scheme,
 		Convertor: scheme,
 		Copier:    scheme,
+		Defaulter: scheme,
 		Typer:     scheme,
 		Linker:    selfLinker,
 		Mapper:    namespaceMapper,
@@ -2579,6 +2580,7 @@ func TestUpdateREST(t *testing.T) {
 			Creater:   scheme,
 			Convertor: scheme,
 			Copier:    scheme,
+			Defaulter: scheme,
 			Typer:     scheme,
 			Linker:    selfLinker,
 
@@ -2663,6 +2665,7 @@ func TestParentResourceIsRequired(t *testing.T) {
 		Creater:   scheme,
 		Convertor: scheme,
 		Copier:    scheme,
+		Defaulter: scheme,
 		Typer:     scheme,
 		Linker:    selfLinker,
 
@@ -2694,6 +2697,7 @@ func TestParentResourceIsRequired(t *testing.T) {
 		Creater:   scheme,
 		Convertor: scheme,
 		Copier:    scheme,
+		Defaulter: scheme,
 		Typer:     scheme,
 		Linker:    selfLinker,
 
@@ -3307,6 +3311,7 @@ func TestXGSubresource(t *testing.T) {
 		Creater:   scheme,
 		Convertor: scheme,
 		Copier:    scheme,
+		Defaulter: scheme,
 		Typer:     scheme,
 		Linker:    selfLinker,
 		Mapper:    namespaceMapper,

staging/src/k8s.io/apiserver/pkg/endpoints/groupversion.go

@@ -69,6 +69,7 @@ type APIGroupVersion struct {
 	Creater         runtime.ObjectCreater
 	Convertor       runtime.ObjectConvertor
 	Copier          runtime.ObjectCopier
+	Defaulter       runtime.ObjectDefaulter
 	Linker          runtime.SelfLinker
 	UnsafeConvertor runtime.ObjectConvertor
 

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go

@@ -81,6 +81,7 @@ func patchObjectJSON(
 // NOTE: Both <originalObject> and <objToUpdate> are supposed to be versioned.
 func strategicPatchObject(
 	codec runtime.Codec,
+	defaulter runtime.ObjectDefaulter,
 	originalObject runtime.Object,
 	patchJS []byte,
 	objToUpdate runtime.Object,
@@ -96,17 +97,18 @@ func strategicPatchObject(
 		return nil, nil, err
 	}
 
-	if err := applyPatchToObject(codec, originalObjMap, patchMap, objToUpdate, versionedObj); err != nil {
+	if err := applyPatchToObject(codec, defaulter, originalObjMap, patchMap, objToUpdate, versionedObj); err != nil {
 		return nil, nil, err
 	}
 	return
 }
 
 // applyPatchToObject applies a strategic merge patch of <patchMap> to
-// <originalMap> and stores the result in <objToUpdate>, though it operates
-// on versioned map[string]interface{} representations.
+// <originalMap> and stores the result in <objToUpdate>.
+// NOTE: <objToUpdate> must be a versioned object.
 func applyPatchToObject(
 	codec runtime.Codec,
+	defaulter runtime.ObjectDefaulter,
 	originalMap map[string]interface{},
 	patchMap map[string]interface{},
 	objToUpdate runtime.Object,
@@ -117,5 +119,12 @@ func applyPatchToObject(
 		return err
 	}
 
-	return unstructured.DefaultConverter.FromUnstructured(patchedObjMap, objToUpdate)
+	// Rather than serialize the patched map to JSON, then decode it to an object, we go directly from a map to an object
+	if err := unstructured.DefaultConverter.FromUnstructured(patchedObjMap, objToUpdate); err != nil {
+		return err
+	}
+	// Decoding from JSON to a versioned object would apply defaults, so we do the same here
+	defaulter.Default(objToUpdate)
+
+	return nil
 }

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest.go

@@ -84,6 +84,7 @@ type RequestScope struct {
 
 	Creater         runtime.ObjectCreater
 	Convertor       runtime.ObjectConvertor
+	Defaulter       runtime.ObjectDefaulter
 	Copier          runtime.ObjectCopier
 	Typer           runtime.ObjectTyper
 	UnsafeConvertor runtime.ObjectConvertor
@@ -525,7 +526,7 @@ func PatchResource(r rest.Patcher, scope RequestScope, admit admission.Interface
 		}
 
 		result, err := patchResource(ctx, updateAdmit, timeout, versionedObj, r, name, patchType, patchJS,
-			scope.Namer, scope.Copier, scope.Creater, scope.UnsafeConvertor, scope.Kind, scope.Resource, codec)
+			scope.Namer, scope.Copier, scope.Creater, scope.Defaulter, scope.UnsafeConvertor, scope.Kind, scope.Resource, codec)
 		if err != nil {
 			scope.err(err, res.ResponseWriter, req.Request)
 			return
@@ -556,6 +557,7 @@ func patchResource(
 	namer ScopeNamer,
 	copier runtime.ObjectCopier,
 	creater runtime.ObjectCreater,
+	defaulter runtime.ObjectDefaulter,
 	unsafeConvertor runtime.ObjectConvertor,
 	kind schema.GroupVersionKind,
 	resource schema.GroupVersionResource,
@@ -619,7 +621,7 @@ func patchResource(
 				if err != nil {
 					return nil, err
 				}
-				originalMap, patchMap, err := strategicPatchObject(codec, currentVersionedObject, patchJS, versionedObjToUpdate, versionedObj)
+				originalMap, patchMap, err := strategicPatchObject(codec, defaulter, currentVersionedObject, patchJS, versionedObjToUpdate, versionedObj)
 				if err != nil {
 					return nil, err
 				}
@@ -716,7 +718,7 @@ func patchResource(
 			if err != nil {
 				return nil, err
 			}
-			if err := applyPatchToObject(codec, currentObjMap, originalPatchMap, versionedObjToUpdate, versionedObj); err != nil {
+			if err := applyPatchToObject(codec, defaulter, currentObjMap, originalPatchMap, versionedObjToUpdate, versionedObj); err != nil {
 				return nil, err
 			}
 			// Convert the object back to unversioned.

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest_test.go

@@ -61,6 +61,7 @@ func TestPatchAnonymousField(t *testing.T) {
 	testGV := schema.GroupVersion{Group: "", Version: "v"}
 	api.Scheme.AddKnownTypes(testGV, &testPatchType{})
 	codec := api.Codecs.LegacyCodec(testGV)
+	defaulter := runtime.ObjectDefaulter(api.Scheme)
 
 	original := &testPatchType{
 		TypeMeta:         metav1.TypeMeta{Kind: "testPatchType", APIVersion: "v"},
@@ -73,7 +74,7 @@ func TestPatchAnonymousField(t *testing.T) {
 	}
 
 	actual := &testPatchType{}
-	_, _, err := strategicPatchObject(codec, original, []byte(patch), actual, &testPatchType{})
+	_, _, err := strategicPatchObject(codec, defaulter, original, []byte(patch), actual, &testPatchType{})
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
@@ -203,6 +204,7 @@ func (tc *patchTestCase) Run(t *testing.T) {
 	namer := &testNamer{namespace, name}
 	copier := runtime.ObjectCopier(api.Scheme)
 	creater := runtime.ObjectCreater(api.Scheme)
+	defaulter := runtime.ObjectDefaulter(api.Scheme)
 	convertor := runtime.UnsafeObjectConvertor(api.Scheme)
 	kind := schema.GroupVersionKind{Group: "", Version: "v1", Kind: "Pod"}
 	resource := schema.GroupVersionResource{Group: "", Version: "v1", Resource: "pods"}
@@ -247,7 +249,7 @@ func (tc *patchTestCase) Run(t *testing.T) {
 
 		}
 
-		resultObj, err := patchResource(ctx, admit, 1*time.Second, versionedObj, testPatcher, name, patchType, patch, namer, copier, creater, convertor, kind, resource, codec)
+		resultObj, err := patchResource(ctx, admit, 1*time.Second, versionedObj, testPatcher, name, patchType, patch, namer, copier, creater, defaulter, convertor, kind, resource, codec)
 		if len(tc.expectedError) != 0 {
 			if err == nil || err.Error() != tc.expectedError {
 				t.Errorf("%s: expected error %v, but got %v", tc.name, tc.expectedError, err)
@@ -269,12 +271,18 @@ func (tc *patchTestCase) Run(t *testing.T) {
 
 		resultPod := resultObj.(*api.Pod)
 
-		// TODO: In the process of applying patches, we are calling
-		// conversions between versioned and unversioned objects.
-		// In case of Pod, conversion from versioned to unversioned
-		// is setting PodSecurityContext, so we need to set it here too.
-		reallyExpectedPod := tc.expectedPod
-		reallyExpectedPod.Spec.SecurityContext = &api.PodSecurityContext{}
+		// roundtrip to get defaulting
+		expectedJS, err := runtime.Encode(codec, tc.expectedPod)
+		if err != nil {
+			t.Errorf("%s: unexpected error: %v", tc.name, err)
+			return
+		}
+		expectedObj, err := runtime.Decode(codec, expectedJS)
+		if err != nil {
+			t.Errorf("%s: unexpected error: %v", tc.name, err)
+			return
+		}
+		reallyExpectedPod := expectedObj.(*api.Pod)
 
 		if !reflect.DeepEqual(*reallyExpectedPod, *resultPod) {
 			t.Errorf("%s mismatch: %v\n", tc.name, diff.ObjectGoPrintDiff(reallyExpectedPod, resultPod))
@@ -286,6 +294,7 @@ func (tc *patchTestCase) Run(t *testing.T) {
 
 func TestNumberConversion(t *testing.T) {
 	codec := api.Codecs.LegacyCodec(schema.GroupVersion{Version: "v1"})
+	defaulter := runtime.ObjectDefaulter(api.Scheme)
 
 	currentVersionedObject := &v1.Service{
 		TypeMeta:   metav1.TypeMeta{Kind: "Service", APIVersion: "v1"},
@@ -305,7 +314,7 @@ func TestNumberConversion(t *testing.T) {
 
 	patchJS := []byte(`{"spec":{"ports":[{"port":80,"nodePort":31789}]}}`)
 
-	_, _, err := strategicPatchObject(codec, currentVersionedObject, patchJS, versionedObjToUpdate, versionedObj)
+	_, _, err := strategicPatchObject(codec, defaulter, currentVersionedObject, patchJS, versionedObjToUpdate, versionedObj)
 	if err != nil {
 		t.Fatal(err)
 	}

staging/src/k8s.io/apiserver/pkg/endpoints/installer.go

@@ -522,6 +522,7 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storag
 		Creater:         a.group.Creater,
 		Convertor:       a.group.Convertor,
 		Copier:          a.group.Copier,
+		Defaulter:       a.group.Defaulter,
 		Typer:           a.group.Typer,
 		UnsafeConvertor: a.group.UnsafeConvertor,
 

staging/src/k8s.io/apiserver/pkg/server/genericapiserver.go

@@ -366,6 +366,7 @@ func (s *GenericAPIServer) newAPIGroupVersion(apiGroupInfo *APIGroupInfo, groupV
 		Convertor:       apiGroupInfo.Scheme,
 		UnsafeConvertor: runtime.UnsafeObjectConvertor(apiGroupInfo.Scheme),
 		Copier:          apiGroupInfo.Scheme,
+		Defaulter:       apiGroupInfo.Scheme,
 		Typer:           apiGroupInfo.Scheme,
 		SubresourceGroupVersionKind: apiGroupInfo.SubresourceGroupVersionKind,
 		Linker: apiGroupInfo.GroupMeta.SelfLinker,