[Feature Request]: Support "Warning" for Validation Webhook

[STRRL]

refer: #1788

After Kubernetes 1.19, the webhook could respond with "warning"s, refer: https://kubernetes.io/docs/reference/access-authn-authz/extensible-admission-controllers/#response

Warnings for validation webhook would bring much help for users with interacting with API.

Currently, users of controller-runtime could implement their "customized" admission.Handler to respond with "warnings". But it's might be not easy to use. Also as mentioned in #1788, using util to wrap warning messages might be helpful. IMHO, controller-runtime should support warning "natively" with Validator and CustomValidator.

I am interested in this feature/enhancement, so maybe there are possible designs and steps to build this feature:

• extend Allowed(), Denied(), ValidationResponse(), make them support warning []string
• bring an interface to extend error, called ErrorWithWarnings like:

type ErrorWithWarnings interface {
	error
	Warnings() []string
}

• it means it user want to respond with warning, it should return this interface with ValidateXXX()
• make type assertion when resolving the error from Validator/CustomValidator, fill Warnings in v1.AdmissionResponse

Some changes might break the existing API, I think it's OK to controller-runtime. If not, I could still try to only introduce new API without breaking the exist one.

[STRRL]

what do you think? If you are not satisfied with this design, I am very happy to make it better. :)

/cc @FillZpp @vincepri @alvaroaleman

[FillZpp]

@STRRL Thanks. Does the existing WithWarnings function not satisfy the requirement?

controller-runtime/pkg/webhook/admission/response.go

Lines 116 to 121 in 26c95ad

	// WithWarnings adds the given warnings to the Response.
	// If any warnings were already given, they will not be overwritten.
	func (r Response) WithWarnings(warnings ...string) Response {
	r.AdmissionResponse.Warnings = append(r.AdmissionResponse.Warnings, warnings...)
	return r
	}

We can use it with any response like Allowed(), Denied() or ValidationResponse(). For example

    // ...
    return admission.Allowed("").WithWarnings("warning message ...")

[STRRL]

Ohh, I missed that method. That would help a lot. We do not require the first step in my original design. ❤️

But the user still needs to implement Handler instead of Validator and CustomValidator, I think maybe introducing ErrorWithWarnings still makes sense.

[FillZpp]

Emm... I don't really understand. Handler implementation just has to define a func Handle(context.Context, Request) Response and its response could also use the WithWarnings.

[STRRL]

I think it's better also support waning on high-level API Validator/CustomValidator based on the provided low-level API Response.WithWarnings. :)

[FillZpp]

Oh, you mean expose the Warnings in the Validator/CustomValidator interfaces? In this way, we should not only expose the Warnings, but the Response with its fields like Warnings, ErrorCode, Reason.

controller-runtime/pkg/webhook/admission/validator.go

Lines 30 to 35 in 26c95ad

	type Validator interface {
	runtime.Object
	ValidateCreate() error
	ValidateUpdate(old runtime.Object) error
	ValidateDelete() error
	}

controller-runtime/pkg/webhook/admission/validator_custom.go

Lines 31 to 35 in 26c95ad

	type CustomValidator interface {
	ValidateCreate(ctx context.Context, obj runtime.Object) error
	ValidateUpdate(ctx context.Context, oldObj, newObj runtime.Object) error
	ValidateDelete(ctx context.Context, obj runtime.Object) error
	}

That's ok to me. But I'm not sure if the API change is necessary. WDYT @alvaroaleman @vincepri

[STRRL]

ErrorCode, Reason could be carried with apierrors.APIStatus:

controller-runtime/pkg/webhook/admission/validator.go

Lines 71 to 78 in b826f39

	err = obj.ValidateCreate()
	if err != nil {
	var apiStatus apierrors.APIStatus
	if goerrors.As(err, &apiStatus) {
	return validationResponseFromStatus(false, apiStatus.Status())
	}
	return Denied(err.Error())
	}

Warning is not considered right now, so I think maybe we could use a similar way. :)

One different thing is a user might want only to respond "warnings" without "error". I realized that my original design might not work well with this situation, because it could not resolve the error is nil.

Maybe make changes on Validator interface (or another interface ValidatorWarning) are required:

 type ValidatorWarning interface { 
 	runtime.Object 
 	ValidateCreate() (err error, warnings []string) 
 	ValidateUpdate(old runtime.Object) (err error, warnings []string)  
 	ValidateDelete() (err error, warnings []string)  
 }

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[invidian]

/remove-lifecycle stale

[STRRL]

Hi @invidian , I create a PR for resolve this issue, PTAL ❤️

[STRRL]

also cc @FillZpp

[joelanford]

I realized that my original design might not work well with this situation, because it could not resolve the error is nil.

We could resolve that right? If the error is an ErrorWithWarnings we can use the embedded error as the "real" error, and that one can be nil while warnings is not.

If the returned error is not an ErrorWithWarnings, then we have the "real" error directly with no warnings.

[sbueringer]

@joelanford's suggestion sounds good to me

I think we have one fundamental choice to make and then everything follows from there:

Is it oky to change the existing Validator/Defaulter and CustomValidator/CustomDefaulter interfaces or not?

1. If yes => we can introduce an additional return parameter for warnings
2. If no => let's go with Joel's suggestion of wrapping errors and warnings in an error

@alvaroaleman @vincepri Sorry for the ping, I know you're pretty busy. What is your opinion on that?

P.S. Does 2. come down to the same as #1788?

[alvaroaleman]

Hm, after skimming the conversation I am actually in favor of breaking the interface over making it hard to use in the context of warnings by having to return a specific error type. It will be a visible break that is easy to fix.

The only comment I have on the suggestion in #1896 (comment) regarding how that would look like is that errors per golang convention should always be the last return parameter.

[sbueringer]

Sounds good to me!

[fabriziopandini]

I am actually in favor of breaking the interface over making it hard to use in the context of warnings by having to return a specific error type. It will be a visible break that is easy to fix.

+1

[STRRL]

I realized that my original design might not work well with this situation, because it could not resolve the error is nil.

We could resolve that right? If the error is an ErrorWithWarnings we can use the embedded error as the "real" error, and that one can be nil while warnings is not.

If the returned error is not an ErrorWithWarnings, then we have the "real" error directly with no warnings.

No. I do not think we could resolve that, because ErrorWithWarnings is an interface(which composes an error interface), not a struct(which embeds an error instance). We could not return a result that carries the "warnings" and also introduces "error is nil" at the same time

[STRRL]

Hm, after skimming the conversation I am actually in favor of breaking the interface over making it hard to use in the context of warnings by having to return a specific error type. It will be a visible break that is easy to fix.

The only comment I have on the suggestion in #1896 (comment) regarding how that would look like is that errors per golang convention should always be the last return parameter.

I would update the PR soon!

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle stale
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[invidian]

/remove-lifecycle stale