Make connection lost retryable in webhook #117251

linxiulei · 2023-04-12T17:47:15Z

When a http2 connection dies due to ping timeout, http2 client gets an error of "http2: client connection lost". This is similar to ConnectionReset case so it should be retryable.

What type of PR is this?

/kind bug

What this PR does / why we need it:

This PR improves resilience of webhook function in kube-apiserver by retrying requests failed on dead tcp connections.

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

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

k8s-ci-robot · 2023-04-12T17:47:25Z

Hi @linxiulei. 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.

wzshiming · 2023-04-13T02:01:28Z

/ok-to-test

wzshiming · 2023-04-13T02:06:38Z

/triage accepted
/priority backlog

linxiulei · 2023-04-13T08:16:01Z

/retest

aojea · 2023-04-26T14:30:23Z

/assign

aojea · 2023-04-29T23:49:33Z

I knew there was some history here #109022

aojea · 2023-05-15T08:53:56Z

staging/src/k8s.io/apiserver/pkg/util/webhook/webhook.go

@@ -62,7 +62,7 @@ type GenericWebhook struct {
 // Otherwise it returns false for an immediate fail.
 func DefaultShouldRetry(err error) bool {
 	// these errors indicate a transient error that should be retried.
-	if utilnet.IsConnectionReset(err) || apierrors.IsInternalError(err) || apierrors.IsTimeout(err) || apierrors.IsTooManyRequests(err) {
+	if utilnet.IsConnectionReset(err) || utilnet.IsConnectionLost(err) || apierrors.IsInternalError(err) || apierrors.IsTimeout(err) || apierrors.IsTooManyRequests(err) {


The only retryable operations should be those that are idempotent or those that the error indicate that there is no risk to retry, IsConnectionLost means that the ping frame was not answered and is a connection level error that impacts all in-flight requests
https://cs.opensource.google/go/x/net/+/master:http2/transport.go

It doesn't seem safe to retry on this error

I am not too sure about if it's idempotent but I do see some similarity between IsConnectionReset and IsConnectionLost.

Anyway, is it safe to make it retryable just for webhook authentication since it's idempotent (ie. review token)? cc @liggitt

Agree with IsConnectionReset, since this can be sent after the server processed some data.
Jordan know better the mechanics of the webhooks, if all operations are idempotent then +1 to retry, otherwise we should alos remove the IsConnectionReset IMHO

authn and authz webhooks are safe to retry

admission webhooks are generally safe to retry, are already retried at a higher level when there's an etcd write conflict, and provide a uid in the review body so a webhook that cared about idempotence (because it had side effects it wanted to minimize) could detect a retry

IIUC, only authn & authz use DefaultShouldRetry https://sourcegraph.com/github.com/kubernetes/kubernetes@b790933e8aefdac194ba3c199b5e552a94107950/-/blob/staging/src/k8s.io/apiserver/pkg/util/webhook/webhook.go?L63%3A6=#tab=references

staging/src/k8s.io/apimachinery/pkg/util/net/util.go

aojea · 2023-05-15T13:30:10Z

/lgtm
#117251 (comment)

aojea · 2023-05-15T18:16:22Z

/lgtm
/assign @liggitt

k8s-ci-robot · 2023-05-15T18:16:28Z

LGTM label has been added.

Git tree hash: 413b6ae0b9216528683e9a28f7ad1e046f3ca674

liggitt · 2023-05-15T20:48:58Z

staging/src/k8s.io/apimachinery/pkg/util/net/util.go

@@ -47,6 +48,11 @@ func IsConnectionReset(err error) bool {
 	return false
 }

+// Returns if the given err is "http2: client connection lost" error.
+func IsHTTP2ConnectionLost(err error) bool {
+	return err != nil && strings.Contains(err.Error(), "http2: client connection lost")


if we're going to just match error strings, shouldn't we have a test that notices if the go string changes and breaks this method?

https://github.com/golang/net/blob/master/http2/transport.go#L1134

I really wish we could be more robust here :-/

@neild how do you feel about exposing the error in golang?

I really wish we could be more robust here :-/

hmm, the test setup would be painful to set up a http2 server and figure out a way to fail the ping health check. OTOH, I feel the possibility of change the error message in golang is extremely low.

Exposing the error in golang is much feasible

the test setup would be painful to set up a http2 server and figure out a way to fail the ping health check.

we do have that test

kubernetes/staging/src/k8s.io/client-go/rest/connection_test.go

Line 87 in bdbfbff

func TestReconnectBrokenTCP(t *testing.T) {

Thanks, it makes things easier. PTAL!

staging/src/k8s.io/apimachinery/pkg/util/net/util_test.go

aojea · 2023-05-20T18:12:58Z

/lgtm

k8s-ci-robot · 2023-05-20T18:13:04Z

LGTM label has been added.

Git tree hash: 3627aff8f08166b94ba412ce5d07349bbd07e5f0

When a http2 connection dies due to ping timeout, http2 client gets an error of "http2: client connection lost". This is similar to ConnectionReset case so it should be retryable. Signed-off-by: Eric Lin <exlin@google.com>

linxiulei · 2023-05-20T20:00:06Z

/retest

aojea · 2023-05-21T12:16:41Z

/lgtm

k8s-ci-robot · 2023-05-21T12:16:46Z

LGTM label has been added.

Git tree hash: 0c2f8024859925eed910d52f069599325af53abb

liggitt · 2023-05-22T15:28:57Z

/lgtm
/approve

k8s-ci-robot · 2023-05-22T15:29:21Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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/apimachinery/pkg/util/OWNERS~~ [liggitt]
~~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

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Apr 12, 2023

k8s-ci-robot requested review from apelisse and liggitt April 12, 2023 17:47

k8s-ci-robot added area/apiserver sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Apr 12, 2023

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 13, 2023

k8s-ci-robot assigned aojea Apr 26, 2023

aojea reviewed May 15, 2023

View reviewed changes

staging/src/k8s.io/apimachinery/pkg/util/net/util.go Show resolved Hide resolved

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 15, 2023

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 15, 2023

k8s-ci-robot requested a review from aojea May 15, 2023 15:19

k8s-ci-robot assigned liggitt May 15, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 15, 2023

liggitt reviewed May 15, 2023

View reviewed changes

linxiulei force-pushed the wh branch from bed9567 to 107f615 Compare May 19, 2023 19:15

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels May 19, 2023

k8s-ci-robot requested a review from liggitt May 19, 2023 19:16

aojea reviewed May 20, 2023

View reviewed changes

staging/src/k8s.io/apimachinery/pkg/util/net/util_test.go Outdated Show resolved Hide resolved

linxiulei force-pushed the wh branch from 107f615 to f77f61d Compare May 20, 2023 17:21

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 20, 2023

Make connection lost retryable in webhook

2658a2b

When a http2 connection dies due to ping timeout, http2 client gets an error of "http2: client connection lost". This is similar to ConnectionReset case so it should be retryable. Signed-off-by: Eric Lin <exlin@google.com>

linxiulei force-pushed the wh branch from f77f61d to 2658a2b Compare May 20, 2023 19:03

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 20, 2023

k8s-ci-robot requested a review from aojea May 20, 2023 19:04

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 21, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 22, 2023

k8s-ci-robot merged commit d9df6b0 into kubernetes:master May 22, 2023

k8s-ci-robot added this to the v1.28 milestone May 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make connection lost retryable in webhook #117251

Make connection lost retryable in webhook #117251

linxiulei commented Apr 12, 2023

k8s-ci-robot commented Apr 12, 2023

wzshiming commented Apr 13, 2023

wzshiming commented Apr 13, 2023

linxiulei commented Apr 13, 2023

aojea commented Apr 26, 2023

aojea commented Apr 29, 2023

aojea May 15, 2023 •

edited

Loading

linxiulei May 15, 2023

aojea May 15, 2023

liggitt May 15, 2023

linxiulei May 15, 2023

aojea commented May 15, 2023

aojea commented May 15, 2023

k8s-ci-robot commented May 15, 2023

liggitt May 15, 2023

liggitt May 15, 2023

aojea May 15, 2023

linxiulei May 16, 2023

aojea May 16, 2023

linxiulei May 19, 2023

aojea commented May 20, 2023

k8s-ci-robot commented May 20, 2023

linxiulei commented May 20, 2023

aojea commented May 21, 2023

k8s-ci-robot commented May 21, 2023

liggitt commented May 22, 2023

k8s-ci-robot commented May 22, 2023

Make connection lost retryable in webhook #117251

Make connection lost retryable in webhook #117251

Conversation

linxiulei commented Apr 12, 2023

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

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

k8s-ci-robot commented Apr 12, 2023

wzshiming commented Apr 13, 2023

wzshiming commented Apr 13, 2023

linxiulei commented Apr 13, 2023

aojea commented Apr 26, 2023

aojea commented Apr 29, 2023

aojea May 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aojea commented May 15, 2023

aojea commented May 15, 2023

k8s-ci-robot commented May 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aojea commented May 20, 2023

k8s-ci-robot commented May 20, 2023

linxiulei commented May 20, 2023

aojea commented May 21, 2023

k8s-ci-robot commented May 21, 2023

liggitt commented May 22, 2023

k8s-ci-robot commented May 22, 2023

aojea May 15, 2023 •

edited

Loading