Trim access-review connector comments
Cut verbose inline comments across the access-review connector changes:
the mechanical fact stays at the read site, while incident backstory
("millions of error logs in prod") and provider-specific rationale (why
Clerk reviews the wrong population) move to the commit history where they
belong. Also tighten a loose "a 4xx" to "an auth/not-found 4xx" so the
terminal-classification contract is not overstated. No behavior change.
Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
This commit is contained in:
@@ -41,14 +41,10 @@ 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).
|
||||
// ErrTerminalNameResolution marks a permanent name-resolution failure
|
||||
// (auth/bad-request) that retrying cannot fix: the source-name worker keeps
|
||||
// the generic name and marks the source synced instead of re-claiming it.
|
||||
// Transient failures (5xx, network) stay plain errors so they keep retrying.
|
||||
var ErrTerminalNameResolution = errors.New("terminal name resolution failure")
|
||||
|
||||
// nameStatusError classifies a non-2xx response from a name-resolution
|
||||
|
||||
@@ -151,9 +151,9 @@ func (d *TailscaleDriver) fetchUsers(ctx context.Context) ([]tailscaleUser, erro
|
||||
}()
|
||||
|
||||
// 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.
|
||||
// tailscaleNameResolver) treats an auth/not-found 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, nameStatusError("tailscale users", httpResp.StatusCode)
|
||||
}
|
||||
|
||||
@@ -146,15 +146,10 @@ func (h *sourceNameHandler) Process(ctx context.Context, source coredata.AccessR
|
||||
},
|
||||
)
|
||||
if err != nil {
|
||||
// Setting up the resolver failed: the connector is gone, its
|
||||
// credentials cannot be decrypted, or an eager token refresh failed
|
||||
// on a revoked OAuth refresh token. Returning nil without marking the
|
||||
// source synced leaves name_synced_at NULL, so the worker re-claims
|
||||
// the same row every drain cycle with no delay — a single dead
|
||||
// connector then hot-loops the vendor token endpoint (millions of
|
||||
// error logs in prod). Treat it as terminal: keep the generic name
|
||||
// and mark the source synced so it stops re-claiming. A
|
||||
// reconnect/reconfigure clears name_synced_at to try again.
|
||||
// Resolver setup failed (missing connector, undecryptable credential,
|
||||
// or an eager refresh on a revoked token). Mark the source synced
|
||||
// rather than returning nil: an unsynced row is re-claimed every poll
|
||||
// with no backoff and hot-loops the vendor. A reconnect clears it.
|
||||
h.logger.WarnCtx(
|
||||
ctx,
|
||||
"cannot set up name resolver, keeping generic name",
|
||||
|
||||
@@ -582,18 +582,14 @@ func (s *Service) SourceNeedsConfiguration(
|
||||
return cfg.SelectedSlug(dbConnector) == "", nil
|
||||
}
|
||||
|
||||
// AutoSelectDefaultOrganization picks the first workspace/org the connector
|
||||
// can see for a freshly linked picker-provider source that has none selected
|
||||
// yet. Without it a connected source stays "needs configuration" until the
|
||||
// user completes the picker; if they skip it, the first campaign silently
|
||||
// resolves no users (the driver requires an org). Defaulting to the first
|
||||
// available makes the source immediately usable; the picker stays available
|
||||
// to switch when several are listed.
|
||||
// AutoSelectDefaultOrganization picks the first workspace/org a freshly linked
|
||||
// picker-provider source can see when none is selected yet, so the source is
|
||||
// usable immediately instead of failing its first campaign fetch. The picker
|
||||
// stays available to switch when several are listed.
|
||||
//
|
||||
// Best-effort: any failure (provider unreachable, nothing listed) leaves the
|
||||
// source in its existing "needs configuration" state, where the picker is the
|
||||
// fallback. It never returns an error and must not fail the create/update
|
||||
// that triggered it.
|
||||
// Best-effort: any failure leaves the source in its "needs configuration"
|
||||
// state (the picker is the fallback); it never errors and must not fail the
|
||||
// create/update that triggered it.
|
||||
func (s *Service) AutoSelectDefaultOrganization(
|
||||
ctx context.Context,
|
||||
scope coredata.Scoper,
|
||||
|
||||
Reference in New Issue
Block a user