Add support for admission webhook warnings. (#2498)

* Add support for admission webhook warnings.

This extends `apis.FieldError` to support designating certain FieldErrors as "warnings" (or explicitly as "errors", however, this is the default for back-compat).

You can turn an `apis.FieldError` into a warning using: `fe.At(apis.WarningLevel)` or force it into an error using: `fe.At(apis.ErrorLevel)`.

You can get the errors at a particular diagnostic level using: `fe.Filter(apis.WarningLevel)`.

This change also hooks this into the admission webhook infrastructure to support surfacing the "warning" level `apis.FieldError`s via the `Warnings` section of the `AdmissionResponse`.

Fixes: #2497

* Add a comment about the use of defer.

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)) {

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 ---

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 {

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
 }

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

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)
+			}
 		})
 	}
 }

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.