diff --git a/apis/deprecated.go b/apis/deprecated.go index fb73306fa..8f07e71b3 100644 --- a/apis/deprecated.go +++ b/apis/deprecated.go @@ -36,12 +36,15 @@ func CheckDeprecated(ctx context.Context, obj interface{}) *FieldError { // CheckDeprecatedUpdate checks whether the provided named deprecated fields // are set in a context where deprecation is disallowed. // This is a json shallow check. We will recursively check inlined structs. -func CheckDeprecatedUpdate(ctx context.Context, obj, original interface{}) *FieldError { +func CheckDeprecatedUpdate(ctx context.Context, obj, original interface{}) (errs *FieldError) { if IsDeprecatedAllowed(ctx) { + // TODO: We should still run through the validation here, but do + // something like: + // defer func() { + // errs = errs.At(WarningLevel) + // }() return nil } - - var errs *FieldError objFields, objInlined := getPrefixedNamedFieldValues(deprecatedPrefix, obj) if nonZero(reflect.ValueOf(original)) { diff --git a/apis/field_error.go b/apis/field_error.go index 5ea51c0e4..7a6fd6a8e 100644 --- a/apis/field_error.go +++ b/apis/field_error.go @@ -28,18 +28,55 @@ import ( // a problem with the current field itself. const CurrentField = "" +// DiagnosticLevel is used to signal the severity of a particular diagnostic +// in the form of a FieldError. +type DiagnosticLevel string + +const ( + // ErrorLevel is used to signify fatal/blocking diagnostics, e.g. those + // that should block admission in a validating admission webhook. + ErrorLevel DiagnosticLevel = "Error" + + // WarningLevel is used to signify information/non-blocking diagnostics, + // e.g. those that should be surfaced as warnings in a validating admission + // webhook. + WarningLevel DiagnosticLevel = "Warning" +) + +// Matches checks whether the provided diagnostic level matches this one, +// including the special case where an empty DiagnosticLevel equals ErrorLevel. +func (l DiagnosticLevel) Matches(r DiagnosticLevel) bool { + switch { + case l == ErrorLevel && r == "": + return true + case r == ErrorLevel && l == "": + return true + default: + return l == r + } +} + // FieldError is used to propagate the context of errors pertaining to // specific fields in a manner suitable for use in a recursive walk, so // that errors contain the appropriate field context. // FieldError methods are non-mutating. // +k8s:deepcopy-gen=true type FieldError struct { + // Message holds the main diagnostic message carried by this FieldError Message string - Paths []string + + // Paths holds a list of paths to which this diagnostic pertains + Paths []string + + // Level holds the severity of the diagnostic. + // If empty, this defaults to ErrorLevel. + Level DiagnosticLevel + // Details contains an optional longer payload. // +optional Details string - errors []FieldError + + errors []FieldError } // FieldError implements error @@ -60,6 +97,7 @@ func (fe *FieldError) ViaField(prefix ...string) *FieldError { // along using .Also(). newErr := &FieldError{ Message: fe.Message, + Level: fe.Level, Details: fe.Details, } @@ -107,6 +145,54 @@ func (fe *FieldError) ViaFieldKey(field, key string) *FieldError { return fe.ViaKey(key).ViaField(field) } +// At is a way to alter the level of the diagnostics held in this FieldError. +// ErrMissingField("foo").At(WarningLevel) +func (fe *FieldError) At(l DiagnosticLevel) *FieldError { + if fe == nil { + return nil + } + // Copy over message and details, paths will be updated and errors come + // along using .Also(). + newErr := &FieldError{ + Message: fe.Message, + Level: l, + Details: fe.Details, + Paths: fe.Paths, + } + + for _, e := range fe.errors { + newErr = newErr.Also(e.At(l)) + } + return newErr +} + +// Filter is a way to access the set of diagnostics having a particular level. +// if err := x.Validate(ctx).Filter(ErrorLevel); err != nil { +// return err +// } +func (fe *FieldError) Filter(l DiagnosticLevel) *FieldError { + if fe == nil { + return nil + } + var newErr *FieldError + if l.Matches(fe.Level) { + newErr = &FieldError{ + Message: fe.Message, + Level: fe.Level, + Details: fe.Details, + Paths: fe.Paths, + } + } + + for _, e := range fe.errors { + newErr = newErr.Also(e.Filter(l)) + } + if newErr.isEmpty() { + return nil + } + return newErr +} + // Also collects errors, returns a new collection of existing errors and new errors. func (fe *FieldError) Also(errs ...*FieldError) *FieldError { // Avoid doing any work, if we don't have to. @@ -154,6 +240,7 @@ func (fe *FieldError) normalized() []*FieldError { if fe.Message != "" { errors = append(errors, &FieldError{ Message: fe.Message, + Level: fe.Level, Paths: fe.Paths, Details: fe.Details, }) @@ -274,6 +361,9 @@ func merge(errs []*FieldError) []*FieldError { // Sort the flattened map. sort.Slice(newErrs, func(i, j int) bool { if newErrs[i].Message == newErrs[j].Message { + if newErrs[i].Details == newErrs[j].Details { + return newErrs[i].Level < newErrs[j].Level + } return newErrs[i].Details < newErrs[j].Details } return newErrs[i].Message < newErrs[j].Message @@ -285,7 +375,11 @@ func merge(errs []*FieldError) []*FieldError { // key returns the key using the fields .Message and .Details. func key(err *FieldError) string { - return fmt.Sprintf("%s-%s", err.Message, err.Details) + l := err.Level + if l == "" { + l = ErrorLevel + } + return fmt.Sprintf("%s-%s-%s", l, err.Message, err.Details) } // Public helpers --- diff --git a/apis/field_error_test.go b/apis/field_error_test.go index 20fdf03cb..da376d2f1 100644 --- a/apis/field_error_test.go +++ b/apis/field_error_test.go @@ -294,6 +294,53 @@ Second: X, Y, Z`, err: ErrOutOfBoundsValue(1*time.Second, 2*time.Second, 5*time.Second, "timeout"), prefixes: [][]string{{"spec"}}, want: `expected 2s <= 1s <= 5s: spec.timeout`, + }, { + name: "Explicit Error", + err: ErrMissingField("foo", "bar").At(ErrorLevel), + want: `missing field(s): bar, foo`, + }, { + name: "Warning", + err: ErrMissingField("foo", "bar").At(WarningLevel), + want: `missing field(s): bar, foo`, + }, { + name: "Explicit Error isn't filtered", + err: ErrMissingField("foo", "bar").At(ErrorLevel).Filter(ErrorLevel), + want: `missing field(s): bar, foo`, + }, { + name: "Implicit Error isn't filtered", + err: ErrMissingField("foo", "bar").Filter(ErrorLevel), + want: `missing field(s): bar, foo`, + }, { + name: "Warning isn't filtered", + err: ErrMissingField("foo", "bar").At(WarningLevel).Filter(WarningLevel), + want: `missing field(s): bar, foo`, + }, { + name: "Explicit Error is filtered", + err: ErrMissingField("foo", "bar").At(ErrorLevel).Filter(WarningLevel), + }, { + name: "Warning is filtered", + err: ErrMissingField("foo", "bar").At(WarningLevel).Filter(ErrorLevel), + }, { + name: "Mix of Errors and Warnings", + err: ErrMissingField("foo").Also(ErrMissingField("bar").At(WarningLevel)), + want: `missing field(s): foo +missing field(s): bar`, + }, { + name: "Mix of Errors and Warnings - Filter Warning", + err: ErrMissingField("foo").Also(ErrMissingField("bar").At(WarningLevel)).Filter(WarningLevel), + want: `missing field(s): bar`, + }, { + name: "Mix of Errors and Warnings - Filter Error", + err: ErrMissingField("foo").Also(ErrMissingField("bar").At(WarningLevel)).Filter(ErrorLevel), + want: `missing field(s): foo`, + }, { + name: "Mix of Errors and Warnings turned to Warnings", + err: ErrMissingField("foo").Also(ErrMissingField("bar").At(WarningLevel)).At(WarningLevel).Filter(WarningLevel), + want: `missing field(s): bar, foo`, + }, { + name: "Mix of Errors and Warnings turned to Errors", + err: ErrMissingField("foo").Also(ErrMissingField("bar").At(WarningLevel)).At(ErrorLevel).Filter(ErrorLevel), + want: `missing field(s): bar, foo`, }} for _, test := range tests { diff --git a/testing/inner_default_resource.go b/testing/inner_default_resource.go index 4f9299c86..9506858b1 100644 --- a/testing/inner_default_resource.go +++ b/testing/inner_default_resource.go @@ -46,9 +46,13 @@ type InnerDefaultSpec struct { FieldWithDefault string `json:"fieldWithDefault,omitempty"` - // Deprecated: This field is deprecated. + // Deprecated: This field is deprecated, and will emit an error if set. DeprecatedField string `json:"field,omitempty"` + // ToBeDeprecatedField: This field is deprecated, and will emit a warning + // if set. + ToBeDeprecatedField string `json:"fieldWillWarn,omitempty"` + SubFields *InnerDefaultSubSpec `json:"subfields,omitempty"` } @@ -153,5 +157,9 @@ func (i *InnerDefaultResource) Validate(ctx context.Context) *apis.FieldError { Also(apis.CheckDeprecated(ctx, i.Spec.SubFields.DeprecatedStruct).ViaField("deprecatedStruct"))) } } + + if i.Spec.ToBeDeprecatedField != "" { + errs = errs.Also(apis.ErrDisallowedFields("fieldWillWarn").At(apis.WarningLevel).ViaField("spec")) + } return errs } diff --git a/webhook/resourcesemantics/validation/validation_admit.go b/webhook/resourcesemantics/validation/validation_admit.go index 0544d88a6..d1e7da94d 100644 --- a/webhook/resourcesemantics/validation/validation_admit.go +++ b/webhook/resourcesemantics/validation/validation_admit.go @@ -60,7 +60,7 @@ func NewCallback(function func(context.Context, *unstructured.Unstructured) erro var _ webhook.AdmissionController = (*reconciler)(nil) // Admit implements AdmissionController -func (ac *reconciler) Admit(ctx context.Context, request *admissionv1.AdmissionRequest) *admissionv1.AdmissionResponse { +func (ac *reconciler) Admit(ctx context.Context, request *admissionv1.AdmissionRequest) (resp *admissionv1.AdmissionResponse) { if ac.withContext != nil { ctx = ac.withContext(ctx) } @@ -77,8 +77,19 @@ func (ac *reconciler) Admit(ctx context.Context, request *admissionv1.AdmissionR return webhook.MakeErrorStatus("decoding request failed: %v", err) } - if err := validate(ctx, resource, request); err != nil { - return webhook.MakeErrorStatus("validation failed: %v", err) + errors, warnings := validate(ctx, resource, request) + if warnings != nil { + // If there were warnings, then keep processing things, but augment + // whatever AdmissionResponse we send with the warnings. We cannot + // simply set `resp.Warnings` directly here because the return paths + // below all overwrite `resp`, but the `defer` affords us one final + // crack at things. + defer func() { + resp.Warnings = []string{warnings.Error()} + }() + } + if errors != nil { + return webhook.MakeErrorStatus("validation failed: %v", errors) } if err := ac.callback(ctx, request, gvk); err != nil { @@ -147,7 +158,7 @@ func (ac *reconciler) decodeRequestAndPrepareContext( return ctx, newObj, nil } -func validate(ctx context.Context, resource resourcesemantics.GenericCRD, req *admissionv1.AdmissionRequest) error { +func validate(ctx context.Context, resource resourcesemantics.GenericCRD, req *admissionv1.AdmissionRequest) (err error, warn error) { logger := logging.FromContext(ctx) // Only run validation for supported create and update validation. @@ -155,25 +166,32 @@ func validate(ctx context.Context, resource resourcesemantics.GenericCRD, req *a case admissionv1.Create, admissionv1.Update: // Supported verbs case admissionv1.Delete: - return nil // Validation handled by optional Callback, but not validatable. + return nil, nil // Validation handled by optional Callback, but not validatable. default: logger.Info("Unhandled webhook validation operation, letting it through ", req.Operation) - return nil + return nil, nil } // None of the validators will accept a nil value for newObj. if resource == nil { - return errMissingNewObject + return errMissingNewObject, nil } - if err := resource.Validate(ctx); err != nil { + if result := resource.Validate(ctx); result != nil { logger.Errorw("Failed the resource specific validation", zap.Error(err)) - // Return the error message as-is to give the validation callback - // discretion over (our portion of) the message that the user sees. - return err + // While we have the strong typing of apis.FieldError, partition the + // returned error into the error-level diagnostics and warning-level + // diagnostics, so that the admission response can embed things into + // the appropriate portions of the response. + // This is expanded like to to avoid problems with typed nils. + if errorResult := result.Filter(apis.ErrorLevel); errorResult != nil { + err = errorResult + } + if warningResult := result.Filter(apis.WarningLevel); warningResult != nil { + warn = warningResult + } } - - return nil + return err, warn } // callback runs optional callbacks on admission diff --git a/webhook/resourcesemantics/validation/validation_deprecated_test.go b/webhook/resourcesemantics/validation/validation_deprecated_test.go index 200e101df..0998c7f65 100644 --- a/webhook/resourcesemantics/validation/validation_deprecated_test.go +++ b/webhook/resourcesemantics/validation/validation_deprecated_test.go @@ -29,7 +29,7 @@ import ( . "knative.dev/pkg/webhook/testing" ) -// In strict mode, you are not allowed to set a deprecated filed when doing a Create. +// In strict mode, you are not allowed to set a deprecated field when doing a Create. func TestStrictValidation(t *testing.T) { newCreateReq := func(new []byte) *admissionv1.AdmissionRequest { @@ -60,9 +60,10 @@ func TestStrictValidation(t *testing.T) { } testCases := map[string]struct { - strict bool - req *admissionv1.AdmissionRequest - wantErrs []string + strict bool + req *admissionv1.AdmissionRequest + wantErrs []string + wantWarnings []string }{ "create, strict": { strict: true, @@ -464,11 +465,13 @@ func TestStrictValidation(t *testing.T) { FieldAsString: "fail", }, }, + ToBeDeprecatedField: "asdf", }, nil)), wantErrs: []string{ "must not update", "spec.subFields.structPtr", }, + wantWarnings: []string{"must not set", "fieldWillWarn"}, }, "create, not strict": { @@ -485,6 +488,14 @@ func TestStrictValidation(t *testing.T) { DeprecatedField: "fail setting.", }, nil)), }, + + "create, not strict, with warning": { + strict: false, + req: newCreateReq(createInnerDefaultResourceWithSpecAndStatus(t, &InnerDefaultSpec{ + ToBeDeprecatedField: "emit a warning", + }, nil)), + wantWarnings: []string{"must not set", "fieldWillWarn"}, + }, } for n, tc := range testCases { @@ -504,6 +515,9 @@ func TestStrictValidation(t *testing.T) { } else { ExpectAllowed(t, resp) } + for _, err := range tc.wantWarnings { + ExpectWarnsWith(t, resp, err) + } }) } } diff --git a/webhook/testing/testing.go b/webhook/testing/testing.go index 0543d91ec..488565180 100644 --- a/webhook/testing/testing.go +++ b/webhook/testing/testing.go @@ -72,6 +72,21 @@ func ExpectFailsWith(t *testing.T, resp *admissionv1.AdmissionResponse, contains } } +// ExpectWarnsWith checks that a given admission response warns on the initiating request +// containing the provided string in its warning message. +func ExpectWarnsWith(t *testing.T, resp *admissionv1.AdmissionResponse, contains string) { + t.Helper() + found := false + for _, warning := range resp.Warnings { + if strings.Contains(warning, contains) { + found = true + } + } + if !found { + t.Errorf("Expected warning containing %q got %v", contains, resp.Warnings) + } +} + // ExpectPatches checks that the provided serialized bytes consist of an expected // collection of patches. This is used to verify the mutations made in a mutating // admission webhook's response.