From c837701099b333ae04761c3a4f885e11826ab81b 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 12:26:12 +0200 Subject: [PATCH] Stop source-name worker looping on permanent failures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The access-review source-name worker never marked a source synced when name resolution errored, so it re-claimed the source on every poll and retried at vendor-latency cadence. Two permanently-failing sources generated millions of error logs (Brex /v2/company 403 and Cloudflare /accounts 400) and hammered vendor APIs (8.6M 403s to Brex in 30 days) -- a ban risk, all for best-effort display metadata. Generalize the Google-403 special case: name resolvers now classify a non-2xx response through nameStatusError, which wraps ErrTerminalNameResolution for permanent client errors (400, 401, 403, 404) and returns a plain, retryable error for everything else (5xx, network). The worker treats a terminal error as done -- it keeps the generic name and marks the source synced -- while transient failures keep retrying as before. Also fix the Cloudflare name resolver's own bug: it requested per_page=1, but Cloudflare's List Accounts endpoint requires per_page in 5..50 and 400s otherwise (the driver already uses 50). That 400 was the sole cause of the Cloudflare retry storm; bump it to 50. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com> --- pkg/accessreview/drivers/name_resolver.go | 58 +++++++++++++------ .../drivers/name_resolver_test.go | 30 ++++++++++ pkg/accessreview/source_name_worker.go | 17 ++++++ 3 files changed, 87 insertions(+), 18 deletions(-) diff --git a/pkg/accessreview/drivers/name_resolver.go b/pkg/accessreview/drivers/name_resolver.go index 37730bd2f..cfa66dfb5 100644 --- a/pkg/accessreview/drivers/name_resolver.go +++ b/pkg/accessreview/drivers/name_resolver.go @@ -41,6 +41,28 @@ type NameResolver interface { ResolveInstanceName(ctx context.Context) (string, error) } +// ErrTerminalNameResolution marks a name-resolution failure as permanent: +// an auth or bad-request response that retrying cannot fix. The +// source-name worker treats it as terminal — it keeps the generic source +// name and marks the source synced instead of re-claiming it every poll. +// Transient failures (5xx, network errors) are returned as plain errors so +// they keep retrying. Name resolution is best-effort display metadata, so a +// permanent failure must never wedge the worker in a retry loop (a single +// unauthorized source otherwise produced millions of error logs in prod). +var ErrTerminalNameResolution = errors.New("terminal name resolution failure") + +// nameStatusError classifies a non-2xx response from a name-resolution +// request. Permanent client errors (400, 401, 403, 404) wrap +// ErrTerminalNameResolution; everything else (notably 5xx) stays retryable. +func nameStatusError(what string, statusCode int) error { + switch statusCode { + case http.StatusBadRequest, http.StatusUnauthorized, http.StatusForbidden, http.StatusNotFound: + return fmt.Errorf("cannot fetch %s: unexpected status %d: %w", what, statusCode, ErrTerminalNameResolution) + default: + return fmt.Errorf("cannot fetch %s: unexpected status %d", what, statusCode) + } +} + // slackNameResolver resolves the Slack workspace name via auth.test. type slackNameResolver struct { httpClient *http.Client @@ -143,7 +165,7 @@ func (r *linearNameResolver) ResolveInstanceName(ctx context.Context) (string, e defer func() { _ = httpResp.Body.Close() }() if httpResp.StatusCode < 200 || httpResp.StatusCode >= 300 { - return "", fmt.Errorf("cannot fetch linear organization: unexpected status %d", httpResp.StatusCode) + return "", nameStatusError("linear organization", httpResp.StatusCode) } var resp struct { @@ -184,7 +206,7 @@ func (r *cloudflareNameResolver) ResolveInstanceName(ctx context.Context) (strin q := cfURL.Query() q.Set("page", "1") - q.Set("per_page", "1") + q.Set("per_page", "50") cfURL.RawQuery = q.Encode() req, err := http.NewRequestWithContext(ctx, http.MethodGet, cfURL.String(), nil) @@ -202,7 +224,7 @@ func (r *cloudflareNameResolver) ResolveInstanceName(ctx context.Context) (strin defer func() { _ = httpResp.Body.Close() }() if httpResp.StatusCode < 200 || httpResp.StatusCode >= 300 { - return "", fmt.Errorf("cannot fetch cloudflare accounts: unexpected status %d", httpResp.StatusCode) + return "", nameStatusError("cloudflare accounts", httpResp.StatusCode) } var resp struct { @@ -251,7 +273,7 @@ func (r *brexNameResolver) ResolveInstanceName(ctx context.Context) (string, err defer func() { _ = httpResp.Body.Close() }() if httpResp.StatusCode < 200 || httpResp.StatusCode >= 300 { - return "", fmt.Errorf("cannot fetch brex company: unexpected status %d", httpResp.StatusCode) + return "", nameStatusError("brex company", httpResp.StatusCode) } var resp struct { @@ -298,7 +320,7 @@ func (r *tallyNameResolver) ResolveInstanceName(ctx context.Context) (string, er defer func() { _ = httpResp.Body.Close() }() if httpResp.StatusCode < 200 || httpResp.StatusCode >= 300 { - return "", fmt.Errorf("cannot fetch tally organization: unexpected status %d", httpResp.StatusCode) + return "", nameStatusError("tally organization", httpResp.StatusCode) } var resp struct { @@ -505,7 +527,7 @@ func (r *hubspotNameResolver) ResolveInstanceName(ctx context.Context) (string, defer func() { _ = httpResp.Body.Close() }() if httpResp.StatusCode < 200 || httpResp.StatusCode >= 300 { - return "", fmt.Errorf("cannot fetch hubspot account info: unexpected status %d", httpResp.StatusCode) + return "", nameStatusError("hubspot account info", httpResp.StatusCode) } var resp struct { @@ -736,7 +758,7 @@ func (r *sentryNameResolver) ResolveInstanceName(ctx context.Context) (string, e } if httpResp.StatusCode < 200 || httpResp.StatusCode >= 300 { - return "", fmt.Errorf("cannot fetch sentry organization: unexpected status %d", httpResp.StatusCode) + return "", nameStatusError("sentry organization", httpResp.StatusCode) } var resp struct { @@ -784,7 +806,7 @@ func (r *githubNameResolver) ResolveInstanceName(ctx context.Context) (string, e defer func() { _ = httpResp.Body.Close() }() if httpResp.StatusCode < 200 || httpResp.StatusCode >= 300 { - return "", fmt.Errorf("cannot fetch github organization: unexpected status %d", httpResp.StatusCode) + return "", nameStatusError("github organization", httpResp.StatusCode) } var resp struct { @@ -916,7 +938,7 @@ func (r *gitlabNameResolver) ResolveInstanceName(ctx context.Context) (string, e defer func() { _ = httpResp.Body.Close() }() if httpResp.StatusCode < 200 || httpResp.StatusCode >= 300 { - return "", fmt.Errorf("cannot fetch gitlab group: unexpected status %d", httpResp.StatusCode) + return "", nameStatusError("gitlab group", httpResp.StatusCode) } var resp struct { @@ -969,7 +991,7 @@ func (r *bitbucketNameResolver) ResolveInstanceName(ctx context.Context) (string defer func() { _ = httpResp.Body.Close() }() if httpResp.StatusCode < 200 || httpResp.StatusCode >= 300 { - return "", fmt.Errorf("cannot fetch bitbucket workspace: unexpected status %d", httpResp.StatusCode) + return "", nameStatusError("bitbucket workspace", httpResp.StatusCode) } var resp struct { @@ -1028,7 +1050,7 @@ func (r *herokuNameResolver) ResolveInstanceName(ctx context.Context) (string, e defer func() { _ = httpResp.Body.Close() }() if httpResp.StatusCode < 200 || httpResp.StatusCode >= 300 { - return "", fmt.Errorf("cannot fetch heroku team: unexpected status %d", httpResp.StatusCode) + return "", nameStatusError("heroku team", httpResp.StatusCode) } var resp struct { @@ -1178,7 +1200,7 @@ func (r *asanaNameResolver) ResolveInstanceName(ctx context.Context) (string, er defer func() { _ = httpResp.Body.Close() }() if httpResp.StatusCode < 200 || httpResp.StatusCode >= 300 { - return "", fmt.Errorf("cannot fetch asana workspace: unexpected status %d", httpResp.StatusCode) + return "", nameStatusError("asana workspace", httpResp.StatusCode) } var resp struct { @@ -1228,7 +1250,7 @@ func (r *netlifyNameResolver) ResolveInstanceName(ctx context.Context) (string, defer func() { _ = httpResp.Body.Close() }() if httpResp.StatusCode < 200 || httpResp.StatusCode >= 300 { - return "", fmt.Errorf("cannot fetch netlify account: unexpected status %d", httpResp.StatusCode) + return "", nameStatusError("netlify account", httpResp.StatusCode) } var resp struct { @@ -1276,7 +1298,7 @@ func (r *clickupNameResolver) ResolveInstanceName(ctx context.Context) (string, defer func() { _ = httpResp.Body.Close() }() if httpResp.StatusCode < 200 || httpResp.StatusCode >= 300 { - return "", fmt.Errorf("cannot fetch clickup team: unexpected status %d", httpResp.StatusCode) + return "", nameStatusError("clickup team", httpResp.StatusCode) } var resp struct { @@ -1345,7 +1367,7 @@ func (r *vercelNameResolver) ResolveInstanceName(ctx context.Context) (string, e } if teamResp.StatusCode != http.StatusNotFound { - return "", fmt.Errorf("cannot fetch vercel team: unexpected status %d", teamResp.StatusCode) + return "", nameStatusError("vercel team", teamResp.StatusCode) } // Personal-account fallback: /v2/teams/ returns 404, but @@ -1399,7 +1421,7 @@ func (r *mondayNameResolver) ResolveInstanceName(ctx context.Context) (string, e defer func() { _ = httpResp.Body.Close() }() if httpResp.StatusCode < 200 || httpResp.StatusCode >= 300 { - return "", fmt.Errorf("cannot fetch monday account: unexpected status %d", httpResp.StatusCode) + return "", nameStatusError("monday account", httpResp.StatusCode) } var resp struct { @@ -1452,7 +1474,7 @@ func (r *notionNameResolver) ResolveInstanceName(ctx context.Context) (string, e defer func() { _ = httpResp.Body.Close() }() if httpResp.StatusCode < 200 || httpResp.StatusCode >= 300 { - return "", fmt.Errorf("cannot fetch notion users/me: unexpected status %d", httpResp.StatusCode) + return "", nameStatusError("notion users/me", httpResp.StatusCode) } var resp struct { @@ -1502,7 +1524,7 @@ func (r *microsoft365NameResolver) ResolveInstanceName(ctx context.Context) (str defer func() { _ = httpResp.Body.Close() }() if httpResp.StatusCode < 200 || httpResp.StatusCode >= 300 { - return "", fmt.Errorf("cannot fetch microsoft 365 organization: unexpected status %d", httpResp.StatusCode) + return "", nameStatusError("microsoft 365 organization", httpResp.StatusCode) } var resp struct { diff --git a/pkg/accessreview/drivers/name_resolver_test.go b/pkg/accessreview/drivers/name_resolver_test.go index 71c83b18a..407830988 100644 --- a/pkg/accessreview/drivers/name_resolver_test.go +++ b/pkg/accessreview/drivers/name_resolver_test.go @@ -22,6 +22,7 @@ package drivers import ( "context" + "errors" "net/http" "net/http/httptest" "net/url" @@ -31,6 +32,35 @@ import ( "github.com/stretchr/testify/require" ) +func TestNameStatusError(t *testing.T) { + t.Parallel() + + terminal := []int{ + http.StatusBadRequest, + http.StatusUnauthorized, + http.StatusForbidden, + http.StatusNotFound, + } + for _, code := range terminal { + err := nameStatusError("thing", code) + require.Error(t, err) + assert.ErrorIs(t, err, ErrTerminalNameResolution, "status %d must be terminal", code) + } + + retryable := []int{ + http.StatusTooManyRequests, + http.StatusInternalServerError, + http.StatusBadGateway, + http.StatusServiceUnavailable, + http.StatusGatewayTimeout, + } + for _, code := range retryable { + err := nameStatusError("thing", code) + require.Error(t, err) + assert.False(t, errors.Is(err, ErrTerminalNameResolution), "status %d must be retryable", code) + } +} + // 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/source_name_worker.go b/pkg/accessreview/source_name_worker.go index 6ecb27ca5..2f2a248a9 100644 --- a/pkg/accessreview/source_name_worker.go +++ b/pkg/accessreview/source_name_worker.go @@ -172,6 +172,23 @@ func (h *sourceNameHandler) Process(ctx context.Context, source coredata.AccessR instanceName, err := resolver.ResolveInstanceName(resolveCtx) if err != nil { + // A permanent failure (auth/bad-request) cannot be fixed by + // retrying: keep the generic name and mark the source synced so the + // worker stops re-claiming it every poll. Returning the error here + // would leave name_synced_at NULL and re-enqueue the source forever + // (a single unauthorized source produced millions of error logs). + if errors.Is(err, drivers.ErrTerminalNameResolution) { + h.logger.WarnCtx( + ctx, + "permanent name resolution failure, keeping generic name", + log.String("source_id", source.ID.String()), + log.String("provider", dbConnector.Provider.String()), + log.Error(err), + ) + + return h.markNameSynced(ctx, &source) + } + h.logger.WarnCtx( ctx, "cannot resolve instance name",