From 94552dbf11fbf3401e48ff3ca65b3c2c9fa0d355 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Aur=C3=A9lien=20Sibiril?=
 <81782+aureliensibiril@users.noreply.github.com>
Date: Wed, 22 Jul 2026 14:36:36 +0200
Subject: [PATCH] Make Grafana/Metabase/Tailscale name errors terminal
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The source-name worker keeps a generic name and marks the source
synced only when a resolver reports ErrTerminalNameResolution. The
Grafana and Metabase name resolvers, plus Tailscale via its shared
fetchUsers, returned a plain error on any non-2xx, so a revoked
credential (401/403) never reached the terminal path and the worker
re-claimed the row every drain cycle with no delay.

Route their non-2xx through nameStatusError so permanent 4xx wrap
ErrTerminalNameResolution while 5xx stay retryable. The sentinel is
inert on the ListAccounts sync path, which does not inspect it.

Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
---
 pkg/accessreview/drivers/grafana.go           |  2 +-
 pkg/accessreview/drivers/metabase.go          |  2 +-
 .../drivers/name_resolver_test.go             | 38 +++++++++++++++++++
 pkg/accessreview/drivers/tailscale.go         |  6 ++-
 4 files changed, 45 insertions(+), 3 deletions(-)

diff --git a/pkg/accessreview/drivers/grafana.go b/pkg/accessreview/drivers/grafana.go
index f2f6daab1..fb60a61ca 100644
--- a/pkg/accessreview/drivers/grafana.go
+++ b/pkg/accessreview/drivers/grafana.go
@@ -208,7 +208,7 @@ func (r *grafanaNameResolver) ResolveInstanceName(ctx context.Context) (string,
 	}()
 
 	if httpResp.StatusCode < 200 || httpResp.StatusCode >= 300 {
-		return "", fmt.Errorf("cannot fetch grafana organization: unexpected status %d", httpResp.StatusCode)
+		return "", nameStatusError("grafana organization", httpResp.StatusCode)
 	}
 
 	var org grafanaOrg
diff --git a/pkg/accessreview/drivers/metabase.go b/pkg/accessreview/drivers/metabase.go
index e48ac0333..aacae2350 100644
--- a/pkg/accessreview/drivers/metabase.go
+++ b/pkg/accessreview/drivers/metabase.go
@@ -236,7 +236,7 @@ func (r *metabaseNameResolver) ResolveInstanceName(ctx context.Context) (string,
 	}()
 
 	if httpResp.StatusCode < 200 || httpResp.StatusCode >= 300 {
-		return "", fmt.Errorf("cannot fetch metabase session properties: unexpected status %d", httpResp.StatusCode)
+		return "", nameStatusError("metabase session properties", httpResp.StatusCode)
 	}
 
 	var props metabaseSessionProperties
diff --git a/pkg/accessreview/drivers/name_resolver_test.go b/pkg/accessreview/drivers/name_resolver_test.go
index 407830988..cc704a1a1 100644
--- a/pkg/accessreview/drivers/name_resolver_test.go
+++ b/pkg/accessreview/drivers/name_resolver_test.go
@@ -61,6 +61,44 @@ func TestNameStatusError(t *testing.T) {
 	}
 }
 
+// TestNameResolversTerminalOnClientError guards the fix that makes the
+// grafana, metabase and tailscale name resolvers surface a permanent 4xx as
+// ErrTerminalNameResolution. Without it a revoked-credential source hot-loops
+// the source-name worker, which never marks such a source synced.
+func TestNameResolversTerminalOnClientError(t *testing.T) {
+	t.Parallel()
+
+	srv := httptest.NewServer(
+		http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+			w.WriteHeader(http.StatusUnauthorized)
+			_, _ = w.Write([]byte(`{"message":"unauthorized"}`))
+		}),
+	)
+	t.Cleanup(srv.Close)
+
+	cases := []struct {
+		name     string
+		resolver NameResolver
+	}{
+		{name: "grafana", resolver: NewGrafanaNameResolver(srv.Client(), srv.URL)},
+		{name: "metabase", resolver: NewMetabaseNameResolver(srv.Client(), srv.URL)},
+		{name: "tailscale", resolver: NewTailscaleNameResolver(&http.Client{Transport: &hostRewriter{target: srv.URL}})},
+	}
+
+	for _, tc := range cases {
+		t.Run(
+			tc.name,
+			func(t *testing.T) {
+				t.Parallel()
+
+				_, err := tc.resolver.ResolveInstanceName(context.Background())
+				require.Error(t, err)
+				assert.ErrorIs(t, err, ErrTerminalNameResolution)
+			},
+		)
+	}
+}
+
 // hostRewriter redirects requests to the configured target host so that
 // resolvers with hardcoded production URLs (api.notion.com, etc.) can be
 // pointed at an httptest server.
diff --git a/pkg/accessreview/drivers/tailscale.go b/pkg/accessreview/drivers/tailscale.go
index b1c633851..1a72d6ae2 100644
--- a/pkg/accessreview/drivers/tailscale.go
+++ b/pkg/accessreview/drivers/tailscale.go
@@ -150,8 +150,12 @@ func (d *TailscaleDriver) fetchUsers(ctx context.Context) ([]tailscaleUser, erro
 		_ = httpResp.Body.Close()
 	}()
 
+	// Classify the status so the source-name worker (which reuses this via
+	// tailscaleNameResolver) treats a 4xx as terminal instead of hot-looping.
+	// The sentinel is inert on the ListAccounts sync path, which does not
+	// inspect it.
 	if httpResp.StatusCode < 200 || httpResp.StatusCode >= 300 {
-		return nil, fmt.Errorf("cannot fetch tailscale users: unexpected status %d", httpResp.StatusCode)
+		return nil, nameStatusError("tailscale users", httpResp.StatusCode)
 	}
 
 	var resp tailscaleUsersResponse