Merge pull request #40522 from wojtek-t/use_new_conversions_in_patch

Automatic merge from submit-queue (batch tested with PRs 41857, 41864, 40522, 41835, 41991)

Use new conversions in patch

Ref #39017

staging/src/k8s.io/apimachinery/pkg/conversion/unstructured/converter.go

@@ -20,6 +20,7 @@ import (
 	"bytes"
 	encodingjson "encoding/json"
 	"fmt"
+	"math"
 	"os"
 	"reflect"
 	"strconv"
@@ -31,6 +32,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/diff"
 	"k8s.io/apimachinery/pkg/util/json"
+	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 
 	"github.com/golang/glog"
 )
@@ -82,7 +84,7 @@ var (
 func parseBool(key string) bool {
 	value, err := strconv.ParseBool(key)
 	if err != nil {
-		glog.Errorf("Couldn't parse %s as bool", key)
+		utilruntime.HandleError(fmt.Errorf("Couldn't parse '%s' as bool for unstructured mismatch detection", key))
 	}
 	return value
 }
@@ -173,8 +175,12 @@ func fromUnstructured(sv, dv reflect.Value) error {
 					dv.Set(sv.Convert(dt))
 					return nil
 				}
+				if sv.Float() == math.Trunc(sv.Float()) {
+					dv.Set(sv.Convert(dt))
+					return nil
+				}
 			}
-			return fmt.Errorf("cannot convert %s to %d", st.String(), dt.String())
+			return fmt.Errorf("cannot convert %s to %s", st.String(), dt.String())
 		}
 	}
 

staging/src/k8s.io/apimachinery/pkg/conversion/unstructured/converter_test.go

@@ -438,3 +438,25 @@ func TestUnrecognized(t *testing.T) {
 		}
 	}
 }
+
+func TestFloatIntConversion(t *testing.T) {
+	unstr := map[string]interface{}{"fd": float64(3)}
+
+	var obj F
+	if err := DefaultConverter.FromUnstructured(unstr, &obj); err != nil {
+		t.Errorf("Unexpected error in FromUnstructured: %v", err)
+	}
+
+	data, err := json.Marshal(unstr)
+	if err != nil {
+		t.Fatalf("Error when marshaling unstructured: %v", err)
+	}
+	var unmarshalled F
+	if err := json.Unmarshal(data, &unmarshalled); err != nil {
+		t.Fatalf("Error when unmarshaling to object: %v", err)
+	}
+
+	if !reflect.DeepEqual(obj, unmarshalled) {
+		t.Errorf("Incorrect conversion, diff: %v", diff.ObjectReflectDiff(obj, unmarshalled))
+	}
+}

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

@@ -65,11 +65,12 @@ type APIGroupVersion struct {
 	Serializer     runtime.NegotiatedSerializer
 	ParameterCodec runtime.ParameterCodec
 
-	Typer     runtime.ObjectTyper
+	Typer           runtime.ObjectTyper
-	Creater   runtime.ObjectCreater
+	Creater         runtime.ObjectCreater
-	Convertor runtime.ObjectConvertor
+	Convertor       runtime.ObjectConvertor
-	Copier    runtime.ObjectCopier
+	Copier          runtime.ObjectCopier
-	Linker    runtime.SelfLinker
+	Linker          runtime.SelfLinker
+	UnsafeConvertor runtime.ObjectConvertor
 
 	Admit   admission.Interface
 	Context request.RequestContextMapper

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

@@ -20,6 +20,7 @@ import (
 	"encoding/json"
 	"fmt"
 
+	"k8s.io/apimachinery/pkg/conversion/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/strategicpatch"
@@ -77,6 +78,7 @@ func patchObjectJSON(
 // <originalObject> and stores the result in <objToUpdate>.
 // It additionally returns the map[string]interface{} representation of the
 // <originalObject> and <patchJS>.
+// NOTE: Both <originalObject> and <objToUpdate> are supposed to be versioned.
 func strategicPatchObject(
 	codec runtime.Codec,
 	originalObject runtime.Object,
@@ -84,14 +86,8 @@ func strategicPatchObject(
 	objToUpdate runtime.Object,
 	versionedObj runtime.Object,
 ) (originalObjMap map[string]interface{}, patchMap map[string]interface{}, retErr error) {
-	// TODO: This should be one-step conversion that doesn't require
-	// json marshaling and unmarshaling once #39017 is fixed.
-	data, err := runtime.Encode(codec, originalObject)
-	if err != nil {
-		return nil, nil, err
-	}
 	originalObjMap = make(map[string]interface{})
-	if err := json.Unmarshal(data, &originalObjMap); err != nil {
+	if err := unstructured.DefaultConverter.ToUnstructured(originalObject, &originalObjMap); err != nil {
 		return nil, nil, err
 	}
 
@@ -121,11 +117,5 @@ func applyPatchToObject(
 		return err
 	}
 
-	// TODO: This should be one-step conversion that doesn't require
+	return unstructured.DefaultConverter.FromUnstructured(patchedObjMap, objToUpdate)
-	// json marshaling and unmarshaling once #39017 is fixed.
-	data, err := json.Marshal(patchedObjMap)
-	if err != nil {
-		return err
-	}
-	return runtime.DecodeInto(codec, data, objToUpdate)
 }

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

@@ -34,6 +34,7 @@ import (
 	"k8s.io/apimachinery/pkg/api/meta"
 	metainternalversion "k8s.io/apimachinery/pkg/apis/meta/internalversion"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/conversion/unstructured"
 	"k8s.io/apimachinery/pkg/fields"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
@@ -81,9 +82,11 @@ type RequestScope struct {
 	Serializer runtime.NegotiatedSerializer
 	runtime.ParameterCodec
 
-	Creater   runtime.ObjectCreater
+	Creater         runtime.ObjectCreater
-	Convertor runtime.ObjectConvertor
+	Convertor       runtime.ObjectConvertor
-	Copier    runtime.ObjectCopier
+	Copier          runtime.ObjectCopier
+	Typer           runtime.ObjectTyper
+	UnsafeConvertor runtime.ObjectConvertor
 
 	Resource    schema.GroupVersionResource
 	Kind        schema.GroupVersionKind
@@ -520,7 +523,8 @@ func PatchResource(r rest.Patcher, scope RequestScope, admit admission.Interface
 			return nil
 		}
 
-		result, err := patchResource(ctx, updateAdmit, timeout, versionedObj, r, name, patchType, patchJS, scope.Namer, scope.Copier, scope.Resource, codec)
+		result, err := patchResource(ctx, updateAdmit, timeout, versionedObj, r, name, patchType, patchJS,
+			scope.Namer, scope.Copier, scope.Creater, scope.UnsafeConvertor, scope.Kind, scope.Resource, codec)
 		if err != nil {
 			scope.err(err, res.ResponseWriter, req.Request)
 			return
@@ -550,6 +554,9 @@ func patchResource(
 	patchJS []byte,
 	namer ScopeNamer,
 	copier runtime.ObjectCopier,
+	creater runtime.ObjectCreater,
+	unsafeConvertor runtime.ObjectConvertor,
+	kind schema.GroupVersionKind,
 	resource schema.GroupVersionResource,
 	codec runtime.Codec,
 ) (runtime.Object, error) {
@@ -594,10 +601,28 @@ func patchResource(
 				}
 				originalObjJS, originalPatchedObjJS = originalJS, patchedJS
 			case types.StrategicMergePatchType:
-				originalMap, patchMap, err := strategicPatchObject(codec, currentObject, patchJS, objToUpdate, versionedObj)
+				// Since the patch is applied on versioned objects, we need to convert the
+				// current object to versioned representation first.
+				currentVersionedObject, err := unsafeConvertor.ConvertToVersion(currentObject, kind.GroupVersion())
 				if err != nil {
 					return nil, err
 				}
+				versionedObjToUpdate, err := creater.New(kind)
+				if err != nil {
+					return nil, err
+				}
+				originalMap, patchMap, err := strategicPatchObject(codec, currentVersionedObject, patchJS, versionedObjToUpdate, versionedObj)
+				if err != nil {
+					return nil, err
+				}
+				// Convert the object back to unversioned.
+				gvk := kind.GroupKind().WithVersion(runtime.APIVersionInternal)
+				unversionedObjToUpdate, err := unsafeConvertor.ConvertToVersion(versionedObjToUpdate, gvk.GroupVersion())
+				if err != nil {
+					return nil, err
+				}
+				objToUpdate = unversionedObjToUpdate
+				// Store unstructured representations for possible retries.
 				originalObjMap, originalPatchMap = originalMap, patchMap
 			}
 			if err := checkName(objToUpdate, name, namespace, namer); err != nil {
@@ -614,14 +639,15 @@ func patchResource(
 			// 3. ensure no conflicts between the two patches
 			// 4. apply the #1 patch to the currentJS object
 
-			// TODO: This should be one-step conversion that doesn't require
+			currentObjMap := make(map[string]interface{})
-			// json marshaling and unmarshaling once #39017 is fixed.
+
-			data, err := runtime.Encode(codec, currentObject)
+			// Since the patch is applied on versioned objects, we need to convert the
+			// current object to versioned representation first.
+			currentVersionedObject, err := unsafeConvertor.ConvertToVersion(currentObject, kind.GroupVersion())
 			if err != nil {
 				return nil, err
 			}
-			currentObjMap := make(map[string]interface{})
+			if err := unstructured.DefaultConverter.ToUnstructured(currentVersionedObject, &currentObjMap); err != nil {
-			if err := json.Unmarshal(data, &currentObjMap); err != nil {
 				return nil, err
 			}
 
@@ -677,8 +703,17 @@ func patchResource(
 				return nil, errors.NewConflict(resource.GroupResource(), name, patchDiffErr)
 			}
 
-			objToUpdate := patcher.New()
+			versionedObjToUpdate, err := creater.New(kind)
-			if err := applyPatchToObject(codec, currentObjMap, originalPatchMap, objToUpdate, versionedObj); err != nil {
+			if err != nil {
+				return nil, err
+			}
+			if err := applyPatchToObject(codec, currentObjMap, originalPatchMap, versionedObjToUpdate, versionedObj); err != nil {
+				return nil, err
+			}
+			// Convert the object back to unversioned.
+			gvk := kind.GroupKind().WithVersion(runtime.APIVersionInternal)
+			objToUpdate, err := unsafeConvertor.ConvertToVersion(versionedObjToUpdate, gvk.GroupVersion())
+			if err != nil {
 				return nil, err
 			}
 

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

@@ -202,6 +202,9 @@ func (tc *patchTestCase) Run(t *testing.T) {
 
 	namer := &testNamer{namespace, name}
 	copier := runtime.ObjectCopier(api.Scheme)
+	creater := runtime.ObjectCreater(api.Scheme)
+	convertor := runtime.UnsafeObjectConvertor(api.Scheme)
+	kind := schema.GroupVersionKind{Group: "", Version: "v1", Kind: "Pod"}
 	resource := schema.GroupVersionResource{Group: "", Version: "v1", Resource: "pods"}
 	versionedObj := &v1.Pod{}
 
@@ -244,7 +247,7 @@ func (tc *patchTestCase) Run(t *testing.T) {
 
 		}
 
-		resultObj, err := patchResource(ctx, admit, 1*time.Second, versionedObj, testPatcher, name, patchType, patch, namer, copier, resource, codec)
+		resultObj, err := patchResource(ctx, admit, 1*time.Second, versionedObj, testPatcher, name, patchType, patch, namer, copier, creater, 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)
@@ -266,18 +269,12 @@ func (tc *patchTestCase) Run(t *testing.T) {
 
 		resultPod := resultObj.(*api.Pod)
 
-		// roundtrip to get defaulting
+		// TODO: In the process of applying patches, we are calling
-		expectedJS, err := runtime.Encode(codec, tc.expectedPod)
+		// conversions between versioned and unversioned objects.
-		if err != nil {
+		// In case of Pod, conversion from versioned to unversioned
-			t.Errorf("%s: unexpected error: %v", tc.name, err)
+		// is setting PodSecurityContext, so we need to set it here too.
-			return
+		reallyExpectedPod := tc.expectedPod
-		}
+		reallyExpectedPod.Spec.SecurityContext = &api.PodSecurityContext{}
-		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))

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

@@ -516,12 +516,14 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storag
 
 	kubeVerbs := map[string]struct{}{}
 	reqScope := handlers.RequestScope{
-		ContextFunc:    ctxFn,
+		ContextFunc:     ctxFn,
-		Serializer:     a.group.Serializer,
+		Serializer:      a.group.Serializer,
-		ParameterCodec: a.group.ParameterCodec,
+		ParameterCodec:  a.group.ParameterCodec,
-		Creater:        a.group.Creater,
+		Creater:         a.group.Creater,
-		Convertor:      a.group.Convertor,
+		Convertor:       a.group.Convertor,
-		Copier:         a.group.Copier,
+		Copier:          a.group.Copier,
+		Typer:           a.group.Typer,
+		UnsafeConvertor: a.group.UnsafeConvertor,
 
 		// TODO: This seems wrong for cross-group subresources. It makes an assumption that a subresource and its parent are in the same group version. Revisit this.
 		Resource:    a.group.GroupVersion.WithResource(resource),

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

@@ -335,12 +335,13 @@ func (s *GenericAPIServer) newAPIGroupVersion(apiGroupInfo *APIGroupInfo, groupV
 		GroupVersion:     groupVersion,
 		MetaGroupVersion: apiGroupInfo.MetaGroupVersion,
 
-		ParameterCodec: apiGroupInfo.ParameterCodec,
+		ParameterCodec:  apiGroupInfo.ParameterCodec,
-		Serializer:     apiGroupInfo.NegotiatedSerializer,
+		Serializer:      apiGroupInfo.NegotiatedSerializer,
-		Creater:        apiGroupInfo.Scheme,
+		Creater:         apiGroupInfo.Scheme,
-		Convertor:      apiGroupInfo.Scheme,
+		Convertor:       apiGroupInfo.Scheme,
-		Copier:         apiGroupInfo.Scheme,
+		UnsafeConvertor: runtime.UnsafeObjectConvertor(apiGroupInfo.Scheme),
-		Typer:          apiGroupInfo.Scheme,
+		Copier:          apiGroupInfo.Scheme,
+		Typer:           apiGroupInfo.Scheme,
 		SubresourceGroupVersionKind: apiGroupInfo.SubresourceGroupVersionKind,
 		Linker: apiGroupInfo.GroupMeta.SelfLinker,
 		Mapper: apiGroupInfo.GroupMeta.RESTMapper,

vendor/BUILD

@@ -8539,6 +8539,7 @@ go_library(
         "//vendor:k8s.io/apimachinery/pkg/runtime",
         "//vendor:k8s.io/apimachinery/pkg/util/diff",
         "//vendor:k8s.io/apimachinery/pkg/util/json",
+        "//vendor:k8s.io/apimachinery/pkg/util/runtime",
     ],
 )
 
@@ -9900,6 +9901,7 @@ go_library(
         "//vendor:k8s.io/apimachinery/pkg/api/meta",
         "//vendor:k8s.io/apimachinery/pkg/apis/meta/internalversion",
         "//vendor:k8s.io/apimachinery/pkg/apis/meta/v1",
+        "//vendor:k8s.io/apimachinery/pkg/conversion/unstructured",
         "//vendor:k8s.io/apimachinery/pkg/fields",
         "//vendor:k8s.io/apimachinery/pkg/runtime",
         "//vendor:k8s.io/apimachinery/pkg/runtime/schema",