From ba1ca0d4591f06f796ea2d93463a4c5caaa33cc7 Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Mon, 13 Sep 2021 11:49:17 -0400 Subject: [PATCH 1/2] Propagate conversion errors --- pkg/apis/extensions/v1beta1/conversion.go | 4 ++-- pkg/apis/networking/v1beta1/conversion.go | 4 ++-- .../pkg/apis/apiextensions/v1/conversion.go | 2 +- .../src/k8s.io/client-go/tools/clientcmd/api/v1/conversion.go | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/pkg/apis/extensions/v1beta1/conversion.go b/pkg/apis/extensions/v1beta1/conversion.go index 4a85d23d39c8..5ea528560563 100644 --- a/pkg/apis/extensions/v1beta1/conversion.go +++ b/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{} diff --git a/pkg/apis/networking/v1beta1/conversion.go b/pkg/apis/networking/v1beta1/conversion.go index 42df29a408d3..5f4553cfe05d 100644 --- a/pkg/apis/networking/v1beta1/conversion.go +++ b/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{} diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/conversion.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/conversion.go index c056dd91ffa9..596655055989 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/conversion.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/conversion.go @@ -103,7 +103,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 { diff --git a/staging/src/k8s.io/client-go/tools/clientcmd/api/v1/conversion.go b/staging/src/k8s.io/client-go/tools/clientcmd/api/v1/conversion.go index c38ebc076045..6eee281bc66c 100644 --- a/staging/src/k8s.io/client-go/tools/clientcmd/api/v1/conversion.go +++ b/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) From 89ae351af92cb6baec8845f19bc82313d9ba8f82 Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Mon, 13 Sep 2021 12:16:47 -0400 Subject: [PATCH 2/2] Fix null JSON round tripping --- .../pkg/apis/apiextensions/v1/conversion.go | 17 +++++- .../apis/apiextensions/v1/conversion_test.go | 55 +++++++++++++++++++ .../pkg/apis/apiextensions/v1/marshal.go | 3 +- .../apis/apiextensions/v1beta1/conversion.go | 17 +++++- .../apiextensions/v1beta1/conversion_test.go | 55 +++++++++++++++++++ .../pkg/apis/apiextensions/v1beta1/marshal.go | 3 +- 6 files changed, 142 insertions(+), 8 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/conversion.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/conversion.go index 596655055989..9bcbe50267bb 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/conversion.go +++ b/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 { diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/conversion_test.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/conversion_test.go index 63a501ae4577..884f2f0e65bf 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/conversion_test.go +++ b/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)) + } + }) + } +} diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/marshal.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/marshal.go index ba7f286eb4e2..12cc2f6f2c92 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/marshal.go +++ b/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 diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/conversion.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/conversion.go index e014ce62fd94..eed3fde63e15 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/conversion.go +++ b/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 { diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/conversion_test.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/conversion_test.go index a697dd9e3ac4..120b5c98db92 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/conversion_test.go +++ b/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)) + } + }) + } +} diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/marshal.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/marshal.go index 9a8fad3b7760..44941d82efff 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/marshal.go +++ b/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