apiextensions: require structural schema for x-kubernetes-* extensions

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

@@ -157,6 +157,9 @@ func validateCustomResourceDefinitionSpec(spec *apiextensions.CustomResourceDefi
 			allErrs = append(allErrs, field.Invalid(fldPath.Child("preserveUnknownFields"), true, "must be false in order to use defaults in the schema"))
 		}
 	}
+	if specHasKubernetesExtensions(spec) {
+		mustBeStructural = true
+	}
 
 	storageFlagCount := 0
 	versionsMap := map[string]bool{}
@@ -957,3 +960,86 @@ func schemaHasDefaults(s *apiextensions.JSONSchemaProps) bool {
 
 	return false
 }
+
+func specHasKubernetesExtensions(spec *apiextensions.CustomResourceDefinitionSpec) bool {
+	if spec.Validation != nil && schemaHasKubernetesExtensions(spec.Validation.OpenAPIV3Schema) {
+		return true
+	}
+	for _, v := range spec.Versions {
+		if v.Schema != nil && schemaHasKubernetesExtensions(v.Schema.OpenAPIV3Schema) {
+			return true
+		}
+	}
+	return false
+}
+
+func schemaHasKubernetesExtensions(s *apiextensions.JSONSchemaProps) bool {
+	if s == nil {
+		return false
+	}
+
+	if s.XEmbeddedResource || s.XPreserveUnknownFields != nil || s.XIntOrString {
+		return true
+	}
+
+	if s.Items != nil {
+		if s.Items != nil && schemaHasKubernetesExtensions(s.Items.Schema) {
+			return true
+		}
+		for _, s := range s.Items.JSONSchemas {
+			if schemaHasKubernetesExtensions(&s) {
+				return true
+			}
+		}
+	}
+	for _, s := range s.AllOf {
+		if schemaHasKubernetesExtensions(&s) {
+			return true
+		}
+	}
+	for _, s := range s.AnyOf {
+		if schemaHasKubernetesExtensions(&s) {
+			return true
+		}
+	}
+	for _, s := range s.OneOf {
+		if schemaHasKubernetesExtensions(&s) {
+			return true
+		}
+	}
+	if schemaHasKubernetesExtensions(s.Not) {
+		return true
+	}
+	for _, s := range s.Properties {
+		if schemaHasKubernetesExtensions(&s) {
+			return true
+		}
+	}
+	if s.AdditionalProperties != nil {
+		if schemaHasKubernetesExtensions(s.AdditionalProperties.Schema) {
+			return true
+		}
+	}
+	for _, s := range s.PatternProperties {
+		if schemaHasKubernetesExtensions(&s) {
+			return true
+		}
+	}
+	if s.AdditionalItems != nil {
+		if schemaHasKubernetesExtensions(s.AdditionalItems.Schema) {
+			return true
+		}
+	}
+	for _, s := range s.Definitions {
+		if schemaHasKubernetesExtensions(&s) {
+			return true
+		}
+	}
+	for _, d := range s.Dependencies {
+		if schemaHasKubernetesExtensions(d.Schema) {
+			return true
+		}
+	}
+
+	return false
+}

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

@@ -1442,8 +1442,12 @@ func TestValidateCustomResourceDefinition(t *testing.T) {
 					},
 					Validation: &apiextensions.CustomResourceValidation{
 						OpenAPIV3Schema: &apiextensions.JSONSchemaProps{
+							Type: "object",
 							Properties: map[string]apiextensions.JSONSchemaProps{
-								"a": {Default: jsonPtr(42.0)},
+								"a": {
+									Type:    "number",
+									Default: jsonPtr(42.0),
+								},
 							},
 						},
 					},
@@ -1457,6 +1461,112 @@ func TestValidateCustomResourceDefinition(t *testing.T) {
 				forbidden("spec", "validation", "openAPIV3Schema", "properties[a]", "default"), // disabled feature-gate
 			},
 		},
+		{
+			name: "x-kubernetes-int-or-string without structural",
+			resource: &apiextensions.CustomResourceDefinition{
+				ObjectMeta: metav1.ObjectMeta{Name: "plural.group.com"},
+				Spec: apiextensions.CustomResourceDefinitionSpec{
+					Group:    "group.com",
+					Version:  "version",
+					Versions: singleVersionList,
+					Scope:    apiextensions.NamespaceScoped,
+					Names: apiextensions.CustomResourceDefinitionNames{
+						Plural:   "plural",
+						Singular: "singular",
+						Kind:     "Plural",
+						ListKind: "PluralList",
+					},
+					Validation: &apiextensions.CustomResourceValidation{
+						OpenAPIV3Schema: &apiextensions.JSONSchemaProps{
+							Properties: map[string]apiextensions.JSONSchemaProps{
+								"intorstring": {
+									XIntOrString: true,
+								},
+							},
+						},
+					},
+					PreserveUnknownFields: pointer.BoolPtr(true),
+				},
+				Status: apiextensions.CustomResourceDefinitionStatus{
+					StoredVersions: []string{"version"},
+				},
+			},
+			errors: []validationMatch{
+				required("spec", "validation", "openAPIV3Schema", "type"),
+			},
+		},
+		{
+			name: "x-kubernetes-preserve-unknown-fields without structural",
+			resource: &apiextensions.CustomResourceDefinition{
+				ObjectMeta: metav1.ObjectMeta{Name: "plural.group.com"},
+				Spec: apiextensions.CustomResourceDefinitionSpec{
+					Group:    "group.com",
+					Version:  "version",
+					Versions: singleVersionList,
+					Scope:    apiextensions.NamespaceScoped,
+					Names: apiextensions.CustomResourceDefinitionNames{
+						Plural:   "plural",
+						Singular: "singular",
+						Kind:     "Plural",
+						ListKind: "PluralList",
+					},
+					Validation: &apiextensions.CustomResourceValidation{
+						OpenAPIV3Schema: &apiextensions.JSONSchemaProps{
+							Properties: map[string]apiextensions.JSONSchemaProps{
+								"raw": {
+									XPreserveUnknownFields: pointer.BoolPtr(true),
+								},
+							},
+						},
+					},
+					PreserveUnknownFields: pointer.BoolPtr(true),
+				},
+				Status: apiextensions.CustomResourceDefinitionStatus{
+					StoredVersions: []string{"version"},
+				},
+			},
+			errors: []validationMatch{
+				required("spec", "validation", "openAPIV3Schema", "type"),
+			},
+		},
+		{
+			name: "x-kubernetes-embedded-resource without structural",
+			resource: &apiextensions.CustomResourceDefinition{
+				ObjectMeta: metav1.ObjectMeta{Name: "plural.group.com"},
+				Spec: apiextensions.CustomResourceDefinitionSpec{
+					Group:    "group.com",
+					Version:  "version",
+					Versions: singleVersionList,
+					Scope:    apiextensions.NamespaceScoped,
+					Names: apiextensions.CustomResourceDefinitionNames{
+						Plural:   "plural",
+						Singular: "singular",
+						Kind:     "Plural",
+						ListKind: "PluralList",
+					},
+					Validation: &apiextensions.CustomResourceValidation{
+						OpenAPIV3Schema: &apiextensions.JSONSchemaProps{
+							Properties: map[string]apiextensions.JSONSchemaProps{
+								"embedded": {
+									Type:              "object",
+									XEmbeddedResource: true,
+									Properties: map[string]apiextensions.JSONSchemaProps{
+										"foo": {Type: "string"},
+									},
+								},
+							},
+						},
+					},
+					PreserveUnknownFields: pointer.BoolPtr(true),
+				},
+				Status: apiextensions.CustomResourceDefinitionStatus{
+					StoredVersions: []string{"version"},
+				},
+			},
+			errors: []validationMatch{
+				required("spec", "validation", "openAPIV3Schema", "type"),
+			},
+		},
 		{
 			name: "defaults with enabled feature gate, unstructural schema",
 			resource: &apiextensions.CustomResourceDefinition{

staging/src/k8s.io/apiextensions-apiserver/test/integration/objectmeta_test.go

@@ -107,8 +107,9 @@ func TestInvalidObjectMetaInStorage(t *testing.T) {
 			Type: "object",
 			Properties: map[string]apiextensionsv1beta1.JSONSchemaProps{
 				"embedded": {
-					Type:              "object",
+					Type:                   "object",
-					XEmbeddedResource: true,
+					XEmbeddedResource:      true,
+					XPreserveUnknownFields: pointer.BoolPtr(true),
 				},
 			},
 		},

staging/src/k8s.io/apiextensions-apiserver/test/integration/validation_test.go

@@ -705,19 +705,20 @@ spec:
 	type Test struct {
 		desc                                  string
 		globalSchema, v1Schema, v1beta1Schema string
-		expectedCreateError                   bool
+		expectedCreateErrors                  []string
+		unexpectedCreateErrors                []string
 		expectedViolations                    []string
 		unexpectedViolations                  []string
 	}
 	tests := []Test{
-		{"empty", "", "", "", false, nil, nil},
+		{"empty", "", "", "", nil, nil, nil, nil},
 		{
 			desc: "int-or-string and preserve-unknown-fields true",
 			globalSchema: `
 x-kubernetes-preserve-unknown-fields: true
 x-kubernetes-int-or-string: true
 `,
-			expectedViolations: []string{
+			expectedCreateErrors: []string{
 				"spec.validation.openAPIV3Schema.x-kubernetes-preserve-unknown-fields: Invalid value: true: must be false if x-kubernetes-int-or-string is true",
 			},
 		},
@@ -728,7 +729,7 @@ type: object
 x-kubernetes-embedded-resource: true
 x-kubernetes-int-or-string: true
 `,
-			expectedViolations: []string{
+			expectedCreateErrors: []string{
 				"spec.validation.openAPIV3Schema.x-kubernetes-embedded-resource: Invalid value: true: must be false if x-kubernetes-int-or-string is true",
 			},
 		},
@@ -738,7 +739,7 @@ x-kubernetes-int-or-string: true
 type: object
 x-kubernetes-embedded-resource: true
 `,
-			expectedViolations: []string{
+			expectedCreateErrors: []string{
 				"spec.validation.openAPIV3Schema.properties: Required value: must not be empty if x-kubernetes-embedded-resource is true without x-kubernetes-preserve-unknown-fields",
 			},
 		},
@@ -773,7 +774,7 @@ type: array
 x-kubernetes-embedded-resource: true
 x-kubernetes-preserve-unknown-fields: true
 `,
-			expectedViolations: []string{
+			expectedCreateErrors: []string{
 				"spec.validation.openAPIV3Schema.type: Invalid value: \"array\": must be object if x-kubernetes-embedded-resource is true",
 			},
 		},
@@ -784,7 +785,7 @@ type: ""
 x-kubernetes-embedded-resource: true
 x-kubernetes-preserve-unknown-fields: true
 `,
-			expectedViolations: []string{
+			expectedCreateErrors: []string{
 				"spec.validation.openAPIV3Schema.type: Required value: must be object if x-kubernetes-embedded-resource is true",
 			},
 		},
@@ -923,7 +924,7 @@ oneOf:
       x-kubernetes-embedded-resource: true
       x-kubernetes-preserve-unknown-fields: true
 `,
-			expectedViolations: []string{
+			expectedCreateErrors: []string{
 				"spec.validation.openAPIV3Schema.allOf[0].properties[embedded-resource].x-kubernetes-preserve-unknown-fields: Forbidden: must be false to be structural",
 				"spec.validation.openAPIV3Schema.allOf[0].properties[embedded-resource].x-kubernetes-embedded-resource: Forbidden: must be false to be structural",
 				"spec.validation.openAPIV3Schema.allOf[0].properties[int-or-string].x-kubernetes-int-or-string: Forbidden: must be false to be structural",
@@ -939,7 +940,7 @@ oneOf:
 			},
 		},
 		{
-			desc: "missing types",
+			desc: "missing types with extensions",
 			globalSchema: `
 properties:
   foo:
@@ -967,7 +968,7 @@ properties:
     properties:
       a: {}
 `,
-			expectedViolations: []string{
+			expectedCreateErrors: []string{
 				"spec.validation.openAPIV3Schema.properties[foo].properties[a].type: Required value: must not be empty for specified object fields",
 				"spec.validation.openAPIV3Schema.properties[foo].type: Required value: must not be empty for specified object fields",
 				"spec.validation.openAPIV3Schema.properties[int-or-string].properties[a].type: Required value: must not be empty for specified object fields",
@@ -985,6 +986,43 @@ properties:
 				"spec.validation.openAPIV3Schema.type: Required value: must not be empty at the root",
 			},
 		},
+		{
+			desc: "missing types without extensions",
+			globalSchema: `
+properties:
+  foo:
+    properties:
+      a: {}
+  bar:
+    items:
+      additionalProperties:
+        properties:
+          a: {}
+        items: {}
+  abc:
+    additionalProperties:
+      properties:
+        a:
+          items:
+            additionalProperties:
+              items:
+`,
+			expectedViolations: []string{
+				"spec.validation.openAPIV3Schema.properties[foo].properties[a].type: Required value: must not be empty for specified object fields",
+				"spec.validation.openAPIV3Schema.properties[foo].type: Required value: must not be empty for specified object fields",
+				"spec.validation.openAPIV3Schema.properties[abc].additionalProperties.properties[a].items.additionalProperties.type: Required value: must not be empty for specified object fields",
+				"spec.validation.openAPIV3Schema.properties[abc].additionalProperties.properties[a].items.type: Required value: must not be empty for specified array items",
+				"spec.validation.openAPIV3Schema.properties[abc].additionalProperties.properties[a].type: Required value: must not be empty for specified object fields",
+				"spec.validation.openAPIV3Schema.properties[abc].additionalProperties.type: Required value: must not be empty for specified object fields",
+				"spec.validation.openAPIV3Schema.properties[abc].type: Required value: must not be empty for specified object fields",
+				"spec.validation.openAPIV3Schema.properties[bar].items.additionalProperties.items.type: Required value: must not be empty for specified array items",
+				"spec.validation.openAPIV3Schema.properties[bar].items.additionalProperties.properties[a].type: Required value: must not be empty for specified object fields",
+				"spec.validation.openAPIV3Schema.properties[bar].items.additionalProperties.type: Required value: must not be empty for specified object fields",
+				"spec.validation.openAPIV3Schema.properties[bar].items.type: Required value: must not be empty for specified array items",
+				"spec.validation.openAPIV3Schema.properties[bar].type: Required value: must not be empty for specified object fields",
+				"spec.validation.openAPIV3Schema.type: Required value: must not be empty at the root",
+			},
+		},
 		{
 			desc: "int-or-string variants",
 			globalSchema: `
@@ -1033,7 +1071,7 @@ properties:
     - type: string
     - type: integer
 `,
-			expectedViolations: []string{
+			expectedCreateErrors: []string{
 				"spec.validation.openAPIV3Schema.properties[d].anyOf[0].type: Forbidden: must be empty to be structural",
 				"spec.validation.openAPIV3Schema.properties[d].anyOf[1].type: Forbidden: must be empty to be structural",
 				"spec.validation.openAPIV3Schema.properties[e].allOf[0].anyOf[0].type: Forbidden: must be empty to be structural",
@@ -1043,7 +1081,7 @@ properties:
 				"spec.validation.openAPIV3Schema.properties[g].anyOf[0].type: Forbidden: must be empty to be structural",
 				"spec.validation.openAPIV3Schema.properties[g].anyOf[1].type: Forbidden: must be empty to be structural",
 			},
-			unexpectedViolations: []string{
+			unexpectedCreateErrors: []string{
 				"spec.validation.openAPIV3Schema.properties[a]",
 				"spec.validation.openAPIV3Schema.properties[b]",
 				"spec.validation.openAPIV3Schema.properties[c]",
@@ -1354,7 +1392,7 @@ properties:
     - type: string
     - type: integer
 `,
-			expectedCreateError: true,
+			expectedCreateErrors: []string{"spec.validation.openAPIV3Schema.properties[slice].items: Forbidden: items must be a schema object and not an array"},
 		},
 		{
 			desc: "items slice in value validation",
@@ -1369,7 +1407,7 @@ properties:
       items:
       - type: string
 `,
-			expectedCreateError: true,
+			expectedCreateErrors: []string{"spec.validation.openAPIV3Schema.properties[slice].not.items: Forbidden: items must be a schema object and not an array"},
 		},
 	}
 
@@ -1394,10 +1432,21 @@ properties:
 
 			// create CRDs
 			crd, err = apiExtensionClient.ApiextensionsV1beta1().CustomResourceDefinitions().Create(crd)
-			if tst.expectedCreateError && err == nil {
+			if len(tst.expectedCreateErrors) > 0 && err == nil {
-				t.Fatalf("expected error, got none")
+				t.Fatalf("expected create errors, got none")
-			} else if !tst.expectedCreateError && err != nil {
+			} else if len(tst.expectedCreateErrors) == 0 && err != nil {
 				t.Fatalf("unexpected create error: %v", err)
+			} else if err != nil {
+				for _, expectedErr := range tst.expectedCreateErrors {
+					if !strings.Contains(err.Error(), expectedErr) {
+						t.Errorf("expected error containing '%s', got '%s'", expectedErr, err.Error())
+					}
+				}
+				for _, unexpectedErr := range tst.unexpectedCreateErrors {
+					if strings.Contains(err.Error(), unexpectedErr) {
+						t.Errorf("unexpected error containing '%s': '%s'", unexpectedErr, err.Error())
+					}
+				}
 			}
 			if err != nil {
 				return