Merge pull request kubernetes#125317 from jpbetz/fix-nop-empty-map

Fix non-semantic apply requests to ignore empty maps

Author: k8s-ci-robot

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

@@ -28,6 +28,7 @@ import (
 	"k8s.io/apimachinery/pkg/api/equality"
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/conversion"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apiserver/pkg/endpoints/metrics"
@@ -152,14 +153,20 @@ func IgnoreManagedFieldsTimestampsTransformer(
 		return newObj, nil
 	}
 
+	eqFn := equalities.DeepEqual
+	if _, ok := newObj.(*unstructured.Unstructured); ok {
+		// Use strict equality with unstructured
+		eqFn = equalities.DeepEqualWithNilDifferentFromEmpty
+	}
+
 	// This condition ensures the managed fields are always compared first. If
 	//	this check fails, the if statement will short circuit. If the check
 	// 	succeeds the slow path is taken which compares entire objects.
-	if !equalities.DeepEqualWithNilDifferentFromEmpty(oldManagedFields, newManagedFields) {
+	if !eqFn(oldManagedFields, newManagedFields) {
 		return newObj, nil
 	}
 
-	if equalities.DeepEqualWithNilDifferentFromEmpty(newObj, oldObj) {
+	if eqFn(newObj, oldObj) {
 		// Remove any changed timestamps, so that timestamp is not the only
 		// change seen by etcd.
 		//

test/integration/apiserver/apply/apply_test.go

@@ -244,6 +244,237 @@ func TestNoOpUpdateSameResourceVersion(t *testing.T) {
 	}
 }
 
+// TestNoOpApplyWithEmptyMap
+func TestNoOpApplyWithEmptyMap(t *testing.T) {
+	client, closeFn := setup(t)
+	defer closeFn()
+
+	deploymentName := "no-op"
+	deploymentsResource := "deployments"
+	deploymentBytes := []byte(`{
+		"apiVersion": "apps/v1",
+		"kind": "Deployment",
+		"metadata": {
+			"name": "` + deploymentName + `",
+			"labels": {
+				"app": "nginx"
+			}
+		},
+		"spec": {
+			"replicas": 1,
+			"selector": {
+				"matchLabels": {
+					"app": "nginx"
+				}
+			},
+			"template": {
+				"metadata": {
+					"annotations": {},
+					"labels": {
+						"app": "nginx"
+					}
+				},
+				"spec": {
+					"containers": [{
+						"name": "nginx",
+						"image": "nginx:1.14.2",
+						"ports": [{
+							"containerPort": 80
+						}]
+					}]
+				}
+			}
+		}
+	}`)
+
+	_, err := client.AppsV1().RESTClient().Patch(types.ApplyPatchType).
+		Namespace("default").
+		Param("fieldManager", "apply_test").
+		Resource(deploymentsResource).
+		Name(deploymentName).
+		Body(deploymentBytes).
+		Do(context.TODO()).
+		Get()
+	if err != nil {
+		t.Fatalf("Failed to create object: %v", err)
+	}
+
+	// This sleep is necessary to consistently produce different timestamps because the time field in managedFields has
+	// 1 second granularity and if both apply requests happen during the same second, this test would flake.
+	time.Sleep(1 * time.Second)
+
+	createdObject, err := client.AppsV1().RESTClient().Get().Namespace("default").Resource(deploymentsResource).Name(deploymentName).Do(context.TODO()).Get()
+	if err != nil {
+		t.Fatalf("Failed to retrieve created object: %v", err)
+	}
+
+	createdAccessor, err := meta.Accessor(createdObject)
+	if err != nil {
+		t.Fatalf("Failed to get meta accessor for created object: %v", err)
+	}
+
+	createdBytes, err := json.MarshalIndent(createdObject, "\t", "\t")
+	if err != nil {
+		t.Fatalf("Failed to marshal created object: %v", err)
+	}
+
+	// Test that we can apply the same object and don't change the RV
+	_, err = client.AppsV1().RESTClient().Patch(types.ApplyPatchType).
+		Namespace("default").
+		Param("fieldManager", "apply_test").
+		Resource(deploymentsResource).
+		Name(deploymentName).
+		Body(deploymentBytes).
+		Do(context.TODO()).
+		Get()
+	if err != nil {
+		t.Fatalf("Failed to create object: %v", err)
+	}
+
+	updatedObject, err := client.AppsV1().RESTClient().Get().Namespace("default").Resource(deploymentsResource).Name(deploymentName).Do(context.TODO()).Get()
+	if err != nil {
+		t.Fatalf("Failed to retrieve updated object: %v", err)
+	}
+
+	updatedAccessor, err := meta.Accessor(updatedObject)
+	if err != nil {
+		t.Fatalf("Failed to get meta accessor for updated object: %v", err)
+	}
+
+	updatedBytes, err := json.MarshalIndent(updatedObject, "\t", "\t")
+	if err != nil {
+		t.Fatalf("Failed to marshal updated object: %v", err)
+	}
+
+	if createdAccessor.GetResourceVersion() != updatedAccessor.GetResourceVersion() {
+		t.Fatalf("Expected same resource version to be %v but got: %v\nold object:\n%v\nnew object:\n%v",
+			createdAccessor.GetResourceVersion(),
+			updatedAccessor.GetResourceVersion(),
+			string(createdBytes),
+			string(updatedBytes),
+		)
+	}
+}
+
+// TestApplyEmptyMarkerStructDifferentFromNil
+func TestApplyEmptyMarkerStructDifferentFromNil(t *testing.T) {
+	client, closeFn := setup(t)
+	defer closeFn()
+
+	podName := "pod-with-empty-dir"
+	podsResource := "pods"
+	podBytesWithEmptyDir := []byte(`{
+		"apiVersion": "v1",
+		"kind": "Pod",
+		"metadata": {
+			"name": "` + podName + `"
+		},
+		"spec": {
+			"containers": [{
+				"name":  "test-container-a",
+				"image": "test-image-one",
+				"volumeMounts": [{
+					"mountPath": "/cache",
+					"name": "cache-volume"
+				}],
+			}],
+			"volumes": [{
+				"name": "cache-volume",
+				"emptyDir": {}
+			}]
+		}
+	}`)
+
+	_, err := client.CoreV1().RESTClient().Patch(types.ApplyPatchType).
+		Namespace("default").
+		Param("fieldManager", "apply_test").
+		Resource(podsResource).
+		Name(podName).
+		Body(podBytesWithEmptyDir).
+		Do(context.TODO()).
+		Get()
+	if err != nil {
+		t.Fatalf("Failed to create object: %v", err)
+	}
+
+	// This sleep is necessary to consistently produce different timestamps because the time field in managedFields has
+	// 1 second granularity and if both apply requests happen during the same second, this test would flake.
+	time.Sleep(1 * time.Second)
+
+	createdObject, err := client.CoreV1().RESTClient().Get().Namespace("default").Resource(podsResource).Name(podName).Do(context.TODO()).Get()
+	if err != nil {
+		t.Fatalf("Failed to retrieve created object: %v", err)
+	}
+
+	createdAccessor, err := meta.Accessor(createdObject)
+	if err != nil {
+		t.Fatalf("Failed to get meta accessor for created object: %v", err)
+	}
+
+	createdBytes, err := json.MarshalIndent(createdObject, "\t", "\t")
+	if err != nil {
+		t.Fatalf("Failed to marshal created object: %v", err)
+	}
+
+	podBytesNoEmptyDir := []byte(`{
+		"apiVersion": "v1",
+		"kind": "Pod",
+		"metadata": {
+			"name": "` + podName + `"
+		},
+		"spec": {
+			"containers": [{
+				"name":  "test-container-a",
+				"image": "test-image-one",
+				"volumeMounts": [{
+					"mountPath": "/cache",
+					"name": "cache-volume"
+				}],
+			}],
+			"volumes": [{
+				"name": "cache-volume"
+			}]
+		}
+	}`)
+
+	// Test that an apply with no emptyDir is recognized as distinct from an empty marker struct emptyDir.
+	_, err = client.CoreV1().RESTClient().Patch(types.ApplyPatchType).
+		Namespace("default").
+		Param("fieldManager", "apply_test").
+		Resource(podsResource).
+		Name(podName).
+		Body(podBytesNoEmptyDir).
+		Do(context.TODO()).
+		Get()
+	if err != nil {
+		t.Fatalf("Failed to create object: %v", err)
+	}
+
+	updatedObject, err := client.CoreV1().RESTClient().Get().Namespace("default").Resource(podsResource).Name(podName).Do(context.TODO()).Get()
+	if err != nil {
+		t.Fatalf("Failed to retrieve updated object: %v", err)
+	}
+
+	updatedAccessor, err := meta.Accessor(updatedObject)
+	if err != nil {
+		t.Fatalf("Failed to get meta accessor for updated object: %v", err)
+	}
+
+	updatedBytes, err := json.MarshalIndent(updatedObject, "\t", "\t")
+	if err != nil {
+		t.Fatalf("Failed to marshal updated object: %v", err)
+	}
+
+	if createdAccessor.GetResourceVersion() == updatedAccessor.GetResourceVersion() {
+		t.Fatalf("Expected different resource version to be %v but got: %v\nold object:\n%v\nnew object:\n%v",
+			createdAccessor.GetResourceVersion(),
+			updatedAccessor.GetResourceVersion(),
+			string(createdBytes),
+			string(updatedBytes),
+		)
+	}
+}
+
 func getRV(obj runtime.Object) (string, error) {
 	acc, err := meta.Accessor(obj)
 	if err != nil {