From ff7f014b35525e7ee7c82b275124b452371155ca Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Fri, 9 Aug 2019 19:47:40 -0400 Subject: [PATCH] CRD v1: require structural schema for v1 --- .../apiextensions/validation/validation.go | 58 ++++- .../validation/validation_test.go | 220 ++++++++++++++++-- 2 files changed, 248 insertions(+), 30 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 3376b83b2b3..370319474dd 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 @@ -67,6 +67,7 @@ func ValidateCustomResourceDefinition(obj *apiextensions.CustomResourceDefinitio requireImmutableNames: false, requireOpenAPISchema: requireOpenAPISchema(requestGV, nil), requireValidPropertyType: requireValidPropertyType(requestGV, nil), + requireStructuralSchema: requireStructuralSchema(requestGV, nil), } allErrs := genericvalidation.ValidateObjectMeta(&obj.ObjectMeta, false, nameValidationFn, field.NewPath("metadata")) @@ -90,6 +91,8 @@ type validationOptions struct { requireOpenAPISchema bool // requireValidPropertyType requires property types specified in the validation schema to be valid openapi v3 types requireValidPropertyType bool + // requireStructuralSchema indicates that any schemas present must be structural + requireStructuralSchema bool } // ValidateCustomResourceDefinitionUpdate statically validates @@ -100,6 +103,7 @@ func ValidateCustomResourceDefinitionUpdate(obj, oldObj *apiextensions.CustomRes requireImmutableNames: apiextensions.IsCRDConditionTrue(oldObj, apiextensions.Established), requireOpenAPISchema: requireOpenAPISchema(requestGV, &oldObj.Spec), requireValidPropertyType: requireValidPropertyType(requestGV, &oldObj.Spec), + requireStructuralSchema: requireStructuralSchema(requestGV, &oldObj.Spec), } allErrs := genericvalidation.ValidateObjectMetaUpdate(&obj.ObjectMeta, &oldObj.ObjectMeta, field.NewPath("metadata")) @@ -146,9 +150,9 @@ func ValidateUpdateCustomResourceDefinitionStatus(obj, oldObj *apiextensions.Cus } // validateCustomResourceDefinitionVersion statically validates. -func validateCustomResourceDefinitionVersion(version *apiextensions.CustomResourceDefinitionVersion, fldPath *field.Path, mustBeStructural, statusEnabled bool, opts validationOptions) field.ErrorList { +func validateCustomResourceDefinitionVersion(version *apiextensions.CustomResourceDefinitionVersion, fldPath *field.Path, statusEnabled bool, opts validationOptions) field.ErrorList { allErrs := field.ErrorList{} - allErrs = append(allErrs, validateCustomResourceDefinitionValidation(version.Schema, mustBeStructural, statusEnabled, opts, fldPath.Child("schema"))...) + allErrs = append(allErrs, validateCustomResourceDefinitionValidation(version.Schema, statusEnabled, opts, fldPath.Child("schema"))...) allErrs = append(allErrs, ValidateCustomResourceDefinitionSubresources(version.Subresources, fldPath.Child("subresources"))...) for i := range version.AdditionalPrinterColumns { allErrs = append(allErrs, ValidateCustomResourceColumnDefinition(&version.AdditionalPrinterColumns[i], fldPath.Child("additionalPrinterColumns").Index(i))...) @@ -170,9 +174,8 @@ func validateCustomResourceDefinitionSpec(spec *apiextensions.CustomResourceDefi allErrs = append(allErrs, validateEnumStrings(fldPath.Child("scope"), string(spec.Scope), []string{string(apiextensions.ClusterScoped), string(apiextensions.NamespaceScoped)}, true)...) // enabling pruning requires structural schemas - mustBeStructural := false if spec.PreserveUnknownFields == nil || *spec.PreserveUnknownFields == false { - mustBeStructural = true + opts.requireStructuralSchema = true } if opts.requireOpenAPISchema { @@ -196,13 +199,13 @@ func validateCustomResourceDefinitionSpec(spec *apiextensions.CustomResourceDefi } } if opts.allowDefaults && specHasDefaults(spec) { - mustBeStructural = true + opts.requireStructuralSchema = true if spec.PreserveUnknownFields == nil || *spec.PreserveUnknownFields == true { 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 + opts.requireStructuralSchema = true } storageFlagCount := 0 @@ -221,7 +224,7 @@ func validateCustomResourceDefinitionSpec(spec *apiextensions.CustomResourceDefi allErrs = append(allErrs, field.Invalid(fldPath.Child("versions").Index(i).Child("name"), spec.Versions[i].Name, strings.Join(errs, ","))) } subresources := getSubresourcesForVersion(spec, version.Name) - allErrs = append(allErrs, validateCustomResourceDefinitionVersion(&version, fldPath.Child("versions").Index(i), mustBeStructural, hasStatusEnabled(subresources), opts)...) + allErrs = append(allErrs, validateCustomResourceDefinitionVersion(&version, fldPath.Child("versions").Index(i), hasStatusEnabled(subresources), opts)...) } // The top-level and per-version fields are mutual exclusive @@ -276,7 +279,7 @@ func validateCustomResourceDefinitionSpec(spec *apiextensions.CustomResourceDefi } allErrs = append(allErrs, ValidateCustomResourceDefinitionNames(&spec.Names, fldPath.Child("names"))...) - allErrs = append(allErrs, validateCustomResourceDefinitionValidation(spec.Validation, mustBeStructural, hasAnyStatusEnabled(spec), opts, fldPath.Child("validation"))...) + allErrs = append(allErrs, validateCustomResourceDefinitionValidation(spec.Validation, hasAnyStatusEnabled(spec), opts, fldPath.Child("validation"))...) allErrs = append(allErrs, ValidateCustomResourceDefinitionSubresources(spec.Subresources, fldPath.Child("subresources"))...) for i := range spec.AdditionalPrinterColumns { @@ -613,7 +616,7 @@ type specStandardValidator interface { } // validateCustomResourceDefinitionValidation statically validates -func validateCustomResourceDefinitionValidation(customResourceValidation *apiextensions.CustomResourceValidation, mustBeStructural, statusSubresourceEnabled bool, opts validationOptions, fldPath *field.Path) field.ErrorList { +func validateCustomResourceDefinitionValidation(customResourceValidation *apiextensions.CustomResourceValidation, statusSubresourceEnabled bool, opts validationOptions, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} if customResourceValidation == nil { @@ -659,7 +662,7 @@ func validateCustomResourceDefinitionValidation(customResourceValidation *apiext } allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(schema, fldPath.Child("openAPIV3Schema"), openAPIV3Schema, true)...) - if mustBeStructural { + if opts.requireStructuralSchema { if ss, err := structuralschema.NewStructural(schema); err != nil { // if the generic schema validation did its job, we should never get an error here. Hence, we hide it if there are validation errors already. if len(allErrs) == 0 { @@ -1178,6 +1181,41 @@ func schemaHasKubernetesExtensions(s *apiextensions.JSONSchemaProps) bool { return false } +// requireStructuralSchema returns true if schemas specified must be structural +func requireStructuralSchema(requestGV schema.GroupVersion, oldCRDSpec *apiextensions.CustomResourceDefinitionSpec) bool { + if requestGV == v1beta1.SchemeGroupVersion { + // for compatibility + return false + } + if oldCRDSpec != nil && specHasNonStructuralSchema(oldCRDSpec) { + // don't tighten validation on existing persisted data + return false + } + return true +} + +func specHasNonStructuralSchema(spec *apiextensions.CustomResourceDefinitionSpec) bool { + if spec.Validation != nil && schemaIsNonStructural(spec.Validation.OpenAPIV3Schema) { + return true + } + for _, v := range spec.Versions { + if v.Schema != nil && schemaIsNonStructural(v.Schema.OpenAPIV3Schema) { + return true + } + } + return false +} +func schemaIsNonStructural(schema *apiextensions.JSONSchemaProps) bool { + if schema == nil { + return false + } + ss, err := structuralschema.NewStructural(schema) + if err != nil { + return true + } + return len(structuralschema.ValidateStructural(ss, nil)) > 0 +} + // 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 { 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 2ac892a4ed5..ded624aafe6 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 @@ -2764,6 +2764,190 @@ func TestValidateCustomResourceDefinitionUpdate(t *testing.T) { }, requestGV: apiextensionsv1.SchemeGroupVersion, }, + { + name: "structural to non-structural 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", + Properties: map[string]apiextensions.JSONSchemaProps{"foo": {Type: "integer"}}, + }, + }, + PreserveUnknownFields: pointer.BoolPtr(true), + }, + 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": {}}, // untyped object + }, + }, + PreserveUnknownFields: pointer.BoolPtr(true), + }, + Status: apiextensions.CustomResourceDefinitionStatus{ + StoredVersions: []string{"version"}, + }, + }, + requestGV: apiextensionsv1beta1.SchemeGroupVersion, + }, + { + name: "structural to non-structural updates not allowed 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", + Properties: map[string]apiextensions.JSONSchemaProps{"foo": {Type: "integer"}}, + }, + }, + PreserveUnknownFields: pointer.BoolPtr(true), + }, + 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": {}}, // untyped object + }, + }, + PreserveUnknownFields: pointer.BoolPtr(true), + }, + Status: apiextensions.CustomResourceDefinitionStatus{ + StoredVersions: []string{"version"}, + }, + }, + requestGV: apiextensionsv1.SchemeGroupVersion, + errors: []validationMatch{ + required("spec.validation.openAPIV3Schema.properties[foo].type"), + }, + }, + { + name: "absent schema to non-structural updates not allowed 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{}, + PreserveUnknownFields: pointer.BoolPtr(true), + }, + 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": {}}, // untyped object + }, + }, + PreserveUnknownFields: pointer.BoolPtr(true), + }, + Status: apiextensions.CustomResourceDefinitionStatus{ + StoredVersions: []string{"version"}, + }, + }, + requestGV: apiextensionsv1.SchemeGroupVersion, + errors: []validationMatch{ + required("spec.validation.openAPIV3Schema.properties[foo].type"), + }, + }, + { + name: "non-structural updates allowed via v1 if old object has non-structural schema", + 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, Schema: &apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ + Type: "object", + Properties: map[string]apiextensions.JSONSchemaProps{"foo": {}}, // untyped object, non-structural + }, + }}, + {Name: "version2", Served: true, Storage: false, Schema: &apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ + Type: "object", + Properties: map[string]apiextensions.JSONSchemaProps{"foo": {Type: "number"}}, // structural + }, + }}, + }, + PreserveUnknownFields: pointer.BoolPtr(true), + }, + 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, Schema: &apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ + Type: "object", + Properties: map[string]apiextensions.JSONSchemaProps{"foo": {Description: "b"}}, // untyped object, non-structural + }, + }}, + {Name: "version2", Served: true, Storage: false, Schema: &apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ + Type: "object", + Properties: map[string]apiextensions.JSONSchemaProps{"foo": {Description: "a"}}, // untyped object, non-structural + }, + }}, + }, + PreserveUnknownFields: pointer.BoolPtr(true), + }, + Status: apiextensions.CustomResourceDefinitionStatus{ + StoredVersions: []string{"version"}, + }, + }, + requestGV: apiextensionsv1.SchemeGroupVersion, + errors: []validationMatch{}, + }, { name: "webhookconfig: should pass on invalid ConversionReviewVersion with old invalid versions", resource: &apiextensions.CustomResourceDefinition{ @@ -4394,12 +4578,11 @@ func TestValidateCustomResourceDefinitionUpdate(t *testing.T) { func TestValidateCustomResourceDefinitionValidation(t *testing.T) { tests := []struct { - name string - input apiextensions.CustomResourceValidation - mustBeStructural bool - statusEnabled bool - opts validationOptions - wantError bool + name string + input apiextensions.CustomResourceValidation + statusEnabled bool + opts validationOptions + wantError bool }{ { name: "empty", @@ -4524,8 +4707,8 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { input: apiextensions.CustomResourceValidation{ OpenAPIV3Schema: &apiextensions.JSONSchemaProps{}, }, - mustBeStructural: true, - wantError: true, + opts: validationOptions{requireStructuralSchema: true}, + wantError: true, }, { name: "must be structural", @@ -4534,8 +4717,8 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { Type: "object", }, }, - mustBeStructural: true, - wantError: false, + opts: validationOptions{requireStructuralSchema: true}, + wantError: false, }, { name: "require valid types, valid", @@ -4544,9 +4727,8 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { Type: "object", }, }, - opts: validationOptions{requireValidPropertyType: true}, - mustBeStructural: true, - wantError: false, + opts: validationOptions{requireValidPropertyType: true, requireStructuralSchema: true}, + wantError: false, }, { name: "require valid types, invalid", @@ -4555,9 +4737,8 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { Type: "null", }, }, - opts: validationOptions{requireValidPropertyType: true}, - mustBeStructural: true, - wantError: true, + opts: validationOptions{requireValidPropertyType: true, requireStructuralSchema: true}, + wantError: true, }, { name: "require valid types, invalid", @@ -4566,14 +4747,13 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { Type: "bogus", }, }, - opts: validationOptions{requireValidPropertyType: true}, - mustBeStructural: true, - wantError: true, + opts: validationOptions{requireValidPropertyType: true, requireStructuralSchema: true}, + wantError: true, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got := validateCustomResourceDefinitionValidation(&tt.input, tt.mustBeStructural, tt.statusEnabled, tt.opts, field.NewPath("spec", "validation")) + got := validateCustomResourceDefinitionValidation(&tt.input, tt.statusEnabled, tt.opts, field.NewPath("spec", "validation")) if !tt.wantError && len(got) > 0 { t.Errorf("Expected no error, but got: %v", got) } else if tt.wantError && len(got) == 0 {