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 5ae99f7f48a..b9f34b3cb4d 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" @@ -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) + } +} 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 5d4397b61ce..c8c8728ee52 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, metav1.UpdateOptions{}, "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") + 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 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") - } - 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, 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") 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 619acc3e0ba..388ca523394 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 24e918604ca..147541bcfd3 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())