Skip to content

Commit

Permalink
Prevent resource version to custom resource on no-op writes
Browse files Browse the repository at this point in the history 
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.

Kubernetes-commit: d691748aa64376e4b90ae2c01aca14d1be2b442c
  • Loading branch information
@nikhita @k8s-publishing-bot
nikhita authored and k8s-publishing-bot committed Aug 20, 2018
1 parent 88ed9a8 commit 313117e
Show file tree
Hide file tree
Showing 2 changed files with 89 additions and 25 deletions.
48 changes: 43 additions & 5 deletions 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 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

0 comments on commit 313117e

Please sign in to comment.