diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/fuzzer/fuzzer.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/fuzzer/fuzzer.go index a7cfb0ae7c..f6b246cf93 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/fuzzer/fuzzer.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/fuzzer/fuzzer.go @@ -81,6 +81,7 @@ func Funcs(codecs runtimeserializer.CodecFactory) []interface{} { }, func(obj *apiextensions.JSONSchemaPropsOrBool, c fuzz.Continue) { if c.RandBool() { + obj.Allows = true obj.Schema = &apiextensions.JSONSchemaProps{} c.Fuzz(obj.Schema) } else { diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/marshal.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/marshal.go index d8f9f164e8..9a8fad3b77 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/marshal.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/marshal.go @@ -45,6 +45,7 @@ func (s *JSONSchemaPropsOrBool) UnmarshalJSON(data []byte) error { if err := json.Unmarshal(data, &sch); err != nil { return err } + nw.Allows = true nw.Schema = &sch case len(data) == 4 && string(data) == "true": nw.Allows = true diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/marshal_test.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/marshal_test.go index 0c6d4d4b52..99065ff95a 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/marshal_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/marshal_test.go @@ -34,13 +34,13 @@ func TestJSONSchemaPropsOrBoolUnmarshalJSON(t *testing.T) { }{ {`{}`, JSONSchemaPropsOrBoolHolder{}}, - {`{"val1": {}}`, JSONSchemaPropsOrBoolHolder{JSPoB: JSONSchemaPropsOrBool{Schema: &JSONSchemaProps{}}}}, - {`{"val1": {"type":"string"}}`, JSONSchemaPropsOrBoolHolder{JSPoB: JSONSchemaPropsOrBool{Schema: &JSONSchemaProps{Type: "string"}}}}, + {`{"val1": {}}`, JSONSchemaPropsOrBoolHolder{JSPoB: JSONSchemaPropsOrBool{Allows: true, Schema: &JSONSchemaProps{}}}}, + {`{"val1": {"type":"string"}}`, JSONSchemaPropsOrBoolHolder{JSPoB: JSONSchemaPropsOrBool{Allows: true, Schema: &JSONSchemaProps{Type: "string"}}}}, {`{"val1": false}`, JSONSchemaPropsOrBoolHolder{JSPoB: JSONSchemaPropsOrBool{}}}, {`{"val1": true}`, JSONSchemaPropsOrBoolHolder{JSPoB: JSONSchemaPropsOrBool{Allows: true}}}, - {`{"val2": {}}`, JSONSchemaPropsOrBoolHolder{JSPoBOmitEmpty: &JSONSchemaPropsOrBool{Schema: &JSONSchemaProps{}}}}, - {`{"val2": {"type":"string"}}`, JSONSchemaPropsOrBoolHolder{JSPoBOmitEmpty: &JSONSchemaPropsOrBool{Schema: &JSONSchemaProps{Type: "string"}}}}, + {`{"val2": {}}`, JSONSchemaPropsOrBoolHolder{JSPoBOmitEmpty: &JSONSchemaPropsOrBool{Allows: true, Schema: &JSONSchemaProps{}}}}, + {`{"val2": {"type":"string"}}`, JSONSchemaPropsOrBoolHolder{JSPoBOmitEmpty: &JSONSchemaPropsOrBool{Allows: true, Schema: &JSONSchemaProps{Type: "string"}}}}, {`{"val2": false}`, JSONSchemaPropsOrBoolHolder{JSPoBOmitEmpty: &JSONSchemaPropsOrBool{}}}, {`{"val2": true}`, JSONSchemaPropsOrBoolHolder{JSPoBOmitEmpty: &JSONSchemaPropsOrBool{Allows: true}}}, } 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 713fcfa905..46ff6adc6c 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 @@ -240,14 +240,36 @@ func ValidateCustomResourceDefinitionOpenAPISchema(schema *apiextensions.JSONSch allErrs = append(allErrs, field.Forbidden(fldPath.Child("uniqueItems"), "uniqueItems cannot be set to true since the runtime complexity becomes quadratic")) } - // additionalProperties contradicts Kubernetes API convention to ignore unknown fields + // additionalProperties and properties are mutual exclusive because otherwise they + // contradict Kubernetes' API convention to ignore unknown fields. + // + // In other words: + // - properties are for structs, + // - additionalProperties are for map[string]interface{} + // + // Note: when patternProperties is added to OpenAPI some day, this will have to be + // restricted like additionalProperties. if schema.AdditionalProperties != nil { - if schema.AdditionalProperties.Allows == false { - allErrs = append(allErrs, field.Forbidden(fldPath.Child("additionalProperties"), "additionalProperties cannot be set to false")) + if len(schema.Properties) != 0 { + if schema.AdditionalProperties.Allows == false || schema.AdditionalProperties.Schema != nil { + allErrs = append(allErrs, field.Forbidden(fldPath.Child("additionalProperties"), "additionalProperties and properties are mutual exclusive")) + } } allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(schema.AdditionalProperties.Schema, fldPath.Child("additionalProperties"), ssv)...) } + if len(schema.Properties) != 0 { + for property, jsonSchema := range schema.Properties { + allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(&jsonSchema, fldPath.Child("properties").Key(property), ssv)...) + } + } + + if len(schema.PatternProperties) != 0 { + for property, jsonSchema := range schema.PatternProperties { + allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(&jsonSchema, fldPath.Child("patternProperties").Key(property), ssv)...) + } + } + if schema.AdditionalItems != nil { allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(schema.AdditionalItems.Schema, fldPath.Child("additionalItems"), ssv)...) } @@ -272,18 +294,6 @@ func ValidateCustomResourceDefinitionOpenAPISchema(schema *apiextensions.JSONSch } } - if len(schema.Properties) != 0 { - for property, jsonSchema := range schema.Properties { - allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(&jsonSchema, fldPath.Child("properties").Key(property), ssv)...) - } - } - - if len(schema.PatternProperties) != 0 { - for property, jsonSchema := range schema.PatternProperties { - allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(&jsonSchema, fldPath.Child("patternProperties").Key(property), ssv)...) - } - } - if len(schema.Definitions) != 0 { for definition, jsonSchema := range schema.Definitions { allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(&jsonSchema, fldPath.Child("definitions").Key(definition), ssv)...) 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 dd712c94f4..c7c9b6d494 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 @@ -41,6 +41,9 @@ func unsupported(path ...string) validationMatch { func immutable(path ...string) validationMatch { return validationMatch{path: field.NewPath(path[0], path[1:]...), errorType: field.ErrorTypeInvalid} } +func forbidden(path ...string) validationMatch { + return validationMatch{path: field.NewPath(path[0], path[1:]...), errorType: field.ErrorTypeForbidden} +} func (v validationMatch) matches(err *field.Error) bool { return err.Type == v.errorType && err.Field == v.path.String() @@ -160,6 +163,62 @@ func TestValidateCustomResourceDefinition(t *testing.T) { invalid("status", "acceptedNames", "listKind"), }, }, + { + name: "additionalProperties and properties forbidden", + resource: &apiextensions.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{Name: "plural.group.com"}, + Spec: apiextensions.CustomResourceDefinitionSpec{ + Group: "group.com", + Version: "version", + 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{ + "foo": {}, + }, + AdditionalProperties: &apiextensions.JSONSchemaPropsOrBool{Allows: false}, + }, + }, + }, + }, + errors: []validationMatch{ + forbidden("spec", "validation", "openAPIV3Schema", "additionalProperties"), + }, + }, + { + name: "additionalProperties without properties allowed (map[string]string)", + resource: &apiextensions.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{Name: "plural.group.com"}, + Spec: apiextensions.CustomResourceDefinitionSpec{ + Group: "group.com", + Version: "version", + Scope: apiextensions.NamespaceScoped, + Names: apiextensions.CustomResourceDefinitionNames{ + Plural: "plural", + Singular: "singular", + Kind: "Plural", + ListKind: "PluralList", + }, + Validation: &apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ + AdditionalProperties: &apiextensions.JSONSchemaPropsOrBool{ + Allows: true, + Schema: &apiextensions.JSONSchemaProps{ + Type: "string", + }, + }, + }, + }, + }, + }, + errors: []validationMatch{}, + }, } for _, tc := range tests {