diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/defaults.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/defaults.go index 2e3c2146a0a..5aae97cf1a2 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/defaults.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/defaults.go @@ -19,12 +19,9 @@ package v1beta1 import ( "strings" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" ) -var swaggerMetadataDescriptions = metav1.ObjectMeta{}.SwaggerDoc() - func addDefaultingFuncs(scheme *runtime.Scheme) error { scheme.AddTypeDefaultingFunc(&CustomResourceDefinition{}, func(obj interface{}) { SetDefaults_CustomResourceDefinition(obj.(*CustomResourceDefinition)) }) // TODO figure out why I can't seem to get my defaulter generated @@ -66,14 +63,19 @@ func SetDefaults_CustomResourceDefinitionSpec(obj *CustomResourceDefinitionSpec) if len(obj.Version) == 0 && len(obj.Versions) != 0 { obj.Version = obj.Versions[0].Name } - if len(obj.AdditionalPrinterColumns) == 0 { - obj.AdditionalPrinterColumns = []CustomResourceColumnDefinition{ - {Name: "Age", Type: "date", Description: swaggerMetadataDescriptions["creationTimestamp"], JSONPath: ".metadata.creationTimestamp"}, - } - } if obj.Conversion == nil { obj.Conversion = &CustomResourceConversion{ Strategy: NoneConverter, } } } + +// hasPerVersionColumns returns true if a CRD uses per-version columns. +func hasPerVersionColumns(versions []CustomResourceDefinitionVersion) bool { + for _, v := range versions { + if len(v.AdditionalPrinterColumns) > 0 { + return true + } + } + return false +} 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 91b01ce70ed..4f16ccce878 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 @@ -18,15 +18,16 @@ package validation import ( "fmt" - "k8s.io/apiserver/pkg/util/webhook" "reflect" "strings" + apiequality "k8s.io/apimachinery/pkg/api/equality" genericvalidation "k8s.io/apimachinery/pkg/api/validation" "k8s.io/apimachinery/pkg/util/sets" validationutil "k8s.io/apimachinery/pkg/util/validation" "k8s.io/apimachinery/pkg/util/validation/field" utilfeature "k8s.io/apiserver/pkg/util/feature" + "k8s.io/apiserver/pkg/util/webhook" "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions" apiservervalidation "k8s.io/apiextensions-apiserver/pkg/apiserver/validation" @@ -99,6 +100,17 @@ func ValidateUpdateCustomResourceDefinitionStatus(obj, oldObj *apiextensions.Cus return allErrs } +// ValidateCustomResourceDefinitionVersion statically validates. +func ValidateCustomResourceDefinitionVersion(version *apiextensions.CustomResourceDefinitionVersion, fldPath *field.Path, statusEnabled bool) field.ErrorList { + allErrs := field.ErrorList{} + allErrs = append(allErrs, ValidateCustomResourceDefinitionValidation(version.Schema, statusEnabled, 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))...) + } + return allErrs +} + // ValidateCustomResourceDefinitionSpec statically validates func ValidateCustomResourceDefinitionSpec(spec *apiextensions.CustomResourceDefinitionSpec, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} @@ -128,7 +140,32 @@ func ValidateCustomResourceDefinitionSpec(spec *apiextensions.CustomResourceDefi if errs := validationutil.IsDNS1035Label(version.Name); len(errs) > 0 { 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), hasStatusEnabled(subresources))...) } + + // The top-level and per-version fields are mutual exclusive + if spec.Validation != nil && hasPerVersionSchema(spec.Versions) { + allErrs = append(allErrs, field.Forbidden(fldPath.Child("validation"), "top-level and per-version schemas are mutually exclusive")) + } + if spec.Subresources != nil && hasPerVersionSubresources(spec.Versions) { + allErrs = append(allErrs, field.Forbidden(fldPath.Child("subresources"), "top-level and per-version subresources are mutually exclusive")) + } + if len(spec.AdditionalPrinterColumns) > 0 && hasPerVersionColumns(spec.Versions) { + allErrs = append(allErrs, field.Forbidden(fldPath.Child("additionalPrinterColumns"), "top-level and per-version additionalPrinterColumns are mutually exclusive")) + } + + // Per-version fields may not all be set to identical values (top-level field should be used instead) + if hasIdenticalPerVersionSchema(spec.Versions) { + allErrs = append(allErrs, field.Invalid(fldPath.Child("versions"), spec.Versions, "per-version schemas may not all be set to identical values (top-level validation should be used instead)")) + } + if hasIdenticalPerVersionSubresources(spec.Versions) { + allErrs = append(allErrs, field.Invalid(fldPath.Child("versions"), spec.Versions, "per-version subresources may not all be set to identical values (top-level subresources should be used instead)")) + } + if hasIdenticalPerVersionColumns(spec.Versions) { + allErrs = append(allErrs, field.Invalid(fldPath.Child("versions"), spec.Versions, "per-version additionalPrinterColumns may not all be set to identical values (top-level additionalPrinterColumns should be used instead)")) + } + if !uniqueNames { allErrs = append(allErrs, field.Invalid(fldPath.Child("versions"), spec.Versions, "must contain unique version names")) } @@ -161,11 +198,7 @@ func ValidateCustomResourceDefinitionSpec(spec *apiextensions.CustomResourceDefi allErrs = append(allErrs, ValidateCustomResourceDefinitionNames(&spec.Names, fldPath.Child("names"))...) if utilfeature.DefaultFeatureGate.Enabled(apiextensionsfeatures.CustomResourceValidation) { - statusEnabled := false - if spec.Subresources != nil && spec.Subresources.Status != nil { - statusEnabled = true - } - allErrs = append(allErrs, ValidateCustomResourceDefinitionValidation(spec.Validation, statusEnabled, fldPath.Child("validation"))...) + allErrs = append(allErrs, ValidateCustomResourceDefinitionValidation(spec.Validation, hasAnyStatusEnabled(spec), fldPath.Child("validation"))...) } else if spec.Validation != nil { allErrs = append(allErrs, field.Forbidden(fldPath.Child("validation"), "disabled by feature-gate CustomResourceValidation")) } @@ -177,7 +210,7 @@ func ValidateCustomResourceDefinitionSpec(spec *apiextensions.CustomResourceDefi } for i := range spec.AdditionalPrinterColumns { - if errs := ValidateCustomResourceColumnDefinition(&spec.AdditionalPrinterColumns[i], fldPath.Child("columns").Index(i)); len(errs) > 0 { + if errs := ValidateCustomResourceColumnDefinition(&spec.AdditionalPrinterColumns[i], fldPath.Child("additionalPrinterColumns").Index(i)); len(errs) > 0 { allErrs = append(allErrs, errs...) } } @@ -250,6 +283,118 @@ func ValidateCustomResourceDefinitionSpecUpdate(spec, oldSpec *apiextensions.Cus return allErrs } +// getSubresourcesForVersion returns the subresources for given version in given CRD spec. +// NOTE That this function assumes version always exist since it's used by the validation process +// that iterates through the existing versions. +func getSubresourcesForVersion(crd *apiextensions.CustomResourceDefinitionSpec, version string) *apiextensions.CustomResourceSubresources { + if !hasPerVersionSubresources(crd.Versions) { + return crd.Subresources + } + for _, v := range crd.Versions { + if version == v.Name { + return v.Subresources + } + } + return nil +} + +// hasAnyStatusEnabled returns true if given CRD spec has at least one Status Subresource set +// among the top-level and per-version Subresources. +func hasAnyStatusEnabled(crd *apiextensions.CustomResourceDefinitionSpec) bool { + if hasStatusEnabled(crd.Subresources) { + return true + } + for _, v := range crd.Versions { + if hasStatusEnabled(v.Subresources) { + return true + } + } + return false +} + +// hasStatusEnabled returns true if given CRD Subresources has non-nil Status set. +func hasStatusEnabled(subresources *apiextensions.CustomResourceSubresources) bool { + if subresources != nil && subresources.Status != nil { + return true + } + return false +} + +// hasPerVersionSchema returns true if a CRD uses per-version schema. +func hasPerVersionSchema(versions []apiextensions.CustomResourceDefinitionVersion) bool { + for _, v := range versions { + if v.Schema != nil { + return true + } + } + return false +} + +// hasPerVersionSubresources returns true if a CRD uses per-version subresources. +func hasPerVersionSubresources(versions []apiextensions.CustomResourceDefinitionVersion) bool { + for _, v := range versions { + if v.Subresources != nil { + return true + } + } + return false +} + +// hasPerVersionColumns returns true if a CRD uses per-version columns. +func hasPerVersionColumns(versions []apiextensions.CustomResourceDefinitionVersion) bool { + for _, v := range versions { + if len(v.AdditionalPrinterColumns) > 0 { + return true + } + } + return false +} + +// hasIdenticalPerVersionSchema returns true if a CRD sets identical non-nil values +// to all per-version schemas +func hasIdenticalPerVersionSchema(versions []apiextensions.CustomResourceDefinitionVersion) bool { + if len(versions) == 0 { + return false + } + value := versions[0].Schema + for _, v := range versions { + if v.Schema == nil || !apiequality.Semantic.DeepEqual(v.Schema, value) { + return false + } + } + return true +} + +// hasIdenticalPerVersionSubresources returns true if a CRD sets identical non-nil values +// to all per-version subresources +func hasIdenticalPerVersionSubresources(versions []apiextensions.CustomResourceDefinitionVersion) bool { + if len(versions) == 0 { + return false + } + value := versions[0].Subresources + for _, v := range versions { + if v.Subresources == nil || !apiequality.Semantic.DeepEqual(v.Subresources, value) { + return false + } + } + return true +} + +// hasIdenticalPerVersionColumns returns true if a CRD sets identical non-nil values +// to all per-version columns +func hasIdenticalPerVersionColumns(versions []apiextensions.CustomResourceDefinitionVersion) bool { + if len(versions) == 0 { + return false + } + value := versions[0].AdditionalPrinterColumns + for _, v := range versions { + if len(v.AdditionalPrinterColumns) == 0 || !apiequality.Semantic.DeepEqual(v.AdditionalPrinterColumns, value) { + return false + } + } + return true +} + // ValidateCustomResourceDefinitionStatus statically validates func ValidateCustomResourceDefinitionStatus(status *apiextensions.CustomResourceDefinitionStatus, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} 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 24cd454bb81..692aa8b0c0a 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 @@ -594,6 +594,55 @@ func TestValidateCustomResourceDefinition(t *testing.T) { }, errors: []validationMatch{}, }, + { + name: "per-version fields may not all be set to identical values (top-level field should be used instead)", + resource: &apiextensions.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{Name: "plural.group.com"}, + Spec: apiextensions.CustomResourceDefinitionSpec{ + Group: "group.com", + Version: "version", + Versions: []apiextensions.CustomResourceDefinitionVersion{ + { + Name: "version", + Served: true, + Storage: true, + Schema: &apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: validValidationSchema, + }, + Subresources: &apiextensions.CustomResourceSubresources{}, + AdditionalPrinterColumns: []apiextensions.CustomResourceColumnDefinition{{Name: "Alpha", Type: "string", JSONPath: ".spec.alpha"}}, + }, + { + Name: "version2", + Served: true, + Storage: false, + Schema: &apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: validValidationSchema, + }, + Subresources: &apiextensions.CustomResourceSubresources{}, + AdditionalPrinterColumns: []apiextensions.CustomResourceColumnDefinition{{Name: "Alpha", Type: "string", JSONPath: ".spec.alpha"}}, + }, + }, + Scope: apiextensions.NamespaceScoped, + Names: apiextensions.CustomResourceDefinitionNames{ + Plural: "plural", + Singular: "singular", + Kind: "Plural", + ListKind: "PluralList", + }, + }, + Status: apiextensions.CustomResourceDefinitionStatus{ + StoredVersions: []string{"version"}, + }, + }, + errors: []validationMatch{ + // Per-version schema/subresources/columns may not all be set to identical values. + // Note that the test will fail if we de-duplicate the expected errors below. + invalid("spec", "versions"), + invalid("spec", "versions"), + invalid("spec", "versions"), + }, + }, } for _, tc := range tests { @@ -1003,6 +1052,87 @@ func TestValidateCustomResourceDefinitionUpdate(t *testing.T) { immutable("spec", "names", "plural"), }, }, + { + name: "top-level and per-version fields are mutually exclusive", + old: &apiextensions.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{ + Name: "plural.group.com", + ResourceVersion: "42", + }, + Spec: apiextensions.CustomResourceDefinitionSpec{ + Group: "group.com", + Version: "version", + Versions: []apiextensions.CustomResourceDefinitionVersion{ + { + Name: "version", + Served: true, + Storage: true, + Subresources: &apiextensions.CustomResourceSubresources{}, + }, + { + Name: "version2", + Served: true, + Storage: false, + }, + }, + Scope: apiextensions.NamespaceScoped, + Names: apiextensions.CustomResourceDefinitionNames{ + Plural: "plural", + Singular: "singular", + Kind: "Plural", + ListKind: "PluralList", + }, + }, + Status: apiextensions.CustomResourceDefinitionStatus{ + StoredVersions: []string{"version"}, + }, + }, + resource: &apiextensions.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{ + Name: "plural.group.com", + ResourceVersion: "42", + }, + Spec: apiextensions.CustomResourceDefinitionSpec{ + Group: "group.com", + Version: "version", + Versions: []apiextensions.CustomResourceDefinitionVersion{ + { + Name: "version", + Served: true, + Storage: true, + }, + { + Name: "version2", + Served: true, + Storage: false, + Schema: &apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: validValidationSchema, + }, + Subresources: &apiextensions.CustomResourceSubresources{}, + AdditionalPrinterColumns: []apiextensions.CustomResourceColumnDefinition{{Name: "Alpha", Type: "string", JSONPath: ".spec.alpha"}}, + }, + }, + Validation: &apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: validValidationSchema, + }, + Subresources: &apiextensions.CustomResourceSubresources{}, + Scope: apiextensions.NamespaceScoped, + Names: apiextensions.CustomResourceDefinitionNames{ + Plural: "plural", + Singular: "singular", + Kind: "Plural", + ListKind: "PluralList", + }, + }, + Status: apiextensions.CustomResourceDefinitionStatus{ + StoredVersions: []string{"version"}, + }, + }, + errors: []validationMatch{ + forbidden("spec", "validation"), + forbidden("spec", "subresources"), + }, + }, } for _, tc := range tests { @@ -1090,36 +1220,7 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { { name: "all allowed fields at the root of the schema with status", input: apiextensions.CustomResourceValidation{ - OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ - Description: "This is a description", - Type: "object", - Format: "date-time", - Title: "This is a title", - Maximum: float64Ptr(10), - ExclusiveMaximum: true, - Minimum: float64Ptr(5), - ExclusiveMinimum: true, - MaxLength: int64Ptr(10), - MinLength: int64Ptr(5), - Pattern: "^[a-z]$", - MaxItems: int64Ptr(10), - MinItems: int64Ptr(5), - MultipleOf: float64Ptr(3), - Required: []string{"spec", "status"}, - Items: &apiextensions.JSONSchemaPropsOrArray{ - Schema: &apiextensions.JSONSchemaProps{ - Description: "This is a schema nested under Items", - }, - }, - Properties: map[string]apiextensions.JSONSchemaProps{ - "spec": {}, - "status": {}, - }, - ExternalDocs: &apiextensions.ExternalDocumentation{ - Description: "This is an external documentation description", - }, - Example: &example, - }, + OpenAPIV3Schema: validValidationSchema, }, statusEnabled: true, wantError: false, @@ -1139,6 +1240,37 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { var example = apiextensions.JSON(`"This is an example"`) +var validValidationSchema = &apiextensions.JSONSchemaProps{ + Description: "This is a description", + Type: "object", + Format: "date-time", + Title: "This is a title", + Maximum: float64Ptr(10), + ExclusiveMaximum: true, + Minimum: float64Ptr(5), + ExclusiveMinimum: true, + MaxLength: int64Ptr(10), + MinLength: int64Ptr(5), + Pattern: "^[a-z]$", + MaxItems: int64Ptr(10), + MinItems: int64Ptr(5), + MultipleOf: float64Ptr(3), + Required: []string{"spec", "status"}, + Items: &apiextensions.JSONSchemaPropsOrArray{ + Schema: &apiextensions.JSONSchemaProps{ + Description: "This is a schema nested under Items", + }, + }, + Properties: map[string]apiextensions.JSONSchemaProps{ + "spec": {}, + "status": {}, + }, + ExternalDocs: &apiextensions.ExternalDocumentation{ + Description: "This is an external documentation description", + }, + Example: &example, +} + func float64Ptr(f float64) *float64 { return &f } diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresourcedefinition/strategy.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresourcedefinition/strategy.go index 8beeaa9d8f8..ee3d8eaf042 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresourcedefinition/strategy.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresourcedefinition/strategy.go @@ -57,9 +57,25 @@ func (strategy) PrepareForCreate(ctx context.Context, obj runtime.Object) { // if the feature gate is disabled, drop the feature. if !utilfeature.DefaultFeatureGate.Enabled(apiextensionsfeatures.CustomResourceValidation) { crd.Spec.Validation = nil + for i := range crd.Spec.Versions { + crd.Spec.Versions[i].Schema = nil + } } if !utilfeature.DefaultFeatureGate.Enabled(apiextensionsfeatures.CustomResourceSubresources) { crd.Spec.Subresources = nil + for i := range crd.Spec.Versions { + crd.Spec.Versions[i].Subresources = nil + } + } + // On CREATE, if the CustomResourceWebhookConversion feature gate is off, we auto-clear + // the per-version fields. This is to be consistent with the other built-in types, as the + // apiserver drops unknown fields. + if !utilfeature.DefaultFeatureGate.Enabled(apiextensionsfeatures.CustomResourceWebhookConversion) { + for i := range crd.Spec.Versions { + crd.Spec.Versions[i].Schema = nil + crd.Spec.Versions[i].Subresources = nil + crd.Spec.Versions[i].AdditionalPrinterColumns = nil + } } if !utilfeature.DefaultFeatureGate.Enabled(apiextensionsfeatures.CustomResourceWebhookConversion) && crd.Spec.Conversion != nil { crd.Spec.Conversion.WebhookClientConfig = nil @@ -96,10 +112,36 @@ func (strategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Object) { if !utilfeature.DefaultFeatureGate.Enabled(apiextensionsfeatures.CustomResourceValidation) { newCRD.Spec.Validation = nil oldCRD.Spec.Validation = nil + for i := range newCRD.Spec.Versions { + newCRD.Spec.Versions[i].Schema = nil + } + for i := range oldCRD.Spec.Versions { + oldCRD.Spec.Versions[i].Schema = nil + } } if !utilfeature.DefaultFeatureGate.Enabled(apiextensionsfeatures.CustomResourceSubresources) { newCRD.Spec.Subresources = nil oldCRD.Spec.Subresources = nil + for i := range newCRD.Spec.Versions { + newCRD.Spec.Versions[i].Subresources = nil + } + for i := range oldCRD.Spec.Versions { + oldCRD.Spec.Versions[i].Subresources = nil + } + } + + // On UPDATE, if the CustomResourceWebhookConversion feature gate is off, we auto-clear + // the per-version fields if the old CRD doesn't use per-version fields already. + // This is to be consistent with the other built-in types, as the apiserver drops unknown + // fields. If the old CRD already uses per-version fields, the CRD is allowed to continue + // use per-version fields. + if !utilfeature.DefaultFeatureGate.Enabled(apiextensionsfeatures.CustomResourceWebhookConversion) && + !hasPerVersionField(oldCRD.Spec.Versions) { + for i := range newCRD.Spec.Versions { + newCRD.Spec.Versions[i].Schema = nil + newCRD.Spec.Versions[i].Subresources = nil + newCRD.Spec.Versions[i].AdditionalPrinterColumns = nil + } } if !utilfeature.DefaultFeatureGate.Enabled(apiextensionsfeatures.CustomResourceWebhookConversion) && newCRD.Spec.Conversion != nil { if oldCRD.Spec.Conversion == nil || newCRD.Spec.Conversion.WebhookClientConfig == nil { @@ -117,6 +159,16 @@ func (strategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Object) { } } +// hasPerVersionField returns true if a CRD uses per-version schema/subresources/columns fields. +func hasPerVersionField(versions []apiextensions.CustomResourceDefinitionVersion) bool { + for _, v := range versions { + if v.Schema != nil || v.Subresources != nil || len(v.AdditionalPrinterColumns) > 0 { + return true + } + } + return false +} + // Validate validates a new CustomResourceDefinition. func (strategy) Validate(ctx context.Context, obj runtime.Object) field.ErrorList { return validation.ValidateCustomResourceDefinition(obj.(*apiextensions.CustomResourceDefinition))