Merge pull request #62063 from atlassian/no-mutation-unstructuredcontent

Automatic merge from submit-queue. 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>.

Make UnstructuredContent return contents without mutating the source

**What this PR does / why we need it**:
This PR solves the issues described in #56316

Before this change:

- A call to `UnstructuredContent()` potentially modified `Object`
- The values returned by `UnstructuredContent()` could be manipulated to modify the value in `Object`. Going through the history it looks like this behavior was added before the addition of `SetUnstructuredContent()`. IMO it makes more sense now to use `SetUnstructuredContent()` or make changes to the exposed `Object` property
- `UnstructuredList` did not implement the behavior described in the godoc. The godoc stated that the value returned should be mutable, but if u.Object == nil the map returned had no effect on Object

With this PR I'm proposing `UnstructuredContent()` returns the data without providing the contract of a mutable map. It also ensures all implementations of the `Unstructured` interface abide by the doc

**Which issue(s) this PR fixes**:
Fixes #56316

**Special notes for your reviewer**:
This PR continues work started in #57713.

**Release note**:
```release-note
NONE
```
/kind bug
/sig api-machinery
/cc sttts deads2k

staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/BUILD

@@ -11,6 +11,7 @@ go_test(
     srcs = [
         "helpers_test.go",
         "unstructured_list_test.go",
+        "unstructured_test.go",
     ],
     embed = [":go_default_library"],
     deps = [

staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured.go

@@ -82,7 +82,7 @@ func (obj *Unstructured) EachListItem(fn func(runtime.Object) error) error {
 
 func (obj *Unstructured) UnstructuredContent() map[string]interface{} {
 	if obj.Object == nil {
-		obj.Object = make(map[string]interface{})
+		return make(map[string]interface{})
 	}
 	return obj.Object
 }

staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured_list.go

@@ -53,19 +53,18 @@ func (u *UnstructuredList) EachListItem(fn func(runtime.Object) error) error {
 }
 
 // UnstructuredContent returns a map contain an overlay of the Items field onto
-// the Object field. Items always overwrites overlay. Changing "items" in the
-// returned object will affect items in the underlying Items field, but changing
-// the "items" slice itself will have no effect.
-// TODO: expose SetUnstructuredContent on runtime.Unstructured that allows
-// items to be changed.
+// the Object field. Items always overwrites overlay.
 func (u *UnstructuredList) UnstructuredContent() map[string]interface{} {
-	out := u.Object
-	if out == nil {
-		out = make(map[string]interface{})
+	out := make(map[string]interface{}, len(u.Object)+1)
+
+	// shallow copy every property
+	for k, v := range u.Object {
+		out[k] = v
 	}
+
 	items := make([]interface{}, len(u.Items))
 	for i, item := range u.Items {
-		items[i] = item.Object
+		items[i] = item.UnstructuredContent()
 	}
 	out["items"] = items
 	return out

staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured_test.go

@@ -0,0 +1,32 @@
+/*
+Copyright 2018 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package unstructured
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestNilUnstructuredContent(t *testing.T) {
+	var u Unstructured
+	uCopy := u.DeepCopy()
+	content := u.UnstructuredContent()
+	expContent := make(map[string]interface{})
+	assert.EqualValues(t, expContent, content)
+	assert.Equal(t, uCopy, &u)
+}

staging/src/k8s.io/apimachinery/pkg/runtime/converter.go

@@ -411,8 +411,7 @@ func (c *unstructuredConverter) ToUnstructured(obj interface{}) (map[string]inte
 	var u map[string]interface{}
 	var err error
 	if unstr, ok := obj.(Unstructured); ok {
-		// UnstructuredContent() mutates the object so we need to make a copy first
-		u = unstr.DeepCopyObject().(Unstructured).UnstructuredContent()
+		u = unstr.UnstructuredContent()
 	} else {
 		t := reflect.TypeOf(obj)
 		value := reflect.ValueOf(obj)
@@ -449,12 +448,20 @@ func DeepCopyJSON(x map[string]interface{}) map[string]interface{} {
 func DeepCopyJSONValue(x interface{}) interface{} {
 	switch x := x.(type) {
 	case map[string]interface{}:
+		if x == nil {
+			// Typed nil - an interface{} that contains a type map[string]interface{} with a value of nil
+			return x
+		}
 		clone := make(map[string]interface{}, len(x))
 		for k, v := range x {
 			clone[k] = DeepCopyJSONValue(v)
 		}
 		return clone
 	case []interface{}:
+		if x == nil {
+			// Typed nil - an interface{} that contains a type []interface{} with a value of nil
+			return x
+		}
 		clone := make([]interface{}, len(x))
 		for i, v := range x {
 			clone[i] = DeepCopyJSONValue(v)

staging/src/k8s.io/apimachinery/pkg/runtime/interfaces.go

@@ -234,9 +234,9 @@ type Object interface {
 // to JSON allowed.
 type Unstructured interface {
 	Object
-	// UnstructuredContent returns a non-nil, mutable map of the contents of this object. Values may be
+	// UnstructuredContent returns a non-nil map with this object's contents. Values may be
 	// []interface{}, map[string]interface{}, or any primitive type. Contents are typically serialized to
-	// and from JSON.
+	// and from JSON. SetUnstructuredContent should be used to mutate the contents.
 	UnstructuredContent() map[string]interface{}
 	// SetUnstructuredContent updates the object content to match the provided map.
 	SetUnstructuredContent(map[string]interface{})

staging/src/k8s.io/apimachinery/pkg/runtime/testing/types.go

@@ -263,7 +263,7 @@ func (obj *Unstructured) EachListItem(fn func(runtime.Object) error) error {
 
 func (obj *Unstructured) UnstructuredContent() map[string]interface{} {
 	if obj.Object == nil {
-		obj.Object = make(map[string]interface{})
+		return make(map[string]interface{})
 	}
 	return obj.Object
 }