From 998e22dd5c5e6d7a72c952bb62b4de2b01414f21 Mon Sep 17 00:00:00 2001 From: Mehdy Bohlool Date: Wed, 31 Oct 2018 11:14:10 -0700 Subject: [PATCH] FeatureGate and API Validation for CRD Webhook Conversion --- pkg/features/kube_features.go | 5 +- .../pkg/apis/apiextensions/fuzzer/fuzzer.go | 5 + .../apis/apiextensions/v1beta1/defaults.go | 5 + .../apiextensions/validation/validation.go | 57 ++++- .../validation/validation_test.go | 226 +++++++++++++++++- .../pkg/features/kube_features.go | 11 +- .../customresourcedefinition/strategy.go | 15 +- 7 files changed, 307 insertions(+), 17 deletions(-) diff --git a/pkg/features/kube_features.go b/pkg/features/kube_features.go index cbfbe76f12a..b4923dc3ff6 100644 --- a/pkg/features/kube_features.go +++ b/pkg/features/kube_features.go @@ -456,8 +456,9 @@ var defaultKubernetesFeatureGates = map[utilfeature.Feature]utilfeature.FeatureS // inherited features from apiextensions-apiserver, relisted here to get a conflict if it is changed // unintentionally on either side: - apiextensionsfeatures.CustomResourceValidation: {Default: true, PreRelease: utilfeature.Beta}, - apiextensionsfeatures.CustomResourceSubresources: {Default: true, PreRelease: utilfeature.Beta}, + apiextensionsfeatures.CustomResourceValidation: {Default: true, PreRelease: utilfeature.Beta}, + apiextensionsfeatures.CustomResourceSubresources: {Default: true, PreRelease: utilfeature.Beta}, + apiextensionsfeatures.CustomResourceWebhookConversion: {Default: false, PreRelease: utilfeature.Alpha}, // features that enable backwards compatibility but are scheduled to be removed // ... 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 ff8cc033469..b948e617917 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 @@ -61,6 +61,11 @@ func Funcs(codecs runtimeserializer.CodecFactory) []interface{} { {Name: "Age", Type: "date", Description: swaggerMetadataDescriptions["creationTimestamp"], JSONPath: ".metadata.creationTimestamp"}, } } + if obj.Conversion == nil { + obj.Conversion = &apiextensions.CustomResourceConversion{ + Strategy: apiextensions.NoneConverter, + } + } }, func(obj *apiextensions.CustomResourceDefinition, c fuzz.Continue) { c.FuzzNoCustom(obj) 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 e3235e8702c..2e3c2146a0a 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 @@ -71,4 +71,9 @@ func SetDefaults_CustomResourceDefinitionSpec(obj *CustomResourceDefinitionSpec) {Name: "Age", Type: "date", Description: swaggerMetadataDescriptions["creationTimestamp"], JSONPath: ".metadata.creationTimestamp"}, } } + if obj.Conversion == nil { + obj.Conversion = &CustomResourceConversion{ + Strategy: NoneConverter, + } + } } 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 0568bc18102..91b01ce70ed 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,6 +18,7 @@ package validation import ( "fmt" + "k8s.io/apiserver/pkg/util/webhook" "reflect" "strings" @@ -110,13 +111,7 @@ func ValidateCustomResourceDefinitionSpec(spec *apiextensions.CustomResourceDefi allErrs = append(allErrs, field.Invalid(fldPath.Child("group"), spec.Group, "should be a domain with at least one dot")) } - switch spec.Scope { - case "": - allErrs = append(allErrs, field.Required(fldPath.Child("scope"), "")) - case apiextensions.ClusterScoped, apiextensions.NamespaceScoped: - default: - allErrs = append(allErrs, field.NotSupported(fldPath.Child("scope"), spec.Scope, []string{string(apiextensions.ClusterScoped), string(apiextensions.NamespaceScoped)})) - } + allErrs = append(allErrs, validateEnumStrings(fldPath.Child("scope"), string(spec.Scope), []string{string(apiextensions.ClusterScoped), string(apiextensions.NamespaceScoped)}, true)...) storageFlagCount := 0 versionsMap := map[string]bool{} @@ -187,6 +182,54 @@ func ValidateCustomResourceDefinitionSpec(spec *apiextensions.CustomResourceDefi } } + allErrs = append(allErrs, ValidateCustomResourceConversion(spec.Conversion, fldPath.Child("conversion"))...) + + return allErrs +} + +func validateEnumStrings(fldPath *field.Path, value string, accepted []string, required bool) field.ErrorList { + if value == "" { + if required { + return field.ErrorList{field.Required(fldPath, "")} + } + return field.ErrorList{} + } + for _, a := range accepted { + if a == value { + return field.ErrorList{} + } + } + return field.ErrorList{field.NotSupported(fldPath, value, accepted)} +} + +// ValidateCustomResourceConversion statically validates +func ValidateCustomResourceConversion(conversion *apiextensions.CustomResourceConversion, fldPath *field.Path) field.ErrorList { + allErrs := field.ErrorList{} + if conversion == nil { + return allErrs + } + allErrs = append(allErrs, validateEnumStrings(fldPath.Child("strategy"), string(conversion.Strategy), []string{string(apiextensions.NoneConverter), string(apiextensions.WebhookConverter)}, true)...) + if conversion.Strategy == apiextensions.WebhookConverter { + if conversion.WebhookClientConfig == nil { + if utilfeature.DefaultFeatureGate.Enabled(apiextensionsfeatures.CustomResourceWebhookConversion) { + allErrs = append(allErrs, field.Required(fldPath.Child("webhookClientConfig"), "required when strategy is set to Webhook")) + } else { + allErrs = append(allErrs, field.Required(fldPath.Child("webhookClientConfig"), "required when strategy is set to Webhook, but not allowed because the CustomResourceWebhookConversion feature is disabled")) + } + } else { + cc := conversion.WebhookClientConfig + switch { + case (cc.URL == nil) == (cc.Service == nil): + allErrs = append(allErrs, field.Required(fldPath.Child("webhookClientConfig"), "exactly one of url or service is required")) + case cc.URL != nil: + allErrs = append(allErrs, webhook.ValidateWebhookURL(fldPath.Child("webhookClientConfig").Child("url"), *cc.URL, true)...) + case cc.Service != nil: + allErrs = append(allErrs, webhook.ValidateWebhookService(fldPath.Child("webhookClientConfig").Child("service"), cc.Service.Name, cc.Service.Namespace, cc.Service.Path)...) + } + } + } else if conversion.WebhookClientConfig != nil { + allErrs = append(allErrs, field.Forbidden(fldPath.Child("webhookClientConfig"), "should not be set when strategy is not set to Webhook")) + } return allErrs } 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 794142b7072..24cd454bb81 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 @@ -49,6 +49,8 @@ func (v validationMatch) matches(err *field.Error) bool { return err.Type == v.errorType && err.Field == v.path.String() } +func strPtr(s string) *string { return &s } + func TestValidateCustomResourceDefinition(t *testing.T) { singleVersionList := []apiextensions.CustomResourceDefinitionVersion{ { @@ -62,6 +64,205 @@ func TestValidateCustomResourceDefinition(t *testing.T) { resource *apiextensions.CustomResourceDefinition errors []validationMatch }{ + { + name: "webhookconfig: blank URL", + 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, + }, + { + Name: "version2", + Served: true, + Storage: false, + }, + }, + Conversion: &apiextensions.CustomResourceConversion{ + Strategy: apiextensions.ConversionStrategyType("Webhook"), + WebhookClientConfig: &apiextensions.WebhookClientConfig{ + URL: strPtr("https://example.com/webhook"), + Service: &apiextensions.ServiceReference{ + Name: "n", + Namespace: "ns", + }, + }, + }, + }, + Status: apiextensions.CustomResourceDefinitionStatus{ + StoredVersions: []string{"version"}, + }, + }, + errors: []validationMatch{ + required("spec", "conversion", "webhookClientConfig"), + }, + }, + { + name: "webhookconfig: both service and URL provided", + 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, + }, + { + Name: "version2", + Served: true, + Storage: false, + }, + }, + Conversion: &apiextensions.CustomResourceConversion{ + Strategy: apiextensions.ConversionStrategyType("Webhook"), + WebhookClientConfig: &apiextensions.WebhookClientConfig{ + URL: strPtr(""), + }, + }, + }, + Status: apiextensions.CustomResourceDefinitionStatus{ + StoredVersions: []string{"version"}, + }, + }, + errors: []validationMatch{ + invalid("spec", "conversion", "webhookClientConfig", "url"), + invalid("spec", "conversion", "webhookClientConfig", "url"), + }, + }, + { + name: "webhookconfig_should_not_be_set", + 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, + }, + { + Name: "version2", + Served: true, + Storage: false, + }, + }, + Conversion: &apiextensions.CustomResourceConversion{ + Strategy: apiextensions.ConversionStrategyType("None"), + WebhookClientConfig: &apiextensions.WebhookClientConfig{ + URL: strPtr("https://example.com/webhook"), + }, + }, + }, + Status: apiextensions.CustomResourceDefinitionStatus{ + StoredVersions: []string{"version"}, + }, + }, + errors: []validationMatch{ + forbidden("spec", "conversion", "webhookClientConfig"), + }, + }, + { + name: "missing_webhookconfig", + 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, + }, + { + Name: "version2", + Served: true, + Storage: false, + }, + }, + Conversion: &apiextensions.CustomResourceConversion{ + Strategy: apiextensions.ConversionStrategyType("Webhook"), + }, + }, + Status: apiextensions.CustomResourceDefinitionStatus{ + StoredVersions: []string{"version"}, + }, + }, + errors: []validationMatch{ + required("spec", "conversion", "webhookClientConfig"), + }, + }, + { + name: "invalid_conversion_strategy", + 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, + }, + { + Name: "version2", + Served: true, + Storage: false, + }, + }, + Conversion: &apiextensions.CustomResourceConversion{ + Strategy: apiextensions.ConversionStrategyType("non_existing_conversion"), + }, + }, + Status: apiextensions.CustomResourceDefinitionStatus{ + StoredVersions: []string{"version"}, + }, + }, + errors: []validationMatch{ + unsupported("spec", "conversion", "strategy"), + }, + }, { name: "no_storage_version", resource: &apiextensions.CustomResourceDefinition{ @@ -87,6 +288,9 @@ func TestValidateCustomResourceDefinition(t *testing.T) { Storage: false, }, }, + Conversion: &apiextensions.CustomResourceConversion{ + Strategy: apiextensions.ConversionStrategyType("None"), + }, }, Status: apiextensions.CustomResourceDefinitionStatus{ StoredVersions: []string{"version"}, @@ -121,6 +325,9 @@ func TestValidateCustomResourceDefinition(t *testing.T) { Storage: true, }, }, + Conversion: &apiextensions.CustomResourceConversion{ + Strategy: apiextensions.ConversionStrategyType("None"), + }, }, Status: apiextensions.CustomResourceDefinitionStatus{ StoredVersions: []string{"version"}, @@ -156,6 +363,9 @@ func TestValidateCustomResourceDefinition(t *testing.T) { Storage: true, }, }, + Conversion: &apiextensions.CustomResourceConversion{ + Strategy: apiextensions.ConversionStrategyType("None"), + }, }, Status: apiextensions.CustomResourceDefinitionStatus{ StoredVersions: []string{"version"}, @@ -185,6 +395,9 @@ func TestValidateCustomResourceDefinition(t *testing.T) { Storage: true, }, }, + Conversion: &apiextensions.CustomResourceConversion{ + Strategy: apiextensions.ConversionStrategyType("None"), + }, }, Status: apiextensions.CustomResourceDefinitionStatus{ StoredVersions: []string{}, @@ -283,6 +496,9 @@ func TestValidateCustomResourceDefinition(t *testing.T) { Group: "group.c(*&om", Version: "version", Versions: singleVersionList, + Conversion: &apiextensions.CustomResourceConversion{ + Strategy: apiextensions.ConversionStrategyType("None"), + }, Names: apiextensions.CustomResourceDefinitionNames{ Plural: "plural", Singular: "singular", @@ -316,7 +532,10 @@ func TestValidateCustomResourceDefinition(t *testing.T) { Group: "group.com", Version: "version", Versions: singleVersionList, - Scope: apiextensions.NamespaceScoped, + Conversion: &apiextensions.CustomResourceConversion{ + Strategy: apiextensions.ConversionStrategyType("None"), + }, + Scope: apiextensions.NamespaceScoped, Names: apiextensions.CustomResourceDefinitionNames{ Plural: "plural", Singular: "singular", @@ -348,7 +567,10 @@ func TestValidateCustomResourceDefinition(t *testing.T) { Group: "group.com", Version: "version", Versions: singleVersionList, - Scope: apiextensions.NamespaceScoped, + Conversion: &apiextensions.CustomResourceConversion{ + Strategy: apiextensions.ConversionStrategyType("None"), + }, + Scope: apiextensions.NamespaceScoped, Names: apiextensions.CustomResourceDefinitionNames{ Plural: "plural", Singular: "singular", diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/features/kube_features.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/features/kube_features.go index 24e72f91e59..0690e10d613 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/features/kube_features.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/features/kube_features.go @@ -40,6 +40,12 @@ const ( // // CustomResourceSubresources defines the subresources for CustomResources CustomResourceSubresources utilfeature.Feature = "CustomResourceSubresources" + + // owner: @mbohlool + // alpha: v1.13 + // + // CustomResourceWebhookConversion defines the webhook conversion for Custom Resources. + CustomResourceWebhookConversion utilfeature.Feature = "CustomResourceWebhookConversion" ) func init() { @@ -50,6 +56,7 @@ func init() { // To add a new feature, define a key for it above and add it here. The features will be // available throughout Kubernetes binaries. var defaultKubernetesFeatureGates = map[utilfeature.Feature]utilfeature.FeatureSpec{ - CustomResourceValidation: {Default: true, PreRelease: utilfeature.Beta}, - CustomResourceSubresources: {Default: true, PreRelease: utilfeature.Beta}, + CustomResourceValidation: {Default: true, PreRelease: utilfeature.Beta}, + CustomResourceSubresources: {Default: true, PreRelease: utilfeature.Beta}, + CustomResourceWebhookConversion: {Default: false, PreRelease: utilfeature.Alpha}, } 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 a0bebb7a7b0..8beeaa9d8f8 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 @@ -20,6 +20,9 @@ import ( "context" "fmt" + "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions" + "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation" + apiextensionsfeatures "k8s.io/apiextensions-apiserver/pkg/features" apiequality "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/fields" "k8s.io/apimachinery/pkg/labels" @@ -29,10 +32,6 @@ import ( "k8s.io/apiserver/pkg/storage" "k8s.io/apiserver/pkg/storage/names" utilfeature "k8s.io/apiserver/pkg/util/feature" - - "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions" - "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation" - apiextensionsfeatures "k8s.io/apiextensions-apiserver/pkg/features" ) // strategy implements behavior for CustomResources. @@ -62,6 +61,9 @@ func (strategy) PrepareForCreate(ctx context.Context, obj runtime.Object) { if !utilfeature.DefaultFeatureGate.Enabled(apiextensionsfeatures.CustomResourceSubresources) { crd.Spec.Subresources = nil } + if !utilfeature.DefaultFeatureGate.Enabled(apiextensionsfeatures.CustomResourceWebhookConversion) && crd.Spec.Conversion != nil { + crd.Spec.Conversion.WebhookClientConfig = nil + } for _, v := range crd.Spec.Versions { if v.Storage { @@ -99,6 +101,11 @@ func (strategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Object) { newCRD.Spec.Subresources = nil oldCRD.Spec.Subresources = nil } + if !utilfeature.DefaultFeatureGate.Enabled(apiextensionsfeatures.CustomResourceWebhookConversion) && newCRD.Spec.Conversion != nil { + if oldCRD.Spec.Conversion == nil || newCRD.Spec.Conversion.WebhookClientConfig == nil { + newCRD.Spec.Conversion.WebhookClientConfig = nil + } + } for _, v := range newCRD.Spec.Versions { if v.Storage {