Merge pull request #51505 from atlassian/fix-unstructured-converter

Automatic merge from submit-queue

Fix pointer receivers handling in unstructured converter

**What this PR does / why we need it**:
Fixes unstructured converter to properly handle types that have `MarshalJSON()` implemented with a pointer receiver. In particular the converter now can roundtrip `*Unstructured` type.

**Which issue this PR fixes**:
Fixes #47889. Similar to #43346.

**Special notes for your reviewer**:
Without the fix the tests are failing:
```console
make test WHAT=./vendor/k8s.io/apimachinery/pkg/conversion/unstructured

Running tests for APIVersion: v1,admissionregistration.k8s.io/v1alpha1,admission.k8s.io/v1alpha1,apps/v1beta1,apps/v1beta2,authentication.k8s.io/v1,authentication.k8s.io/v1beta1,authorization.k8s.io/v1,authorization.k8s.io/v1beta1,autoscaling/v1,autoscaling/v2alpha1,batch/v1,batch/v1beta1,batch/v2alpha1,certificates.k8s.io/v1beta1,extensions/v1beta1,imagepolicy.k8s.io/v1alpha1,networking.k8s.io/v1,policy/v1beta1,rbac.authorization.k8s.io/v1,rbac.authorization.k8s.io/v1beta1,rbac.authorization.k8s.io/v1alpha1,scheduling.k8s.io/v1alpha1,settings.k8s.io/v1alpha1,storage.k8s.io/v1beta1,storage.k8s.io/v1,,federation/v1beta1
+++ [0829 16:39:36] Running tests without code coverage
--- FAIL: TestCustomToUnstructured (0.00s)
    --- FAIL: TestCustomToUnstructured/false (0.00s)
        Error Trace:    converter_test.go:500
        Error:          Not equal:
                        expected: bool(false)
                        actual: map[string]interface {}(map[string]interface {}{"data":"ZmFsc2U="})
        Messages:       customPointer1
    --- FAIL: TestCustomToUnstructured/[1] (0.00s)
        Error Trace:    converter_test.go:500
        Error:          Not equal:
                        expected: []interface {}([]interface {}{1})
                        actual: map[string]interface {}(map[string]interface {}{"data":"WzFd"})
        Messages:       customPointer1
    --- FAIL: TestCustomToUnstructured/true (0.00s)
        Error Trace:    converter_test.go:500
        Error:          Not equal:
                        expected: bool(true)
                        actual: map[string]interface {}(map[string]interface {}{"data":"dHJ1ZQ=="})
        Messages:       customPointer1
    --- FAIL: TestCustomToUnstructured/0 (0.00s)
        Error Trace:    converter_test.go:500
        Error:          Not equal:
                        expected: int64(0)
                        actual: map[string]interface {}(map[string]interface {}{"data":"MA=="})
        Messages:       customPointer1
    --- FAIL: TestCustomToUnstructured/null (0.00s)
        Error Trace:    converter_test.go:500
        Error:          Not equal:
                        expected: <nil>(<nil>)
                        actual: map[string]interface {}(map[string]interface {}{"data":"bnVsbA=="})
        Messages:       customPointer1
    --- FAIL: TestCustomToUnstructured/[] (0.00s)
        Error Trace:    converter_test.go:500
        Error:          Not equal:
                        expected: []interface {}([]interface {}{})
                        actual: map[string]interface {}(map[string]interface {}{"data":"W10="})
        Messages:       customPointer1
    --- FAIL: TestCustomToUnstructured/{"a":1} (0.00s)
        Error Trace:    converter_test.go:500
        Error:          Not equal:
                        expected: map[string]interface {}{"a":1}
                        actual: map[string]interface {}{"data":"eyJhIjoxfQ=="}

                        Diff:
                        --- Expected
                        +++ Actual
                        @@ -1,3 +1,3 @@
                         (map[string]interface {}) (len=1) {
                        - (string) (len=1) "a": (int64) 1
                        + (string) (len=4) "data": (string) (len=12) "eyJhIjoxfQ=="
                         }
        Messages:       customPointer1
    --- FAIL: TestCustomToUnstructured/0.0 (0.00s)
        Error Trace:    converter_test.go:500
        Error:          Not equal:
                        expected: float64(0)
                        actual: map[string]interface {}(map[string]interface {}{"data":"MC4w"})
        Messages:       customPointer1
    --- FAIL: TestCustomToUnstructured/{} (0.00s)
        Error Trace:    converter_test.go:500
        Error:          Not equal:
                        expected: map[string]interface {}{}
                        actual: map[string]interface {}{"data":"e30="}

                        Diff:
                        --- Expected
                        +++ Actual
                        @@ -1,2 +1,3 @@
                        -(map[string]interface {}) {
                        +(map[string]interface {}) (len=1) {
                        + (string) (len=4) "data": (string) (len=4) "e30="
                         }
        Messages:       customPointer1
--- FAIL: TestCustomToUnstructuredTopLevel (0.00s)
    --- FAIL: TestCustomToUnstructuredTopLevel/1 (0.00s)
        Error Trace:    converter_test.go:519
        Error:          Not equal:
                        expected: map[string]interface {}{"a":1}
                        actual: map[string]interface {}{"data":"eyJhIjoxfQ=="}

                        Diff:
                        --- Expected
                        +++ Actual
                        @@ -1,3 +1,3 @@
                         (map[string]interface {}) (len=1) {
                        - (string) (len=1) "a": (int64) 1
                        + (string) (len=4) "data": (string) (len=12) "eyJhIjoxfQ=="
                         }
FAIL
FAIL    k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/conversion/unstructured    0.047s
make: *** [test] Error 1
```

```console
make test WHAT=./vendor/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured

Running tests for APIVersion: v1,admissionregistration.k8s.io/v1alpha1,admission.k8s.io/v1alpha1,apps/v1beta1,apps/v1beta2,authentication.k8s.io/v1,authentication.k8s.io/v1beta1,authorization.k8s.io/v1,authorization.k8s.io/v1beta1,autoscaling/v1,autoscaling/v2alpha1,batch/v1,batch/v1beta1,batch/v2alpha1,certificates.k8s.io/v1beta1,extensions/v1beta1,imagepolicy.k8s.io/v1alpha1,networking.k8s.io/v1,policy/v1beta1,rbac.authorization.k8s.io/v1,rbac.authorization.k8s.io/v1beta1,rbac.authorization.k8s.io/v1alpha1,scheduling.k8s.io/v1alpha1,settings.k8s.io/v1alpha1,storage.k8s.io/v1beta1,storage.k8s.io/v1,,federation/v1beta1
+++ [0829 16:40:38] Running tests without code coverage
--- FAIL: TestConversionRoundtrip (0.00s)
    unstructured_test.go:111: FromUnstructured failed: Object 'Kind' is missing in '{"object":{"apiVersion":"v1","kind":"Foo","metadata":{"name":"foo1"}}}'
FAIL
FAIL    k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured  0.046s
make: *** [test] Error 1
```

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

staging/src/k8s.io/apimachinery/pkg/conversion/unstructured/BUILD

@@ -3,17 +3,6 @@ package(default_visibility = ["//visibility:public"])
 load(
     "@io_bazel_rules_go//go:def.bzl",
     "go_library",
-    "go_test",
-)
-
-go_test(
-    name = "go_default_test",
-    srcs = ["converter_test.go"],
-    library = ":go_default_library",
-    deps = [
-        "//vendor/k8s.io/apimachinery/pkg/util/diff:go_default_library",
-        "//vendor/k8s.io/apimachinery/pkg/util/json:go_default_library",
-    ],
 )
 
 go_library(
@@ -40,6 +29,9 @@ filegroup(
 
 filegroup(
     name = "all-srcs",
-    srcs = [":package-srcs"],
+    srcs = [
+        ":package-srcs",
+        "//staging/src/k8s.io/apimachinery/pkg/conversion/unstructured/testing:all-srcs",
+    ],
     tags = ["automanaged"],
 )

staging/src/k8s.io/apimachinery/pkg/conversion/unstructured/converter.go

@@ -426,17 +426,29 @@ var (
 	falseBytes = []byte("false")
 )
 
-func toUnstructured(sv, dv reflect.Value) error {
+func getMarshaler(v reflect.Value) (encodingjson.Marshaler, bool) {
-	st, dt := sv.Type(), dv.Type()
+	// Check value receivers if v is not a pointer and pointer receivers if v is a pointer
+	if v.Type().Implements(marshalerType) {
+		return v.Interface().(encodingjson.Marshaler), true
+	}
+	// Check pointer receivers if v is not a pointer
+	if v.Kind() != reflect.Ptr && v.CanAddr() {
+		v = v.Addr()
+		if v.Type().Implements(marshalerType) {
+			return v.Interface().(encodingjson.Marshaler), true
+		}
+	}
+	return nil, false
+}
 
+func toUnstructured(sv, dv reflect.Value) error {
 	// Check if the object has a custom JSON marshaller/unmarshaller.
-	if st.Implements(marshalerType) {
+	if marshaler, ok := getMarshaler(sv); ok {
 		if sv.Kind() == reflect.Ptr && sv.IsNil() {
 			// We're done - we don't need to store anything.
 			return nil
 		}
 
-		marshaler := sv.Interface().(encodingjson.Marshaler)
 		data, err := marshaler.MarshalJSON()
 		if err != nil {
 			return err
@@ -496,6 +508,7 @@ func toUnstructured(sv, dv reflect.Value) error {
 		return nil
 	}
 
+	st, dt := sv.Type(), dv.Type()
 	switch st.Kind() {
 	case reflect.String:
 		if dt.Kind() == reflect.Interface && dv.NumMethod() == 0 {

staging/src/k8s.io/apimachinery/pkg/conversion/unstructured/testing/BUILD

@@ -0,0 +1,28 @@
+load("@io_bazel_rules_go//go:def.bzl", "go_test")
+
+go_test(
+    name = "go_default_test",
+    srcs = ["converter_test.go"],
+    deps = [
+        "//vendor/github.com/stretchr/testify/assert:go_default_library",
+        "//vendor/github.com/stretchr/testify/require:go_default_library",
+        "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured:go_default_library",
+        "//vendor/k8s.io/apimachinery/pkg/conversion/unstructured:go_default_library",
+        "//vendor/k8s.io/apimachinery/pkg/util/diff:go_default_library",
+        "//vendor/k8s.io/apimachinery/pkg/util/json:go_default_library",
+    ],
+)
+
+filegroup(
+    name = "package-srcs",
+    srcs = glob(["**"]),
+    tags = ["automanaged"],
+    visibility = ["//visibility:private"],
+)
+
+filegroup(
+    name = "all-srcs",
+    srcs = [":package-srcs"],
+    tags = ["automanaged"],
+    visibility = ["//visibility:public"],
+)

staging/src/k8s.io/apimachinery/pkg/conversion/unstructured/testing/converter_test.go

@@ -14,15 +14,25 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-package unstructured
+// These tests are in a separate package to break cyclic dependency in tests.
+// Unstructured type depends on unstructured converter package but we want to test how the converter handles
+// the Unstructured type so we need to import both.
+
+package testing
 
 import (
 	"fmt"
 	"reflect"
+	"strconv"
 	"testing"
 
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	conversionunstructured "k8s.io/apimachinery/pkg/conversion/unstructured"
 	"k8s.io/apimachinery/pkg/util/diff"
 	"k8s.io/apimachinery/pkg/util/json"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 // Definte a number of test types.
@@ -72,14 +82,27 @@ type F struct {
 }
 
 type G struct {
-	Custom Custom `json:"custom"`
+	CustomValue1   CustomValue    `json:"customValue1"`
+	CustomValue2   *CustomValue   `json:"customValue2"`
+	CustomPointer1 CustomPointer  `json:"customPointer1"`
+	CustomPointer2 *CustomPointer `json:"customPointer2"`
 }
 
-type Custom struct {
+type CustomValue struct {
 	data []byte
 }
 
-func (c Custom) MarshalJSON() ([]byte, error) {
+// MarshalJSON has a value receiver on this type.
+func (c CustomValue) MarshalJSON() ([]byte, error) {
+	return c.data, nil
+}
+
+type CustomPointer struct {
+	data []byte
+}
+
+// MarshalJSON has a pointer receiver on this type.
+func (c *CustomPointer) MarshalJSON() ([]byte, error) {
 	return c.data, nil
 }
 
@@ -113,14 +136,14 @@ func doRoundTrip(t *testing.T, item interface{}) {
 		return
 	}
 
-	newUnstr, err := DefaultConverter.ToUnstructured(item)
+	newUnstr, err := conversionunstructured.DefaultConverter.ToUnstructured(item)
 	if err != nil {
 		t.Errorf("ToUnstructured failed: %v", err)
 		return
 	}
 
 	newObj := reflect.New(reflect.TypeOf(item).Elem()).Interface()
-	err = DefaultConverter.FromUnstructured(newUnstr, newObj)
+	err = conversionunstructured.DefaultConverter.FromUnstructured(newUnstr, newObj)
 	if err != nil {
 		t.Errorf("FromUnstructured failed: %v", err)
 		return
@@ -136,6 +159,17 @@ func TestRoundTrip(t *testing.T) {
 	testCases := []struct {
 		obj interface{}
 	}{
+		{
+			obj: &unstructured.Unstructured{
+				Object: map[string]interface{}{
+					"apiVersion": "v1",
+					"kind":       "Foo",
+					"metadata": map[string]interface{}{
+						"name": "foo1",
+					},
+				},
+			},
+		},
 		{
 			// This (among others) tests nil map, slice and pointer.
 			obj: &C{
@@ -230,7 +264,7 @@ func doUnrecognized(t *testing.T, jsonData string, item interface{}, expectedErr
 		return
 	}
 	newObj := reflect.New(reflect.TypeOf(item).Elem()).Interface()
-	err = DefaultConverter.FromUnstructured(unstr, newObj)
+	err = conversionunstructured.DefaultConverter.FromUnstructured(unstr, newObj)
 	if (err != nil) != (expectedErr != nil) {
 		t.Errorf("Unexpected error in FromUnstructured: %v, expected: %v", err, expectedErr)
 	}
@@ -434,7 +468,7 @@ func TestFloatIntConversion(t *testing.T) {
 	unstr := map[string]interface{}{"fd": float64(3)}
 
 	var obj F
-	if err := DefaultConverter.FromUnstructured(unstr, &obj); err != nil {
+	if err := conversionunstructured.DefaultConverter.FromUnstructured(unstr, &obj); err != nil {
 		t.Errorf("Unexpected error in FromUnstructured: %v", err)
 	}
 
@@ -469,18 +503,37 @@ func TestCustomToUnstructured(t *testing.T) {
 	}
 
 	for _, tc := range testcases {
-		result, err := DefaultConverter.ToUnstructured(&G{Custom: Custom{data: []byte(tc.Data)}})
+		tc := tc
-		if err != nil {
+		t.Run(tc.Data, func(t *testing.T) {
-			t.Errorf("%s: %v", tc.Data, err)
+			t.Parallel()
-			continue
+			result, err := conversionunstructured.DefaultConverter.ToUnstructured(&G{
-		}
+				CustomValue1:   CustomValue{data: []byte(tc.Data)},
-
+				CustomValue2:   &CustomValue{data: []byte(tc.Data)},
-		fieldResult := result["custom"]
+				CustomPointer1: CustomPointer{data: []byte(tc.Data)},
-		if !reflect.DeepEqual(fieldResult, tc.Expected) {
+				CustomPointer2: &CustomPointer{data: []byte(tc.Data)},
-			t.Errorf("%s: expected %v, got %v", tc.Data, tc.Expected, fieldResult)
+			})
-			// t.Log("expected", spew.Sdump(tc.Expected))
+			require.NoError(t, err)
-			// t.Log("actual", spew.Sdump(fieldResult))
+			for field, fieldResult := range result {
-			continue
+				assert.Equal(t, tc.Expected, fieldResult, field)
-		}
+			}
+		})
+	}
+}
+
+func TestCustomToUnstructuredTopLevel(t *testing.T) {
+	// Only objects are supported at the top level
+	topLevelCases := []interface{}{
+		&CustomValue{data: []byte(`{"a":1}`)},
+		&CustomPointer{data: []byte(`{"a":1}`)},
+	}
+	expected := map[string]interface{}{"a": int64(1)}
+	for i, obj := range topLevelCases {
+		obj := obj
+		t.Run(strconv.Itoa(i), func(t *testing.T) {
+			t.Parallel()
+			result, err := conversionunstructured.DefaultConverter.ToUnstructured(obj)
+			require.NoError(t, err)
+			assert.Equal(t, expected, result)
+		})
 	}
 }