Repository navigation
[CXH-2721] - Add Unity Catalog catalog sync - Databricks Connector - #71
mateoHernandez123 wants to merge 3 commits into
Conversation
e894653 to
019c458
Compare
00adc02 to
5933db6
Compare
FeliLucero1
left a comment
FeliLucero1
left a comment
There was a problem hiding this comment.
Did a pass on this one. Main thing is the first comment: Revoke can say "already revoked" while the privilege still exists. The rest are perf and dupe stuff.
Comments tagged (out of scope / non-blocking) are low-confidence or nits I didn't fully verify, so treat them as FYI. Also out of scope: the databricks-connector: error prefix vs baton-databricks: is a convention that predates this PR, not something it introduced.
Heads up: the unreachable-catalog thing below is still the same in #72 / #73.
5933db6 to
6add251
Compare
sergiocorral-conductorone
left a comment
sergiocorral-conductorone
left a comment
There was a problem hiding this comment.
Automated review
drafted 1 → confirmed 1, low-confidence 0, dropped 0, added 0 (panel: opus-5.5 / gpt-6-astra / gemini-4-argon, all high).
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
CXH-2721. Catalogs hang from the metastore, keyed {metastore_id}::{name},
with one entitlement per catalog privilege plus a read-only owner. The
connector reads them through every running in-scope workspace attached to
the metastore, caches the routes in the sync session store, and grants and
revokes privileges with a live read that bypasses the response cache.
The tree is unchanged from 60501e2, which is the state reviewed on #71.
The branch is rebuilt on top of the staging-to-main landing so the PR shows
only the catalog layer instead of the 42 staging commits it used to carry.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
60501e2 to
a343b68
Compare
Superseded — see the current review report for commit
|
Connector PR Review: [CXH-2721] - Add Unity Catalog catalog sync - Databricks ConnectorBlocking Issues: 1 | Suggestions: 2 | Threads Resolved: 0 Review SummaryThis PR adds an opt-in Unity Catalog The pagination, the 401/403/404 handling (no empty-list fallbacks, and the Security IssuesNone found. Correctness Issues
Suggestions
Prompt for AI agentsReviewed commit: |
There was a problem hiding this comment.
Blocking issues found — see the full review report
GetCatalog joined the raw catalog name into the path and skipped the
addressesSecurable guard that permissionsUrl applies. prepareRequest
unescapes the URL again before sending it, so a name holding "?", "#" or
"%" addressed a different catalog: GetCatalog("sales?x") read "sales" and
returned it.
That answer reached a write. catalogThrough checked only MetastoreID, and
routedEntitlement rebuilds the resource from the returned catalog.Name, so
Grant and Revoke sent their PATCH to the catalog Databricks answered with
rather than the one the entitlement named.
GetCatalog now runs the same guard permissionsUrl runs, and rejects an
answer whose name is not the name it asked for. catalogThrough passes
InvalidArgument through instead of folding it into catalogListingError,
which would have degraded it to Unknown.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The profile workspace was reused whenever it was still attached, but it was picked under the owner the last sync saw, and the owner decides which workspaces read every privilege: with the connector owning the metastore any of them does, otherwise only the ones it is a direct Admin of. After a transfer the recorded workspace can be one the connector no longer reads, while another attached workspace still satisfies the documented rule. Grant and Revoke refused the write with PermissionDenied and told the operator to grant Admin, on a metastore that was reachable. The route is now kept only when it still reads under the live owner, so the saved account round trip survives a transfer that does not move what the connector can read, and otherwise pickReadableWorkspace runs again. Also corrects the GetUncached comment: Cache-Control is part of the cache key, so the no-cache read does not refresh the entry normal reads look up. The behaviour is right and the comment described the opposite. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Dismissing: this review is on a343b68 and both findings have moved since.
The blocking one, GetCatalog addressing a different catalog, is fixed in 2855a2b. GetCatalog now runs the same addressesSecurable guard permissionsUrl runs and rejects an answer whose name is not the one it asked for, and catalogThrough passes the InvalidArgument through instead of degrading it to Unknown. Two tests cover it and both fail without the guard.
The CANCELLING suggestion is declined, with the reasoning in the thread. Dropping CANCELLING from workspaceIsUsable takes the workspace out of scope, which leaves the metastore that routed through it out of the sync, and ConductorOne reads that as a deletion and removes its grants. The status set is deliberate; the comments that described it as dropped were the wrong half and are corrected in #90.
The openapi spec note is tracked as a follow-up.
FeliLucero1
left a comment
FeliLucero1
left a comment
There was a problem hiding this comment.
Pass on f8cd1917. CI is green and this looks close: the errMetastoreGone / %w fixes, the GetCatalog guard (I traced ?, #, %2F and ..; all rejected), the system/samples skip and the session-store routes all check out. There's one low inline, and I bumped the CATALOG_DOES_NOT_EXIST thread.
| // it is a direct Admin of. A transfer can therefore leave the recorded | ||
| // workspace unreadable while another attached one still works, so the route | ||
| // is kept only when it still reads under the live owner. | ||
| if workspace != nil && profileOwner != liveOwner { |
There was a problem hiding this comment.
[low] The readability re-check only runs when the owner string changed. With the owner unchanged and the connector not owning it, the route was picked because we're a direct Admin there. If that Admin assignment gets removed on this workspace but stays on another attached one, we keep this route and resolveChange fails with PermissionDenied (fatal in C1), even though the other workspace can read. It fails closed and fixes itself on the next sync, so it's non-blocking. Dropping the profileOwner != liveOwner part would cover it for one extra ListWorkspaceMembers.
Description
Second of four stacked PRs for Unity Catalog (CXH-2721). A catalog is the named container a customer picks in an access review. Its parent is the metastore. The workspace host is only the access path.
#70 is merged. This PR is based on #91, which lands
stagingonmain.What the connector syncs
Before, on #91. The metastore is a leaf.
After this PR. Catalogs hang from the metastore, with their own entitlements and grants.
staginglanded onmainmain#90 is a fix to the metastore layer, not a layer of this stack. It branches from #91 beside this PR and changes five of the same files. Whichever lands second must merge the other and resolve them by hand.
Sync:
catalog, NEW, opt-in) — child ofmetastore, keyed{metastore_id}::{name}. One entitlement per catalog privilege, plus a read-onlyowner.ISOLATEDcatalog is listed only by the workspaces bound to it, so each workspace is read. A catalog that several workspaces list is emitted once per route. The store keys a resource by(external_id, sync_id)and upserts, so the copies collapse into one row. Measured on a live tenant: with every route pushed twice, the metastore still synced one catalog per name.Listresolves each metastore's running attached workspaces and its owner once and writes them there; catalogListreads them back, soListWorkspaces,ListMetastoreWorkspacesandListMetastoresdo not run again per metastore. A targeted sync that never ran metastoreListresolves them once and stores the result. The page token carries a workspace name, not a serialised route. This needs the session store, somain.gopassesconnectorrunner.WithSessionStoreEnabled().PermissionDenied. The error names the catalog and the remedy. ConductorOne does not remove the catalog. See Auth below.CATALOG_DOES_NOT_EXIST. Any other 404 isFailedPrecondition, so a refusal never reads as an empty page.systemcatalog and anything Databricks types asSYSTEM_CATALOG, which includessamples, are excluded whatever the connector owns. They used to appear while the service principal owned the metastore and disappear when it stopped, taking their grants with them. Measured on the Baton account: the sync goes from 3 catalogs and 2647 grants to 1 catalog and 1087 grants, and the 1560 that go aresamples(1300) andsystem(260). The one real catalog is unchanged at 29.Provisioning:
PATCH .../permissions/catalog/{name}. A pre-read past the cache decidesGrantAlreadyExistsandGrantAlreadyRevoked. The echoed assignments confirm the change.InvalidArgument.GetCatalogthat names this metastore proves the workspace still serves it, and the account's workspace listing and the metastore's assignment listing are skipped. Any other answer falls through to those listings and the full walk, and the workspace already read is not read twice. Scope is answered locally, since the profile carries the deployment name, the workspace name and the numeric id, which are the three identifiers the configuration matches on.GrantAlreadyRevokedwhile the privilege was still held. The resource is rebuilt from the live read, so trust is decided from the owners Databricks returns now, not from the owners the last sync stored.A catalog that no workspace lists is not proof of deletion. An unbound workspace answers the same 404 as a deleted catalog. The connector asks the metastore owner for the listing with
include_unbound=truebefore it answers:flowchart TD start[Grant or Revoke] --> route[rebuild the route] route -->|a workspace serves the catalog| write[PATCH permissions] route -->|every workspace answers CATALOG_DOES_NOT_EXIST| proof{connector owns the metastore?} proof -->|no| unknown[FailedPrecondition: deletion cannot be proved] proof -->|yes| unbound[list with include_unbound=true] unbound -->|catalog is in the listing| exists[FailedPrecondition: it exists, out of reach] unbound -->|catalog is absent| gone[Revoke: GrantAlreadyRevoked / Grant: NotFound]A deleted metastore takes the same two answers: Revoke returns
GrantAlreadyRevoked, Grant returnsNotFound.Measured on the Baton account, where the service principal owns the metastore:
GrantAlreadyRevokedwith no error.NotFound.Group Test1heldUSE_CATALOG,BROWSEwas granted and revoked, and the group was left holdingUSE_CATALOGagain. The fallback is visible in the timing: 4.56s through the profile route against 6.57s when the listings and the walk have to run.GET /catalogs/{name}andGET /permissions/catalog/{name}both answer 404CATALOG_DOES_NOT_EXISTfor a catalog that does not exist.Auth:
Credentials are unchanged. There is no new config field.
A catalog is synced when the service principal owns the metastore, owns the catalog, holds
MANAGEon the catalog, or is a direct Admin of the workspace whose admins group owns it. The admins group is named_workspace_admins_<workspace>_<id>, and the connector reads the workspace id off that name. The check runs against that workspace whatever route the catalog is listed through, because permission visibility follows the caller, not the host.Databricks answers anyone else with a 200 and only their own grants. A catalog that meets none of these rules fails List with
PermissionDeniednaming the remedy. ConductorOne does not remove it. Grant and Revoke on such a catalog fail withPermissionDeniedand send no PATCH.Architecture highlights:
securableLevelholds what differs per level — the securable type, the privilege set, the remedies, the name parser, the gone error codes and the trust rule.securableOpsholds what the shared path needs and is embedded in both builders, so the three operations are methods:m.syncGrants,m.grant,m.revoke.catalogRoutesreads the metastore's routes from the sync session and resolves them live only on a miss. Thepagination.Bagholds one page state per workspace, keyed by deployment name. The workspace Admin check is remembered per workspace for the length of a sync; provisioning has no sync session and always reads the current assignments.Privilegeenum omits.include_unboundanswers whether a catalog exists. It does not grant access to it, so List does not use it. A workspace cannot read the grants of a catalog it is not bound to.Useful links:
Merge order and release flow
staginglands onmainas its own PR, #91. The rest of the stack is rebased on top of it, so each PR below shows only the layer it adds. #84 is independent of this stack and merges on its own.staginglanded onmain(46 files)#90 and #71 target #91 until it merges, and GitHub retargets them to
mainat that point. #72 and #73 keep their parent branch as base.Rebased on 2026-11-03, as requested on #90. Before this, #90 and #71 showed 42
staging-only commits and 50+ files, which hid the layer each one adds. Every branch was rebuilt on top of #91. The tree at each head is byte-identical to the one reviewed before the rebase, so no code changed. The commits are squashed into one per PR, which marks earlier review threads as outdated; they stay readable.Release flow. Cut a test release (
-test) from the feature branch and move the PR to draft. The ticket moves to validation. QA validates that release. Merge tomain. Cut the official release frommain, and the ticket moves to rollout.