Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support for ignoring bad response from mutating admissions webhook #129459

Open
michael-aldridge-tracebit opened this issue Jan 2, 2025 · 2 comments
Open

Support for ignoring bad response from mutating admissions webhook #129459

michael-aldridge-tracebit opened this issue Jan 2, 2025 · 2 comments
Labels
kind/feature Categorizes issue or PR as related to a new feature. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery.

Comments

@michael-aldridge-tracebit
Copy link

What would you like to be added?

Currently, you can configure the kubernetes control plane to ignore errors when calling a mutating admission webhook

webhooks.failurePolicy (string)

FailurePolicy defines how unrecognized errors from the admission endpoint are handled - allowed values are Ignore or Fail. > Defaults to Fail.

~ https://kubernetes.io/docs/reference/kubernetes-api/extend-resources/mutating-webhook-configuration-v1/

This is great, as some webhooks are 'nice to have' rather than critical and I do not want their failure to block resource creation.

However, it seems as though failurePolicy only applies to errors calling the webhook, and not from handeling the response. For example, I created an (intentionally faulty) webhook that returns an invalid patch under certain conditions (specifically it tries to add an entry to /metadata/annotations even if it does not exist). When I tried to create an appropriate pod, I got an error response, even though I had set this webhook to have an Ignore failure policy. e.g.

% kubectl create -f dummy-pod.yaml
 Error from server (InternalError): error when creating "dummy-pod.yaml": Internal error occurred: add operation does  
 not apply: doc is missing path: "/metadata/annotations/webhook-test": missing value

I had a look through the source and I think this might be where the decision is being made about which errors to fail open for.

kubernetes/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/dispatcher.go

Lines 204 to 214 in 35f5841

if callErr, ok := err.(*webhookutil.ErrCallingWebhook); ok {
if ignoreClientCallFailures {
// Ignore context cancelled from webhook metrics
if errors.Is(callErr.Reason, context.Canceled) {
klog.Warningf("Context canceled when calling webhook %v", hook.Name)
} else {
klog.Warningf("Failed calling webhook, failing open %v: %v", hook.Name, callErr)
admissionmetrics.Metrics.ObserveWebhookFailOpen(ctx, hook.Name, "admit")
annotator.addFailedOpenAnnotation()
}
utilruntime.HandleError(callErr)

It would be great if there was some way to tell kubernetes to ignore invalid responses (as well as errors calling the webhook). Of course, if there is already a way to do this, please do let me know!

Why is this needed?

This would increase the safety of using a webhook.
@michael-aldridge-tracebit michael-aldridge-tracebit added the kind/feature Categorizes issue or PR as related to a new feature. label Jan 2, 2025
@k8s-ci-robot k8s-ci-robot added needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jan 2, 2025
@k8s-ci-robot
Copy link
Contributor

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@michael-aldridge-tracebit
Copy link
Author

/sig api-machinery

@k8s-ci-robot k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jan 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
kind/feature Categorizes issue or PR as related to a new feature. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@k8s-ci-robot @michael-aldridge-tracebit