fix(observability-proxy): do not cache reviews that failed to decide

A TokenReview whose authenticator failed comes back unauthenticated
with status.error set, and a SubjectAccessReview whose authorizer
failed comes back with an evaluation error and no decision. Both were
cached as refusals, so a valid caller stayed locked out for the cache
TTL after the outage ended.

Such answers are now refused for the current request only. A review
that failed to decide still answers 401 or 403 rather than 503: RBAC
sets an evaluation error for any binding to a missing role, which a
retryable status would turn into a permanent 503 for that user.

Assisted-by: LLM
Signed-off-by: ZverGuy <maximbel2003@gmail.com>

internal/observabilityproxy/auth.go

@@ -37,8 +37,9 @@ var ErrUnauthenticated = errors.New("unauthenticated")
 
 // Authorizer asks kube-apiserver who a token belongs to and whether that
 // identity may read a scope. Answers, including negative ones, are cached
-// for the TTL; transport errors are not, so an apiserver blip is retried on
-// the next request instead of locking a user out for the TTL.
+// for the TTL; transport errors and reviews that failed to decide are not,
+// so an outage is retried on the next request instead of locking a user
+// out for the TTL.
 type Authorizer struct {
 	client    kubernetes.Interface
 	now       func() time.Time
@@ -72,14 +73,16 @@ func (a *Authorizer) Authenticate(ctx context.Context, token string) (*authentic
 	if err != nil {
 		return nil, fmt.Errorf("token review: %w", err)
 	}
-	var u *authenticationv1.UserInfo
-	if tr.Status.Authenticated && tr.Status.User.Username != "" {
-		u = tr.Status.User.DeepCopy()
-	}
-	a.tokens.set(key, u, a.now())
-	if u == nil {
+	if !tr.Status.Authenticated || tr.Status.User.Username == "" {
+		// status.error also marks an authenticator that failed to decide;
+		// a cached refusal would outlast its recovery.
+		if tr.Status.Error == "" {
+			a.tokens.set(key, nil, a.now())
+		}
 		return nil, ErrUnauthenticated
 	}
+	u := tr.Status.User.DeepCopy()
+	a.tokens.set(key, u, a.now())
 	return u, nil
 }
 
@@ -109,7 +112,12 @@ func (a *Authorizer) Authorize(ctx context.Context, user *authenticationv1.UserI
 		return false, fmt.Errorf("subject access review: %w", err)
 	}
 	allowed := sar.Status.Allowed && !sar.Status.Denied
-	a.decisions.set(key, allowed, a.now())
+	// An evaluation error without a decision denies this request only: it
+	// may be a webhook authorizer that is down, and RBAC reports one for
+	// any binding to a missing role, so it cannot be told from a refusal.
+	if allowed || sar.Status.Denied || sar.Status.EvaluationError == "" {
+		a.decisions.set(key, allowed, a.now())
+	}
 	return allowed, nil
 }
 

internal/observabilityproxy/auth_test.go

@@ -38,6 +38,10 @@ type fakeKube struct {
 	sars     atomic.Int32
 	lastSAR  atomic.Pointer[authorizationv1.SubjectAccessReviewSpec]
 	reviewFn func() error
+	// reviewError and evaluationError, when set, are what the
+	// authenticator or the authorizer report as failing to decide.
+	reviewError     atomic.Pointer[string]
+	evaluationError atomic.Pointer[string]
 }
 
 func newFakeKube(users map[string]string, allow func(user string, attrs *authorizationv1.ResourceAttributes) bool) *fakeKube {
@@ -50,7 +54,9 @@ func newFakeKube(users map[string]string, allow func(user string, attrs *authori
 			}
 		}
 		tr := action.(k8stesting.CreateAction).GetObject().(*authenticationv1.TokenReview).DeepCopy()
-		if name, ok := users[tr.Spec.Token]; ok {
+		if e := f.reviewError.Load(); e != nil {
+			tr.Status = authenticationv1.TokenReviewStatus{Error: *e}
+		} else if name, ok := users[tr.Spec.Token]; ok {
 			tr.Status = authenticationv1.TokenReviewStatus{Authenticated: true, User: authenticationv1.UserInfo{Username: name, Groups: []string{"tenant-a-view"}}}
 		}
 		return true, tr, nil
@@ -60,6 +66,10 @@ func newFakeKube(users map[string]string, allow func(user string, attrs *authori
 		sar := action.(k8stesting.CreateAction).GetObject().(*authorizationv1.SubjectAccessReview).DeepCopy()
 		spec := sar.Spec
 		f.lastSAR.Store(&spec)
+		if e := f.evaluationError.Load(); e != nil {
+			sar.Status.EvaluationError = *e
+			return true, sar, nil
+		}
 		sar.Status.Allowed = allow(sar.Spec.User, sar.Spec.ResourceAttributes)
 		return true, sar, nil
 	})
@@ -171,3 +181,40 @@ func TestAuthorize(t *testing.T) {
 		t.Fatalf("a different group set must not hit another identity's cache entry, SARs = %d", n)
 	}
 }
+
+// TestAuthenticateFailedReviewNotCached: a TokenReview the authenticator
+// could not decide carries status.error; caching it would refuse a valid
+// token for the TTL after the authenticator recovers.
+func TestAuthenticateFailedReviewNotCached(t *testing.T) {
+	kube := newFakeKube(map[string]string{"good": "alice"}, allowNamespace("tenant-a"))
+	outage := "webhook authenticator unreachable"
+	kube.reviewError.Store(&outage)
+	a := NewAuthorizer(kube.client, 10*time.Second)
+	ctx := context.Background()
+	if _, err := a.Authenticate(ctx, "good"); !errors.Is(err, ErrUnauthenticated) {
+		t.Fatalf("err = %v, want ErrUnauthenticated", err)
+	}
+	kube.reviewError.Store(nil)
+	if u, err := a.Authenticate(ctx, "good"); err != nil || u.Username != "alice" {
+		t.Fatalf("after recovery: %+v, %v", u, err)
+	}
+}
+
+// TestAuthorizeEvaluationErrorNotCached: a SubjectAccessReview with an
+// evaluation error and no decision is a denial for this request only.
+func TestAuthorizeEvaluationErrorNotCached(t *testing.T) {
+	kube := newFakeKube(map[string]string{"good": "alice"}, allowNamespace("tenant-a"))
+	failure := "webhook authorizer unreachable"
+	kube.evaluationError.Store(&failure)
+	a := NewAuthorizer(kube.client, 10*time.Second)
+	ctx := context.Background()
+	user := &authenticationv1.UserInfo{Username: "alice"}
+	attrs := Route{Kind: NamespaceLogs, Namespace: "tenant-a"}.ResourceAttributes()
+	if ok, err := a.Authorize(ctx, user, attrs); err != nil || ok {
+		t.Fatalf("undecided review = %v, %v; want a denial", ok, err)
+	}
+	kube.evaluationError.Store(nil)
+	if ok, err := a.Authorize(ctx, user, attrs); err != nil || !ok {
+		t.Fatalf("after recovery: %v, %v", ok, err)
+	}
+}