diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go index 656033ab828..ca3a5d47876 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go +++ b/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 +} diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation_test.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation_test.go index fdc9d1bac0f..655a377e461 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation_test.go +++ b/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{ diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/objectmeta_test.go b/staging/src/k8s.io/apiextensions-apiserver/test/integration/objectmeta_test.go index 447ccc84c88..c5e5e27cfc1 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/objectmeta_test.go +++ b/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", - XEmbeddedResource: true, + Type: "object", + XEmbeddedResource: true, + XPreserveUnknownFields: pointer.BoolPtr(true), }, }, }, diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/validation_test.go b/staging/src/k8s.io/apiextensions-apiserver/test/integration/validation_test.go index 2a53c490b6a..0fe71006059 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/validation_test.go +++ b/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 { - t.Fatalf("expected error, got none") - } else if !tst.expectedCreateError && err != nil { + if len(tst.expectedCreateErrors) > 0 && err == nil { + t.Fatalf("expected create errors, got none") + } 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