Merge pull request #55871 from atlassian/unstructured-converter-no-mutation

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>.

Fix potential unexpected object mutation that can lead to data races

**What this PR does / why we need it**:
In #51526 I introduced an optimization - do a deep copy instead of to and from JSON roundtrip to convert anything that implements `runtime.Unstructured`. I just discovered that the method that is used there `UnstructuredContent()` in both `Unstructured` and `UnstructuredList` may mutate the original object.
2008750398/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured.go (L87-L92)
7c10cbc642/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured_list.go (L58-L75)
This is problematic because previously (before #51526) there was no mutation and because this is unexpected and may lead to data races - it is bad behaviour to mutate original object when you just want a copy of it.
This PR fixes the issue.

Without the fix the tests I've added are failing because when comparison is done original object is not the same:
```
converter_test.go:154: Object changed, diff: 
object.Object[items]:
  a: []interface {}{}
  b: <nil>
converter_test.go:154: Object changed, diff: 
object.Object[items]:
  a: []interface {}{map[string]interface {}{"kind":"Pod"}}
  b: <nil>
```

However the underlying issue is not fixed here - `UnstructuredContent()` is brittle and dangerous. Method name does not imply that it mutates data when you call it. And godoc does not mention that either:
509df603b1/staging/src/k8s.io/apimachinery/pkg/runtime/interfaces.go (L233-L249)
Something needs to be done about it IMO.
Also `UnstructuredContent()` implementation in `UnstructuredList` does not implement the behaviour required by godoc in `runtime.Unstructured`.

**Release note**:
```release-note
NONE
```
/kind bug
/sig api-machinery
/assign @sttts

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

@@ -411,7 +411,8 @@ func (c *unstructuredConverter) ToUnstructured(obj interface{}) (map[string]inte
 	var u map[string]interface{}
 	var err error
 	if unstr, ok := obj.(Unstructured); ok {
-		u = DeepCopyJSON(unstr.UnstructuredContent())
+		// UnstructuredContent() mutates the object so we need to make a copy first
+		u = unstr.DeepCopyObject().(Unstructured).UnstructuredContent()
 	} else {
 		t := reflect.TypeOf(obj)
 		value := reflect.ValueOf(obj)

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

@@ -135,7 +135,7 @@ func doRoundTrip(t *testing.T, item interface{}) {
 		return
 	}
 	unmarshalledObj := reflect.New(reflect.TypeOf(item).Elem()).Interface()
-	err = json.Unmarshal(data, &unmarshalledObj)
+	err = json.Unmarshal(data, unmarshalledObj)
 	if err != nil {
 		t.Errorf("Error when unmarshaling to object: %v", err)
 		return
@@ -169,6 +169,38 @@ func TestRoundTrip(t *testing.T) {
 	testCases := []struct {
 		obj interface{}
 	}{
+		{
+			obj: &unstructured.UnstructuredList{
+				Object: map[string]interface{}{
+					"kind": "List",
+				},
+				// Not testing a list with nil Items because items is a non-optional field and hence
+				// is always marshaled into an empty array which is not equal to nil when unmarshalled and will fail.
+				// That is expected.
+				Items: []unstructured.Unstructured{},
+			},
+		},
+		{
+			obj: &unstructured.UnstructuredList{
+				Object: map[string]interface{}{
+					"kind": "List",
+				},
+				Items: []unstructured.Unstructured{
+					{
+						Object: map[string]interface{}{
+							"kind": "Pod",
+						},
+					},
+				},
+			},
+		},
+		{
+			obj: &unstructured.Unstructured{
+				Object: map[string]interface{}{
+					"kind": "Pod",
+				},
+			},
+		},
 		{
 			obj: &unstructured.Unstructured{
 				Object: map[string]interface{}{
@@ -260,7 +292,7 @@ func TestRoundTrip(t *testing.T) {
 // produces the same object.
 func doUnrecognized(t *testing.T, jsonData string, item interface{}, expectedErr error) {
 	unmarshalledObj := reflect.New(reflect.TypeOf(item).Elem()).Interface()
-	err := json.Unmarshal([]byte(jsonData), &unmarshalledObj)
+	err := json.Unmarshal([]byte(jsonData), unmarshalledObj)
 	if (err != nil) != (expectedErr != nil) {
 		t.Errorf("Unexpected error when unmarshaling to object: %v, expected: %v", err, expectedErr)
 		return
@@ -465,11 +497,10 @@ func TestUnrecognized(t *testing.T) {
 		},
 	}
 
-	for i := range testCases {
-		doUnrecognized(t, testCases[i].data, testCases[i].obj, testCases[i].err)
-		if t.Failed() {
-			break
-		}
+	for _, tc := range testCases {
+		t.Run(tc.data, func(t *testing.T) {
+			doUnrecognized(t, tc.data, tc.obj, tc.err)
+		})
 	}
 }
 

test/e2e/apimachinery/garbage_collector.go

@@ -919,8 +919,8 @@ var _ = SIGDescribe("Garbage collector", func() {
 				"kind":       definition.Spec.Names.Kind,
 				"metadata": map[string]interface{}{
 					"name": dependentName,
-					"ownerReferences": []map[string]string{
-						{
+					"ownerReferences": []interface{}{
+						map[string]interface{}{
 							"uid":        string(persistedOwner.GetUID()),
 							"apiVersion": apiVersion,
 							"kind":       definition.Spec.Names.Kind,