Skip to content

K
k3s
Commit d691748a authored by Nikhita Raghunath's avatar Nikhita Raghunath
Browse files
Options

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.
parent abdfd7ce
Hide whitespace changes
Inline Side-by-side
Showing with 99 additions and 29 deletions
staging/src/k8s.io/apiextensions-apiserver/test/integration/basic_test.go
View file @ d691748a
......@@ -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)
}
}
staging/src/k8s.io/apiextensions-apiserver/test/integration/subresources_test.go
View file @ d691748a
......@@ -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")
......
staging/src/k8s.io/apiserver/pkg/registry/rest/create.go
View file @ d691748a
......@@ -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)
......
staging/src/k8s.io/apiserver/pkg/registry/rest/update.go
View file @ d691748a
......@@ -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())
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment