From b3b15a9e260ac65edfa14dd80add9046f3fa9b1f Mon Sep 17 00:00:00 2001 From: "Dr. Stefan Schimanski" Date: Fri, 15 Nov 2019 17:15:12 +0100 Subject: [PATCH] apiextensions: ratcheting update validation for atomic item of set list-type --- .../apiextensions/validation/validation.go | 53 +++++-- .../validation/validation_test.go | 137 ++++++++++++++++++ 2 files changed, 177 insertions(+), 13 deletions(-) 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 331a39c3db7..f570dd82a4b 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 @@ -65,6 +65,7 @@ func ValidateCustomResourceDefinition(obj *apiextensions.CustomResourceDefinitio requireValidPropertyType: requireValidPropertyType(requestGV, nil), requireStructuralSchema: requireStructuralSchema(requestGV, nil), requirePrunedDefaults: true, + requireAtomicSetType: true, } allErrs := genericvalidation.ValidateObjectMeta(&obj.ObjectMeta, false, nameValidationFn, field.NewPath("metadata")) @@ -92,6 +93,8 @@ type validationOptions struct { requireStructuralSchema bool // requirePrunedDefaults indicates that defaults must be pruned requirePrunedDefaults bool + // requireAtomicSetType indicates that the items type for a x-kubernetes-list-type=set list must be atomic. + requireAtomicSetType bool } // ValidateCustomResourceDefinitionUpdate statically validates @@ -104,6 +107,7 @@ func ValidateCustomResourceDefinitionUpdate(obj, oldObj *apiextensions.CustomRes requireValidPropertyType: requireValidPropertyType(requestGV, &oldObj.Spec), requireStructuralSchema: requireStructuralSchema(requestGV, &oldObj.Spec), requirePrunedDefaults: requirePrunedDefaults(&oldObj.Spec), + requireAtomicSetType: requireAtomicSetType(&oldObj.Spec), } allErrs := genericvalidation.ValidateObjectMetaUpdate(&obj.ObjectMeta, &oldObj.ObjectMeta, field.NewPath("metadata")) @@ -660,7 +664,7 @@ func validateCustomResourceDefinitionValidation(customResourceValidation *apiext requireValidPropertyType: opts.requireValidPropertyType, } - allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(schema, fldPath.Child("openAPIV3Schema"), openAPIV3Schema, true)...) + allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(schema, fldPath.Child("openAPIV3Schema"), openAPIV3Schema, true, &opts)...) if opts.requireStructuralSchema { if ss, err := structuralschema.NewStructural(schema); err != nil { @@ -691,7 +695,7 @@ func validateCustomResourceDefinitionValidation(customResourceValidation *apiext var metaFields = sets.NewString("metadata", "kind", "apiVersion") // ValidateCustomResourceDefinitionOpenAPISchema statically validates -func ValidateCustomResourceDefinitionOpenAPISchema(schema *apiextensions.JSONSchemaProps, fldPath *field.Path, ssv specStandardValidator, isRoot bool) field.ErrorList { +func ValidateCustomResourceDefinitionOpenAPISchema(schema *apiextensions.JSONSchemaProps, fldPath *field.Path, ssv specStandardValidator, isRoot bool, opts *validationOptions) field.ErrorList { allErrs := field.ErrorList{} if schema == nil { @@ -726,7 +730,7 @@ func ValidateCustomResourceDefinitionOpenAPISchema(schema *apiextensions.JSONSch // we have to forbid defaults inside additionalProperties because pruning without actual value is ambiguous subSsv = ssv.withForbiddenDefaults("inside additionalProperties applying to object metadata") } - allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(schema.AdditionalProperties.Schema, fldPath.Child("additionalProperties"), subSsv, false)...) + allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(schema.AdditionalProperties.Schema, fldPath.Child("additionalProperties"), subSsv, false, opts)...) } if len(schema.Properties) != 0 { @@ -740,48 +744,48 @@ func ValidateCustomResourceDefinitionOpenAPISchema(schema *apiextensions.JSONSch subSsv = subSsv.withForbiddenDefaults(fmt.Sprintf("in top-level %s", property)) } } - allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(&jsonSchema, fldPath.Child("properties").Key(property), subSsv, false)...) + allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(&jsonSchema, fldPath.Child("properties").Key(property), subSsv, false, opts)...) } } - allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(schema.Not, fldPath.Child("not"), ssv, false)...) + allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(schema.Not, fldPath.Child("not"), ssv, false, opts)...) if len(schema.AllOf) != 0 { for i, jsonSchema := range schema.AllOf { - allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(&jsonSchema, fldPath.Child("allOf").Index(i), ssv, false)...) + allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(&jsonSchema, fldPath.Child("allOf").Index(i), ssv, false, opts)...) } } if len(schema.OneOf) != 0 { for i, jsonSchema := range schema.OneOf { - allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(&jsonSchema, fldPath.Child("oneOf").Index(i), ssv, false)...) + allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(&jsonSchema, fldPath.Child("oneOf").Index(i), ssv, false, opts)...) } } if len(schema.AnyOf) != 0 { for i, jsonSchema := range schema.AnyOf { - allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(&jsonSchema, fldPath.Child("anyOf").Index(i), ssv, false)...) + allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(&jsonSchema, fldPath.Child("anyOf").Index(i), ssv, false, opts)...) } } if len(schema.Definitions) != 0 { for definition, jsonSchema := range schema.Definitions { - allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(&jsonSchema, fldPath.Child("definitions").Key(definition), ssv, false)...) + allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(&jsonSchema, fldPath.Child("definitions").Key(definition), ssv, false, opts)...) } } if schema.Items != nil { - allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(schema.Items.Schema, fldPath.Child("items"), ssv, false)...) + allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(schema.Items.Schema, fldPath.Child("items"), ssv, false, opts)...) if len(schema.Items.JSONSchemas) != 0 { for i, jsonSchema := range schema.Items.JSONSchemas { - allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(&jsonSchema, fldPath.Child("items").Index(i), ssv, false)...) + allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(&jsonSchema, fldPath.Child("items").Index(i), ssv, false, opts)...) } } } if schema.Dependencies != nil { for dependency, jsonSchemaPropsOrStringArray := range schema.Dependencies { - allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(jsonSchemaPropsOrStringArray.Schema, fldPath.Child("dependencies").Key(dependency), ssv, false)...) + allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(jsonSchemaPropsOrStringArray.Schema, fldPath.Child("dependencies").Key(dependency), ssv, false, opts)...) } } @@ -807,7 +811,7 @@ func ValidateCustomResourceDefinitionOpenAPISchema(schema *apiextensions.JSONSch } else { allErrs = append(allErrs, field.Invalid(fldPath.Child("type"), schema.Type, "must be array if x-kubernetes-list-type is specified")) } - } else if schema.XListType != nil && *schema.XListType == "set" && schema.Items != nil && schema.Items.Schema != nil { // by structural schema items are present + } else if opts.requireAtomicSetType && schema.XListType != nil && *schema.XListType == "set" && schema.Items != nil && schema.Items.Schema != nil { // by structural schema items are present is := schema.Items.Schema switch is.Type { case "array": @@ -1247,6 +1251,29 @@ func schemaHasUnprunedDefaults(schema *apiextensions.JSONSchemaProps) (bool, err return !reflect.DeepEqual(ss, pruned), nil } +// requireAtomicSetType returns true if the old CRD spec as at least one x-kubernetes-list-type=set with non-atomic items type. +func requireAtomicSetType(oldCRDSpec *apiextensions.CustomResourceDefinitionSpec) bool { + return !hasSchemaWith(oldCRDSpec, hasNonAtomicSetType) +} + +// hasNonAtomicSetType recurses over the schema and returns whether any list of type "set" as non-atomic item types. +func hasNonAtomicSetType(schema *apiextensions.JSONSchemaProps) bool { + return schemaHas(schema, func(schema *apiextensions.JSONSchemaProps) bool { + if schema.XListType != nil && *schema.XListType == "set" && schema.Items != nil && schema.Items.Schema != nil { // we don't support schema.Items.JSONSchemas + is := schema.Items.Schema + switch is.Type { + case "array": + return is.XListType != nil && *is.XListType != "atomic" // atomic is the implicit default behaviour if unset, hence != atomic is wrong + case "object": + return is.XMapType == nil || *is.XMapType != "atomic" // granular is the implicit default behaviour if unset, hence nil and != atomic are wrong + default: + return false // scalar types are always atomic + } + } + return false + }) +} + // requireValidPropertyType returns true if valid openapi v3 types should be required for the given API version func requireValidPropertyType(requestGV schema.GroupVersion, oldCRDSpec *apiextensions.CustomResourceDefinitionSpec) bool { if requestGV == apiextensionsv1beta1.SchemeGroupVersion { 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 0266d799bc0..9186e0b8378 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 @@ -4297,6 +4297,140 @@ func TestValidateCustomResourceDefinitionUpdate(t *testing.T) { }, requestGV: apiextensionsv1.SchemeGroupVersion, }, + { + name: "non-atomic items in lists of type set allowed if pre-existing", + old: &apiextensions.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{Name: "plural.group.com", ResourceVersion: "1"}, + Spec: apiextensions.CustomResourceDefinitionSpec{ + Group: "group.com", + Scope: apiextensions.ResourceScope("Cluster"), + Names: apiextensions.CustomResourceDefinitionNames{ + Plural: "plural", + Singular: "singular", + Kind: "Plural", + ListKind: "PluralList", + }, + Versions: []apiextensions.CustomResourceDefinitionVersion{{Name: "version", Served: true, Storage: true}}, + Validation: &apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ + Type: "object", + Properties: map[string]apiextensions.JSONSchemaProps{"foo": { + Type: "array", + XListType: strPtr("set"), + Items: &apiextensions.JSONSchemaPropsOrArray{ + Schema: &apiextensions.JSONSchemaProps{ + Type: "object", // non-atomic + Properties: map[string]apiextensions.JSONSchemaProps{}, + }, + }, + }}, + }, + }, + PreserveUnknownFields: pointer.BoolPtr(false), + }, + Status: apiextensions.CustomResourceDefinitionStatus{ + StoredVersions: []string{"version"}, + }, + }, + resource: &apiextensions.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{Name: "plural.group.com", ResourceVersion: "1"}, + Spec: apiextensions.CustomResourceDefinitionSpec{ + Group: "group.com", + Scope: apiextensions.ResourceScope("Cluster"), + Names: apiextensions.CustomResourceDefinitionNames{ + Plural: "plural", + Singular: "singular", + Kind: "Plural", + ListKind: "PluralList", + }, + Versions: []apiextensions.CustomResourceDefinitionVersion{{Name: "version", Served: true, Storage: true}}, + Validation: &apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ + Type: "object", + Properties: map[string]apiextensions.JSONSchemaProps{"bar": { + Type: "array", + XListType: strPtr("set"), + Items: &apiextensions.JSONSchemaPropsOrArray{ + Schema: &apiextensions.JSONSchemaProps{ + Type: "object", // non-atomic + Properties: map[string]apiextensions.JSONSchemaProps{}, + }, + }, + }}, + }, + }, + PreserveUnknownFields: pointer.BoolPtr(false), + }, + Status: apiextensions.CustomResourceDefinitionStatus{ + StoredVersions: []string{"version"}, + }, + }, + requestGV: apiextensionsv1.SchemeGroupVersion, + }, + { + name: "reject non-atomic items in lists of type set if not pre-existing", + old: &apiextensions.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{Name: "plural.group.com", ResourceVersion: "1"}, + Spec: apiextensions.CustomResourceDefinitionSpec{ + Group: "group.com", + Scope: apiextensions.ResourceScope("Cluster"), + Names: apiextensions.CustomResourceDefinitionNames{ + Plural: "plural", + Singular: "singular", + Kind: "Plural", + ListKind: "PluralList", + }, + Versions: []apiextensions.CustomResourceDefinitionVersion{{Name: "version", Served: true, Storage: true}}, + Validation: &apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ + Type: "object", + Properties: map[string]apiextensions.JSONSchemaProps{}, + }, + }, + PreserveUnknownFields: pointer.BoolPtr(false), + }, + Status: apiextensions.CustomResourceDefinitionStatus{ + StoredVersions: []string{"version"}, + }, + }, + resource: &apiextensions.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{Name: "plural.group.com", ResourceVersion: "1"}, + Spec: apiextensions.CustomResourceDefinitionSpec{ + Group: "group.com", + Scope: apiextensions.ResourceScope("Cluster"), + Names: apiextensions.CustomResourceDefinitionNames{ + Plural: "plural", + Singular: "singular", + Kind: "Plural", + ListKind: "PluralList", + }, + Versions: []apiextensions.CustomResourceDefinitionVersion{{Name: "version", Served: true, Storage: true}}, + Validation: &apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ + Type: "object", + Properties: map[string]apiextensions.JSONSchemaProps{"bar": { + Type: "array", + XListType: strPtr("set"), + Items: &apiextensions.JSONSchemaPropsOrArray{ + Schema: &apiextensions.JSONSchemaProps{ + Type: "object", // non-atomic + Properties: map[string]apiextensions.JSONSchemaProps{}, + }, + }, + }}, + }, + }, + PreserveUnknownFields: pointer.BoolPtr(false), + }, + Status: apiextensions.CustomResourceDefinitionStatus{ + StoredVersions: []string{"version"}, + }, + }, + requestGV: apiextensionsv1.SchemeGroupVersion, + errors: []validationMatch{ + invalid("spec", "validation", "openAPIV3Schema", "properties[bar]", "items", "x-kubernetes-map-type"), + }, + }, { name: "structural to non-structural updates allowed via v1beta1", old: &apiextensions.CustomResourceDefinition{ @@ -6382,6 +6516,7 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { }, }, }, + opts: validationOptions{requireAtomicSetType: true}, wantError: true, }, { @@ -6400,6 +6535,7 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { }, }, }, + opts: validationOptions{requireAtomicSetType: true}, wantError: true, }, { @@ -6462,6 +6598,7 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { }, }, }, + opts: validationOptions{requireAtomicSetType: true}, wantError: true, }, {