Repository navigation
[CXH-2720] - Add Unity Catalog client, access-path routing, and metastore sync - Databricks Connector - #70
Conversation
Superseded — see the current review report for commit
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Superseded — see the current review report for commit
|
There was a problem hiding this comment.
Blocking issues found — see the full review report
…and metastore sync Co-authored-by: Cursor <[email protected]>
e894653 to
019c458
Compare
Superseded — see the current review report for commit
|
Triaged all three findings. The 404 swallow in Grants() was real and is fixed in 019c458 across all five securable levels, and the metadata description is now scoped per branch in the same commit; both threads are resolved. The workspaces[0] probe is a real robustness gap but it is deferred with evidence on the thread: the documented setup makes the first workspace always eligible, the failure is a loud PermissionDenied rather than a silent revoke, and the fix changes access-path selection, which is the one part of this stack validated live and cannot be re-validated while the Databricks account is out of credits. Dismissing the blocking verdict per team convention; the bot does not approve, so it would otherwise block merge indefinitely.
There was a problem hiding this comment.
Blocking issues found — see the full review report
Declining. Routing stays the first running in-scope workspace. Admin is checked only there, and the sync fails closed otherwise.
A partial permissions read looks like an empty grant list, so List and provisioning pick the first running in-scope workspace where the connector owns the metastore or is a direct Admin. Co-authored-by: Cursor <[email protected]>
Keep the quoted SCIM filters and route group reads through GetUncached. Account group writes now patch, so the full-replacement displayName guard is gone. Co-authored-by: Cursor <[email protected]>
Superseded — see the current review report for commit
|
Connector PR Review: [CXH-2720] - Add Unity Catalog client, access-path routing, and metastore sync - Databricks ConnectorBlocking Issues: 0 | Suggestions: 5 | Threads Resolved: 0 Review SummaryThe PR adds a Unity Catalog client with host-scoped caching and an opt-in Security IssuesNone found. Correctness IssuesNone found. Suggestions
Resolved prior findings
Prompt for AI agentsReviewed commit: |
There was a problem hiding this comment.
No blocking issues found — see the full review report
…anch Co-authored-by: Cursor <[email protected]>
…unity-catalog-foundation-and-metastore Co-authored-by: Cursor <[email protected]> # Conflicts: # pkg/connector/connector.go # pkg/connector/roles.go # pkg/databricks/request.go
FeliLucero1
left a comment
FeliLucero1
left a comment
There was a problem hiding this comment.
Round 4: all the round-3 items check out (repro'd on dd1a5ac). Two mediums left, both about re-coding errors: a List 404 still escapes as NotFound, and Grants now hides 429/5xx as FailedPrecondition. Inline.
One for the body (out of scope for the inline stuff): Validate now hard-fails when the workspaces allowlist matches nothing (connector.go:153). That's what sergio/luisina asked for and the README covers it, but it's a behavior change from v0.2.5 (which synced account users/groups/roles with an empty workspace sync), and it also hits tenants that never enabled metastores (opt-in). Worth a line in connector.mdx (the Workspaces field) and the release notes. The PR body still describes the old behavior, and the "sync will be empty" Warn at workspaces.go:104 can't be reached anymore.
sergiocorral-conductorone
left a comment
sergiocorral-conductorone
left a comment
There was a problem hiding this comment.
Automated review (follow-up/first pass) — drafted 2 → confirmed 2, low-confidence 0, dropped 0, added 0 (panel: opus-5.5 / gpt-6-astra / gemini-4-argon, all high).
A 404 on the metastore's workspace list, on the account's workspace list, or on ListMetastores reached the SDK as codes.NotFound. The metastore List is a typed child of the account, so the SDK runs it with allowWarning, reads that as "no metastores" and ends the sync successfully with none. ConductorOne then deletes every metastore and its grants. Only the not-found case is re-coded to FailedPrecondition. Wrapping every failure that way, as metastore Grants did, turned a 429 or a 5xx into a code the SDK does not retry, so one throttled permissions page failed the whole sync instead of backing off. Measured against a live tenant through a proxy that injects the failure: an injected 404 on the assignment list used to exit 0 with zero metastores and now fails the sync, and an injected 429 on the permissions read used to stop after one call and now retries with backoff. Validate no longer reports a workspace that databricks-exclude-workspaces removed as absent from the account, since ListWorkspaces drops those before the check sees them. Co-authored-by: Cursor <[email protected]>
FeliLucero1
left a comment
FeliLucero1
left a comment
There was a problem hiding this comment.
Round 5: M1/M2 check out. I repro'd on e76e8e3 (unit + a real-SDK e2e): a 404 on the assignment list, /metastores or /workspaces now fails the sync instead of emptying it, and 429/503 come through as Unavailable and get retried. Nice. Two leftover NotFound paths are inline (the SCIM lookups), plus a few nits.
For the body, out of scope for inline:
- PR body is stale. It still says 401/403/404 on the assignment list give FailedPrecondition (only 404 does now; 401/403 are Unauthenticated/PermissionDenied). It says the Validate error "separates a name that is also in databricks-exclude-workspaces" (it's one message, and the case can't happen because of the mutual exclusion). And it says the post-PATCH re-read makes "a later sync see the new state", which isn't true with Cache-Control in the key. It also doesn't mention the Validate change from v0.2.5 or that 429/5xx now retry.
- Still pending from the last round, as promised: prime the cache in
TestASyncReadAfterAGrantSeesThePostChangeState, fix the two cache comments, restore the dropped tests (GrantAlreadyExists, ownerGrantImmutable,emitPrincipal, host-scoped cache key), the connector.mdx line for the Validate change, and the release notes. - (out of scope, pre-existing): request.go:327 returns the bare parseJSON error when an error body isn't JSON (e.g. a gateway HTML page), so a 404/429/503 loses its status and comes out as Unknown. It fails safe, but behind a proxy a throttle wouldn't be retried. Worth keeping the APIError (status + err) on parse failure at some point.
…ace error Co-authored-by: Cursor <[email protected]>
Description
First of four stacked PRs for Unity Catalog (CXH-2720). A metastore lives on the Databricks account plane. Every securable under it (catalog, schema, table, volume) only answers on a workspace host that the metastore is assigned to. This PR adds the Unity Catalog client, that host routing, and the
metastoreresource.This PR delivers the blue node. Later PRs add catalog, schema, table and volume.
mainSync:
metastore, NEW, opt-in) — listed fromGET /api/2.0/accounts/{account_id}/metastores, keyed by metastore id. One entitlement per metastore privilege, plus a read-onlyowner.workspacesordatabricks-exclude-workspaces, is not synced.FailedPrecondition; the error names the assignment list.PROVISIONING,NOT_PROVISIONED, andCANCELLINGstay pending and fail the same way, with the status in the message.CANCELLINGis still in process, so pruning it would delete the metastore before the cancellation finishes.FAILED,BANNED, and a status Databricks has not listed are unusable: a metastore whose only workspace is in that state is not synced, so the sync does not fail on every run.workspacesallowlist that matches no workspace fails the whole sync, including users, groups, service principals, and workspaces. The error separates a name that is also indatabricks-exclude-workspacesfrom a name that is not on the account.METASTORE_DOES_NOT_EXISTand the account no longer lists the metastore. Databricks also uses that code when the workspace is not assigned to it. On the first page Grants rebuilds the routing: it returns no grants when the metastore is gone or has left the sync scope, and retries once when the account now names another workspace. That retry is kept only when the new workspace answers in a single page, because a later page resolves its workspace from the profile again; a reassignment whose grants span pages fails, and the next sync re-routes from List. Every other case answersFailedPrecondition. Past the first page it does not re-route, because a page token belongs to the workspace that minted it and ending the pages early would report the grants already emitted as the complete set. Any other 404 isFailedPreconditiontoo, so the SDK does not skip the page and ConductorOne does not delete the bindings.Provisioning:
PATCH /api/2.1/unity-catalog/permissions/metastore/{metastore_id}. Databricks no-ops a privilege that is already held or already absent, so the connector confirms the change from the echoed assignment list (GrantAlreadyExists/GrantAlreadyRevoked).GrantAlreadyRevoked, Grant returnsNotFound.FailedPreconditionand send no PATCH.MANAGEandALL_PRIVILEGESare rejected on a metastore.Auth:
Unchanged account-level OAuth service principal. No new required flag. To read a metastore's grants, the service principal must own the metastore or be assigned Admin directly on a workspace attached to it. Admin inherited through a group is not detected.
Architecture highlights:
GET /api/2.0/accounts/{account_id}/metastores/{metastore_id}/workspacescall per metastore. A workspace that no list names is not on Unity Catalog. A 401, 403 or 404 on a list marks that metastore unknown, so it is never pruned.GetUser, then a filtered group list (id eq, count 1), thenGetServicePrincipal. AccountGET /Groups/{id}is not used, because it returnsmembers. Find* by name runs only when an assignment has noprincipal_id. The connector does not walk all of account SCIM. SCIM filter values are quoted (RFC 7644).resolveSecurableChangeandmatchHeldPrivilegeonsecurableDepshold the Grant/Revoke logic. The later PRs reuse them.metastorestays in capabilities and C1 can show its opt-in toggle. The metastore builder receives explicit booleans for the principal types it may emit grants to. This is the same shape as baton-grafana.max_resultsis 0, so that path is rare.error_codeis kept on API errors, so*_DOES_NOT_EXISTis distinct from a route 404.flowchart LR ms[ListMetastores] --> assign[ListMetastoreWorkspaces<br/>per metastore] ws[ListWorkspaces<br/>allowlist / exclude / RUNNING] --> assign assign --> q{usable workspace?} q -->|yes| probe[try each workspace<br/>until owner or Admin] q -->|none attached / out of scope / FAILED or BANNED| pruned[not synced] q -->|list unreadable or PROVISIONING or NOT_PROVISIONED| fail[FailedPrecondition] probe --> perms[permissions on that workspace host]flowchart TD grants[Grants page] --> perms[ListPermissions] perms --> each{each assignment} each -->|has principal_id| byid[GetUser / group list id eq / GetServicePrincipal] each -->|name only| byname[Find* by name] perms --> owner[owner from profile] --> byname byid --> emit[emit grants] byname --> emitUseful links: