CXH-2350: surface workspace-token sync limits and fix PAT setup docs - #57
Conversation
Connector PR Review: CXH-2350: surface workspace-token sync limits and fix PAT setup docsBlocking Issues: 0 | Suggestions: 2 | Threads Resolved: 0 Review SummaryThe new commit resolves two of the four previously flagged findings: the duplicate Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agentsNote: this run could not emit the machine-readable |
Feli: repo convention forbids Warn; drop the account-API-unreachable notice to Debug and remove the now-inaccurate 'logs a warning' sentence from the README. Luisina: don't error out. Workspace-token/PAT auth is a valid mode where the account plane is inherently unreachable; the docs are the customer-facing surface, the log is an operator diagnostic. Add a mutation-checked regression test that fails if the level is flipped back to Warn.
…failure A blanket Debug was wrong for the OAuth path: an account-plane probe that fails under OAuth is a real whole-tenant degradation and must stay visible at Warn (baton-admin skip-and-continue Rule 4). Under workspace-token/PAT auth the account plane is out of scope by design and the branch fires every sync, so Debug is right there (avoids per-sync Warn noise). Gate on IsTokenAuth(); test both legs.
The ticket's minimum-to-unblock is explicitly "emit a warning when the account plane is unreachable under workspace-token auth", and the whole ticket is that PAT's degradation is invisible. Debug is invisible at the default info level, so the reviewer nit to drop Warn->Debug would re-hide the whole-tenant drop the ticket was filed to surface (also against baton-admin skip-and-continue Rule 4). Revert the earlier Debug/split attempts, keep Warn, add a regression test that fails on Debug, and note the why in a comment.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| env: | ||
| BATON_DATABRICKS: ./baton-databricks | ||
| uses: ConductorOne/github-workflows/actions/sync-test@v2 | ||
| uses: ConductorOne/github-workflows/actions/sync-test@v4 |
There was a problem hiding this comment.
🟠 Bug: sync-test@v4 adds an "Auth Error" step that runs the connector with invalidated credentials and requires exit code 16 (Unauthenticated) or 7 (PermissionDenied); this connector exits 2 (Unknown), so the bump makes the test job fail deterministically (run log: Connector exited 2 with invalid credentials; expected Unauthenticated (16) or PermissionDenied (7)).
Root cause is pkg/connector/connector.go:126 (and the sibling error paths at :138/:146) returning a bare fmt.Errorf, which gRPC maps to Unknown. Wrap credential failures with a status code — e.g. uhttp.WrapErrors(codes.Unauthenticated, "databricks-connector: failed to list workspaces", err) — or hold the bump until that's done.
The sync-test @v4 action added an auth-error check that runs the connector with bad credentials and expects a gRPC Unauthenticated/PermissionDenied exit. An OAuth2 invalid_client failure happens in the token transport before any API response, so uhttp never maps it and it surfaced as codes.Unknown (exit 2). Wrap the transport-level oauth2.RetrieveError as Unauthenticated.
…its-visibility-and-docs # Conflicts: # pkg/databricks/request_test.go
wrapTransportAuthError branched every *oauth2.RetrieveError to Unauthenticated, but oauth2 returns RetrieveError for any non-2xx from the token endpoint. Branch on the response status so a transient 5xx stays retryable (Unavailable), 403 is PermissionDenied, and 4xx/no response stays Unauthenticated. Extend TestWrapTransportAuthError with 401/403/503 cases. Hoist the account-plane probe error out of its if-block and attach it to the 'account API unreachable' warning via zap.Error, so operators get a cause (403 scope vs 5xx vs wrong hostname) under OAuth. Stays nil under token auth where the probe is skipped.
Databricks rate-limits the OIDC token endpoint, so a throttled token refresh returned a 429 that fell through to Unauthenticated (non-retryable). Map it to ResourceExhausted so it stays retryable, and add a test case.
ResourceExhausted tells the baton SDK to stop retrying, the opposite of the intended retryable throttle handling. A 429 from the OIDC token endpoint is a rate limit, so map it to Unavailable (matching the 5xx case) which the SDK auto-retries.
Under OAuth a failed account API check now returns an error instead of warning and continuing, so a permissions failure no longer silently drops account-level data. Workspace-token auth keeps the startup warning (its reduced scope is by design) and now also surfaces the limit at config level via the auth group help text. Also fixes the duplicate BATON_ACCOUNT_ID key in the workspace-token docs example so the manifest applies.
…y, document OAuth account-API requirement - docs/connector.mdx: remove the duplicate BATON_ACCOUNT_ID key from the workspace-token example so kubectl no longer rejects the manifest. - pkg/config/config.go: drop ExcludeWorkspacesField from the workspace-token group; it can never be validly set there (workspace-tokens requires workspaces, which is mutually exclusive with exclude-workspaces). - README.md and docs/connector.mdx: note that OAuth requires a reachable account API and fails validation if it is unreachable.
Bot-only review. Blocking finding (invalid-credential exit code) is fixed: the test job's sync-test Auth Error step passes on the head commit. OAuth account-API requirement is now documented (100cfb0), and the bot's later review reported no blocking issues. Dismissing the stale changes-requested.
4 participants
Workspace-token (PAT) syncs now log a startup warning that account-level access is not covered, so a switch to PAT no longer silently drops data from access reviews. Also fixes the PAT setup example, which previously could not start, and corrects the workspace/token pairing instructions.