Merge pull request #74804 from sttts/sttts-crd-validation-nullable

api/openapi-spec/swagger.json

@@ -16268,6 +16268,9 @@
         "not": {
           "$ref": "#/definitions/io.k8s.apiextensions-apiserver.pkg.apis.apiextensions.v1beta1.JSONSchemaProps"
         },
+        "nullable": {
+          "type": "boolean"
+        },
         "oneOf": {
           "items": {
             "$ref": "#/definitions/io.k8s.apiextensions-apiserver.pkg.apis.apiextensions.v1beta1.JSONSchemaProps"

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

@@ -20,7 +20,7 @@ import (
 	"reflect"
 	"strings"
 
-	"github.com/google/gofuzz"
+	fuzz "github.com/google/gofuzz"
 
 	"k8s.io/apiextensions-apiserver/pkg/apis/apiextensions"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -113,6 +113,9 @@ func Funcs(codecs runtimeserializer.CodecFactory) []interface{} {
 				validRef := "validRef"
 				obj.Ref = &validRef
 			}
+			if len(obj.Type) == 0 {
+				obj.Nullable = false // because this does not roundtrip through go-openapi
+			}
 		},
 		func(obj *apiextensions.JSONSchemaPropsOrBool, c fuzz.Continue) {
 			if c.RandBool() {
@@ -143,5 +146,9 @@ func Funcs(codecs runtimeserializer.CodecFactory) []interface{} {
 				c.Fuzz(&obj.Property)
 			}
 		},
+		func(obj *int64, c fuzz.Continue) {
+			// JSON only supports 53 bits because everything is a float
+			*obj = int64(c.Uint64()) & ((int64(1) << 53) - 1)
+		},
 	}
 }

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

@@ -23,6 +23,7 @@ type JSONSchemaProps struct {
 	Ref                  *string
 	Description          string
 	Type                 string
+	Nullable             bool
 	Format               string
 	Title                string
 	Default              *JSON

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

@@ -54,6 +54,7 @@ type JSONSchemaProps struct {
 	Definitions          JSONSchemaDefinitions      `json:"definitions,omitempty" protobuf:"bytes,34,opt,name=definitions"`
 	ExternalDocs         *ExternalDocumentation     `json:"externalDocs,omitempty" protobuf:"bytes,35,opt,name=externalDocs"`
 	Example              *JSON                      `json:"example,omitempty" protobuf:"bytes,36,opt,name=example"`
+	Nullable             bool                       `json:"nullable,omitempty" protobuf:"bytes,37,opt,name=nullable"`
 }
 
 // JSON represents any valid JSON value.

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

@@ -902,6 +902,7 @@ func autoConvert_v1beta1_JSONSchemaProps_To_apiextensions_JSONSchemaProps(in *JS
 	} else {
 		out.Example = nil
 	}
+	out.Nullable = in.Nullable
 	return nil
 }
 
@@ -916,6 +917,7 @@ func autoConvert_apiextensions_JSONSchemaProps_To_v1beta1_JSONSchemaProps(in *ap
 	out.Ref = (*string)(unsafe.Pointer(in.Ref))
 	out.Description = in.Description
 	out.Type = in.Type
+	out.Nullable = in.Nullable
 	out.Format = in.Format
 	out.Title = in.Title
 	if in.Default != nil {

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

@@ -497,6 +497,10 @@ func ValidateCustomResourceDefinitionValidation(customResourceValidation *apiext
 			}
 		}
 
+		if schema.Nullable {
+			allErrs = append(allErrs, field.Forbidden(fldPath.Child("openAPIV3Schema.nullable"), fmt.Sprintf(`nullable cannot be true at the root`)))
+		}
+
 		openAPIV3Schema := &specStandardValidatorV3{}
 		allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(schema, fldPath.Child("openAPIV3Schema"), openAPIV3Schema)...)
 	}
@@ -641,7 +645,7 @@ func (v *specStandardValidatorV3) validate(schema *apiextensions.JSONSchemaProps
 	}
 
 	if schema.Type == "null" {
-		allErrs = append(allErrs, field.Forbidden(fldPath.Child("type"), "type cannot be set to null"))
+		allErrs = append(allErrs, field.Forbidden(fldPath.Child("type"), "type cannot be set to null, use nullable as an alternative"))
 	}
 
 	if schema.Items != nil && len(schema.Items.JSONSchemas) != 0 {

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation_test.go

@@ -1225,6 +1225,68 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) {
 			statusEnabled: true,
 			wantError:     false,
 		},
+		{
+			name: "null type",
+			input: apiextensions.CustomResourceValidation{
+				OpenAPIV3Schema: &apiextensions.JSONSchemaProps{
+					Properties: map[string]apiextensions.JSONSchemaProps{
+						"null": {
+							Type: "null",
+						},
+					},
+				},
+			},
+			wantError: true,
+		},
+		{
+			name: "nullable at the root",
+			input: apiextensions.CustomResourceValidation{
+				OpenAPIV3Schema: &apiextensions.JSONSchemaProps{
+					Type:     "object",
+					Nullable: true,
+				},
+			},
+			wantError: true,
+		},
+		{
+			name: "nullable without type",
+			input: apiextensions.CustomResourceValidation{
+				OpenAPIV3Schema: &apiextensions.JSONSchemaProps{
+					Properties: map[string]apiextensions.JSONSchemaProps{
+						"nullable": {
+							Nullable: true,
+						},
+					},
+				},
+			},
+			wantError: false,
+		},
+		{
+			name: "nullable with types",
+			input: apiextensions.CustomResourceValidation{
+				OpenAPIV3Schema: &apiextensions.JSONSchemaProps{
+					Properties: map[string]apiextensions.JSONSchemaProps{
+						"object": {
+							Type:     "object",
+							Nullable: true,
+						},
+						"array": {
+							Type:     "array",
+							Nullable: true,
+						},
+						"number": {
+							Type:     "number",
+							Nullable: true,
+						},
+						"string": {
+							Type:     "string",
+							Nullable: true,
+						},
+					},
+				},
+			},
+			wantError: false,
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/validation.go

@@ -70,6 +70,9 @@ func ConvertJSONSchemaPropsWithPostProcess(in *apiextensions.JSONSchemaProps, ou
 	out.Description = in.Description
 	if in.Type != "" {
 		out.Type = spec.StringOrArray([]string{in.Type})
+		if in.Nullable {
+			out.Type = append(out.Type, "null")
+		}
 	}
 	out.Format = in.Format
 	out.Title = in.Title

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/validation_test.go

@@ -21,16 +21,14 @@ import (
 	"testing"
 
 	"github.com/go-openapi/spec"
-
+	"k8s.io/apiextensions-apiserver/pkg/apis/apiextensions"
+	apiextensionsfuzzer "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/fuzzer"
+	apiextensionsv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
 	"k8s.io/apimachinery/pkg/api/apitesting/fuzzer"
 	apiequality "k8s.io/apimachinery/pkg/api/equality"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/serializer"
 	"k8s.io/apimachinery/pkg/util/json"
-
-	"k8s.io/apiextensions-apiserver/pkg/apis/apiextensions"
-	apiextensionsfuzzer "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/fuzzer"
-	apiextensionsv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
 )
 
 // TestRoundTrip checks the conversion to go-openapi types.
@@ -48,6 +46,7 @@ func TestRoundTrip(t *testing.T) {
 	}
 
 	seed := rand.Int63()
+	t.Logf("seed: %d", seed)
 	fuzzerFuncs := fuzzer.MergeFuzzerFuncs(apiextensionsfuzzer.Funcs)
 	f := fuzzer.FuzzerFor(fuzzerFuncs, rand.NewSource(seed), codecs)
 
@@ -68,6 +67,17 @@ func TestRoundTrip(t *testing.T) {
 			t.Fatal(err)
 		}
 
+		// JSON -> in-memory JSON => convertNullTypeToNullable => JSON
+		var j interface{}
+		if err := json.Unmarshal(openAPIJSON, &j); err != nil {
+			t.Fatal(err)
+		}
+		j = convertNullTypeToNullable(j)
+		openAPIJSON, err = json.Marshal(j)
+		if err != nil {
+			t.Fatal(err)
+		}
+
 		// JSON -> external
 		external := &apiextensionsv1beta1.JSONSchemaProps{}
 		if err := json.Unmarshal(openAPIJSON, external); err != nil {
@@ -81,7 +91,164 @@ func TestRoundTrip(t *testing.T) {
 		}
 
 		if !apiequality.Semantic.DeepEqual(internal, internalRoundTripped) {
-			t.Fatalf("expected\n\t%#v, got \n\t%#v", internal, internalRoundTripped)
+			t.Fatalf("%d: expected\n\t%#v, got \n\t%#v", i, internal, internalRoundTripped)
+		}
+	}
+}
+
+func convertNullTypeToNullable(x interface{}) interface{} {
+	switch x := x.(type) {
+	case map[string]interface{}:
+		if t, found := x["type"]; found {
+			switch t := t.(type) {
+			case []interface{}:
+				for i, typ := range t {
+					if s, ok := typ.(string); !ok || s != "null" {
+						continue
+					}
+					t = append(t[:i], t[i+1:]...)
+					switch len(t) {
+					case 0:
+						delete(x, "type")
+					case 1:
+						x["type"] = t[0]
+					default:
+						x["type"] = t
+					}
+					x["nullable"] = true
+					break
+				}
+			case string:
+				if t == "null" {
+					delete(x, "type")
+					x["nullable"] = true
+				}
+			}
+		}
+		for k := range x {
+			x[k] = convertNullTypeToNullable(x[k])
 		}
+		return x
+	case []interface{}:
+		for i := range x {
+			x[i] = convertNullTypeToNullable(x[i])
+		}
+		return x
+	default:
+		return x
+	}
+}
+
+func TestNullable(t *testing.T) {
+	type args struct {
+		schema apiextensions.JSONSchemaProps
+		object interface{}
+	}
+	tests := []struct {
+		name    string
+		args    args
+		wantErr bool
+	}{
+		{"!nullable against non-null", args{
+			apiextensions.JSONSchemaProps{
+				Properties: map[string]apiextensions.JSONSchemaProps{
+					"field": {
+						Type:     "object",
+						Nullable: false,
+					},
+				},
+			},
+			map[string]interface{}{"field": map[string]interface{}{}},
+		}, false},
+		{"!nullable against null", args{
+			apiextensions.JSONSchemaProps{
+				Properties: map[string]apiextensions.JSONSchemaProps{
+					"field": {
+						Type:     "object",
+						Nullable: false,
+					},
+				},
+			},
+			map[string]interface{}{"field": nil},
+		}, true},
+		{"!nullable against undefined", args{
+			apiextensions.JSONSchemaProps{
+				Properties: map[string]apiextensions.JSONSchemaProps{
+					"field": {
+						Type:     "object",
+						Nullable: false,
+					},
+				},
+			},
+			map[string]interface{}{},
+		}, false},
+		{"nullable against non-null", args{
+			apiextensions.JSONSchemaProps{
+				Properties: map[string]apiextensions.JSONSchemaProps{
+					"field": {
+						Type:     "object",
+						Nullable: true,
+					},
+				},
+			},
+			map[string]interface{}{"field": map[string]interface{}{}},
+		}, false},
+		{"nullable against null", args{
+			apiextensions.JSONSchemaProps{
+				Properties: map[string]apiextensions.JSONSchemaProps{
+					"field": {
+						Type:     "object",
+						Nullable: true,
+					},
+				},
+			},
+			map[string]interface{}{"field": nil},
+		}, false},
+		{"!nullable against undefined", args{
+			apiextensions.JSONSchemaProps{
+				Properties: map[string]apiextensions.JSONSchemaProps{
+					"field": {
+						Type:     "object",
+						Nullable: true,
+					},
+				},
+			},
+			map[string]interface{}{},
+		}, false},
+		{"nullable and no type against non-nil", args{
+			apiextensions.JSONSchemaProps{
+				Properties: map[string]apiextensions.JSONSchemaProps{
+					"field": {
+						Nullable: true,
+					},
+				},
+			},
+			map[string]interface{}{"field": 42},
+		}, false},
+		{"nullable and no type against nil", args{
+			apiextensions.JSONSchemaProps{
+				Properties: map[string]apiextensions.JSONSchemaProps{
+					"field": {
+						Nullable: true,
+					},
+				},
+			},
+			map[string]interface{}{"field": nil},
+		}, false},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			validator, _, err := NewSchemaValidator(&apiextensions.CustomResourceValidation{OpenAPIV3Schema: &tt.args.schema})
+			if err != nil {
+				t.Fatal(err)
+			}
+			if err := ValidateCustomResource(tt.args.object, validator); (err != nil) != tt.wantErr {
+				if err == nil {
+					t.Error("expected error, but didn't get one")
+				} else {
+					t.Errorf("unexpected validation error: %v", err)
+				}
+			}
+		})
 	}
 }

staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/conversion.go

@@ -62,12 +62,11 @@ func ConvertJSONSchemaPropsToOpenAPIv2Schema(in *apiextensions.JSONSchemaProps)
 			}
 		}
 
-		if len(p.Type) > 1 {
-			// https://github.com/kubernetes/kube-openapi/pull/143/files#diff-62afddb578e9db18fb32ffb6b7802d92R272
-			// We also set Properties to null to enforce parseArbitrary at https://github.com/kubernetes/kube-openapi/blob/814a8073653e40e0e324205d093770d4e7bb811f/pkg/util/proto/document.go#L247
+		switch {
+		case len(p.Type) == 2 && (p.Type[0] == "null" || p.Type[1] == "null"):
+			// https://github.com/kubernetes/kube-openapi/pull/143/files#diff-ce77fea74b9dd098045004410023e0c3R219
 			p.Type = nil
-			p.Properties = nil
-		} else if len(p.Type) == 1 {
+		case len(p.Type) == 1:
 			switch p.Type[0] {
 			case "null":
 				// https://github.com/kubernetes/kube-openapi/pull/143/files#diff-ce77fea74b9dd098045004410023e0c3R219
@@ -80,7 +79,12 @@ func ConvertJSONSchemaPropsToOpenAPIv2Schema(in *apiextensions.JSONSchemaProps)
 					p.Items = nil
 				}
 			}
-		} else {
+		case len(p.Type) > 1:
+			// https://github.com/kubernetes/kube-openapi/pull/143/files#diff-62afddb578e9db18fb32ffb6b7802d92R272
+			// We also set Properties to null to enforce parseArbitrary at https://github.com/kubernetes/kube-openapi/blob/814a8073653e40e0e324205d093770d4e7bb811f/pkg/util/proto/document.go#L247
+			p.Type = nil
+			p.Properties = nil
+		default:
 			// https://github.com/kubernetes/kube-openapi/pull/143/files#diff-62afddb578e9db18fb32ffb6b7802d92R248
 			p.Properties = nil
 		}