Incorporating feedback on 119341

[divyasri537]

What type of PR is this?

/kind bug

What this PR does / why we need it:

Incorporating feedback on PR #119341

Which issue(s) this PR fixes:

Fixes #119341

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Incorporating feedback on PR #119341

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

[k8s-ci-robot]

Hi @divyasri537. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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/test-infra repository.

[divyasri537]

/assign @liggitt

[ivelichkovich]

I don't think we want to break here either, we probably want to flow through to the end

[divyasri537]

I have added break statement here, so the metric counter in next steps wont pick the canceled context.

[ivelichkovich]

did you look into checking err instead of ctx.Err?

[divyasri537]

Yes I checked and added the comments in the old PR #119341

[divyasri537]

The err that is from callAttrMutatingHook method [ref] based on the provided ctx . The err.Reason returns customized error response that picks up the ctx error so I don’t think anything wrong by checking this way ctx.Err() == context.Canceled as it is similar to checking the err obj.

Also The err.Reason returns
%!(EXTRA string=failed to call webhook: Post "https://127.0.0.1:21375/validate?timeout=2s": context canceled) which is a customized error and may break if someone modifies the error message so I think its better to go with checking ctx.Err().

[ivelichkovich]

same thing with break here

[ivelichkovich]

/ok-to-test

[divyasri537]

/kind bug

[ivelichkovich]

same here add comment

[ivelichkovich]

update comment, it doesn't only apply to failopen metrics

[ivelichkovich]

so here just doing

errCh <- apierrors.NewInternalError(err)
return

is not necessarily the same behavior. The if ignoreClientCallFailures block uses utilruntime.HandlerError while the other block does the errCh <- apierrors.NewInternalError(err)

[divyasri537]

Lets see what liggitt thinks here.

[ivelichkovich]

I think it probably makes sense to always do errCh <- apierrors.NewInternalError(err) for context cancelled but will defer to @liggitt

[ivelichkovich]

same here, not necessarily the same behavior. The blocks for ignoreClientCallFailures and outside that block handle the error differently

[divyasri537]

utilruntime.HandleError(callErr) returns "failed calling webhook "eks-aws-auth-configmap-validation-webhook.amazonaws.com": failed to call webhook: Post "https://127.0.0.1:21375/mutate?timeout=2s": context canceled" as we wanted to put customized log msg for failopen context canceled so keeping this change and return apierrors.NewInternalError(err).

[ivelichkovich]

So @liggitt I think if context is cancelled it makes sense to just return this error. Previous behavior was it would continue if ignoreClientCallFailures { but we can probably just return and bail out

[alexzielenski]

/triage accepted

[ivelichkovich]

this doesn't only apply to failopen metric

[liggitt]

skipping the webhook rejection metric for cancelled requests seems ok, but do we really want to lose visibility to the fact that the webhook was called at all and the latency we hit for canceled requests?

I was hoping we could revert back to the state at 5e5b302 and narrowly skip the ObserveWebhookRejection and ObserveWebhookFailOpen/addFailedOpenAnnotation bits when the reason was a context cancel

something like this:

$ git diff -w 5e5b3029f3bbfc93c3569f07ad300a5c6057fc58 HEAD -- staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/dispatcher.go
diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/dispatcher.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/dispatcher.go
index c1d1ca6ff6b..61b17932df1 100644
--- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/dispatcher.go
+++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/dispatcher.go
@@ -20,6 +20,7 @@ package mutating
 
 import (
        "context"
+       "errors"
        "fmt"
        "time"
 
@@ -164,14 +165,18 @@ func (a *mutatingDispatcher) Dispatch(ctx context.Context, attr admission.Attrib
                annotator := newWebhookAnnotator(versionedAttr, round, i, hook.Name, invocation.Webhook.GetConfigurationName())
                changed, err := a.callAttrMutatingHook(ctx, hook, invocation, versionedAttr, annotator, o, round, i)
                ignoreClientCallFailures := hook.FailurePolicy != nil && *hook.FailurePolicy == admissionregistrationv1.Ignore
+
                rejected := false
                if err != nil {
                        switch err := err.(type) {
                        case *webhookutil.ErrCallingWebhook:
                                if !ignoreClientCallFailures {
                                        rejected = true
+                                       // Ignore context cancelled from webhook metrics
+                                       if !errors.Is(err.Reason, context.Canceled) {
                                                admissionmetrics.Metrics.ObserveWebhookRejection(ctx, hook.Name, "admit", string(versionedAttr.Attributes.GetOperation()), admissionmetrics.WebhookRejectionCallingWebhookError, int(err.Status.ErrStatus.Code))
                                        }
+                               }
                                admissionmetrics.Metrics.ObserveWebhook(ctx, hook.Name, time.Since(t), rejected, versionedAttr.Attributes, "admit", int(err.Status.ErrStatus.Code))
                        case *webhookutil.ErrWebhookRejection:
                                rejected = true
@@ -199,9 +204,14 @@ func (a *mutatingDispatcher) Dispatch(ctx context.Context, attr admission.Attrib
 
                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)

[liggitt]

(same comments apply to the validating dispatcher)

[ivelichkovich]

Sure that makes sense

[ivelichkovich]

There was some feedback though that we wanted the math to add up in the metrics correctly. #119341 (comment) so that total = success + failure. So we were looking at skipping the metrics or adding a new metric that counts cancelled. #119341 (comment).

[liggitt]

if we're going to be backporting this, we should err on the side of fewer changes, imo. narrowly skipping the misleading rejection/failopen bits for cancelled requests and changing ~nothing else w.r.t. return values makes sense for a backport

if we want to add a new metric or label or label value to distinguish cancellation as a follow up in 1.29+ only, that also seems ok

[ivelichkovich]

Yeah that makes sense

[ivelichkovich]

/lgtm

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 3cf1a9f2fd98141cbdeb8027e961d9700c3666ac

[liggitt]

/lgtm
/approve

[liggitt]

/hold

please squash down to a single commit before merge

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: divyasri537, liggitt

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• staging/src/k8s.io/apiserver/OWNERS [liggitt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[divyasri537]

/hold

please squash down to a single commit before merge

@liggitt Squashed to one commit.

[liggitt]

/lgtm
/hold cancel

[divyasri537]

/retest