From 59079e554adc66100d60e2ace84e4291610866d3 Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Wed, 7 Aug 2019 13:15:56 -0400 Subject: [PATCH 1/2] Move CRD approval validation into validation package --- .../pkg/apis/apiextensions/validation/BUILD | 3 + .../apiextensions/validation/validation.go | 49 +++++++++++++++- .../validation/validation_test.go | 34 +++++++++-- .../registry/customresourcedefinition/BUILD | 7 ++- .../customresourcedefinition/strategy.go | 56 ++++--------------- .../customresourcedefinition/strategy_test.go | 56 ++++++++++++++++--- 6 files changed, 143 insertions(+), 62 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/BUILD b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/BUILD index 43e7f86bb39..390653b2217 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/BUILD +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/BUILD @@ -12,6 +12,7 @@ go_library( importmap = "k8s.io/kubernetes/vendor/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation", importpath = "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation", deps = [ + "//staging/src/k8s.io/apiextensions-apiserver/pkg/apihelpers:go_default_library", "//staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions:go_default_library", "//staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1:go_default_library", "//staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema:go_default_library", @@ -22,6 +23,7 @@ go_library( "//staging/src/k8s.io/apimachinery/pkg/api/equality:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/api/validation:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/runtime/schema:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/sets:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/validation:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/validation/field:go_default_library", @@ -44,6 +46,7 @@ go_test( "//staging/src/k8s.io/apimachinery/pkg/api/apitesting/fuzzer:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/runtime/schema:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/runtime/serializer:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/json:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/validation/field:go_default_library", 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 43bb7350f81..a1b4f538707 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 @@ -25,9 +25,11 @@ import ( govalidate "github.com/go-openapi/validate" schemaobjectmeta "k8s.io/apiextensions-apiserver/pkg/apiserver/schema/objectmeta" + "k8s.io/apiextensions-apiserver/pkg/apihelpers" apiequality "k8s.io/apimachinery/pkg/api/equality" genericvalidation "k8s.io/apimachinery/pkg/api/validation" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/util/sets" utilvalidation "k8s.io/apimachinery/pkg/util/validation" "k8s.io/apimachinery/pkg/util/validation/field" @@ -48,7 +50,7 @@ var ( ) // ValidateCustomResourceDefinition statically validates -func ValidateCustomResourceDefinition(obj *apiextensions.CustomResourceDefinition) field.ErrorList { +func ValidateCustomResourceDefinition(obj *apiextensions.CustomResourceDefinition, requestGV schema.GroupVersion) field.ErrorList { nameValidationFn := func(name string, prefix bool) []string { ret := genericvalidation.NameIsDNSSubdomain(name, prefix) requiredName := obj.Spec.Names.Plural + "." + obj.Spec.Group @@ -62,15 +64,17 @@ func ValidateCustomResourceDefinition(obj *apiextensions.CustomResourceDefinitio allErrs = append(allErrs, ValidateCustomResourceDefinitionSpec(&obj.Spec, field.NewPath("spec"))...) allErrs = append(allErrs, ValidateCustomResourceDefinitionStatus(&obj.Status, field.NewPath("status"))...) allErrs = append(allErrs, ValidateCustomResourceDefinitionStoredVersions(obj.Status.StoredVersions, obj.Spec.Versions, field.NewPath("status").Child("storedVersions"))...) + allErrs = append(allErrs, validateAPIApproval(obj, nil, requestGV)...) return allErrs } // ValidateCustomResourceDefinitionUpdate statically validates -func ValidateCustomResourceDefinitionUpdate(obj, oldObj *apiextensions.CustomResourceDefinition) field.ErrorList { +func ValidateCustomResourceDefinitionUpdate(obj, oldObj *apiextensions.CustomResourceDefinition, requestGV schema.GroupVersion) field.ErrorList { allErrs := genericvalidation.ValidateObjectMetaUpdate(&obj.ObjectMeta, &oldObj.ObjectMeta, field.NewPath("metadata")) allErrs = append(allErrs, ValidateCustomResourceDefinitionSpecUpdate(&obj.Spec, &oldObj.Spec, apiextensions.IsCRDConditionTrue(oldObj, apiextensions.Established), field.NewPath("spec"))...) allErrs = append(allErrs, ValidateCustomResourceDefinitionStatus(&obj.Status, field.NewPath("status"))...) allErrs = append(allErrs, ValidateCustomResourceDefinitionStoredVersions(obj.Status.StoredVersions, obj.Spec.Versions, field.NewPath("status").Child("storedVersions"))...) + allErrs = append(allErrs, validateAPIApproval(obj, oldObj, requestGV)...) return allErrs } @@ -1094,3 +1098,44 @@ func schemaHasKubernetesExtensions(s *apiextensions.JSONSchemaProps) bool { return false } + +// 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. + + if requestGV == v1beta1.SchemeGroupVersion { + // no-op for compatibility with v1beta1 + return nil + } + if !apihelpers.IsProtectedCommunityGroup(newCRD.Spec.Group) { + // no-op for non-protected groups + return nil + } + + // default to a state that allows missing values to continue to be missing + var oldApprovalState *apihelpers.APIApprovalState + if oldCRD != nil { + t, _ := apihelpers.GetAPIApprovalState(oldCRD.Annotations) + oldApprovalState = &t + } + newApprovalState, reason := apihelpers.GetAPIApprovalState(newCRD.Annotations) + + // if the approval state hasn't changed, never fail on approval validation + // this is allowed so that a v1 client that is simply updating spec and not mutating this value doesn't get rejected. Imagine a controller controlling a CRD spec. + if oldApprovalState != nil && *oldApprovalState == newApprovalState { + return nil + } + + // in v1, we require valid approval strings + switch newApprovalState { + case apihelpers.APIApprovalInvalid: + return field.ErrorList{field.Invalid(field.NewPath("metadata", "annotations").Key(v1beta1.KubeAPIApprovedAnnotation), newCRD.Annotations[v1beta1.KubeAPIApprovedAnnotation], reason)} + case apihelpers.APIApprovalMissing: + return field.ErrorList{field.Required(field.NewPath("metadata", "annotations").Key(v1beta1.KubeAPIApprovedAnnotation), reason)} + case apihelpers.APIApproved, apihelpers.APIApprovalBypassed: + // success + return nil + default: + return field.ErrorList{field.Invalid(field.NewPath("metadata", "annotations").Key(v1beta1.KubeAPIApprovedAnnotation), newCRD.Annotations[v1beta1.KubeAPIApprovedAnnotation], reason)} + } +} 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 fbccf9875cf..4b2f13ce60a 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 @@ -28,6 +28,7 @@ import ( "k8s.io/apimachinery/pkg/api/apitesting/fuzzer" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/runtime/serializer" "k8s.io/apimachinery/pkg/util/json" "k8s.io/apimachinery/pkg/util/validation/field" @@ -78,6 +79,7 @@ func TestValidateCustomResourceDefinition(t *testing.T) { tests := []struct { name string resource *apiextensions.CustomResourceDefinition + requestGV schema.GroupVersion errors []validationMatch enabledFeatures []featuregate.Feature }{ @@ -315,6 +317,7 @@ func TestValidateCustomResourceDefinition(t *testing.T) { StoredVersions: []string{"version"}, }, }, + requestGV: apiextensionsv1beta1.SchemeGroupVersion, errors: []validationMatch{ forbidden("spec", "conversion", "webhookClientConfig"), }, @@ -354,6 +357,7 @@ func TestValidateCustomResourceDefinition(t *testing.T) { StoredVersions: []string{"version"}, }, }, + requestGV: apiextensionsv1beta1.SchemeGroupVersion, errors: []validationMatch{ forbidden("spec", "conversion", "conversionReviewVersions"), }, @@ -665,7 +669,8 @@ func TestValidateCustomResourceDefinition(t *testing.T) { StoredVersions: []string{"version1"}, }, }, - errors: []validationMatch{}, + requestGV: apiextensionsv1beta1.SchemeGroupVersion, + errors: []validationMatch{}, }, { name: "webhook conversion without preserveUnknownFields=false", @@ -705,6 +710,7 @@ func TestValidateCustomResourceDefinition(t *testing.T) { StoredVersions: []string{"version1"}, }, }, + requestGV: apiextensionsv1beta1.SchemeGroupVersion, errors: []validationMatch{ invalid("spec", "conversion", "strategy"), }, @@ -788,6 +794,7 @@ func TestValidateCustomResourceDefinition(t *testing.T) { StoredVersions: []string{"version"}, }, }, + requestGV: apiextensionsv1beta1.SchemeGroupVersion, errors: []validationMatch{ invalid("spec", "versions"), }, @@ -826,6 +833,7 @@ func TestValidateCustomResourceDefinition(t *testing.T) { StoredVersions: []string{"version"}, }, }, + requestGV: apiextensionsv1beta1.SchemeGroupVersion, errors: []validationMatch{ invalid("spec", "versions"), invalid("status", "storedVersions"), @@ -865,6 +873,7 @@ func TestValidateCustomResourceDefinition(t *testing.T) { StoredVersions: []string{"version"}, }, }, + requestGV: apiextensionsv1beta1.SchemeGroupVersion, errors: []validationMatch{ invalid("status", "storedVersions"), }, @@ -898,6 +907,7 @@ func TestValidateCustomResourceDefinition(t *testing.T) { StoredVersions: []string{}, }, }, + requestGV: apiextensionsv1beta1.SchemeGroupVersion, errors: []validationMatch{ invalid("status", "storedVersions"), }, @@ -914,6 +924,7 @@ func TestValidateCustomResourceDefinition(t *testing.T) { PreserveUnknownFields: pointer.BoolPtr(true), }, }, + requestGV: apiextensionsv1beta1.SchemeGroupVersion, errors: []validationMatch{ invalid("status", "storedVersions"), invalid("metadata", "name"), @@ -966,6 +977,7 @@ func TestValidateCustomResourceDefinition(t *testing.T) { }, }, }, + requestGV: apiextensionsv1beta1.SchemeGroupVersion, errors: []validationMatch{ invalid("status", "storedVersions"), invalid("metadata", "name"), @@ -1014,6 +1026,7 @@ func TestValidateCustomResourceDefinition(t *testing.T) { StoredVersions: []string{"version"}, }, }, + requestGV: apiextensionsv1beta1.SchemeGroupVersion, errors: []validationMatch{ invalid("metadata", "name"), invalid("spec", "group"), @@ -1054,6 +1067,7 @@ func TestValidateCustomResourceDefinition(t *testing.T) { StoredVersions: []string{"version"}, }, }, + requestGV: apiextensionsv1beta1.SchemeGroupVersion, errors: []validationMatch{ forbidden("spec", "validation", "openAPIV3Schema", "additionalProperties"), }, @@ -1092,7 +1106,8 @@ func TestValidateCustomResourceDefinition(t *testing.T) { StoredVersions: []string{"version"}, }, }, - errors: []validationMatch{}, + requestGV: apiextensionsv1beta1.SchemeGroupVersion, + errors: []validationMatch{}, }, { name: "per-version fields may not all be set to identical values (top-level field should be used instead)", @@ -1136,6 +1151,7 @@ func TestValidateCustomResourceDefinition(t *testing.T) { StoredVersions: []string{"version"}, }, }, + requestGV: apiextensionsv1beta1.SchemeGroupVersion, 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. @@ -1421,6 +1437,7 @@ func TestValidateCustomResourceDefinition(t *testing.T) { StoredVersions: []string{"version0"}, }, }, + requestGV: apiextensionsv1beta1.SchemeGroupVersion, errors: []validationMatch{ invalid("spec", "versions[3]", "subresources", "scale", "labelSelectorPath"), }, @@ -1457,6 +1474,7 @@ func TestValidateCustomResourceDefinition(t *testing.T) { StoredVersions: []string{"version"}, }, }, + requestGV: apiextensionsv1beta1.SchemeGroupVersion, errors: []validationMatch{ forbidden("spec", "validation", "openAPIV3Schema", "properties[a]", "default"), // disabled feature-gate }, @@ -1491,6 +1509,7 @@ func TestValidateCustomResourceDefinition(t *testing.T) { StoredVersions: []string{"version"}, }, }, + requestGV: apiextensionsv1beta1.SchemeGroupVersion, errors: []validationMatch{ required("spec", "validation", "openAPIV3Schema", "type"), }, @@ -1525,6 +1544,7 @@ func TestValidateCustomResourceDefinition(t *testing.T) { StoredVersions: []string{"version"}, }, }, + requestGV: apiextensionsv1beta1.SchemeGroupVersion, errors: []validationMatch{ required("spec", "validation", "openAPIV3Schema", "type"), }, @@ -1563,6 +1583,7 @@ func TestValidateCustomResourceDefinition(t *testing.T) { StoredVersions: []string{"version"}, }, }, + requestGV: apiextensionsv1beta1.SchemeGroupVersion, errors: []validationMatch{ required("spec", "validation", "openAPIV3Schema", "type"), }, @@ -1626,6 +1647,7 @@ func TestValidateCustomResourceDefinition(t *testing.T) { StoredVersions: []string{"version"}, }, }, + requestGV: apiextensionsv1beta1.SchemeGroupVersion, errors: []validationMatch{ forbidden("spec", "validation", "openAPIV3Schema", "properties[embedded]", "properties[metadata]", "x-kubernetes-embedded-resource"), forbidden("spec", "validation", "openAPIV3Schema", "properties[embedded]", "properties[apiVersion]", "properties[foo]", "x-kubernetes-embedded-resource"), @@ -2388,7 +2410,7 @@ func TestValidateCustomResourceDefinition(t *testing.T) { if tc.resource.Spec.Conversion != nil && tc.resource.Spec.Conversion.Strategy == apiextensions.WebhookConverter && len(tc.resource.Spec.Conversion.ConversionReviewVersions) == 0 { tc.resource.Spec.Conversion.ConversionReviewVersions = []string{"v1beta1"} } - errs := ValidateCustomResourceDefinition(tc.resource) + errs := ValidateCustomResourceDefinition(tc.resource, tc.requestGV) seenErrs := make([]bool, len(errs)) for _, expectedError := range tc.errors { @@ -2420,6 +2442,7 @@ func TestValidateCustomResourceDefinitionUpdate(t *testing.T) { name string old *apiextensions.CustomResourceDefinition resource *apiextensions.CustomResourceDefinition + requestGV schema.GroupVersion errors []validationMatch enabledFeatures []featuregate.Feature }{ @@ -3350,7 +3373,8 @@ func TestValidateCustomResourceDefinitionUpdate(t *testing.T) { StoredVersions: []string{"version"}, }, }, - errors: []validationMatch{}, + requestGV: apiextensionsv1beta1.SchemeGroupVersion, + errors: []validationMatch{}, }, { name: "setting defaults with enabled feature gate", @@ -3528,7 +3552,7 @@ func TestValidateCustomResourceDefinitionUpdate(t *testing.T) { defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, gate, true)() } - errs := ValidateCustomResourceDefinitionUpdate(tc.resource, tc.old) + errs := ValidateCustomResourceDefinitionUpdate(tc.resource, tc.old, tc.requestGV) seenErrs := make([]bool, len(errs)) for _, expectedError := range tc.errors { diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresourcedefinition/BUILD b/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresourcedefinition/BUILD index 36ae182ed74..32e63e4af88 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresourcedefinition/BUILD +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresourcedefinition/BUILD @@ -11,9 +11,7 @@ go_library( importmap = "k8s.io/kubernetes/vendor/k8s.io/apiextensions-apiserver/pkg/registry/customresourcedefinition", importpath = "k8s.io/apiextensions-apiserver/pkg/registry/customresourcedefinition", deps = [ - "//staging/src/k8s.io/apiextensions-apiserver/pkg/apihelpers:go_default_library", "//staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions:go_default_library", - "//staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1:go_default_library", "//staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation:go_default_library", "//staging/src/k8s.io/apiextensions-apiserver/pkg/features:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/api/equality:go_default_library", @@ -22,6 +20,7 @@ go_library( "//staging/src/k8s.io/apimachinery/pkg/fields:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/labels:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/runtime/schema:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/validation/field:go_default_library", "//staging/src/k8s.io/apiserver/pkg/endpoints/request:go_default_library", "//staging/src/k8s.io/apiserver/pkg/registry/generic:go_default_library", @@ -55,12 +54,14 @@ go_test( deps = [ "//staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions:go_default_library", "//staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1:go_default_library", + "//staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation:go_default_library", "//staging/src/k8s.io/apiextensions-apiserver/pkg/features:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/runtime/schema:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/diff:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/validation/field:go_default_library", - "//staging/src/k8s.io/apiserver/pkg/endpoints/request:go_default_library", "//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library", "//staging/src/k8s.io/component-base/featuregate/testing:go_default_library", + "//vendor/k8s.io/utils/pointer:go_default_library", ], ) 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 557fe88225f..0569f17bedc 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,17 +20,16 @@ import ( "context" "fmt" - "k8s.io/apiextensions-apiserver/pkg/apihelpers" "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions" - "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1" "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" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/util/validation/field" - "k8s.io/apiserver/pkg/endpoints/request" + genericapirequest "k8s.io/apiserver/pkg/endpoints/request" "k8s.io/apiserver/pkg/registry/generic" "k8s.io/apiserver/pkg/storage" "k8s.io/apiserver/pkg/storage/names" @@ -101,8 +100,12 @@ func (strategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Object) { // Validate validates a new CustomResourceDefinition. func (strategy) Validate(ctx context.Context, obj runtime.Object) field.ErrorList { - fieldErrors := validation.ValidateCustomResourceDefinition(obj.(*apiextensions.CustomResourceDefinition)) - return append(fieldErrors, validateAPIApproval(ctx, obj.(*apiextensions.CustomResourceDefinition), nil)...) + var groupVersion schema.GroupVersion + if requestInfo, found := genericapirequest.RequestInfoFrom(ctx); found { + groupVersion = schema.GroupVersion{Group: requestInfo.APIGroup, Version: requestInfo.APIVersion} + } + + return validation.ValidateCustomResourceDefinition(obj.(*apiextensions.CustomResourceDefinition), groupVersion) } // AllowCreateOnUpdate is false for CustomResourceDefinition; this means a POST is @@ -122,47 +125,12 @@ func (strategy) Canonicalize(obj runtime.Object) { // ValidateUpdate is the default update validation for an end user updating status. func (strategy) ValidateUpdate(ctx context.Context, obj, old runtime.Object) field.ErrorList { - fieldErrors := validation.ValidateCustomResourceDefinitionUpdate(obj.(*apiextensions.CustomResourceDefinition), old.(*apiextensions.CustomResourceDefinition)) - - return append(fieldErrors, validateAPIApproval(ctx, obj.(*apiextensions.CustomResourceDefinition), old.(*apiextensions.CustomResourceDefinition))...) -} - -// validateAPIApproval returns a list of errors if the API approval annotation isn't valid -func validateAPIApproval(ctx context.Context, newCRD, oldCRD *apiextensions.CustomResourceDefinition) field.ErrorList { - // check to see if we need confirm API approval for kube group. Do nothing for non-protected groups and do nothing in v1beta1. - if requestInfo, ok := request.RequestInfoFrom(ctx); !ok || requestInfo.APIVersion == "v1beta1" { - return field.ErrorList{} - } - if !apihelpers.IsProtectedCommunityGroup(newCRD.Spec.Group) { - return field.ErrorList{} + var groupVersion schema.GroupVersion + if requestInfo, found := genericapirequest.RequestInfoFrom(ctx); found { + groupVersion = schema.GroupVersion{Group: requestInfo.APIGroup, Version: requestInfo.APIVersion} } - // default to a state that allows missing values to continue to be missing - var oldApprovalState *apihelpers.APIApprovalState - if oldCRD != nil { - t, _ := apihelpers.GetAPIApprovalState(oldCRD.Annotations) - oldApprovalState = &t - } - newApprovalState, reason := apihelpers.GetAPIApprovalState(newCRD.Annotations) - - // if the approval state hasn't changed, never fail on approval validation - // this is allowed so that a v1 client that is simply updating spec and not mutating this value doesn't get rejected. Imagine a controller controlling a CRD spec. - if oldApprovalState != nil && *oldApprovalState == newApprovalState { - return field.ErrorList{} - } - - // in v1, we require valid approval strings - switch newApprovalState { - case apihelpers.APIApprovalInvalid: - return field.ErrorList{field.Invalid(field.NewPath("metadata", "annotations").Key(v1beta1.KubeAPIApprovedAnnotation), newCRD.Annotations[v1beta1.KubeAPIApprovedAnnotation], reason)} - case apihelpers.APIApprovalMissing: - return field.ErrorList{field.Required(field.NewPath("metadata", "annotations").Key(v1beta1.KubeAPIApprovedAnnotation), reason)} - case apihelpers.APIApproved, apihelpers.APIApprovalBypassed: - // success - return field.ErrorList{} - default: - return field.ErrorList{field.Invalid(field.NewPath("metadata", "annotations").Key(v1beta1.KubeAPIApprovedAnnotation), newCRD.Annotations[v1beta1.KubeAPIApprovedAnnotation], reason)} - } + return validation.ValidateCustomResourceDefinitionUpdate(obj.(*apiextensions.CustomResourceDefinition), old.(*apiextensions.CustomResourceDefinition), groupVersion) } type statusStrategy struct { diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresourcedefinition/strategy_test.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresourcedefinition/strategy_test.go index 12d454d8f51..8b9f9c832fa 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresourcedefinition/strategy_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresourcedefinition/strategy_test.go @@ -17,20 +17,21 @@ limitations under the License. package customresourcedefinition import ( - "context" "fmt" "reflect" "testing" "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions" "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1" + "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation" apiextensionsfeatures "k8s.io/apiextensions-apiserver/pkg/features" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/util/diff" "k8s.io/apimachinery/pkg/util/validation/field" - "k8s.io/apiserver/pkg/endpoints/request" utilfeature "k8s.io/apiserver/pkg/util/feature" featuregatetesting "k8s.io/component-base/featuregate/testing" + "k8s.io/utils/pointer" ) func TestDropDisableFieldsCustomResourceDefinition(t *testing.T) { @@ -517,6 +518,9 @@ func TestValidateAPIApproval(t *testing.T) { annotationValue: "invalid", validateError: func(t *testing.T, errors field.ErrorList) { t.Helper() + if len(errors) == 0 { + t.Fatal("expected errors, got none") + } if e, a := `metadata.annotations[api-approved.kubernetes.io]: Invalid value: "invalid": protected groups must have approval annotation "api-approved.kubernetes.io" with either a URL or a reason starting with "unapproved", see https://github.com/kubernetes/enhancements/pull/1111`, errors.ToAggregate().Error(); e != a { t.Fatal(errors) } @@ -538,6 +542,9 @@ func TestValidateAPIApproval(t *testing.T) { oldAnnotationValue: strPtr("invalid"), validateError: func(t *testing.T, errors field.ErrorList) { t.Helper() + if len(errors) == 0 { + t.Fatal("expected errors, got none") + } if e, a := `metadata.annotations[api-approved.kubernetes.io]: Required value: protected groups must have approval annotation "api-approved.kubernetes.io", see https://github.com/kubernetes/enhancements/pull/1111`, errors.ToAggregate().Error(); e != a { t.Fatal(errors) } @@ -551,6 +558,9 @@ func TestValidateAPIApproval(t *testing.T) { oldAnnotationValue: strPtr(""), validateError: func(t *testing.T, errors field.ErrorList) { t.Helper() + if len(errors) == 0 { + t.Fatal("expected errors, got none") + } if e, a := `metadata.annotations[api-approved.kubernetes.io]: Invalid value: "invalid": protected groups must have approval annotation "api-approved.kubernetes.io" with either a URL or a reason starting with "unapproved", see https://github.com/kubernetes/enhancements/pull/1111`, errors.ToAggregate().Error(); e != a { t.Fatal(errors) } @@ -563,6 +573,9 @@ func TestValidateAPIApproval(t *testing.T) { annotationValue: "", validateError: func(t *testing.T, errors field.ErrorList) { t.Helper() + if len(errors) == 0 { + t.Fatal("expected errors, got none") + } if e, a := `metadata.annotations[api-approved.kubernetes.io]: Required value: protected groups must have approval annotation "api-approved.kubernetes.io", see https://github.com/kubernetes/enhancements/pull/1111`, errors.ToAggregate().Error(); e != a { t.Fatal(errors) } @@ -597,6 +610,9 @@ func TestValidateAPIApproval(t *testing.T) { annotationValue: "invalid", validateError: func(t *testing.T, errors field.ErrorList) { t.Helper() + if len(errors) == 0 { + t.Fatal("expected errors, got none") + } if e, a := `metadata.annotations[api-approved.kubernetes.io]: Invalid value: "invalid": protected groups must have approval annotation "api-approved.kubernetes.io" with either a URL or a reason starting with "unapproved", see https://github.com/kubernetes/enhancements/pull/1111`, errors.ToAggregate().Error(); e != a { t.Fatal(errors) } @@ -606,24 +622,48 @@ func TestValidateAPIApproval(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - ctx := request.WithRequestInfo(context.TODO(), &request.RequestInfo{APIVersion: test.version}) crd := &apiextensions.CustomResourceDefinition{ - ObjectMeta: metav1.ObjectMeta{Name: "foo", Annotations: map[string]string{v1beta1.KubeAPIApprovedAnnotation: test.annotationValue}}, + ObjectMeta: metav1.ObjectMeta{Name: "foos." + test.group, Annotations: map[string]string{v1beta1.KubeAPIApprovedAnnotation: test.annotationValue}, ResourceVersion: "1"}, Spec: apiextensions.CustomResourceDefinitionSpec{ - Group: test.group, + Group: test.group, + Scope: apiextensions.NamespaceScoped, + Version: "v1", + Versions: []apiextensions.CustomResourceDefinitionVersion{{Name: "v1", Storage: true, Served: true}}, + Names: apiextensions.CustomResourceDefinitionNames{Plural: "foos", Singular: "foo", Kind: "Foo", ListKind: "FooList"}, + Validation: &apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensions.JSONSchemaProps{Type: "object", XPreserveUnknownFields: pointer.BoolPtr(true)}, + }, + }, + Status: apiextensions.CustomResourceDefinitionStatus{ + StoredVersions: []string{"v1"}, }, } var oldCRD *apiextensions.CustomResourceDefinition if test.oldAnnotationValue != nil { oldCRD = &apiextensions.CustomResourceDefinition{ - ObjectMeta: metav1.ObjectMeta{Name: "foo", Annotations: map[string]string{v1beta1.KubeAPIApprovedAnnotation: *test.oldAnnotationValue}}, + ObjectMeta: metav1.ObjectMeta{Name: "foos." + test.group, Annotations: map[string]string{v1beta1.KubeAPIApprovedAnnotation: *test.oldAnnotationValue}, ResourceVersion: "1"}, Spec: apiextensions.CustomResourceDefinitionSpec{ - Group: test.group, + Group: test.group, + Scope: apiextensions.NamespaceScoped, + Version: "v1", + Versions: []apiextensions.CustomResourceDefinitionVersion{{Name: "v1", Storage: true, Served: true}}, + Names: apiextensions.CustomResourceDefinitionNames{Plural: "foos", Singular: "foo", Kind: "Foo", ListKind: "FooList"}, + Validation: &apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensions.JSONSchemaProps{Type: "object", XPreserveUnknownFields: pointer.BoolPtr(true)}, + }, + }, + Status: apiextensions.CustomResourceDefinitionStatus{ + StoredVersions: []string{"v1"}, }, } } - actual := validateAPIApproval(ctx, crd, oldCRD) + var actual field.ErrorList + if oldCRD == nil { + actual = validation.ValidateCustomResourceDefinition(crd, schema.GroupVersion{Group: "apiextensions.k8s.io", Version: test.version}) + } else { + actual = validation.ValidateCustomResourceDefinitionUpdate(crd, oldCRD, schema.GroupVersion{Group: "apiextensions.k8s.io", Version: test.version}) + } test.validateError(t, actual) }) } From 2741dd5e7fafa0f1d59628acb8972975ef0844a0 Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Wed, 7 Aug 2019 16:12:09 -0400 Subject: [PATCH 2/2] Refactor validation options --- .../apiextensions/validation/validation.go | 75 ++++++++++++------- .../validation/validation_test.go | 3 +- 2 files changed, 49 insertions(+), 29 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 a1b4f538707..3f7cfd812a3 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 @@ -60,18 +60,40 @@ func ValidateCustomResourceDefinition(obj *apiextensions.CustomResourceDefinitio return ret } + opts := validationOptions{ + allowDefaults: allowDefaults(requestGV), + requireRecognizedConversionReviewVersion: true, + requireImmutableNames: false, + } + allErrs := genericvalidation.ValidateObjectMeta(&obj.ObjectMeta, false, nameValidationFn, field.NewPath("metadata")) - allErrs = append(allErrs, ValidateCustomResourceDefinitionSpec(&obj.Spec, field.NewPath("spec"))...) + allErrs = append(allErrs, validateCustomResourceDefinitionSpec(&obj.Spec, opts, field.NewPath("spec"))...) allErrs = append(allErrs, ValidateCustomResourceDefinitionStatus(&obj.Status, field.NewPath("status"))...) allErrs = append(allErrs, ValidateCustomResourceDefinitionStoredVersions(obj.Status.StoredVersions, obj.Spec.Versions, field.NewPath("status").Child("storedVersions"))...) allErrs = append(allErrs, validateAPIApproval(obj, nil, requestGV)...) return allErrs } +// validationOptions groups several validation options, to avoid passing multiple bool parameters to methods +type validationOptions struct { + // allowDefaults permits the validation schema to contain default attributes + allowDefaults bool + // requireRecognizedConversionReviewVersion requires accepted webhook conversion versions to contain a recognized version + requireRecognizedConversionReviewVersion bool + // requireImmutableNames disables changing spec.names + requireImmutableNames bool +} + // ValidateCustomResourceDefinitionUpdate statically validates func ValidateCustomResourceDefinitionUpdate(obj, oldObj *apiextensions.CustomResourceDefinition, requestGV schema.GroupVersion) field.ErrorList { + opts := validationOptions{ + allowDefaults: allowDefaults(requestGV) || specHasDefaults(&oldObj.Spec), + requireRecognizedConversionReviewVersion: oldObj.Spec.Conversion == nil || hasValidConversionReviewVersionOrEmpty(oldObj.Spec.Conversion.ConversionReviewVersions), + requireImmutableNames: apiextensions.IsCRDConditionTrue(oldObj, apiextensions.Established), + } + allErrs := genericvalidation.ValidateObjectMetaUpdate(&obj.ObjectMeta, &oldObj.ObjectMeta, field.NewPath("metadata")) - allErrs = append(allErrs, ValidateCustomResourceDefinitionSpecUpdate(&obj.Spec, &oldObj.Spec, apiextensions.IsCRDConditionTrue(oldObj, apiextensions.Established), field.NewPath("spec"))...) + allErrs = append(allErrs, validateCustomResourceDefinitionSpecUpdate(&obj.Spec, &oldObj.Spec, opts, field.NewPath("spec"))...) allErrs = append(allErrs, ValidateCustomResourceDefinitionStatus(&obj.Status, field.NewPath("status"))...) allErrs = append(allErrs, ValidateCustomResourceDefinitionStoredVersions(obj.Status.StoredVersions, obj.Spec.Versions, field.NewPath("status").Child("storedVersions"))...) allErrs = append(allErrs, validateAPIApproval(obj, oldObj, requestGV)...) @@ -112,10 +134,10 @@ func ValidateUpdateCustomResourceDefinitionStatus(obj, oldObj *apiextensions.Cus return allErrs } -// ValidateCustomResourceDefinitionVersion statically validates. -func ValidateCustomResourceDefinitionVersion(version *apiextensions.CustomResourceDefinitionVersion, fldPath *field.Path, mustBeStructural, statusEnabled, allowDefaults bool) field.ErrorList { +// validateCustomResourceDefinitionVersion statically validates. +func validateCustomResourceDefinitionVersion(version *apiextensions.CustomResourceDefinitionVersion, fldPath *field.Path, mustBeStructural, statusEnabled bool, opts validationOptions) field.ErrorList { allErrs := field.ErrorList{} - allErrs = append(allErrs, ValidateCustomResourceDefinitionValidation(version.Schema, mustBeStructural, statusEnabled, allowDefaults, fldPath.Child("schema"))...) + allErrs = append(allErrs, validateCustomResourceDefinitionValidation(version.Schema, mustBeStructural, 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))...) @@ -123,13 +145,7 @@ func ValidateCustomResourceDefinitionVersion(version *apiextensions.CustomResour return allErrs } -// ValidateCustomResourceDefinitionSpec statically validates -func ValidateCustomResourceDefinitionSpec(spec *apiextensions.CustomResourceDefinitionSpec, fldPath *field.Path) field.ErrorList { - allowDefaults := utilfeature.DefaultFeatureGate.Enabled(apiextensionsfeatures.CustomResourceDefaulting) - return validateCustomResourceDefinitionSpec(spec, true, allowDefaults, fldPath) -} - -func validateCustomResourceDefinitionSpec(spec *apiextensions.CustomResourceDefinitionSpec, requireRecognizedVersion, allowDefaults bool, fldPath *field.Path) field.ErrorList { +func validateCustomResourceDefinitionSpec(spec *apiextensions.CustomResourceDefinitionSpec, opts validationOptions, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} if len(spec.Group) == 0 { @@ -155,7 +171,7 @@ func validateCustomResourceDefinitionSpec(spec *apiextensions.CustomResourceDefi } } } - if allowDefaults && specHasDefaults(spec) { + if opts.allowDefaults && specHasDefaults(spec) { mustBeStructural = 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")) @@ -181,7 +197,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), allowDefaults)...) + allErrs = append(allErrs, validateCustomResourceDefinitionVersion(&version, fldPath.Child("versions").Index(i), mustBeStructural, hasStatusEnabled(subresources), opts)...) } // The top-level and per-version fields are mutual exclusive @@ -236,7 +252,7 @@ func validateCustomResourceDefinitionSpec(spec *apiextensions.CustomResourceDefi } allErrs = append(allErrs, ValidateCustomResourceDefinitionNames(&spec.Names, fldPath.Child("names"))...) - allErrs = append(allErrs, ValidateCustomResourceDefinitionValidation(spec.Validation, mustBeStructural, hasAnyStatusEnabled(spec), allowDefaults, fldPath.Child("validation"))...) + allErrs = append(allErrs, validateCustomResourceDefinitionValidation(spec.Validation, mustBeStructural, hasAnyStatusEnabled(spec), opts, fldPath.Child("validation"))...) allErrs = append(allErrs, ValidateCustomResourceDefinitionSubresources(spec.Subresources, fldPath.Child("subresources"))...) for i := range spec.AdditionalPrinterColumns { @@ -248,7 +264,7 @@ func validateCustomResourceDefinitionSpec(spec *apiextensions.CustomResourceDefi if (spec.Conversion != nil && spec.Conversion.Strategy != apiextensions.NoneConverter) && (spec.PreserveUnknownFields == nil || *spec.PreserveUnknownFields) { allErrs = append(allErrs, field.Invalid(fldPath.Child("conversion").Child("strategy"), spec.Conversion.Strategy, "must be None if spec.preserveUnknownFields is true")) } - allErrs = append(allErrs, validateCustomResourceConversion(spec.Conversion, requireRecognizedVersion, fldPath.Child("conversion"))...) + allErrs = append(allErrs, validateCustomResourceConversion(spec.Conversion, opts.requireRecognizedConversionReviewVersion, fldPath.Child("conversion"))...) return allErrs } @@ -363,16 +379,11 @@ func validateCustomResourceConversion(conversion *apiextensions.CustomResourceCo return allErrs } -// ValidateCustomResourceDefinitionSpecUpdate statically validates -func ValidateCustomResourceDefinitionSpecUpdate(spec, oldSpec *apiextensions.CustomResourceDefinitionSpec, established bool, fldPath *field.Path) field.ErrorList { - requireRecognizedVersion := oldSpec.Conversion == nil || hasValidConversionReviewVersionOrEmpty(oldSpec.Conversion.ConversionReviewVersions) +// validateCustomResourceDefinitionSpecUpdate statically validates +func validateCustomResourceDefinitionSpecUpdate(spec, oldSpec *apiextensions.CustomResourceDefinitionSpec, opts validationOptions, fldPath *field.Path) field.ErrorList { + allErrs := validateCustomResourceDefinitionSpec(spec, opts, fldPath) - // find out whether any schema had default before. Then we keep allowing it. - allowDefaults := utilfeature.DefaultFeatureGate.Enabled(apiextensionsfeatures.CustomResourceDefaulting) || specHasDefaults(oldSpec) - - allErrs := validateCustomResourceDefinitionSpec(spec, requireRecognizedVersion, allowDefaults, fldPath) - - if established { + if opts.requireImmutableNames { // these effect the storage and cannot be changed therefore allErrs = append(allErrs, genericvalidation.ValidateImmutableField(spec.Scope, oldSpec.Scope, fldPath.Child("scope"))...) allErrs = append(allErrs, genericvalidation.ValidateImmutableField(spec.Names.Kind, oldSpec.Names.Kind, fldPath.Child("names", "kind"))...) @@ -577,8 +588,8 @@ type specStandardValidator interface { withInsideResourceMeta() specStandardValidator } -// ValidateCustomResourceDefinitionValidation statically validates -func ValidateCustomResourceDefinitionValidation(customResourceValidation *apiextensions.CustomResourceValidation, mustBeStructural, statusSubresourceEnabled, allowDefaults bool, fldPath *field.Path) field.ErrorList { +// validateCustomResourceDefinitionValidation statically validates +func validateCustomResourceDefinitionValidation(customResourceValidation *apiextensions.CustomResourceValidation, mustBeStructural, statusSubresourceEnabled bool, opts validationOptions, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} if customResourceValidation == nil { @@ -619,7 +630,7 @@ func ValidateCustomResourceDefinitionValidation(customResourceValidation *apiext } openAPIV3Schema := &specStandardValidatorV3{ - allowDefaults: allowDefaults, + allowDefaults: opts.allowDefaults, } allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(schema, fldPath.Child("openAPIV3Schema"), openAPIV3Schema, true)...) @@ -933,6 +944,14 @@ func allowedAtRootSchema(field string) bool { return false } +// allowDefaults returns true if the defaulting feature is enabled and the request group version allows adding defaults +func allowDefaults(requestGV schema.GroupVersion) bool { + if !utilfeature.DefaultFeatureGate.Enabled(apiextensionsfeatures.CustomResourceDefaulting) { + return false + } + return true +} + func specHasDefaults(spec *apiextensions.CustomResourceDefinitionSpec) bool { if spec.Validation != nil && schemaHasDefaults(spec.Validation.OpenAPIV3Schema) { return true 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 4b2f13ce60a..a340bf8a5f4 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 @@ -3585,6 +3585,7 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { input apiextensions.CustomResourceValidation mustBeStructural bool statusEnabled bool + opts validationOptions wantError bool }{ { @@ -3726,7 +3727,7 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got := ValidateCustomResourceDefinitionValidation(&tt.input, tt.mustBeStructural, tt.statusEnabled, false, field.NewPath("spec", "validation")) + got := validateCustomResourceDefinitionValidation(&tt.input, tt.mustBeStructural, 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 {