Skip to content

Commit

Permalink
Merge pull request #67562 from nikhita/customresource-subresource-patch
Browse files Browse the repository at this point in the history 
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 #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.
```
  • Loading branch information
Kubernetes Submit Queue committed Aug 21, 2018
2 parents b666113 + d691748 commit 816f2a4
Show file tree
Hide file tree
Showing 4 changed files with 99 additions and 29 deletions.
48 changes: 43 additions & 5 deletions staging/src/k8s.io/apiextensions-apiserver/test/integration/basic_test.go
View file
Expand Up @@ -20,6 +20,7 @@ import (
"fmt"
"reflect"
"sort"
"strings"
"testing"
"time"

Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
}
}
66 changes: 46 additions & 20 deletions staging/src/k8s.io/apiextensions-apiserver/test/integration/subresources_test.go
View file
Expand Up @@ -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)
Expand All @@ -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")
}
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.
Expand All @@ -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
Expand All @@ -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)
Expand All @@ -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")
Expand Down
6 changes: 4 additions & 2 deletions staging/src/k8s.io/apiserver/pkg/registry/rest/create.go
View file
Expand Up @@ -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)
Expand All @@ -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)
Expand Down
8 changes: 6 additions & 2 deletions staging/src/k8s.io/apiserver/pkg/registry/rest/update.go
View file
Expand Up @@ -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 {
Expand All @@ -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 {
Expand All @@ -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())
Expand Down

0 comments on commit 816f2a4

Please sign in to comment.