Merge pull request #104969 from liggitt/crd-error-validation

Fix conversion of literal null JSON values

pkg/apis/extensions/v1beta1/conversion.go

@@ -189,7 +189,7 @@ func Convert_networking_IngressBackend_To_v1beta1_IngressBackend(in *networking.
 
 func Convert_v1beta1_IngressSpec_To_networking_IngressSpec(in *extensionsv1beta1.IngressSpec, out *networking.IngressSpec, s conversion.Scope) error {
 	if err := autoConvert_v1beta1_IngressSpec_To_networking_IngressSpec(in, out, s); err != nil {
-		return nil
+		return err
 	}
 	if in.Backend != nil {
 		out.DefaultBackend = &networking.IngressBackend{}
@@ -202,7 +202,7 @@ func Convert_v1beta1_IngressSpec_To_networking_IngressSpec(in *extensionsv1beta1
 
 func Convert_networking_IngressSpec_To_v1beta1_IngressSpec(in *networking.IngressSpec, out *extensionsv1beta1.IngressSpec, s conversion.Scope) error {
 	if err := autoConvert_networking_IngressSpec_To_v1beta1_IngressSpec(in, out, s); err != nil {
-		return nil
+		return err
 	}
 	if in.DefaultBackend != nil {
 		out.Backend = &extensionsv1beta1.IngressBackend{}

pkg/apis/networking/v1beta1/conversion.go

@@ -52,7 +52,7 @@ func Convert_networking_IngressBackend_To_v1beta1_IngressBackend(in *networking.
 }
 func Convert_v1beta1_IngressSpec_To_networking_IngressSpec(in *v1beta1.IngressSpec, out *networking.IngressSpec, s conversion.Scope) error {
 	if err := autoConvert_v1beta1_IngressSpec_To_networking_IngressSpec(in, out, s); err != nil {
-		return nil
+		return err
 	}
 	if in.Backend != nil {
 		out.DefaultBackend = &networking.IngressBackend{}
@@ -65,7 +65,7 @@ func Convert_v1beta1_IngressSpec_To_networking_IngressSpec(in *v1beta1.IngressSp
 
 func Convert_networking_IngressSpec_To_v1beta1_IngressSpec(in *networking.IngressSpec, out *v1beta1.IngressSpec, s conversion.Scope) error {
 	if err := autoConvert_networking_IngressSpec_To_v1beta1_IngressSpec(in, out, s); err != nil {
-		return nil
+		return err
 	}
 	if in.DefaultBackend != nil {
 		out.Backend = &v1beta1.IngressBackend{}

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/conversion.go

@@ -17,6 +17,8 @@ limitations under the License.
 package v1
 
 import (
+	"bytes"
+
 	"k8s.io/apiextensions-apiserver/pkg/apis/apiextensions"
 	apiequality "k8s.io/apimachinery/pkg/api/equality"
 	"k8s.io/apimachinery/pkg/conversion"
@@ -36,20 +38,29 @@ func Convert_apiextensions_JSONSchemaProps_To_v1_JSONSchemaProps(in *apiextensio
 	return nil
 }
 
+var nullLiteral = []byte(`null`)
+
 func Convert_apiextensions_JSON_To_v1_JSON(in *apiextensions.JSON, out *JSON, s conversion.Scope) error {
 	raw, err := json.Marshal(*in)
 	if err != nil {
 		return err
 	}
-	out.Raw = raw
+	if len(raw) == 0 || bytes.Equal(raw, nullLiteral) {
+		// match JSON#UnmarshalJSON treatment of literal nulls
+		out.Raw = nil
+	} else {
+		out.Raw = raw
+	}
 	return nil
 }
 
 func Convert_v1_JSON_To_apiextensions_JSON(in *JSON, out *apiextensions.JSON, s conversion.Scope) error {
 	if in != nil {
 		var i interface{}
-		if err := json.Unmarshal(in.Raw, &i); err != nil {
-			return err
+		if len(in.Raw) > 0 && !bytes.Equal(in.Raw, nullLiteral) {
+			if err := json.Unmarshal(in.Raw, &i); err != nil {
+				return err
+			}
 		}
 		*out = i
 	} else {
@@ -103,7 +114,7 @@ func Convert_apiextensions_CustomResourceDefinitionSpec_To_v1_CustomResourceDefi
 
 func Convert_v1_CustomResourceDefinitionSpec_To_apiextensions_CustomResourceDefinitionSpec(in *CustomResourceDefinitionSpec, out *apiextensions.CustomResourceDefinitionSpec, s conversion.Scope) error {
 	if err := autoConvert_v1_CustomResourceDefinitionSpec_To_apiextensions_CustomResourceDefinitionSpec(in, out, s); err != nil {
-		return nil
+		return err
 	}
 
 	if len(out.Versions) == 0 {

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/conversion_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package v1
 
 import (
+	"encoding/json"
 	"reflect"
 	"strings"
 	"testing"
@@ -605,3 +606,57 @@ func TestJSONConversion(t *testing.T) {
 		}
 	}
 }
+
+func TestJSONRoundTrip(t *testing.T) {
+	testcases := []struct {
+		name string
+		in   string
+		out  string
+	}{
+		{
+			name: "nulls",
+			in:   `{"default":null,"enum":null,"example":null}`,
+			out:  `{}`,
+		},
+		{
+			name: "null values",
+			in:   `{"default":{"test":null},"enum":[null],"example":{"test":null}}`,
+			out:  `{"default":{"test":null},"enum":[null],"example":{"test":null}}`,
+		},
+	}
+
+	scheme := runtime.NewScheme()
+	// add internal and external types
+	if err := apiextensions.AddToScheme(scheme); err != nil {
+		t.Fatal(err)
+	}
+	if err := AddToScheme(scheme); err != nil {
+		t.Fatal(err)
+	}
+
+	for _, tc := range testcases {
+		t.Run(tc.name, func(t *testing.T) {
+			external := &JSONSchemaProps{}
+			if err := json.Unmarshal([]byte(tc.in), external); err != nil {
+				t.Fatal(err)
+			}
+
+			internal := &apiextensions.JSONSchemaProps{}
+			if err := scheme.Convert(external, internal, nil); err != nil {
+				t.Fatalf("unexpected error: %v", err)
+			}
+			roundtripped := &JSONSchemaProps{}
+			if err := scheme.Convert(internal, roundtripped, nil); err != nil {
+				t.Fatalf("unexpected error: %v", err)
+			}
+
+			out, err := json.Marshal(roundtripped)
+			if err != nil {
+				t.Fatal(err)
+			}
+			if string(out) != string(tc.out) {
+				t.Fatalf("expected\n%s\ngot\n%s", string(tc.out), string(out))
+			}
+		})
+	}
+}

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/marshal.go

@@ -17,6 +17,7 @@ limitations under the License.
 package v1
 
 import (
+	"bytes"
 	"errors"
 
 	"k8s.io/apimachinery/pkg/util/json"
@@ -128,7 +129,7 @@ func (s JSON) MarshalJSON() ([]byte, error) {
 }
 
 func (s *JSON) UnmarshalJSON(data []byte) error {
-	if len(data) > 0 && string(data) != "null" {
+	if len(data) > 0 && !bytes.Equal(data, nullLiteral) {
 		s.Raw = data
 	}
 	return nil

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/conversion.go

@@ -17,6 +17,8 @@ limitations under the License.
 package v1beta1
 
 import (
+	"bytes"
+
 	"k8s.io/apimachinery/pkg/conversion"
 	"k8s.io/apimachinery/pkg/util/json"
 
@@ -36,20 +38,29 @@ func Convert_apiextensions_JSONSchemaProps_To_v1beta1_JSONSchemaProps(in *apiext
 	return nil
 }
 
+var nullLiteral = []byte(`null`)
+
 func Convert_apiextensions_JSON_To_v1beta1_JSON(in *apiextensions.JSON, out *JSON, s conversion.Scope) error {
 	raw, err := json.Marshal(*in)
 	if err != nil {
 		return err
 	}
-	out.Raw = raw
+	if len(raw) == 0 || bytes.Equal(raw, nullLiteral) {
+		// match JSON#UnmarshalJSON treatment of literal nulls
+		out.Raw = nil
+	} else {
+		out.Raw = raw
+	}
 	return nil
 }
 
 func Convert_v1beta1_JSON_To_apiextensions_JSON(in *JSON, out *apiextensions.JSON, s conversion.Scope) error {
 	if in != nil {
 		var i interface{}
-		if err := json.Unmarshal(in.Raw, &i); err != nil {
-			return err
+		if len(in.Raw) > 0 && !bytes.Equal(in.Raw, nullLiteral) {
+			if err := json.Unmarshal(in.Raw, &i); err != nil {
+				return err
+			}
 		}
 		*out = i
 	} else {

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/conversion_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package v1beta1
 
 import (
+	"encoding/json"
 	"reflect"
 	"testing"
 
@@ -111,3 +112,57 @@ func TestJSONConversion(t *testing.T) {
 		}
 	}
 }
+
+func TestJSONRoundTrip(t *testing.T) {
+	testcases := []struct {
+		name string
+		in   string
+		out  string
+	}{
+		{
+			name: "nulls",
+			in:   `{"default":null,"enum":null,"example":null}`,
+			out:  `{}`,
+		},
+		{
+			name: "null values",
+			in:   `{"default":{"test":null},"enum":[null],"example":{"test":null}}`,
+			out:  `{"default":{"test":null},"enum":[null],"example":{"test":null}}`,
+		},
+	}
+
+	scheme := runtime.NewScheme()
+	// add internal and external types
+	if err := apiextensions.AddToScheme(scheme); err != nil {
+		t.Fatal(err)
+	}
+	if err := AddToScheme(scheme); err != nil {
+		t.Fatal(err)
+	}
+
+	for _, tc := range testcases {
+		t.Run(tc.name, func(t *testing.T) {
+			external := &JSONSchemaProps{}
+			if err := json.Unmarshal([]byte(tc.in), external); err != nil {
+				t.Fatal(err)
+			}
+
+			internal := &apiextensions.JSONSchemaProps{}
+			if err := scheme.Convert(external, internal, nil); err != nil {
+				t.Fatalf("unexpected error: %v", err)
+			}
+			roundtripped := &JSONSchemaProps{}
+			if err := scheme.Convert(internal, roundtripped, nil); err != nil {
+				t.Fatalf("unexpected error: %v", err)
+			}
+
+			out, err := json.Marshal(roundtripped)
+			if err != nil {
+				t.Fatal(err)
+			}
+			if string(out) != string(tc.out) {
+				t.Fatalf("expected\n%s\ngot\n%s", string(tc.out), string(out))
+			}
+		})
+	}
+}

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/marshal.go

@@ -17,6 +17,7 @@ limitations under the License.
 package v1beta1
 
 import (
+	"bytes"
 	"errors"
 
 	"k8s.io/apimachinery/pkg/util/json"
@@ -128,7 +129,7 @@ func (s JSON) MarshalJSON() ([]byte, error) {
 }
 
 func (s *JSON) UnmarshalJSON(data []byte) error {
-	if len(data) > 0 && string(data) != "null" {
+	if len(data) > 0 && !bytes.Equal(data, nullLiteral) {
 		s.Raw = data
 	}
 	return nil

staging/src/k8s.io/client-go/tools/clientcmd/api/v1/conversion.go

@@ -165,7 +165,7 @@ func Convert_Map_string_To_runtime_Object_To_Slice_v1_NamedExtension(in *map[str
 		newExtension := (*in)[key]
 		oldExtension := runtime.RawExtension{}
 		if err := runtime.Convert_runtime_Object_To_runtime_RawExtension(&newExtension, &oldExtension, s); err != nil {
-			return nil
+			return err
 		}
 		namedExtension := NamedExtension{key, oldExtension}
 		*out = append(*out, namedExtension)