SuggestClientDelay is not about retrying, clarify message and header

SuggestClientDelay is returning whether the server has requested that
the client delay their next action. It is *not* about whether the client
should retry the action. Webhook was using it incorrectly, and the
method is now up to date.

staging/src/k8s.io/apimachinery/pkg/api/errors/errors.go

@@ -462,13 +462,20 @@ func IsUnexpectedObjectError(err error) bool {
 }
 
 // SuggestsClientDelay returns true if this error suggests a client delay as well as the
-// suggested seconds to wait, or false if the error does not imply a wait.
+// suggested seconds to wait, or false if the error does not imply a wait. It does not
+// address whether the error *should* be retried, since some errors (like a 3xx) may
+// request delay without retry.
 func SuggestsClientDelay(err error) (int, bool) {
 	switch t := err.(type) {
 	case APIStatus:
 		if t.Status().Details != nil {
 			switch t.Status().Reason {
-			case metav1.StatusReasonServerTimeout, metav1.StatusReasonTimeout:
+			// this StatusReason explicitly requests the caller to delay the action
+			case metav1.StatusReasonServerTimeout:
+				return int(t.Status().Details.RetryAfterSeconds), true
+			}
+			// If the client requests that we retry after a certain number of seconds
+			if t.Status().Details.RetryAfterSeconds > 0 {
 				return int(t.Status().Details.RetryAfterSeconds), true
 			}
 		}

staging/src/k8s.io/apimachinery/pkg/api/errors/errors_test.go

@@ -83,15 +83,34 @@ func TestErrorNew(t *testing.T) {
 	if !IsServerTimeout(NewServerTimeout(resource("tests"), "reason", 0)) {
 		t.Errorf("expected to be %s", metav1.StatusReasonServerTimeout)
 	}
-	if time, ok := SuggestsClientDelay(NewServerTimeout(resource("tests"), "doing something", 10)); time != 10 || !ok {
-		t.Errorf("expected to be %s", metav1.StatusReasonServerTimeout)
-	}
-	if time, ok := SuggestsClientDelay(NewTimeoutError("test reason", 10)); time != 10 || !ok {
-		t.Errorf("expected to be %s", metav1.StatusReasonTimeout)
-	}
 	if !IsMethodNotSupported(NewMethodNotSupported(resource("foos"), "delete")) {
 		t.Errorf("expected to be %s", metav1.StatusReasonMethodNotAllowed)
 	}
+
+	if time, ok := SuggestsClientDelay(NewServerTimeout(resource("tests"), "doing something", 10)); time != 10 || !ok {
+		t.Errorf("unexpected %d", time)
+	}
+	if time, ok := SuggestsClientDelay(NewServerTimeout(resource("tests"), "doing something", 0)); time != 0 || !ok {
+		t.Errorf("unexpected %d", time)
+	}
+	if time, ok := SuggestsClientDelay(NewTimeoutError("test reason", 10)); time != 10 || !ok {
+		t.Errorf("unexpected %d", time)
+	}
+	if time, ok := SuggestsClientDelay(NewTooManyRequests("doing something", 10)); time != 10 || !ok {
+		t.Errorf("unexpected %d", time)
+	}
+	if time, ok := SuggestsClientDelay(NewTooManyRequests("doing something", 1)); time != 1 || !ok {
+		t.Errorf("unexpected %d", time)
+	}
+	if time, ok := SuggestsClientDelay(NewGenericServerResponse(429, "get", resource("tests"), "test", "doing something", 10, true)); time != 10 || !ok {
+		t.Errorf("unexpected %d", time)
+	}
+	if time, ok := SuggestsClientDelay(NewGenericServerResponse(500, "get", resource("tests"), "test", "doing something", 10, true)); time != 10 || !ok {
+		t.Errorf("unexpected %d", time)
+	}
+	if time, ok := SuggestsClientDelay(NewGenericServerResponse(429, "get", resource("tests"), "test", "doing something", 0, true)); time != 0 || ok {
+		t.Errorf("unexpected %d", time)
+	}
 }
 
 func TestNewInvalid(t *testing.T) {

staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/types.go

@@ -481,7 +481,9 @@ type StatusDetails struct {
 	// failure. Not all StatusReasons may provide detailed causes.
 	// +optional
 	Causes []StatusCause `json:"causes,omitempty" protobuf:"bytes,4,rep,name=causes"`
-	// If specified, the time in seconds before the operation should be retried.
+	// If specified, the time in seconds before the operation should be retried. Some errors may indicate
+	// the client must take an alternate action - for those errors this field may indicate how long to wait
+	// before taking the alternate action.
 	// +optional
 	RetryAfterSeconds int32 `json:"retryAfterSeconds,omitempty" protobuf:"varint,5,opt,name=retryAfterSeconds"`
 }

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

@@ -90,6 +90,11 @@ 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) {
+			return false, nil
+		}
+		// if the error sends the Retry-After header, we respect it as an explicit confirmation we should retry.
 		if _, shouldRetry := apierrors.SuggestsClientDelay(err); shouldRetry {
 			return false, nil
 		}