Merge pull request kubernetes#75151 from jennybuckley/fix-conversion-bug

Fix apply conversion bug

Author: k8s-ci-robot

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

@@ -72,7 +72,7 @@ func NewCRDFieldManager(objectConverter runtime.ObjectConvertor, objectDefaulter
 		groupVersion:    gv,
 		hubVersion:      hub,
 		updater: merge.Updater{
-			Converter: internal.NewVersionConverter(internal.DeducedTypeConverter{}, objectConverter, hub),
+			Converter: internal.NewCRDVersionConverter(internal.DeducedTypeConverter{}, objectConverter, hub),
 		},
 	}
 }

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/internal/versionconverter.go

@@ -17,8 +17,6 @@ limitations under the License.
 package internal
 
 import (
-	"fmt"
-
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"sigs.k8s.io/structured-merge-diff/fieldpath"
@@ -31,7 +29,7 @@ import (
 type versionConverter struct {
 	typeConverter   TypeConverter
 	objectConvertor runtime.ObjectConvertor
-	hubVersion      schema.GroupVersion
+	hubGetter       func(from schema.GroupVersion) schema.GroupVersion
 }
 
 var _ merge.Converter = &versionConverter{}
@@ -41,7 +39,23 @@ func NewVersionConverter(t TypeConverter, o runtime.ObjectConvertor, h schema.Gr
 	return &versionConverter{
 		typeConverter:   t,
 		objectConvertor: o,
-		hubVersion:      h,
+		hubGetter: func(from schema.GroupVersion) schema.GroupVersion {
+			return schema.GroupVersion{
+				Group:   from.Group,
+				Version: h.Version,
+			}
+		},
+	}
+}
+
+// NewCRDVersionConverter builds a VersionConverter for CRDs from a TypeConverter and an ObjectConvertor.
+func NewCRDVersionConverter(t TypeConverter, o runtime.ObjectConvertor, h schema.GroupVersion) merge.Converter {
+	return &versionConverter{
+		typeConverter:   t,
+		objectConvertor: o,
+		hubGetter: func(from schema.GroupVersion) schema.GroupVersion {
+			return h
+		},
 	}
 }
 
@@ -60,22 +74,21 @@ func (v *versionConverter) Convert(object typed.TypedValue, version fieldpath.AP
 	}
 
 	// If attempting to convert to the same version as we already have, just return it.
-	if objectToConvert.GetObjectKind().GroupVersionKind().GroupVersion() == groupVersion {
+	fromVersion := objectToConvert.GetObjectKind().GroupVersionKind().GroupVersion()
+	if fromVersion == groupVersion {
 		return object, nil
 	}
 
 	// Convert to internal
-	internalObject, err := v.objectConvertor.ConvertToVersion(objectToConvert, v.hubVersion)
+	internalObject, err := v.objectConvertor.ConvertToVersion(objectToConvert, v.hubGetter(fromVersion))
 	if err != nil {
-		return object, fmt.Errorf("failed to convert object (%v to %v): %v",
-			objectToConvert.GetObjectKind().GroupVersionKind(), v.hubVersion, err)
+		return object, err
 	}
 
 	// Convert the object into the target version
 	convertedObject, err := v.objectConvertor.ConvertToVersion(internalObject, groupVersion)
 	if err != nil {
-		return object, fmt.Errorf("failed to convert object (%v to %v): %v",
-			internalObject.GetObjectKind().GroupVersionKind(), groupVersion, err)
+		return object, err
 	}
 
 	// Convert the object back to a smd typed value and return it.

test/integration/apiserver/apply/BUILD

@@ -15,6 +15,7 @@ go_test(
         "//pkg/master:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/api/errors:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/api/meta:go_default_library",
+        "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/runtime/schema:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/types:go_default_library",
         "//staging/src/k8s.io/apiserver/pkg/features:go_default_library",

test/integration/apiserver/apply/apply_test.go

@@ -24,6 +24,7 @@ import (
 
 	"k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
 	genericfeatures "k8s.io/apiserver/pkg/features"
@@ -438,3 +439,106 @@ func TestApplyRequiresFieldManager(t *testing.T) {
 		t.Fatalf("Apply failed to create with fieldManager: %v", err)
 	}
 }
+
+// TestApplyRemoveContainerPort removes a container port from a deployment
+func TestApplyRemoveContainerPort(t *testing.T) {
+	defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, genericfeatures.ServerSideApply, true)()
+
+	_, client, closeFn := setup(t)
+	defer closeFn()
+
+	obj := []byte(`{
+		"apiVersion": "extensions/v1beta1",
+		"kind": "Deployment",
+		"metadata": {
+			"name": "deployment",
+			"labels": {"app": "nginx"}
+		},
+		"spec": {
+			"replicas": 3,
+			"selector": {
+				"matchLabels": {
+					"app": "nginx"
+				}
+			},
+			"template": {
+				"metadata": {
+					"labels": {
+						"app": "nginx"
+					}
+				},
+				"spec": {
+					"containers": [{
+						"name":  "nginx",
+						"image": "nginx:latest",
+						"ports": [{
+							"containerPort": 80,
+							"protocol": "TCP"
+						}]
+					}]
+				}
+			}
+		}
+	}`)
+
+	_, err := client.CoreV1().RESTClient().Patch(types.ApplyPatchType).
+		AbsPath("/apis/extensions/v1beta1").
+		Namespace("default").
+		Resource("deployments").
+		Name("deployment").
+		Param("fieldManager", "apply_test").
+		Body(obj).Do().Get()
+	if err != nil {
+		t.Fatalf("Failed to create object using Apply patch: %v", err)
+	}
+
+	obj = []byte(`{
+		"apiVersion": "apps/v1",
+		"kind": "Deployment",
+		"metadata": {
+			"name": "deployment",
+			"labels": {"app": "nginx"}
+		},
+		"spec": {
+			"replicas": 3,
+			"selector": {
+				"matchLabels": {
+					"app": "nginx"
+				}
+			},
+			"template": {
+				"metadata": {
+					"labels": {
+						"app": "nginx"
+					}
+				},
+				"spec": {
+					"containers": [{
+						"name":  "nginx",
+						"image": "nginx:latest"
+					}]
+				}
+			}
+		}
+	}`)
+
+	_, err = client.CoreV1().RESTClient().Patch(types.ApplyPatchType).
+		AbsPath("/apis/apps/v1").
+		Namespace("default").
+		Resource("deployments").
+		Name("deployment").
+		Param("fieldManager", "apply_test").
+		Body(obj).Do().Get()
+	if err != nil {
+		t.Fatalf("Failed to remove container port using Apply patch: %v", err)
+	}
+
+	deployment, err := client.AppsV1().Deployments("default").Get("deployment", metav1.GetOptions{})
+	if err != nil {
+		t.Fatalf("Failed to retrieve object: %v", err)
+	}
+
+	if len(deployment.Spec.Template.Spec.Containers[0].Ports) > 0 {
+		t.Fatalf("Expected no container ports but got: %v", deployment.Spec.Template.Spec.Containers[0].Ports)
+	}
+}