From 4f3bdcda39500afa0dfae39c68046a8560980a0d Mon Sep 17 00:00:00 2001 From: Nikhita Raghunath Date: Mon, 20 Aug 2018 20:13:55 +0530 Subject: [PATCH] Prevent resource version to custom resource on no-op writes For ObjectMeta pruning, we round trip through 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. The zero values are set in BeforeCreate and BeforeUpdate. This commit updates BeforeUpdate such that zero values are only set when the object does not have a zero value for the respective field. --- .../test/integration/basic_test.go | 48 ++++++++++++-- .../test/integration/subresources_test.go | 66 +++++++++++++------ .../apiserver/pkg/registry/rest/create.go | 6 +- .../apiserver/pkg/registry/rest/update.go | 8 ++- 4 files changed, 99 insertions(+), 29 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/basic_test.go b/staging/src/k8s.io/apiextensions-apiserver/test/integration/basic_test.go index a53c4ca94a24..888675438f6d 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/basic_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/test/integration/basic_test.go @@ -20,6 +20,7 @@ import ( "fmt" "reflect" "sort" + "strings" "testing" "time" @@ -599,31 +600,49 @@ func TestPatch(t *testing.T) { ns := "not-the-default" noxuNamespacedResourceClient := newNamespacedCustomResourceClient(ns, dynamicClient, noxuDefinition) + t.Logf("Creating foo") noxuInstanceToCreate := testserver.NewNoxuInstance(ns, "foo") - createdNoxuInstance, err := noxuNamespacedResourceClient.Create(noxuInstanceToCreate) + _, err = noxuNamespacedResourceClient.Create(noxuInstanceToCreate) 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) + patchedNoxuInstance, err := noxuNamespacedResourceClient.Patch("foo", types.MergePatchType, patch) 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) + t.Logf("Patching .num.num2 again to 999") + patchedNoxuInstance, err = noxuNamespacedResourceClient.Patch("foo", types.MergePatchType, patch) 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(`{}`)) + t.Logf("Applying empty patch") + patchedNoxuInstance, err = noxuNamespacedResourceClient.Patch("foo", types.MergePatchType, []byte(`{}`)) 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) } @@ -874,3 +893,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) + } +} diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/subresources_test.go b/staging/src/k8s.io/apiextensions-apiserver/test/integration/subresources_test.go index bce63a4a5aba..4421d840dc3a 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/subresources_test.go +++ b/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, "status") if err != nil { t.Fatalf("unexpected error: %v", err) } - // .spec.num should remain 10 - specNum, found, err := unstructured.NestedInt64(patchedNoxuInstance.Object, "spec", "num") - if !found || err != nil { - t.Fatalf("unable to get .spec.num") - } - if specNum != 10 { - t.Fatalf(".spec.num: expected: %v, got: %v", 10, specNum) - } - - // .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") + expectInt64(t, patchedNoxuInstance.UnstructuredContent(), 999, "status", "num") // .status.num should be 999 + expectInt64(t, patchedNoxuInstance.UnstructuredContent(), 10, "spec", "num") // .spec.num should remain 10 + rv, found, err := unstructured.NestedString(patchedNoxuInstance.UnstructuredContent(), "metadata", "resourceVersion") + if err != nil { + t.Fatal(err) } - if statusNum != 999 { - t.Fatalf(".status.num: expected: %v, got: %v", 999, statusNum) + if !found { + t.Fatalf("metadata.resourceVersion not found") } // 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, "status") + t.Logf("Patching .status.num again to 999") + patchedNoxuInstance, err = noxuResourceClient.Patch("foo", types.MergePatchType, patch, "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(`{}`), "status") + t.Logf("Applying empty patch") + patchedNoxuInstance, err = noxuResourceClient.Patch("foo", types.MergePatchType, []byte(`{}`), "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, "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, "scale") + t.Logf("Patching .spec.replicas again to 7") + patchedNoxuInstance, err = noxuResourceClient.Patch("foo", types.MergePatchType, patch, "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(`{}`), "scale") + t.Logf("Applying empty patch") + patchedNoxuInstance, err = noxuResourceClient.Patch("foo", types.MergePatchType, []byte(`{}`), "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, "status") diff --git a/staging/src/k8s.io/apiserver/pkg/registry/rest/create.go b/staging/src/k8s.io/apiserver/pkg/registry/rest/create.go index f399ebdc2116..59d9d200de8c 100644 --- a/staging/src/k8s.io/apiserver/pkg/registry/rest/create.go +++ b/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 - objectMeta.SetClusterName("") + if len(objectMeta.GetClusterName()) > 0 { + objectMeta.SetClusterName("") + } if errs := strategy.Validate(ctx, obj); len(errs) > 0 { return errors.NewInvalid(kind.GroupKind(), objectMeta.GetName(), errs) diff --git a/staging/src/k8s.io/apiserver/pkg/registry/rest/update.go b/staging/src/k8s.io/apiserver/pkg/registry/rest/update.go index b08dd1479181..d3e669e93e05 100644 --- a/staging/src/k8s.io/apiserver/pkg/registry/rest/update.go +++ b/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 - objectMeta.SetClusterName("") + if len(objectMeta.GetClusterName()) > 0 { + objectMeta.SetClusterName("") + } // Use the existing UID if none is provided if len(objectMeta.GetUID()) == 0 { objectMeta.SetUID(oldMeta.GetUID())