CXH-2163: gracefully handle 403 on inaccessible workspaces - #51
CXH-2163: gracefully handle 403 on inaccessible workspaces#51al-conductorone wants to merge 6 commits into
Conversation
…e sync When the OAuth2 service principal has account-level access but is not an admin on a specific workspace, listing users, groups, or service principals for that workspace's roles returns 403 and previously failed the entire sync. Skip the workspace-scoped listing on a 403 (log and return empty) instead, mirroring the existing 400 handling in workspaces.go. Account-scoped 403s still propagate.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // Grants returns all the grants for a given role. | ||
| // Since Databricks API does not support listing grants for a role, so that it aligns with the sdk API, | ||
| // we have to go through all the users, groups and servicePrincipals to check if they have the role. | ||
| // isWorkspaceAccessForbidden reports whether err is a 403 from a workspace-scoped | ||
| // API call. This happens when the service principal has account-level access but is | ||
| // not an admin on that specific workspace; such workspaces are skipped so one | ||
| // inaccessible workspace does not fail the whole sync. | ||
| func isWorkspaceAccessForbidden(err error) bool { | ||
| var apiErr *databricks.APIError | ||
| return errors.As(err, &apiErr) && apiErr.StatusCode == http.StatusForbidden | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Inserting isWorkspaceAccessForbidden here orphans the Grants doc comment (lines 135-137). With no blank line between line 137 and 138, godoc now attaches the whole block to isWorkspaceAccessForbidden, and Grants (line 147) is left undocumented. Move the helper above the Grants doc comment (or below the Grants function) so each comment documents its intended function. (confidence: high)
| if err != nil { | ||
| return nil, nil, fmt.Errorf("databricks-connector: failed to list users: %w", err) | ||
| if isWorkspaceRole && isWorkspaceAccessForbidden(err) { | ||
| l.Info("databricks-connector: workspace not accessible to service principal, skipping users", |
There was a problem hiding this comment.
🟡 Suggestion: Skipping an inaccessible workspace is logged at Info. The repo-local log-level criteria (L1/L4) classify a 4xx skip-and-continue as Warn so operators can see that a workspace's role grants were dropped from the sync — Info is often filtered out. Same applies to the groups (line 243) and service-principals (line 288) branches. (confidence: medium)
Connector PR Review: CXH-2163: gracefully handle 403 on inaccessible workspacesBlocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0 Review SummaryThe new commit is the deprecated-trait migration: profile and status move from trait-level options to resource-level, and all reads switch to rs.GetProfile(resource). This is SDK-supported (GetProfile/GetStatus in baton-sdk v0.20.5 read the resource field first and fall back to the deprecated trait fields), and the connector sets and reads consistently, so grants/entitlements resolve correctly. The UserTrait status enum swap to resource-level status is value-identical per the SDK. The full PR diff was scanned for security and correctness; the 403 skip-and-continue behavior is unchanged and terminates pagination correctly. No new issues found. Note: this commit also lowered the workspace-403 skip logs from Info to Debug; the previously-raised log-level suggestion (L4 leans toward Warn for skip-and-continue so operators retain visibility of skipped workspaces) still applies and is not re-posted. Security IssuesNone found. Correctness IssuesNone found. SuggestionsNone. |
Skip-and-continue on a per-workspace 403 was logged at Info. Portfolio bans Warn (log cost/noise) and prescribes Debug for non-error diagnostics, so drop these skip notices to Debug. The pr-review bot suggested Warn per the repo's baton-admin error-handling criteria; the portfolio's no-Warn rule overrides it.
golangci-lint (staticcheck SA1019) fails on the deprecated RoleTrait/GroupTrait/ UserTrait Profile field and the WithRoleProfile/WithGroupProfile/WithUserProfile/ WithStatus trait options, which the recent baton-sdk bump marked deprecated in favor of resource-level attributes. Writes now use WithResourceProfile / WithResourceStatus resource options; reads use rs.GetProfile(resource). Behavior is unchanged: NewXResource still attaches the trait, GetProfile falls back to trait-level data, and UserTrait_Status_Status and Status_ResourceStatus share identical enum values. Clears all 21 SA1019 lint errors (verify/lint was red here and on main from the SDK bump).
2 participants
Databricks syncs no longer fail when the connector can read the account but lacks admin access to one workspace. That workspace's roles are skipped and the rest of the sync completes, instead of the whole sync erroring out.