From 064763ebddf2ee2b9eb33f6fccf70fc025ce2665 Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Sat, 5 Feb 2022 11:22:42 -0500 Subject: [PATCH 1/2] Make JSON schema round tripping test more strict --- .../src/k8s.io/apiextensions-apiserver/go.mod | 1 + .../apiserver/validation/validation_test.go | 33 +++++++++++++++---- 2 files changed, 28 insertions(+), 6 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/go.mod b/staging/src/k8s.io/apiextensions-apiserver/go.mod index b81adf5b3ba8..ec5fbe0967aa 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/go.mod +++ b/staging/src/k8s.io/apiextensions-apiserver/go.mod @@ -30,6 +30,7 @@ require ( k8s.io/klog/v2 v2.40.1 k8s.io/kube-openapi v0.0.0-20211115234752-e816edb12b65 k8s.io/utils v0.0.0-20211208161948-7d6a63dca704 + sigs.k8s.io/json v0.0.0-20211208200746-9f7c6b3444d2 sigs.k8s.io/structured-merge-diff/v4 v4.2.1 sigs.k8s.io/yaml v1.2.0 ) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/validation_test.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/validation_test.go index 2d71ead9e985..eadddc1113c6 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/validation_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/validation_test.go @@ -18,7 +18,14 @@ package validation import ( "math/rand" + "os" + "strconv" "testing" + "time" + + "github.com/google/go-cmp/cmp" + + kjson "sigs.k8s.io/json" "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions" apiextensionsfuzzer "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/fuzzer" @@ -46,12 +53,21 @@ func TestRoundTrip(t *testing.T) { t.Fatal(err) } - seed := rand.Int63() - t.Logf("seed: %d", seed) + seed := int64(time.Now().Nanosecond()) + if override := os.Getenv("TEST_RAND_SEED"); len(override) > 0 { + overrideSeed, err := strconv.Atoi(override) + if err != nil { + t.Fatal(err) + } + seed = int64(overrideSeed) + t.Logf("using overridden seed: %d", seed) + } else { + t.Logf("seed (override with TEST_RAND_SEED if desired): %d", seed) + } fuzzerFuncs := fuzzer.MergeFuzzerFuncs(apiextensionsfuzzer.Funcs) f := fuzzer.FuzzerFor(fuzzerFuncs, rand.NewSource(seed), codecs) - for i := 0; i < 20; i++ { + for i := 0; i < 50; i++ { // fuzz internal types internal := &apiextensions.JSONSchemaProps{} f.Fuzz(internal) @@ -70,8 +86,10 @@ func TestRoundTrip(t *testing.T) { // JSON -> in-memory JSON => convertNullTypeToNullable => JSON var j interface{} - if err := json.Unmarshal(openAPIJSON, &j); err != nil { + if strictErrs, err := kjson.UnmarshalStrict(openAPIJSON, &j); err != nil { t.Fatal(err) + } else if len(strictErrs) > 0 { + t.Fatal(strictErrs) } j = stripIntOrStringType(j) openAPIJSON, err = json.Marshal(j) @@ -81,8 +99,10 @@ func TestRoundTrip(t *testing.T) { // JSON -> external external := &apiextensionsv1.JSONSchemaProps{} - if err := json.Unmarshal(openAPIJSON, external); err != nil { + if strictErrs, err := kjson.UnmarshalStrict(openAPIJSON, external); err != nil { t.Fatal(err) + } else if len(strictErrs) > 0 { + t.Fatal(strictErrs) } // external -> internal @@ -92,7 +112,8 @@ func TestRoundTrip(t *testing.T) { } if !apiequality.Semantic.DeepEqual(internal, internalRoundTripped) { - t.Fatalf("%d: expected\n\t%#v, got \n\t%#v", i, internal, internalRoundTripped) + t.Log(string(openAPIJSON)) + t.Fatalf("%d: unexpected diff\n\t%s", i, cmp.Diff(internal, internalRoundTripped)) } } } From 5efe1e648b097cfb1dd78e9a96384464bba9d277 Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Sat, 5 Feb 2022 17:58:07 -0500 Subject: [PATCH 2/2] Use serializable struct for x-kubernetes-validations in openapi --- .../pkg/apis/apiextensions/v1/conversion.go | 6 ++ .../apis/apiextensions/v1/conversion_test.go | 69 +++++++++++++++++++ .../v1/zz_generated.conversion.go | 5 ++ .../apiserver/schema/cel/compilation_test.go | 2 +- .../pkg/apiserver/schema/cel/validation.go | 2 +- .../apiserver/schema/cel/validation_test.go | 2 +- .../pkg/apiserver/schema/convert.go | 5 +- .../pkg/apiserver/schema/structural.go | 5 +- .../apiserver/schema/zz_generated.deepcopy.go | 4 +- .../pkg/apiserver/validation/validation.go | 7 +- 10 files changed, 98 insertions(+), 9 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 9bcbe50267bb..4d29ff8235d8 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 @@ -18,6 +18,7 @@ package v1 import ( "bytes" + unsafe "unsafe" "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions" apiequality "k8s.io/apimachinery/pkg/api/equality" @@ -207,3 +208,8 @@ func Convert_apiextensions_CustomResourceConversion_To_v1_CustomResourceConversi } return nil } + +func Convert_apiextensions_ValidationRules_To_v1_ValidationRules(in *apiextensions.ValidationRules, out *ValidationRules, s conversion.Scope) error { + *out = *(*ValidationRules)(unsafe.Pointer(in)) + return nil +} 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 884f2f0e65bf..677cffd65b28 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 @@ -18,6 +18,7 @@ package v1 import ( "encoding/json" + fmt "fmt" "reflect" "strings" "testing" @@ -660,3 +661,71 @@ func TestJSONRoundTrip(t *testing.T) { }) } } + +func TestMemoryEqual(t *testing.T) { + testcases := []struct { + a interface{} + b interface{} + }{ + {apiextensions.JSONSchemaProps{}.XValidations, JSONSchemaProps{}.XValidations}, + } + + for _, tc := range testcases { + aType := reflect.TypeOf(tc.a) + bType := reflect.TypeOf(tc.b) + t.Run(aType.String(), func(t *testing.T) { + assertEqualTypes(t, nil, aType, bType) + }) + } +} + +func assertEqualTypes(t *testing.T, path []string, a, b reflect.Type) { + if a == b { + return + } + + if a.Kind() != b.Kind() { + fatalTypeError(t, path, a, b, "mismatched Kind") + } + + switch a.Kind() { + case reflect.Struct: + aFields := a.NumField() + bFields := b.NumField() + if aFields != bFields { + fatalTypeError(t, path, a, b, "mismatched field count") + } + for i := 0; i < aFields; i++ { + aField := a.Field(i) + bField := b.Field(i) + if aField.Name != bField.Name { + fatalTypeError(t, path, a, b, fmt.Sprintf("mismatched field name %d: %s %s", i, aField.Name, bField.Name)) + } + if aField.Offset != bField.Offset { + fatalTypeError(t, path, a, b, fmt.Sprintf("mismatched field offset %d: %v %v", i, aField.Offset, bField.Offset)) + } + if aField.Anonymous != bField.Anonymous { + fatalTypeError(t, path, a, b, fmt.Sprintf("mismatched field anonymous %d: %v %v", i, aField.Anonymous, bField.Anonymous)) + } + if !reflect.DeepEqual(aField.Index, bField.Index) { + fatalTypeError(t, path, a, b, fmt.Sprintf("mismatched field index %d: %v %v", i, aField.Index, bField.Index)) + } + path = append(path, aField.Name) + assertEqualTypes(t, path, aField.Type, bField.Type) + path = path[:len(path)-1] + } + + case reflect.Ptr, reflect.Slice: + aElemType := a.Elem() + bElemType := b.Elem() + assertEqualTypes(t, path, aElemType, bElemType) + + default: + fatalTypeError(t, path, a, b, "unhandled kind") + } +} + +func fatalTypeError(t *testing.T, path []string, a, b reflect.Type, message string) { + t.Helper() + t.Fatalf("%s: %s: %s %s", strings.Join(path, "."), message, a, b) +} diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/zz_generated.conversion.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/zz_generated.conversion.go index 77d22c16cec4..95a58529b117 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/zz_generated.conversion.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/zz_generated.conversion.go @@ -242,6 +242,11 @@ func RegisterConversions(s *runtime.Scheme) error { }); err != nil { return err } + if err := s.AddConversionFunc((*apiextensions.ValidationRules)(nil), (*ValidationRules)(nil), func(a, b interface{}, scope conversion.Scope) error { + return Convert_apiextensions_ValidationRules_To_v1_ValidationRules(a.(*apiextensions.ValidationRules), b.(*ValidationRules), scope) + }); err != nil { + return err + } if err := s.AddConversionFunc((*CustomResourceConversion)(nil), (*apiextensions.CustomResourceConversion)(nil), func(a, b interface{}, scope conversion.Scope) error { return Convert_v1_CustomResourceConversion_To_apiextensions_CustomResourceConversion(a.(*CustomResourceConversion), b.(*apiextensions.CustomResourceConversion), scope) }); err != nil { diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/compilation_test.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/compilation_test.go index cfda9b25b78c..f9118ea7a017 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/compilation_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/compilation_test.go @@ -20,7 +20,7 @@ import ( "strings" "testing" - "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions" + apiextensions "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" "k8s.io/apiextensions-apiserver/pkg/apiserver/schema" ) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation.go index acaa7478bd41..02aa271f7cc0 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation.go @@ -24,7 +24,7 @@ import ( "github.com/google/cel-go/common/types/ref" "github.com/google/cel-go/interpreter" - "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions" + apiextensions "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" "k8s.io/apiextensions-apiserver/pkg/apiserver/schema" "k8s.io/apiextensions-apiserver/third_party/forked/celopenapi/model" "k8s.io/apimachinery/pkg/util/validation/field" diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation_test.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation_test.go index 33c8c72d001e..a322756e243c 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation_test.go @@ -22,7 +22,7 @@ import ( "strings" "testing" - "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions" + apiextensions "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" "k8s.io/apiextensions-apiserver/pkg/apiserver/schema" "k8s.io/apimachinery/pkg/util/validation/field" ) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/convert.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/convert.go index 4b6b1eedf61f..9ec23b3322de 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/convert.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/convert.go @@ -19,6 +19,7 @@ package schema import ( "fmt" "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions" + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" ) // NewStructural converts an OpenAPI v3 schema into a structural schema. A pre-validated JSONSchemaProps will @@ -246,7 +247,9 @@ func newExtensions(s *apiextensions.JSONSchemaProps) (*Extensions, error) { XListMapKeys: s.XListMapKeys, XListType: s.XListType, XMapType: s.XMapType, - XValidations: s.XValidations, + } + if err := apiextensionsv1.Convert_apiextensions_ValidationRules_To_v1_ValidationRules(&s.XValidations, &ret.XValidations, nil); err != nil { + return nil, err } if s.XPreserveUnknownFields != nil { diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/structural.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/structural.go index 1948b7ba47a9..234998d904a2 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/structural.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/structural.go @@ -17,7 +17,7 @@ limitations under the License. package schema import ( - "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions" + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" "k8s.io/apimachinery/pkg/runtime" ) @@ -130,7 +130,8 @@ type Extensions struct { XMapType *string // x-kubernetes-validations describes a list of validation rules for expression validation. - XValidations apiextensions.ValidationRules + // Use the v1 struct since this gets serialized as an extension. + XValidations apiextensionsv1.ValidationRules } // +k8s:deepcopy-gen=true diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/zz_generated.deepcopy.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/zz_generated.deepcopy.go index 3ce631066b47..271b7b03884d 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/zz_generated.deepcopy.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/zz_generated.deepcopy.go @@ -22,7 +22,7 @@ limitations under the License. package schema import ( - apiextensions "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions" + v1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" ) // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. @@ -45,7 +45,7 @@ func (in *Extensions) DeepCopyInto(out *Extensions) { } if in.XValidations != nil { in, out := &in.XValidations, &out.XValidations - *out = make(apiextensions.ValidationRules, len(*in)) + *out = make(v1.ValidationRules, len(*in)) copy(*out, *in) } return diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/validation.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/validation.go index 14081c94461e..b325abf9ac66 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/validation.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/validation.go @@ -21,6 +21,7 @@ import ( "strings" "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions" + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" "k8s.io/apimachinery/pkg/util/validation/field" openapierrors "k8s.io/kube-openapi/pkg/validation/errors" "k8s.io/kube-openapi/pkg/validation/spec" @@ -255,7 +256,11 @@ func ConvertJSONSchemaPropsWithPostProcess(in *apiextensions.JSONSchemaProps, ou out.VendorExtensible.AddExtension("x-kubernetes-map-type", *in.XMapType) } if len(in.XValidations) != 0 { - out.VendorExtensible.AddExtension("x-kubernetes-validations", in.XValidations) + var serializationValidationRules apiextensionsv1.ValidationRules + if err := apiextensionsv1.Convert_apiextensions_ValidationRules_To_v1_ValidationRules(&in.XValidations, &serializationValidationRules, nil); err != nil { + return err + } + out.VendorExtensible.AddExtension("x-kubernetes-validations", serializationValidationRules) } return nil }