New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent resourceVersion updates for custom resources on no-op writes #67562

Merged
merged 1 commit into from Aug 21, 2018
Jump to file or symbol
Failed to load files and symbols.
+99 −29
Diff settings

Always

Just for now

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.
  • Loading branch information...
nikhita committed Aug 20, 2018
commit d691748aa64376e4b90ae2c01aca14d1be2b442c
@@ -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)
}
}
@@ -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")
}
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, 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")
@@ -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)
@@ -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())
ProTip! Use n and p to navigate between commits in a pull request.