From ae2b353fbf519b29d168c534f88c373fd67a1c31 Mon Sep 17 00:00:00 2001
From: Abu Kashem <akashem@redhat.com>
Date: Thu, 7 Jan 2021 16:14:18 -0500
Subject: [PATCH] handle webhook authenticator and authorizer error

webhook.WithExponentialBackoff returns an error, and the priority is:
- A: if the last invocation of the webhook function returned an error
  that error should be returned, otherwise
- B: the error associated with the context if it has been canceled or
  it has expired, or the ErrWaitTimeout returned by the wait package
  once all retries have been exhausted.

caller should check the error returned by webhook.WithExponentialBackoff
to handle both A and B. Currently, we only handle A.
---
 .../pkg/util/webhook/webhook_test.go          | 27 +++++++++++++++++++
 .../authenticator/token/webhook/webhook.go    | 12 ++++-----
 .../plugin/pkg/authorizer/webhook/webhook.go  | 18 ++++++-------
 3 files changed, 41 insertions(+), 16 deletions(-)

diff --git a/staging/src/k8s.io/apiserver/pkg/util/webhook/webhook_test.go b/staging/src/k8s.io/apiserver/pkg/util/webhook/webhook_test.go
index 755e957c364..0c4f208cfb3 100644
--- a/staging/src/k8s.io/apiserver/pkg/util/webhook/webhook_test.go
+++ b/staging/src/k8s.io/apiserver/pkg/util/webhook/webhook_test.go
@@ -711,6 +711,33 @@ func TestWithExponentialBackoffWebhookErrorIsMostImportant(t *testing.T) {
 	}
 }
 
+func TestWithExponentialBackoffWithRetryExhaustedWhileContextIsNotCanceled(t *testing.T) {
+	alwaysRetry := func(e error) bool {
+		return true
+	}
+
+	ctx, cancel := context.WithCancel(context.TODO())
+	defer cancel()
+
+	attemptsGot := 0
+	errExpected := errors.New("webhook not available")
+	webhookFunc := func() error {
+		attemptsGot++
+		return errExpected
+	}
+
+	// webhook err has higher priority than ctx error. we expect the webhook error to be returned.
+	retryBackoff := wait.Backoff{Steps: 5}
+	err := WithExponentialBackoff(ctx, retryBackoff, webhookFunc, alwaysRetry)
+
+	if attemptsGot != 5 {
+		t.Errorf("expected %d webhook attempts, but got: %d", 1, attemptsGot)
+	}
+	if errExpected != err {
+		t.Errorf("expected error: %v, but got: %v", errExpected, err)
+	}
+}
+
 func TestWithExponentialBackoffParametersNotSet(t *testing.T) {
 	alwaysRetry := func(e error) bool {
 		return true
diff --git a/staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/webhook/webhook.go b/staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/webhook/webhook.go
index d4bf1b45a91..5bedf4e5985 100644
--- a/staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/webhook/webhook.go
+++ b/staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/webhook/webhook.go
@@ -104,14 +104,14 @@ func (w *WebhookTokenAuthenticator) AuthenticateToken(ctx context.Context, token
 	}
 	var (
 		result *authenticationv1.TokenReview
-		err    error
 		auds   authenticator.Audiences
 	)
-	webhook.WithExponentialBackoff(ctx, w.retryBackoff, func() error {
-		result, err = w.tokenReview.Create(ctx, r, metav1.CreateOptions{})
-		return err
-	}, webhook.DefaultShouldRetry)
-	if err != nil {
+	// WithExponentialBackoff will return tokenreview create error (tokenReviewErr) if any.
+	if err := webhook.WithExponentialBackoff(ctx, w.retryBackoff, func() error {
+		var tokenReviewErr error
+		result, tokenReviewErr = w.tokenReview.Create(ctx, r, metav1.CreateOptions{})
+		return tokenReviewErr
+	}, webhook.DefaultShouldRetry); err != nil {
 		// An error here indicates bad configuration or an outage. Log for debugging.
 		klog.Errorf("Failed to make webhook authenticator request: %v", err)
 		return nil, false, err
diff --git a/staging/src/k8s.io/apiserver/plugin/pkg/authorizer/webhook/webhook.go b/staging/src/k8s.io/apiserver/plugin/pkg/authorizer/webhook/webhook.go
index 5c9f28ad40c..c31bd4a504e 100644
--- a/staging/src/k8s.io/apiserver/plugin/pkg/authorizer/webhook/webhook.go
+++ b/staging/src/k8s.io/apiserver/plugin/pkg/authorizer/webhook/webhook.go
@@ -192,19 +192,17 @@ func (w *WebhookAuthorizer) Authorize(ctx context.Context, attr authorizer.Attri
 	if entry, ok := w.responseCache.Get(string(key)); ok {
 		r.Status = entry.(authorizationv1.SubjectAccessReviewStatus)
 	} else {
-		var (
-			result *authorizationv1.SubjectAccessReview
-			err    error
-		)
-		webhook.WithExponentialBackoff(ctx, w.retryBackoff, func() error {
-			result, err = w.subjectAccessReview.Create(ctx, r, metav1.CreateOptions{})
-			return err
-		}, webhook.DefaultShouldRetry)
-		if err != nil {
-			// An error here indicates bad configuration or an outage. Log for debugging.
+		var result *authorizationv1.SubjectAccessReview
+		// WithExponentialBackoff will return SAR create error (sarErr) if any.
+		if err := webhook.WithExponentialBackoff(ctx, w.retryBackoff, func() error {
+			var sarErr error
+			result, sarErr = w.subjectAccessReview.Create(ctx, r, metav1.CreateOptions{})
+			return sarErr
+		}, webhook.DefaultShouldRetry); err != nil {
 			klog.Errorf("Failed to make webhook authorizer request: %v", err)
 			return w.decisionOnError, "", err
 		}
+
 		r.Status = result.Status
 		if shouldCache(attr) {
 			if r.Status.Allowed {