From f69544e87d494e4df891af93d7b3ccdbb37150c1 Mon Sep 17 00:00:00 2001 From: Nikhita Raghunath Date: Tue, 21 Aug 2018 11:55:42 +0530 Subject: [PATCH 1/2] Fix unstructured metadata accessors to respect omitempty semantics ObjectMeta has fields with omitempty json tags. This means that when the fields have zero values, they should not be persisted in the object. Before this commit, some of the metadata accessors for unstructured objects did not respect these semantics i.e they would persist a field even if it had a zero value. This commit updates the accessors so that the field is removed from the unstructured object map if it contains a zero value. --- hack/.golint_failures | 1 - .../pkg/apis/meta/v1/unstructured/BUILD | 6 + .../apis/meta/v1/unstructured/unstructured.go | 61 +++++++- .../meta/v1/unstructured/unstructured_test.go | 134 +++++++++++++++++- 4 files changed, 195 insertions(+), 7 deletions(-) diff --git a/hack/.golint_failures b/hack/.golint_failures index 3ac36325ff3..d3fe5c73e38 100644 --- a/hack/.golint_failures +++ b/hack/.golint_failures @@ -478,7 +478,6 @@ staging/src/k8s.io/apimachinery/pkg/api/validation/path staging/src/k8s.io/apimachinery/pkg/apis/config/v1alpha1 staging/src/k8s.io/apimachinery/pkg/apis/meta/fuzzer staging/src/k8s.io/apimachinery/pkg/apis/meta/internalversion -staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation staging/src/k8s.io/apimachinery/pkg/apis/meta/v1beta1 staging/src/k8s.io/apimachinery/pkg/apis/testapigroup diff --git a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/BUILD b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/BUILD index f5b2d8557d5..3450d869db7 100644 --- a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/BUILD +++ b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/BUILD @@ -15,7 +15,13 @@ go_test( ], embed = [":go_default_library"], deps = [ + "//staging/src/k8s.io/apimachinery/pkg/api/apitesting/fuzzer:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/api/equality:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/apis/meta/fuzzer:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/runtime/serializer:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/util/diff:go_default_library", "//vendor/github.com/stretchr/testify/assert:go_default_library", "//vendor/github.com/stretchr/testify/require:go_default_library", ], diff --git a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured.go b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured.go index 548a01e59a9..781469ec265 100644 --- a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured.go +++ b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured.go @@ -179,6 +179,11 @@ func (u *Unstructured) GetOwnerReferences() []metav1.OwnerReference { } func (u *Unstructured) SetOwnerReferences(references []metav1.OwnerReference) { + if references == nil { + RemoveNestedField(u.Object, "metadata", "ownerReferences") + return + } + newReferences := make([]interface{}, 0, len(references)) for _, reference := range references { out, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&reference) @@ -212,6 +217,10 @@ func (u *Unstructured) GetNamespace() string { } func (u *Unstructured) SetNamespace(namespace string) { + if len(namespace) == 0 { + RemoveNestedField(u.Object, "metadata", "namespace") + return + } u.setNestedField(namespace, "metadata", "namespace") } @@ -220,6 +229,10 @@ func (u *Unstructured) GetName() string { } func (u *Unstructured) SetName(name string) { + if len(name) == 0 { + RemoveNestedField(u.Object, "metadata", "name") + return + } u.setNestedField(name, "metadata", "name") } @@ -227,8 +240,12 @@ func (u *Unstructured) GetGenerateName() string { return getNestedString(u.Object, "metadata", "generateName") } -func (u *Unstructured) SetGenerateName(name string) { - u.setNestedField(name, "metadata", "generateName") +func (u *Unstructured) SetGenerateName(generateName string) { + if len(generateName) == 0 { + RemoveNestedField(u.Object, "metadata", "generateName") + return + } + u.setNestedField(generateName, "metadata", "generateName") } func (u *Unstructured) GetUID() types.UID { @@ -236,6 +253,10 @@ func (u *Unstructured) GetUID() types.UID { } func (u *Unstructured) SetUID(uid types.UID) { + if len(string(uid)) == 0 { + RemoveNestedField(u.Object, "metadata", "uid") + return + } u.setNestedField(string(uid), "metadata", "uid") } @@ -243,8 +264,12 @@ func (u *Unstructured) GetResourceVersion() string { return getNestedString(u.Object, "metadata", "resourceVersion") } -func (u *Unstructured) SetResourceVersion(version string) { - u.setNestedField(version, "metadata", "resourceVersion") +func (u *Unstructured) SetResourceVersion(resourceVersion string) { + if len(resourceVersion) == 0 { + RemoveNestedField(u.Object, "metadata", "resourceVersion") + return + } + u.setNestedField(resourceVersion, "metadata", "resourceVersion") } func (u *Unstructured) GetGeneration() int64 { @@ -256,6 +281,10 @@ func (u *Unstructured) GetGeneration() int64 { } func (u *Unstructured) SetGeneration(generation int64) { + if generation == 0 { + RemoveNestedField(u.Object, "metadata", "generation") + return + } u.setNestedField(generation, "metadata", "generation") } @@ -264,6 +293,10 @@ func (u *Unstructured) GetSelfLink() string { } func (u *Unstructured) SetSelfLink(selfLink string) { + if len(selfLink) == 0 { + RemoveNestedField(u.Object, "metadata", "selfLink") + return + } u.setNestedField(selfLink, "metadata", "selfLink") } @@ -272,6 +305,10 @@ func (u *Unstructured) GetContinue() string { } func (u *Unstructured) SetContinue(c string) { + if len(c) == 0 { + RemoveNestedField(u.Object, "metadata", "continue") + return + } u.setNestedField(c, "metadata", "continue") } @@ -330,6 +367,10 @@ func (u *Unstructured) GetLabels() map[string]string { } func (u *Unstructured) SetLabels(labels map[string]string) { + if labels == nil { + RemoveNestedField(u.Object, "metadata", "labels") + return + } u.setNestedMap(labels, "metadata", "labels") } @@ -339,6 +380,10 @@ func (u *Unstructured) GetAnnotations() map[string]string { } func (u *Unstructured) SetAnnotations(annotations map[string]string) { + if annotations == nil { + RemoveNestedField(u.Object, "metadata", "annotations") + return + } u.setNestedMap(annotations, "metadata", "annotations") } @@ -387,6 +432,10 @@ func (u *Unstructured) GetFinalizers() []string { } func (u *Unstructured) SetFinalizers(finalizers []string) { + if finalizers == nil { + RemoveNestedField(u.Object, "metadata", "finalizers") + return + } u.setNestedSlice(finalizers, "metadata", "finalizers") } @@ -395,5 +444,9 @@ func (u *Unstructured) GetClusterName() string { } func (u *Unstructured) SetClusterName(clusterName string) { + if len(clusterName) == 0 { + RemoveNestedField(u.Object, "metadata", "clusterName") + return + } u.setNestedField(clusterName, "metadata", "clusterName") } diff --git a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured_test.go b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured_test.go index cbcbbcef34b..4a03fff654e 100644 --- a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured_test.go +++ b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured_test.go @@ -14,19 +14,149 @@ See the License for the specific language governing permissions and limitations under the License. */ -package unstructured +package unstructured_test import ( + "math/rand" + "reflect" "testing" "github.com/stretchr/testify/assert" + + "k8s.io/apimachinery/pkg/api/apitesting/fuzzer" + "k8s.io/apimachinery/pkg/api/equality" + metafuzzer "k8s.io/apimachinery/pkg/apis/meta/fuzzer" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/serializer" + "k8s.io/apimachinery/pkg/util/diff" ) func TestNilUnstructuredContent(t *testing.T) { - var u Unstructured + var u unstructured.Unstructured uCopy := u.DeepCopy() content := u.UnstructuredContent() expContent := make(map[string]interface{}) assert.EqualValues(t, expContent, content) assert.Equal(t, uCopy, &u) } + +// TestUnstructuredMetadataRoundTrip checks that metadata accessors +// correctly set the metadata for unstructured objects. +// First, it fuzzes an empty ObjectMeta and sets this value as the metadata for an unstructured object. +// Next, it uses metadata accessor methods to set these fuzzed values to another unstructured object. +// Finally, it checks that both the unstructured objects are equal. +func TestUnstructuredMetadataRoundTrip(t *testing.T) { + scheme := runtime.NewScheme() + codecs := serializer.NewCodecFactory(scheme) + seed := rand.Int63() + fuzzer := fuzzer.FuzzerFor(metafuzzer.Funcs, rand.NewSource(seed), codecs) + + N := 1000 + for i := 0; i < N; i++ { + u := &unstructured.Unstructured{Object: map[string]interface{}{}} + uCopy := u.DeepCopy() + metadata := &metav1.ObjectMeta{} + fuzzer.Fuzz(metadata) + + if err := setObjectMeta(u, metadata); err != nil { + t.Fatalf("unexpected error setting fuzzed ObjectMeta: %v", err) + } + setObjectMetaUsingAccessors(u, uCopy) + + // TODO: remove this special casing when creationTimestamp becomes a pointer. + // Right now, creationTimestamp is a struct (metav1.Time) so omitempty holds no meaning for it. + // However, the current behaviour is to remove the field if it holds an empty struct. + // This special casing exists here because custom marshallers for metav1.Time marshal + // an empty value to "null", which gets converted to nil when converting to an unstructured map by "ToUnstructured". + if err := unstructured.SetNestedField(uCopy.UnstructuredContent(), nil, "metadata", "creationTimestamp"); err != nil { + t.Fatalf("unexpected error setting creationTimestamp as nil: %v", err) + } + + if !equality.Semantic.DeepEqual(u, uCopy) { + t.Errorf("diff: %v", diff.ObjectReflectDiff(u, uCopy)) + } + } +} + +// TestUnstructuredMetadataOmitempty checks that ObjectMeta omitempty +// semantics are enforced for unstructured objects. +// The fuzzing test above should catch these cases but this is here just to be safe. +// Example: the metadata.clusterName field has the omitempty json tag +// so if it is set to it's zero value (""), it should be removed from the metadata map. +func TestUnstructuredMetadataOmitempty(t *testing.T) { + scheme := runtime.NewScheme() + codecs := serializer.NewCodecFactory(scheme) + seed := rand.Int63() + fuzzer := fuzzer.FuzzerFor(metafuzzer.Funcs, rand.NewSource(seed), codecs) + + // fuzz to make sure we don't miss any function calls below + u := &unstructured.Unstructured{Object: map[string]interface{}{}} + metadata := &metav1.ObjectMeta{} + fuzzer.Fuzz(metadata) + if err := setObjectMeta(u, metadata); err != nil { + t.Fatalf("unexpected error setting fuzzed ObjectMeta: %v", err) + } + + // set zero values for all fields in metadata explicitly + // to check that omitempty fields having zero values are never set + u.SetName("") + u.SetGenerateName("") + u.SetNamespace("") + u.SetSelfLink("") + u.SetUID("") + u.SetResourceVersion("") + u.SetGeneration(0) + u.SetCreationTimestamp(metav1.Time{}) + u.SetDeletionTimestamp(nil) + u.SetDeletionGracePeriodSeconds(nil) + u.SetLabels(nil) + u.SetAnnotations(nil) + u.SetOwnerReferences(nil) + u.SetInitializers(nil) + u.SetFinalizers(nil) + u.SetClusterName("") + + gotMetadata, _, err := unstructured.NestedFieldNoCopy(u.UnstructuredContent(), "metadata") + if err != nil { + t.Error(err) + } + emptyMetadata := make(map[string]interface{}) + + if !reflect.DeepEqual(gotMetadata, emptyMetadata) { + t.Errorf("expected %v, got %v", emptyMetadata, gotMetadata) + } +} + +func setObjectMeta(u *unstructured.Unstructured, objectMeta *metav1.ObjectMeta) error { + if objectMeta == nil { + unstructured.RemoveNestedField(u.UnstructuredContent(), "metadata") + return nil + } + metadata, err := runtime.DefaultUnstructuredConverter.ToUnstructured(objectMeta) + if err != nil { + return err + } + u.UnstructuredContent()["metadata"] = metadata + return nil +} + +func setObjectMetaUsingAccessors(u, uCopy *unstructured.Unstructured) { + uCopy.SetName(u.GetName()) + uCopy.SetGenerateName(u.GetGenerateName()) + uCopy.SetNamespace(u.GetNamespace()) + uCopy.SetSelfLink(u.GetSelfLink()) + uCopy.SetUID(u.GetUID()) + uCopy.SetResourceVersion(u.GetResourceVersion()) + uCopy.SetGeneration(u.GetGeneration()) + uCopy.SetCreationTimestamp(u.GetCreationTimestamp()) + uCopy.SetDeletionTimestamp(u.GetDeletionTimestamp()) + uCopy.SetDeletionGracePeriodSeconds(u.GetDeletionGracePeriodSeconds()) + uCopy.SetLabels(u.GetLabels()) + uCopy.SetAnnotations(u.GetAnnotations()) + uCopy.SetOwnerReferences(u.GetOwnerReferences()) + uCopy.SetInitializers(u.GetInitializers()) + uCopy.SetFinalizers(u.GetFinalizers()) + uCopy.SetClusterName(u.GetClusterName()) +} From dabd56f7df207c1babc83871c43c90cf9451e896 Mon Sep 17 00:00:00 2001 From: Nikhita Raghunath Date: Sun, 19 Aug 2018 01:04:28 +0530 Subject: [PATCH 2/2] Fix tests to support ObjectMeta omitempty semantics --- .../cmd/testdata/edit/testcase-create-list-error/1.request | 1 - .../cmd/testdata/edit/testcase-create-list-error/3.request | 1 - .../edit/testcase-unknown-field-known-group-kind/2.request | 3 +-- .../edit/testcase-unknown-version-known-group-kind/2.request | 3 +-- .../apiserver/pkg/registry/generic/registry/dryrun_test.go | 4 ++-- 5 files changed, 4 insertions(+), 8 deletions(-) diff --git a/pkg/kubectl/cmd/testdata/edit/testcase-create-list-error/1.request b/pkg/kubectl/cmd/testdata/edit/testcase-create-list-error/1.request index 8b60bbb47b6..460ffec4b00 100755 --- a/pkg/kubectl/cmd/testdata/edit/testcase-create-list-error/1.request +++ b/pkg/kubectl/cmd/testdata/edit/testcase-create-list-error/1.request @@ -8,7 +8,6 @@ }, "name": "svc1", "namespace": "edit-test", - "resourceVersion": "", "selfLink": "/api/v1/namespaces/edit-test/services/svc1", "uid": "4149f70e-e9dc-11e6-8c3b-acbc32c1ca87" }, diff --git a/pkg/kubectl/cmd/testdata/edit/testcase-create-list-error/3.request b/pkg/kubectl/cmd/testdata/edit/testcase-create-list-error/3.request index fb9c3d1c0ed..d18b4cad78d 100755 --- a/pkg/kubectl/cmd/testdata/edit/testcase-create-list-error/3.request +++ b/pkg/kubectl/cmd/testdata/edit/testcase-create-list-error/3.request @@ -8,7 +8,6 @@ }, "name": "svc2", "namespace": "edit-test", - "resourceVersion": "", "selfLink": "/api/v1/namespaces/edit-test/services/svc2", "uid": "3e9b10db-e9dc-11e6-8c3b-acbc32c1ca87" }, diff --git a/pkg/kubectl/cmd/testdata/edit/testcase-unknown-field-known-group-kind/2.request b/pkg/kubectl/cmd/testdata/edit/testcase-unknown-field-known-group-kind/2.request index c8ee0725e9c..5d79cb5c305 100755 --- a/pkg/kubectl/cmd/testdata/edit/testcase-unknown-field-known-group-kind/2.request +++ b/pkg/kubectl/cmd/testdata/edit/testcase-unknown-field-known-group-kind/2.request @@ -2,8 +2,7 @@ "metadata": { "labels": { "label2": "value2" - }, - "namespace": "" + } }, "unknownClientField": { "clientdata": true diff --git a/pkg/kubectl/cmd/testdata/edit/testcase-unknown-version-known-group-kind/2.request b/pkg/kubectl/cmd/testdata/edit/testcase-unknown-version-known-group-kind/2.request index 654d91e4e66..b87c5cbb9c7 100755 --- a/pkg/kubectl/cmd/testdata/edit/testcase-unknown-version-known-group-kind/2.request +++ b/pkg/kubectl/cmd/testdata/edit/testcase-unknown-version-known-group-kind/2.request @@ -5,7 +5,6 @@ "metadata": { "labels": { "label2": "value2" - }, - "namespace": "" + } } } \ No newline at end of file diff --git a/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/dryrun_test.go b/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/dryrun_test.go index 129c925afa5..8fbf219cfdb 100644 --- a/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/dryrun_test.go +++ b/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/dryrun_test.go @@ -215,7 +215,7 @@ func TestDryRunUpdateReturnsObject(t *testing.T) { if err != nil { t.Fatalf("Failed to dry-run update: %v", err) } - out = UnstructuredOrDie(`{"field": "value", "kind": "Pod", "metadata": {"resourceVersion": "2", "selfLink": ""}}`) + out = UnstructuredOrDie(`{"field": "value", "kind": "Pod", "metadata": {"resourceVersion": "2"}}`) if !reflect.DeepEqual(obj, out) { t.Fatalf("Returned object %#v different from expected %#v", obj, out) } @@ -268,7 +268,7 @@ func TestDryRunDeleteReturnsObject(t *testing.T) { } out = UnstructuredOrDie(`{}`) - expected := UnstructuredOrDie(`{"kind": "Pod", "metadata": {"resourceVersion": "2", "selfLink": ""}}`) + expected := UnstructuredOrDie(`{"kind": "Pod", "metadata": {"resourceVersion": "2"}}`) err = s.Delete(context.Background(), "key", out, nil, true) if err != nil { t.Fatalf("Failed to delete with valid precondition: %v", err)