Merge pull request #67635 from nikhita/customresource-subresource-patch-04

Automatic merge from submit-queue (batch tested with PRs 67298, 67518, 67635, 67673). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Fix unstructured metadata accessors to respect omitempty semantics

Fixes #67541
Fixes #48211 
Fixes #49075
Follow up of #67562

`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 PR, 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 PR updates the accessors so that the field is removed from the unstructured object map if it contains a zero value.

/sig api-machinery
/kind bug
/area custom-resources
/cc sttts liggitt yue9944882 roycaihw 
/assign sttts liggitt 

**Release note**:

```release-note
NONE
```

hack/.golint_failures

@@ -475,7 +475,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

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"
 	},

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"
 	},

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

pkg/kubectl/cmd/testdata/edit/testcase-unknown-version-known-group-kind/2.request

@@ -5,7 +5,6 @@
 	"metadata": {
 		"labels": {
 			"label2": "value2"
-		},
+		}
-		"namespace": ""
 	}
 }

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",
     ],

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) {
+func (u *Unstructured) SetGenerateName(generateName string) {
-	u.setNestedField(name, "metadata", "generateName")
+	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) {
+func (u *Unstructured) SetResourceVersion(resourceVersion string) {
-	u.setNestedField(version, "metadata", "resourceVersion")
+	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")
 }

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())
+}

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)