Always retry connection reset error in webhook #53947

crassirostris · 2017-10-15T14:58:35Z

Audit logging uses webhook to send events to the backend and currently even a little blip in networking can cause several hundreds of events to be lost. This PR adds an additional check, that is similar to the one in the rest package, but ignores the fact that the request is not GET and always retries "Connection reset by peers" error.

Webhook always retries connection reset error.

sttts · 2017-10-16T08:06:54Z

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

@@ -112,6 +113,11 @@ func WithExponentialBackoff(initialBackoff time.Duration, webhookFn func() error
 		if _, shouldRetry := apierrors.SuggestsClientDelay(err); shouldRetry {
 			return false, nil
 		}
+		// "Connection reset by peer" is usually a transient error, but it's not
+		// retried by the rest client because the request is not GET.
+		if net.IsConnectionReset(err) {


shouldn't this move up to the first if-clause?

I would move IsConnectionReset and also IsInternalError up. Both are unconditional, as the others. Then for all other errors we call SuggestsClientDelay. Now in the current shape it looks like the order of the tests matter. But it doesn't.

crassirostris · 2017-10-16T08:09:12Z

I feel that this is kind of special case, requiring a separate explanation. This is not a delay, but an outage

…

On Oct 16, 2017 10:07, "Dr. Stefan Schimanski" ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In staging/src/k8s.io/apiserver/pkg/util/webhook/webhook.go <#53947 (comment)> : > @@ -112,6 +113,11 @@ func WithExponentialBackoff(initialBackoff time.Duration, webhookFn func() error if _, shouldRetry := apierrors.SuggestsClientDelay(err); shouldRetry { return false, nil } + // "Connection reset by peer" is usually a transient error, but it's not + // retried by the rest client because the request is not GET. + if net.IsConnectionReset(err) { shouldn't this move up to the first if-clause? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#53947 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABrNWZaFqGkWOsSuEbHX_29TwS55pp68ks5ssw67gaJpZM4P5xku> .

Signed-off-by: Mik Vyatskov <vmik@google.com>

crassirostris · 2017-10-16T10:27:26Z

@sttts PTAL

I removed the IsServerTimeout check, b/c it's included in the SuggestsClientDelay function

sttts · 2017-10-16T10:41:08Z

Which other webhook does this apply to? Admission? Authorization?

/cc @liggitt

crassirostris · 2017-10-16T11:00:40Z

Which other webhook does this apply to? Admission? Authorization?

All of them, yes. But I think this change makes sense for all webhooks

crassirostris · 2017-10-16T11:00:44Z

/retest

liggitt · 2017-10-16T13:44:18Z

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

@@ -104,17 +105,14 @@ func WithExponentialBackoff(initialBackoff time.Duration, webhookFn func() error
 	var err error
 	wait.ExponentialBackoff(backoff, func() (bool, error) {
 		err = webhookFn()
-		// these errors indicate a need to retry an authentication check
-		if apierrors.IsServerTimeout(err) || apierrors.IsTimeout(err) || apierrors.IsTooManyRequests(err) {


why did you remove IsServerTimeout?

Because it's already checked in SuggestsClientDelay. I can return it, if you want the check to be more explicit

liggitt · 2017-10-16T13:50:37Z

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

-		// these errors indicate a need to retry an authentication check
-		if apierrors.IsServerTimeout(err) || apierrors.IsTimeout(err) || apierrors.IsTooManyRequests(err) {
+		// these errors indicate a transient error that should be retried.
+		if net.IsConnectionReset(err) || apierrors.IsInternalError(err) || apierrors.IsTimeout(err) || apierrors.IsTooManyRequests(err) {


doesn't client-go already turn connection errors into internal server errors?

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

Lines 652 to 667 in 4a6fec7

if err != nil {

// "Connection reset by peer" is usually a transient error.

// Thus in case of "GET" operations, we simply retry it.

// We are not automatically retrying "write" operations, as

// they are not idempotent.

if !net.IsConnectionReset(err) || r.verb != "GET" {

return err

}

// For the purpose of retry, we set the artificial "retry-after" response.

// TODO: Should we clean the original response if it exists?

resp = &http.Response{

StatusCode: http.StatusInternalServerError,

Header: http.Header{"Retry-After": []string{"1"}},

Body: ioutil.NopCloser(bytes.NewReader([]byte{})),

}

}

client-go only retries on GET calls because it doesn't want to replay non-idempotent calls. this code should not assume all webhook calls are idempotent, either. seems like it would be better to be able to indicate a request is idempotent

Only if it's a GET, but e.g. request with audit logs is POST

right... this shouldn't assume all webhook calls are idempotent

Writing audit logs is not an idempotent call, but it's still better to write audit logs twice than loose, esp. considering that audit logs have ids in it, that allow deduplication

seems like it would be better to be able to indicate a request is idempotent

Is there really a need to do so? We still cannot guarantee that a request hasn't been processed if e.g. a timeout is returned. Additionally, for all known to me webhooks (auth[nz], audit) I see no benefit in not retrying connection resets

I mean, if we really care about a request being idempotent in this case, we shouldn't unconditionally retry e.g. 5XX. However, nobody complained about this code before, so I think it's safe to assume that retrying is more important than idempotence, WDYT?

I mean, if we really care about a request being idempotent in this case, we shouldn't unconditionally retry e.g. 5XX. However, nobody complained about this code before, so I think it's safe to assume that retrying is more important than idempotence, WDYT?

Hmm, that's probably true

crassirostris · 2017-10-16T14:55:47Z

@sttts @liggitt Do you want me to change something?

ericchiang · 2017-10-16T16:11:20Z

At this point, would it be easier to list the cases where we don't want to retry?

crassirostris · 2017-10-17T09:50:15Z

@ericchiang

At this point, would it be easier to list the cases where we don't want to retry?

I'm not sure that is the right strategy: IMO it's better to be conservative. I.e. if some new not anticipated problem occurs, it's better to be as vocal about it as possible. For example, backend suddenly starts to reply with 418. It's better to notice this in monitoring and fix earlier, than retry each request hoping backend will stop acting as a teapot.

sttts · 2017-10-17T10:28:39Z

I am happy with the current shape, but lack the knowledge about the webhook context to forsee any side-effects. @liggitt any doubt this can go in?

liggitt · 2017-10-17T21:22:56Z

I think this is ok

sttts · 2017-10-18T07:21:26Z

/lgtm
/approve no-issue

k8s-github-robot · 2017-10-18T07:21:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: crassirostris, sttts

Associated issue: 52909

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

Needs approval from an approver in each of these OWNERS Files:

~~staging/src/k8s.io/apiserver/OWNERS~~ [sttts]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-ci-robot · 2017-10-18T12:12:40Z

@crassirostris: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-gce-gpu	`59bacba`	link	`/test pull-kubernetes-e2e-gce-gpu`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

k8s-github-robot · 2017-10-18T13:44:02Z

Automatic merge from submit-queue (batch tested with PRs 53958, 53947). If you want to cherry-pick this change to another branch, please follow the instructions here.

…-#53947-upstream-release-1.8 Automatic merge from submit-queue. Automated cherry pick of #53947 Cherry pick of #53947 on release-1.8. #53947: Always retry network connection error in webhook ```release-note Webhook always retries connection reset error. ```

crassirostris added this to the v1.9 milestone Oct 15, 2017

crassirostris assigned sttts Oct 15, 2017

crassirostris requested a review from sttts October 15, 2017 14:58

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 15, 2017

crassirostris force-pushed the retry-webhook-net-errors branch from b4ebbf4 to 40161ed Compare October 15, 2017 14:59

sttts reviewed Oct 16, 2017

View reviewed changes

Always retry network connection error in webhook

59bacba

Signed-off-by: Mik Vyatskov <vmik@google.com>

crassirostris force-pushed the retry-webhook-net-errors branch from 40161ed to 59bacba Compare October 16, 2017 10:26

k8s-ci-robot requested a review from liggitt October 16, 2017 10:41

liggitt reviewed Oct 16, 2017

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 18, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 18, 2017

crassirostris closed this Oct 18, 2017

crassirostris reopened this Oct 18, 2017

k8s-github-robot merged commit 14a1a15 into kubernetes:master Oct 18, 2017

crassirostris mentioned this pull request Oct 18, 2017

Automated cherry pick of #53947 #54153

Merged

crassirostris mentioned this pull request Oct 25, 2017

Advanced Auditing 1.9 umbrella bug #54551

Closed

14 tasks

erictune added area/admission-control and removed area/admission-control approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 26, 2017

akotlar mentioned this pull request Dec 28, 2018

batch hung hail-is/hail#4984

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Always retry connection reset error in webhook #53947

Always retry connection reset error in webhook #53947

crassirostris commented Oct 15, 2017 •

edited

Loading

sttts Oct 16, 2017

sttts Oct 16, 2017

crassirostris commented Oct 16, 2017 via email

crassirostris commented Oct 16, 2017

sttts commented Oct 16, 2017

crassirostris commented Oct 16, 2017 •

edited

Loading

crassirostris commented Oct 16, 2017

liggitt Oct 16, 2017

crassirostris Oct 16, 2017

liggitt Oct 16, 2017

liggitt Oct 16, 2017

crassirostris Oct 16, 2017

liggitt Oct 16, 2017

crassirostris Oct 16, 2017

crassirostris Oct 16, 2017 •

edited

Loading

liggitt Oct 16, 2017

crassirostris commented Oct 16, 2017

ericchiang commented Oct 16, 2017 •

edited

Loading

crassirostris commented Oct 17, 2017

sttts commented Oct 17, 2017

liggitt commented Oct 17, 2017

sttts commented Oct 18, 2017

k8s-github-robot commented Oct 18, 2017

k8s-ci-robot commented Oct 18, 2017 •

edited

Loading

k8s-github-robot commented Oct 18, 2017

	if err != nil {
	// "Connection reset by peer" is usually a transient error.
	// Thus in case of "GET" operations, we simply retry it.
	// We are not automatically retrying "write" operations, as
	// they are not idempotent.
	if !net.IsConnectionReset(err) \|\| r.verb != "GET" {
	return err
	}
	// For the purpose of retry, we set the artificial "retry-after" response.
	// TODO: Should we clean the original response if it exists?
	resp = &http.Response{
	StatusCode: http.StatusInternalServerError,
	Header: http.Header{"Retry-After": []string{"1"}},
	Body: ioutil.NopCloser(bytes.NewReader([]byte{})),
	}
	}

Always retry connection reset error in webhook #53947

Always retry connection reset error in webhook #53947

Conversation

crassirostris commented Oct 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crassirostris commented Oct 16, 2017 via email

crassirostris commented Oct 16, 2017

sttts commented Oct 16, 2017

crassirostris commented Oct 16, 2017 • edited Loading

crassirostris commented Oct 16, 2017

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

Choose a reason for hiding this comment

crassirostris Oct 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crassirostris commented Oct 16, 2017

ericchiang commented Oct 16, 2017 • edited Loading

crassirostris commented Oct 17, 2017

sttts commented Oct 17, 2017

liggitt commented Oct 17, 2017

sttts commented Oct 18, 2017

k8s-github-robot commented Oct 18, 2017

k8s-ci-robot commented Oct 18, 2017 • edited Loading

k8s-github-robot commented Oct 18, 2017

crassirostris commented Oct 15, 2017 •

edited

Loading

crassirostris commented Oct 16, 2017 •

edited

Loading

crassirostris Oct 16, 2017 •

edited

Loading

ericchiang commented Oct 16, 2017 •

edited

Loading

k8s-ci-robot commented Oct 18, 2017 •

edited

Loading