From 7c448e37b35a16baf4acbf44e794e068005d6a48 Mon Sep 17 00:00:00 2001 From: Solly Ross Date: Tue, 5 Nov 2019 17:07:03 -0800 Subject: [PATCH] Give a reason when rejecting defaulting in CRDs The current message for rejecting defaulting is fairly unclear -- I wasn't sure why it was happening till I read the kubernetes source code. That's probably not something we want our users to have to do, so this adds more reasons for rejecting the defaulting to the message given to the user. We previously added reasons in certain cases, but did not in the common cases of using apiextensions/v1beta1 or having defaulting disabled in a feature gate. Now we have reasons for all cases. --- .../apiextensions/validation/validation.go | 22 +++++++++++++------ .../validation/validation_test.go | 8 +++++-- 2 files changed, 21 insertions(+), 9 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 82e4e9d15dd..e44e7bb9943 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 @@ -55,8 +55,10 @@ func ValidateCustomResourceDefinition(obj *apiextensions.CustomResourceDefinitio return ret } + allowDefaults, rejectDefaultsReason := allowDefaults(requestGV, nil) opts := validationOptions{ - allowDefaults: allowDefaults(requestGV, nil), + allowDefaults: allowDefaults, + disallowDefaultsReason: rejectDefaultsReason, requireRecognizedConversionReviewVersion: true, requireImmutableNames: false, requireOpenAPISchema: requireOpenAPISchema(requestGV, nil), @@ -80,6 +82,8 @@ func ValidateCustomResourceDefinition(obj *apiextensions.CustomResourceDefinitio type validationOptions struct { // allowDefaults permits the validation schema to contain default attributes allowDefaults bool + // disallowDefaultsReason gives a reason as to why allowDefaults is false (for better user feedback) + disallowDefaultsReason string // requireRecognizedConversionReviewVersion requires accepted webhook conversion versions to contain a recognized version requireRecognizedConversionReviewVersion bool // requireImmutableNames disables changing spec.names @@ -102,8 +106,10 @@ type validationOptions struct { // ValidateCustomResourceDefinitionUpdate statically validates func ValidateCustomResourceDefinitionUpdate(obj, oldObj *apiextensions.CustomResourceDefinition, requestGV schema.GroupVersion) field.ErrorList { + allowDefaults, rejectDefaultsReason := allowDefaults(requestGV, &oldObj.Spec) opts := validationOptions{ - allowDefaults: allowDefaults(requestGV, &oldObj.Spec), + allowDefaults: allowDefaults, + disallowDefaultsReason: rejectDefaultsReason, requireRecognizedConversionReviewVersion: oldObj.Spec.Conversion == nil || hasValidConversionReviewVersionOrEmpty(oldObj.Spec.Conversion.ConversionReviewVersions), requireImmutableNames: apiextensions.IsCRDConditionTrue(oldObj, apiextensions.Established), requireOpenAPISchema: requireOpenAPISchema(requestGV, &oldObj.Spec), @@ -661,6 +667,7 @@ func validateCustomResourceDefinitionValidation(customResourceValidation *apiext openAPIV3Schema := &specStandardValidatorV3{ allowDefaults: opts.allowDefaults, + disallowDefaultsReason: opts.disallowDefaultsReason, requireValidPropertyType: opts.requireValidPropertyType, } @@ -1110,16 +1117,17 @@ func allVersionsSpecifyOpenAPISchema(spec *apiextensions.CustomResourceDefinitio return true } -// allowDefaults returns true if the defaulting feature is enabled and the request group version allows adding defaults -func allowDefaults(requestGV schema.GroupVersion, oldCRDSpec *apiextensions.CustomResourceDefinitionSpec) bool { +// allowDefaults returns true if the defaulting feature is enabled and the request group version allows adding defaults, +// or false and a reason for the user if defaults are not allowed. +func allowDefaults(requestGV schema.GroupVersion, oldCRDSpec *apiextensions.CustomResourceDefinitionSpec) (bool, string) { if oldCRDSpec != nil && specHasDefaults(oldCRDSpec) { // don't tighten validation on existing persisted data - return true + return true, "" } if requestGV == apiextensionsv1beta1.SchemeGroupVersion { - return false + return false, "(cannot set default values in apiextensions.k8s.io/v1beta1 CRDs, must use apiextensions.k8s.io/v1)" } - return true + return true, "" } func specHasDefaults(spec *apiextensions.CustomResourceDefinitionSpec) bool { 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 d34cd045d59..7dc6a8fc2d9 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 @@ -38,6 +38,7 @@ import ( type validationMatch struct { path *field.Path errorType field.ErrorType + contains string } func required(path ...string) validationMatch { @@ -58,9 +59,12 @@ func immutable(path ...string) validationMatch { func forbidden(path ...string) validationMatch { return validationMatch{path: field.NewPath(path[0], path[1:]...), errorType: field.ErrorTypeForbidden} } +func forbiddenContains(contains string, path ...string) validationMatch { + return validationMatch{path: field.NewPath(path[0], path[1:]...), errorType: field.ErrorTypeForbidden, contains: contains} +} func (v validationMatch) matches(err *field.Error) bool { - return err.Type == v.errorType && err.Field == v.path.String() + return err.Type == v.errorType && err.Field == v.path.String() && strings.Contains(err.Error(), v.contains) } func strPtr(s string) *string { return &s } @@ -1641,7 +1645,7 @@ func TestValidateCustomResourceDefinition(t *testing.T) { }, requestGV: apiextensionsv1beta1.SchemeGroupVersion, errors: []validationMatch{ - forbidden("spec", "validation", "openAPIV3Schema", "properties[a]", "default"), // disallowed via v1beta1 + forbiddenContains("cannot set default values in apiextensions.k8s.io/v1beta1", "spec", "validation", "openAPIV3Schema", "properties[a]", "default"), // disallowed via v1beta1 }, }, {