From 9c3eedea1800090fb499900a4c7ebee6502951e8 Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Wed, 7 Aug 2019 16:16:11 -0400 Subject: [PATCH] CRD v1: require valid openapiv3 types --- .../apiextensions/validation/validation.go | 43 ++- .../validation/validation_test.go | 252 ++++++++++++++++++ 2 files changed, 291 insertions(+), 4 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 06a70cd1134..3376b83b2b3 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 @@ -66,6 +66,7 @@ func ValidateCustomResourceDefinition(obj *apiextensions.CustomResourceDefinitio requireRecognizedConversionReviewVersion: true, requireImmutableNames: false, requireOpenAPISchema: requireOpenAPISchema(requestGV, nil), + requireValidPropertyType: requireValidPropertyType(requestGV, nil), } allErrs := genericvalidation.ValidateObjectMeta(&obj.ObjectMeta, false, nameValidationFn, field.NewPath("metadata")) @@ -87,6 +88,8 @@ type validationOptions struct { requireImmutableNames bool // requireOpenAPISchema requires an openapi V3 schema be specified requireOpenAPISchema bool + // requireValidPropertyType requires property types specified in the validation schema to be valid openapi v3 types + requireValidPropertyType bool } // ValidateCustomResourceDefinitionUpdate statically validates @@ -96,6 +99,7 @@ func ValidateCustomResourceDefinitionUpdate(obj, oldObj *apiextensions.CustomRes requireRecognizedConversionReviewVersion: oldObj.Spec.Conversion == nil || hasValidConversionReviewVersionOrEmpty(oldObj.Spec.Conversion.ConversionReviewVersions), requireImmutableNames: apiextensions.IsCRDConditionTrue(oldObj, apiextensions.Established), requireOpenAPISchema: requireOpenAPISchema(requestGV, &oldObj.Spec), + requireValidPropertyType: requireValidPropertyType(requestGV, &oldObj.Spec), } allErrs := genericvalidation.ValidateObjectMetaUpdate(&obj.ObjectMeta, &oldObj.ObjectMeta, field.NewPath("metadata")) @@ -650,7 +654,8 @@ func validateCustomResourceDefinitionValidation(customResourceValidation *apiext } openAPIV3Schema := &specStandardValidatorV3{ - allowDefaults: opts.allowDefaults, + allowDefaults: opts.allowDefaults, + requireValidPropertyType: opts.requireValidPropertyType, } allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(schema, fldPath.Child("openAPIV3Schema"), openAPIV3Schema, true)...) @@ -779,9 +784,10 @@ func ValidateCustomResourceDefinitionOpenAPISchema(schema *apiextensions.JSONSch } type specStandardValidatorV3 struct { - allowDefaults bool - disallowDefaultsReason string - isInsideResourceMeta bool + allowDefaults bool + disallowDefaultsReason string + isInsideResourceMeta bool + requireValidPropertyType bool } func (v *specStandardValidatorV3) withForbiddenDefaults(reason string) specStandardValidator { @@ -813,6 +819,10 @@ func (v *specStandardValidatorV3) validate(schema *apiextensions.JSONSchemaProps // WARNING: if anything new is allowed below, NewStructural must be adapted to support it. // + if v.requireValidPropertyType && len(schema.Type) > 0 && !openapiV3Types.Has(schema.Type) { + allErrs = append(allErrs, field.NotSupported(fldPath.Child("type"), schema.Type, openapiV3Types.List())) + } + if schema.Default != nil { if v.allowDefaults { if s, err := structuralschema.NewStructural(schema); err == nil { @@ -1168,6 +1178,19 @@ func schemaHasKubernetesExtensions(s *apiextensions.JSONSchemaProps) bool { 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 == v1beta1.SchemeGroupVersion { + // for compatibility + return false + } + if oldCRDSpec != nil && specHasInvalidTypes(oldCRDSpec) { + // don't tighten validation on existing persisted data + return false + } + return true +} + // validateAPIApproval returns a list of errors if the API approval annotation isn't valid func validateAPIApproval(newCRD, oldCRD *apiextensions.CustomResourceDefinition, requestGV schema.GroupVersion) field.ErrorList { // check to see if we need confirm API approval for kube group. @@ -1228,6 +1251,18 @@ func validatePreserveUnknownFields(crd, oldCRD *apiextensions.CustomResourceDefi return errs } +func specHasInvalidTypes(spec *apiextensions.CustomResourceDefinitionSpec) bool { + if spec.Validation != nil && SchemaHasInvalidTypes(spec.Validation.OpenAPIV3Schema) { + return true + } + for _, v := range spec.Versions { + if v.Schema != nil && SchemaHasInvalidTypes(v.Schema.OpenAPIV3Schema) { + return true + } + } + return false +} + // SchemaHasInvalidTypes returns true if it contains invalid offending openapi-v3 specification. func SchemaHasInvalidTypes(s *apiextensions.JSONSchemaProps) bool { if s == nil { 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 ee85292bd40..2ac892a4ed5 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 @@ -84,6 +84,68 @@ func TestValidateCustomResourceDefinition(t *testing.T) { errors []validationMatch enabledFeatures []featuregate.Feature }{ + { + name: "invalid types allowed via v1beta1", + resource: &apiextensions.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{Name: "plural.group.com"}, + 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: "bogus"}}, + }, + }, + }, + Status: apiextensions.CustomResourceDefinitionStatus{ + StoredVersions: []string{"version"}, + }, + }, + requestGV: apiextensionsv1beta1.SchemeGroupVersion, + }, + { + name: "invalid types disallowed via v1", + resource: &apiextensions.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{Name: "plural.group.com"}, + 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: "bogus"}}, + }, + }, + PreserveUnknownFields: pointer.BoolPtr(false), + }, + Status: apiextensions.CustomResourceDefinitionStatus{ + StoredVersions: []string{"version"}, + }, + }, + requestGV: apiextensionsv1.SchemeGroupVersion, + errors: []validationMatch{ + unsupported("spec.validation.openAPIV3Schema.properties[foo].type"), + }, + }, { name: "webhookconfig: invalid port 0", resource: &apiextensions.CustomResourceDefinition{ @@ -2545,6 +2607,163 @@ func TestValidateCustomResourceDefinitionUpdate(t *testing.T) { errors []validationMatch enabledFeatures []featuregate.Feature }{ + { + name: "invalid type updates allowed via v1beta1", + 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", + }, + }, + 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{"foo": {Type: "bogus"}}, + }, + }, + PreserveUnknownFields: pointer.BoolPtr(false), + }, + Status: apiextensions.CustomResourceDefinitionStatus{ + StoredVersions: []string{"version"}, + }, + }, + requestGV: apiextensionsv1beta1.SchemeGroupVersion, + }, + { + name: "invalid types updates disallowed via v1", + 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", + }, + }, + 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{"foo": {Type: "bogus"}}, + }, + }, + PreserveUnknownFields: pointer.BoolPtr(false), + }, + Status: apiextensions.CustomResourceDefinitionStatus{ + StoredVersions: []string{"version"}, + }, + }, + requestGV: apiextensionsv1.SchemeGroupVersion, + errors: []validationMatch{ + unsupported("spec.validation.openAPIV3Schema.properties[foo].type"), + }, + }, + { + name: "invalid types updates allowed via v1 if old object has invalid types", + 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: "bogus"}}, + }, + }, + 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{"foo": {Type: "bogus2"}}, + }, + }, + PreserveUnknownFields: pointer.BoolPtr(false), + }, + Status: apiextensions.CustomResourceDefinitionStatus{ + StoredVersions: []string{"version"}, + }, + }, + requestGV: apiextensionsv1.SchemeGroupVersion, + }, { name: "webhookconfig: should pass on invalid ConversionReviewVersion with old invalid versions", resource: &apiextensions.CustomResourceDefinition{ @@ -4318,6 +4537,39 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { mustBeStructural: true, wantError: false, }, + { + name: "require valid types, valid", + input: apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ + Type: "object", + }, + }, + opts: validationOptions{requireValidPropertyType: true}, + mustBeStructural: true, + wantError: false, + }, + { + name: "require valid types, invalid", + input: apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ + Type: "null", + }, + }, + opts: validationOptions{requireValidPropertyType: true}, + mustBeStructural: true, + wantError: true, + }, + { + name: "require valid types, invalid", + input: apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ + Type: "bogus", + }, + }, + opts: validationOptions{requireValidPropertyType: true}, + mustBeStructural: true, + wantError: true, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) {