Repository navigation
[CXH-3600] - Sync metastore privileges held by the workspace admins group - Databricks Connector - #90
Conversation
manuel-ts-14
left a comment
manuel-ts-14
left a comment
There was a problem hiding this comment.
QA review from the CXH-2720 validation. The fix resolves the root cause reported in CXH-3600: reading the workspace ID from the alias and confirming admins on that workspace's SCIM is the right approach. The ID-reuse guard, the immutable grants and the FailedPrecondition on provisioning all look good. There are three findings, and none of them blocks validation:
- The docs say CANCELLING fails the sync, but the code skips it (inline on
docs/docs-info.md). - Every workspace-scoped admins group is named just
admins(inline onpkg/connector/groups.go). - The description and the commit message say the workspace syncer "no longer reads any Unity Catalog endpoint" and that "a 403 from Unity Catalog used to fail the whole workspace type". I can't find that change in the diff against
staging. At4b956905,pkg/connector/workspaces.gohas no Unity Catalog read:git grep -i -E "unity|metastore" 4b956905 -- pkg/connector/workspaces.gomatches only the comment at line 390. If that sentence describes an earlier iteration of this branch, could you drop it? If I've missed the hunk, please point me at it. QA scopes regression cases from the description, so a behaviour change that isn't in the diff sends us testing something that never changed.
luisina-santos
left a comment
luisina-santos
left a comment
There was a problem hiding this comment.
Requesting changes because this PR's diff doesn't match what it says it does.
Against main, the diff has 42 commits and 52 files. The fix itself is one commit (860d82e2) touching 15 files. The rest comes from staging:
- #70 (Unity Catalog foundation and metastore sync) and #56 (incremental sync, audit event feed, test-server). These are only on
staging. - #81 as
6384cfa1.mainalready has it as0c145865.
The description is also off in a few places:
- The stack table lists this PR's base as
staging, but it targetsmain. - It says the
staging-only commits were "never reviewed as their own PRs". #56 and #70 were reviewed and merged intostaging. - #88 (on
stagingsince 2026-11-02) isn't in this branch, so it wouldn't reachmainthrough this PR.
Could you:
860d82e to
f4e64dc
Compare
Superseded — see the current review report for commit
|
Connector PR Review: [CXH-3600] - Sync metastore privileges held by the workspace admins group - Databricks ConnectorBlocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0 Review SummaryMetastore privilege rows for the Unity Catalog alias RUNNING workspaces now declare a I scanned the full PR diff (15 files) for security and correctness, using the base criteria plus the connector mixin. No repo-local criteria loaded. Points I checked:
There were no prior findings to audit. Coverage gaps: none material. Security IssuesNone found. Correctness IssuesNone found. SuggestionsNone. Reviewed commit: |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
No blocking issues found — see the full review report
…mins group Unity Catalog writes a metastore privilege row for each workspace's built-in admins group under the alias _workspace_admins_<name>_<id>. That principal is workspace-local, so the account SCIM API answers its id with a 404 and the row was dropped: 7 privileges on AWS and 8 on Azure were absent from the bundle while the sync exited 0 and named the drop only at Debug. A RUNNING workspace now offers its admins group as a child, and the metastore resolver maps the alias to that group through the workspace id the alias carries, rather than through the workspace the permissions were read with. The grants are immutable and provisioning refuses a workspace-local group, because Databricks does not allow a revoked privilege to be granted back to the system group. The group's membership is read-only for the same reason: an identity-federated workspace rejects a membership PATCH with 400. Resolving the alias before the account lookups skips the three reads that would always miss, which is what logged the two 404 warnings. The workspace syncer decides this from the workspace status and reads no Unity Catalog endpoint. Asking Unity Catalog which workspaces carry a metastore was the alternative, and it is rejected here: metastores are opt-in while workspaces always sync, so a 403 on that plane would fail the whole workspace type, and its role and group children with it, on any tenant that cannot read it. No such read is removed; there was none. A member the account does not hold is skipped instead of emitted, since only the account syncs users, service principals and nested groups. The filtered workspace listing reports every group in totalResults, so it asks for no second page, and a workspace in CANCELLING offers no admins group, because it never becomes RUNNING. Metastore routing treats that same status differently and keeps waiting, so a metastore is not pruned while its workspace is still being deleted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…h workspace holds a metastore Databricks names the built-in group "admins" in every workspace, so a metastore shared by several of them listed one holder per workspace under one indistinguishable name, and only the parent resource told them apart in an access review. A workspace-scoped group is now named "admins (<workspace name>)". The unqualified name stays on the resource profile and the resource ID does not change, so the grants are unchanged: 875 on AWS and 98 on Azure before and after. The name costs one account workspace listing per sync, which the response cache already holds. The docs said a privilege row whose workspace is provisioning or cancelling fails the sync before SCIM. The resolver skips a cancelling workspace instead. Routing treats CANCELLING as pending so the metastore is not pruned while the deletion runs: pruning would report the metastore to ConductorOne as gone and delete its grants, and the wait ends on its own when the account stops listing the workspace. That reason lived in the Unity Catalog layer and was lost when the layer was folded into the metastore syncer, which left a comment grouping CANCELLING with the terminal statuses while the code did not. It is restored. The error that covers the wait said only that the metastore has no running workspace in sync scope, which reads as a misconfigured scope, and the other path said the workspaces are on their way to RUNNING, which a workspace being deleted is not. Routing now returns the workspaces that hold the metastore with their status, and both messages name them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…rkspace Two comments still described CANCELLING as dropped from scope, which is what workspaceIsUsable deliberately does not do: dropping the workspace prunes the metastores that routed through it, and ConductorOne then deletes grants that are still in force while the deletion runs. The comment on the status constants already says this; the one on the scope loop and the one on the status test did not. The two answers are both intentional. The workspace syncer offers no admins group for a cancelling workspace, because it will never be RUNNING. Metastore routing keeps it, because the metastore is still there until the account stops listing the workspace. No behaviour changes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
d5403a1 to
9e8cdd8
Compare
The mock answered every workspace group listing with the admins group and never read the filter, so both List tests passed whether or not `displayName eq "admins"` was sent. It now serves three groups and filters them: dropping the filter fails TestWorkspaceGroupListStopsAfterTheAdminsGroup and TestWorkspaceGroupNameNamesItsWorkspace instead of passing silently. The group name is a shared constant now, because the listing filter and the alias check in metastore_workspace_admins.go have to agree on it and each spelled it out. The List godoc said it returns all groups, which stopped being true under a workspace parent, and the alias comment in metastore Grants said the alias skips three account lookups without saying that a row it cannot resolve still runs them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ber type accountHolds asked the account SCIM endpoint about one member at a time, so a workspace admins group of n members cost n filtered reads. The members of a page are now resolved in one request per member type, batched at 100 "id eq" terms because Databricks refuses a SCIM filter above 200 nodes and an n-term chain is 2n-1 of them. A filtered read that answers fewer rows than it matched now fails instead of reading as "the account does not hold these ids". The members a short page left out would be skipped, and a membership grant missing from a sync is revoked access in ConductorOne. The test mock now applies the whole or-chain rather than its first term, which a caller that dropped the rest would otherwise have passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Description
QA found this during validation of CXH-2720 on
v0.2.6-metastore-sync(#70, merged at4b956905). Both QA metastores carry a privilege row for the workspace's built-in admins group. Unity Catalog names that principal_workspace_admins_<workspace_name>_<workspace_id>. The group is workspace-local, so the account SCIM API answers its id with a 404. The resolver found no match and dropped the whole row.The cost is a silent gap. On AWS the row holds 7 privileges, on Azure 8. None of them reached the bundle. The sync still exited 0, and the only log line above Debug was a pair of 404 warnings. An access review that asks who can create a storage credential on the metastore got an incomplete answer, and nothing said so.
This PR resolves the alias to the group that holds the privileges, and syncs that group so the grants have a principal to point at.
What the connector syncs
Before. Groups hang off the account only. A workspace offers roles.
After. A RUNNING workspace also offers its
adminsgroup. The metastore grants point at it.How the alias resolves
Before. The account plane is the only scope the resolver reads.
After. The alias ends in the workspace id, so the connector reads the scope that holds the group.
The alias names its own workspace. The connector does not use the workspace it read the permissions through, because a metastore can attach to more than one.
staginglanded onmainmainThis PR is not a layer of the Unity Catalog stack. It fixes the metastore layer that #70 already shipped, so it branches from #91 beside #71.
Merge this PR before #71. The two branches change the same five files, and a merge test shows a content conflict in
docs/connector.mdx,pkg/connector/connector.go,pkg/connector/metastore.go,pkg/connector/metastore_helpers.goandpkg/connector/metastore_test.go. The rollup shows both as clean, because GitHub compares each one against #91. Re-tested after the rebase: the same five files still conflict. The second branch to land must merge the first and resolve them by hand.Sync:
group) — a RUNNING workspace now declares itsadminsgroup as a child. The listing is filtered to that one group. The connector emits a member grant only when the account holds the member, because it does not sync workspace-local identities.metastore) — the privilege rows for a workspace admins group now become grants. The grants carryGrantImmutable.workspace) — unchanged surface. Whether a workspace offers itsadminsgroup is decided from the workspace status, so the syncer reads no Unity Catalog endpoint.Provisioning:
FailedPrecondition. Databricks does not allow a revoked privilege to be granted back to the system group, so the operation is one way.adminsgroup's membership entitlement is not grantable. An identity-federated workspace rejects a membership PATCH with 400 and answers that the group is managed in the account. Account groups keep the entitlement they had.GrantAlreadyExistsandGrantAlreadyRevokedannotations.Auth:
Unchanged. No new flag. The connector keeps account-level OAuth.
Architecture highlights:
4b956905, with Unity Catalog answering 403,workspaceBuilder.Listalready returned every workspace with no error.CANCELLINGis skipped. It never becomes RUNNING, so waiting for it fails every sync.totalResults. The connector asks for no second page.Validation
Measured against the two QA tenants, with a binary built from
4b956905and a binary built from this branch.231086632550479Azure shows the same shape. The 8 privileges, which add
CREATE_CLEAN_ROOM, now appear onworkspace/adb-2531901403506481.1/group/517918373872763.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.