From f69544e87d494e4df891af93d7b3ccdbb37150c1 Mon Sep 17 00:00:00 2001 From: Nikhita Raghunath Date: Tue, 21 Aug 2018 11:55:42 +0530 Subject: [PATCH] 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()) +}