Merge pull request #107970 from liggitt/validations-round-trip

Fix serialization of x-kubernetes-validations OpenAPI extension

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
 )

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
+}

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)
+}

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 {

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"
 )
 

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"

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"
 )

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 {

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

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

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
 }

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()
+	seed := int64(time.Now().Nanosecond())
-	t.Logf("seed: %d", seed)
+	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))
 		}
 	}
 }