Merge pull request #67562 from nikhita/customresource-subresource-patch

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Prevent resourceVersion updates for custom resources on no-op writes

Fixes partly https://github.com/kubernetes/kubernetes/issues/67541

For ObjectMeta pruning, we round trip by marshalling and unmarshalling. If the ObjectMeta contained any strings with `""` (or other fields with empty values)  _and_ the respective fields are `omitempty`, those fields will be lost in the round trip process.

This makes ObjectMeta after the no-op write different from the one before the write.

Resource version is incremented every time data is written to etcd. Writes to etcd short-circuit if the bytes being written are identical to the bytes already present.

So this ends up incrementing the `resourceVersion` even on no-op writes. This PR updates the `BeforeUpdate` function such that omitempty fields have values set only if they are non-zero so that they produce an unstructured object that matches ObjectMeta omitempty semantics.

/sig api-machinery
/kind bug
/area custom-resources
/assign sttts liggitt 

**Release note**:

```release-note
Prevent `resourceVersion` updates for custom resources on no-op writes.
```

staging/src/k8s.io/apiextensions-apiserver/test/integration/basic_test.go

@@ -20,6 +20,7 @@ import (
 	"fmt"
 	"reflect"
 	"sort"
+	"strings"
 	"testing"
 	"time"
 
@@ -667,31 +668,49 @@ func TestPatch(t *testing.T) {
 	ns := "not-the-default"
 	noxuNamespacedResourceClient := newNamespacedCustomResourceClient(ns, dynamicClient, noxuDefinition)
 
+	t.Logf("Creating foo")
 	noxuInstanceToCreate := fixtures.NewNoxuInstance(ns, "foo")
-	createdNoxuInstance, err := noxuNamespacedResourceClient.Create(noxuInstanceToCreate, metav1.CreateOptions{})
+	_, err = noxuNamespacedResourceClient.Create(noxuInstanceToCreate, metav1.CreateOptions{})
 	if err != nil {
 		t.Fatal(err)
 	}
 
+	t.Logf("Patching .num.num2 to 999")
 	patch := []byte(`{"num": {"num2":999}}`)
-	createdNoxuInstance, err = noxuNamespacedResourceClient.Patch("foo", types.MergePatchType, patch, metav1.UpdateOptions{})
+	patchedNoxuInstance, err := noxuNamespacedResourceClient.Patch("foo", types.MergePatchType, patch, metav1.UpdateOptions{})
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
+	expectInt64(t, patchedNoxuInstance.UnstructuredContent(), 999, "num", "num2")
+	rv, found, err := unstructured.NestedString(patchedNoxuInstance.UnstructuredContent(), "metadata", "resourceVersion")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if !found {
+		t.Fatalf("metadata.resourceVersion not found")
+	}
 
 	// a patch with no change
-	createdNoxuInstance, err = noxuNamespacedResourceClient.Patch("foo", types.MergePatchType, patch, metav1.UpdateOptions{})
+	t.Logf("Patching .num.num2 again to 999")
+	patchedNoxuInstance, err = noxuNamespacedResourceClient.Patch("foo", types.MergePatchType, patch, metav1.UpdateOptions{})
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
+	// make sure no-op patch does not increment resourceVersion
+	expectInt64(t, patchedNoxuInstance.UnstructuredContent(), 999, "num", "num2")
+	expectString(t, patchedNoxuInstance.UnstructuredContent(), rv, "metadata", "resourceVersion")
 
 	// an empty patch
-	createdNoxuInstance, err = noxuNamespacedResourceClient.Patch("foo", types.MergePatchType, []byte(`{}`), metav1.UpdateOptions{})
+	t.Logf("Applying empty patch")
+	patchedNoxuInstance, err = noxuNamespacedResourceClient.Patch("foo", types.MergePatchType, []byte(`{}`), metav1.UpdateOptions{})
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
+	// an empty patch is a no-op patch. make sure it does not increment resourceVersion
+	expectInt64(t, patchedNoxuInstance.UnstructuredContent(), 999, "num", "num2")
+	expectString(t, patchedNoxuInstance.UnstructuredContent(), rv, "metadata", "resourceVersion")
 
-	originalJSON, err := runtime.Encode(unstructured.UnstructuredJSONScheme, createdNoxuInstance)
+	originalJSON, err := runtime.Encode(unstructured.UnstructuredJSONScheme, patchedNoxuInstance)
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
@@ -942,3 +961,22 @@ func TestStatusGetAndPatch(t *testing.T) {
 		t.Fatal(err)
 	}
 }
+
+func expectInt64(t *testing.T, obj map[string]interface{}, value int64, pth ...string) {
+	if v, found, err := unstructured.NestedInt64(obj, pth...); err != nil {
+		t.Fatalf("failed to access .%s: %v", strings.Join(pth, "."), err)
+	} else if !found {
+		t.Fatalf("failed to find .%s", strings.Join(pth, "."))
+	} else if v != value {
+		t.Fatalf("wanted %d at .%s, got %d", value, strings.Join(pth, "."), v)
+	}
+}
+func expectString(t *testing.T, obj map[string]interface{}, value string, pth ...string) {
+	if v, found, err := unstructured.NestedString(obj, pth...); err != nil {
+		t.Fatalf("failed to access .%s: %v", strings.Join(pth, "."), err)
+	} else if !found {
+		t.Fatalf("failed to find .%s", strings.Join(pth, "."))
+	} else if v != value {
+		t.Fatalf("wanted %q at .%s, got %q", value, strings.Join(pth, "."), v)
+	}
+}

staging/src/k8s.io/apiextensions-apiserver/test/integration/subresources_test.go

@@ -633,6 +633,8 @@ func TestSubresourcePatch(t *testing.T) {
 
 	ns := "not-the-default"
 	noxuResourceClient := newNamespacedCustomResourceClient(ns, dynamicClient, noxuDefinition)
+
+	t.Logf("Creating foo")
 	_, err = instantiateCustomResource(t, NewNoxuSubresourceInstance(ns, "foo"), noxuResourceClient, noxuDefinition)
 	if err != nil {
 		t.Fatalf("unable to create noxu instance: %v", err)
@@ -643,28 +645,21 @@ func TestSubresourcePatch(t *testing.T) {
 		t.Fatal(err)
 	}
 
+	t.Logf("Patching .status.num to 999")
 	patch := []byte(`{"spec": {"num":999}, "status": {"num":999}}`)
 	patchedNoxuInstance, err := noxuResourceClient.Patch("foo", types.MergePatchType, patch, metav1.UpdateOptions{}, "status")
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
 
-	// .spec.num should remain 10
+	expectInt64(t, patchedNoxuInstance.UnstructuredContent(), 999, "status", "num") // .status.num should be 999
-	specNum, found, err := unstructured.NestedInt64(patchedNoxuInstance.Object, "spec", "num")
+	expectInt64(t, patchedNoxuInstance.UnstructuredContent(), 10, "spec", "num")    // .spec.num should remain 10
-	if !found || err != nil {
+	rv, found, err := unstructured.NestedString(patchedNoxuInstance.UnstructuredContent(), "metadata", "resourceVersion")
-		t.Fatalf("unable to get .spec.num")
+	if err != nil {
+		t.Fatal(err)
 	}
-	if specNum != 10 {
+	if !found {
-		t.Fatalf(".spec.num: expected: %v, got: %v", 10, specNum)
+		t.Fatalf("metadata.resourceVersion not found")
-	}
-
-	// .status.num should be 999
-	statusNum, found, err := unstructured.NestedInt64(patchedNoxuInstance.Object, "status", "num")
-	if !found || err != nil {
-		t.Fatalf("unable to get .status.num")
-	}
-	if statusNum != 999 {
-		t.Fatalf(".status.num: expected: %v, got: %v", 999, statusNum)
 	}
 
 	// this call waits for the resourceVersion to be reached in the cache before returning.
@@ -679,23 +674,44 @@ func TestSubresourcePatch(t *testing.T) {
 	}
 
 	// no-op patch
-	_, err = noxuResourceClient.Patch("foo", types.MergePatchType, patch, metav1.UpdateOptions{}, "status")
+	t.Logf("Patching .status.num again to 999")
+	patchedNoxuInstance, err = noxuResourceClient.Patch("foo", types.MergePatchType, patch, metav1.UpdateOptions{}, "status")
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
+	// make sure no-op patch does not increment resourceVersion
+	expectInt64(t, patchedNoxuInstance.UnstructuredContent(), 999, "status", "num")
+	expectInt64(t, patchedNoxuInstance.UnstructuredContent(), 10, "spec", "num")
+	expectString(t, patchedNoxuInstance.UnstructuredContent(), rv, "metadata", "resourceVersion")
 
 	// empty patch
-	_, err = noxuResourceClient.Patch("foo", types.MergePatchType, []byte(`{}`), metav1.UpdateOptions{}, "status")
+	t.Logf("Applying empty patch")
+	patchedNoxuInstance, err = noxuResourceClient.Patch("foo", types.MergePatchType, []byte(`{}`), metav1.UpdateOptions{}, "status")
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
+	// an empty patch is a no-op patch. make sure it does not increment resourceVersion
+	expectInt64(t, patchedNoxuInstance.UnstructuredContent(), 999, "status", "num")
+	expectInt64(t, patchedNoxuInstance.UnstructuredContent(), 10, "spec", "num")
+	expectString(t, patchedNoxuInstance.UnstructuredContent(), rv, "metadata", "resourceVersion")
 
+	t.Logf("Patching .spec.replicas to 7")
 	patch = []byte(`{"spec": {"replicas":7}, "status": {"replicas":7}}`)
 	patchedNoxuInstance, err = noxuResourceClient.Patch("foo", types.MergePatchType, patch, metav1.UpdateOptions{}, "scale")
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
 
+	expectInt64(t, patchedNoxuInstance.UnstructuredContent(), 7, "spec", "replicas")
+	expectInt64(t, patchedNoxuInstance.UnstructuredContent(), 0, "status", "replicas") // .status.replicas should remain 0
+	rv, found, err = unstructured.NestedString(patchedNoxuInstance.UnstructuredContent(), "metadata", "resourceVersion")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if !found {
+		t.Fatalf("metadata.resourceVersion not found")
+	}
+
 	// this call waits for the resourceVersion to be reached in the cache before returning.
 	// We need to do this because the patch gets its initial object from the storage, and the cache serves that.
 	// If it is out of date, then our initial patch is applied to an old resource version, which conflicts
@@ -707,7 +723,7 @@ func TestSubresourcePatch(t *testing.T) {
 		t.Fatalf("unexpected error: %v", err)
 	}
 
-	// Scale.Spec.Replicas = 7 but Scale.Status.Replicas should remain 7
+	// Scale.Spec.Replicas = 7 but Scale.Status.Replicas should remain 0
 	gottenScale, err := scaleClient.Scales("not-the-default").Get(groupResource, "foo")
 	if err != nil {
 		t.Fatal(err)
@@ -720,16 +736,26 @@ func TestSubresourcePatch(t *testing.T) {
 	}
 
 	// no-op patch
-	_, err = noxuResourceClient.Patch("foo", types.MergePatchType, patch, metav1.UpdateOptions{}, "scale")
+	t.Logf("Patching .spec.replicas again to 7")
+	patchedNoxuInstance, err = noxuResourceClient.Patch("foo", types.MergePatchType, patch, metav1.UpdateOptions{}, "scale")
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
+	// make sure no-op patch does not increment resourceVersion
+	expectInt64(t, patchedNoxuInstance.UnstructuredContent(), 7, "spec", "replicas")
+	expectInt64(t, patchedNoxuInstance.UnstructuredContent(), 0, "status", "replicas")
+	expectString(t, patchedNoxuInstance.UnstructuredContent(), rv, "metadata", "resourceVersion")
 
 	// empty patch
-	_, err = noxuResourceClient.Patch("foo", types.MergePatchType, []byte(`{}`), metav1.UpdateOptions{}, "scale")
+	t.Logf("Applying empty patch")
+	patchedNoxuInstance, err = noxuResourceClient.Patch("foo", types.MergePatchType, []byte(`{}`), metav1.UpdateOptions{}, "scale")
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
+	// an empty patch is a no-op patch. make sure it does not increment resourceVersion
+	expectInt64(t, patchedNoxuInstance.UnstructuredContent(), 7, "spec", "replicas")
+	expectInt64(t, patchedNoxuInstance.UnstructuredContent(), 0, "status", "replicas")
+	expectString(t, patchedNoxuInstance.UnstructuredContent(), rv, "metadata", "resourceVersion")
 
 	// make sure strategic merge patch is not supported for both status and scale
 	_, err = noxuResourceClient.Patch("foo", types.StrategicMergePatchType, patch, metav1.UpdateOptions{}, "status")

staging/src/k8s.io/apiserver/pkg/registry/rest/create.go

@@ -81,7 +81,7 @@ func BeforeCreate(strategy RESTCreateStrategy, ctx context.Context, obj runtime.
 		if !ValidNamespace(ctx, objectMeta) {
 			return errors.NewBadRequest("the namespace of the provided object does not match the namespace sent on the request")
 		}
-	} else {
+	} else if len(objectMeta.GetNamespace()) > 0 {
 		objectMeta.SetNamespace(metav1.NamespaceNone)
 	}
 	objectMeta.SetDeletionTimestamp(nil)
@@ -98,7 +98,9 @@ func BeforeCreate(strategy RESTCreateStrategy, ctx context.Context, obj runtime.
 	}
 
 	// ClusterName is ignored and should not be saved
+	if len(objectMeta.GetClusterName()) > 0 {
 		objectMeta.SetClusterName("")
+	}
 
 	if errs := strategy.Validate(ctx, obj); len(errs) > 0 {
 		return errors.NewInvalid(kind.GroupKind(), objectMeta.GetName(), errs)

staging/src/k8s.io/apiserver/pkg/registry/rest/update.go

@@ -83,6 +83,7 @@ func validateCommonFields(obj, old runtime.Object, strategy RESTUpdateStrategy)
 // BeforeUpdate ensures that common operations for all resources are performed on update. It only returns
 // errors that can be converted to api.Status. It will invoke update validation with the provided existing
 // and updated objects.
+// It sets zero values only if the object does not have a zero value for the respective field.
 func BeforeUpdate(strategy RESTUpdateStrategy, ctx context.Context, obj, old runtime.Object) error {
 	objectMeta, kind, kerr := objectMetaAndKind(strategy, obj)
 	if kerr != nil {
@@ -92,9 +93,10 @@ func BeforeUpdate(strategy RESTUpdateStrategy, ctx context.Context, obj, old run
 		if !ValidNamespace(ctx, objectMeta) {
 			return errors.NewBadRequest("the namespace of the provided object does not match the namespace sent on the request")
 		}
-	} else {
+	} else if len(objectMeta.GetNamespace()) > 0 {
 		objectMeta.SetNamespace(metav1.NamespaceNone)
 	}
+
 	// Ensure requests cannot update generation
 	oldMeta, err := meta.Accessor(old)
 	if err != nil {
@@ -111,7 +113,9 @@ func BeforeUpdate(strategy RESTUpdateStrategy, ctx context.Context, obj, old run
 	strategy.PrepareForUpdate(ctx, obj, old)
 
 	// ClusterName is ignored and should not be saved
+	if len(objectMeta.GetClusterName()) > 0 {
 		objectMeta.SetClusterName("")
+	}
 	// Use the existing UID if none is provided
 	if len(objectMeta.GetUID()) == 0 {
 		objectMeta.SetUID(oldMeta.GetUID())