Repository navigation
Conversation
Sync Unity Catalog catalogs, schemas, and tables as resources with per-privilege entitlements and permission grants for access reviews. Resolve UC principals (user, group, service principal by application ID) and surface securable owners. Support grant/revoke via the permissions API.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| for _, priv := range a.Privileges { | ||
| g, err := securableGrant(ctx, c, resource, workspaceId, priv, principalID) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| rv = append(rv, g) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: the privilege lists at lines 34-48 aren't actually supersets of what the API can return (EXTERNAL_USE_SCHEMA is missing at catalog/schema level, and Databricks keeps adding privileges), so this loop can emit a grant whose entitlement ID was never produced by Entitlements(). Either skip-and-Warn on privileges not in the level's list, or drive entitlement creation from the same source of truth so the invariant in the comment holds.
| func resolvePrincipalResourceID(ctx context.Context, c *databricks.Client, workspaceId, principal string) (*v2.ResourceId, error) { | ||
| if userID, _, err := c.FindUserID(ctx, workspaceId, principal); err == nil && userID != "" { | ||
| return &v2.ResourceId{ResourceType: userResourceType.Id, Resource: userID}, nil | ||
| } else if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if groupID, _, err := c.FindGroupID(ctx, workspaceId, principal); err == nil && groupID != "" { | ||
| return &v2.ResourceId{ResourceType: groupResourceType.Id, Resource: groupID}, nil | ||
| } else if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if spID, _, err := c.FindServicePrincipalID(ctx, workspaceId, principal); err == nil && spID != "" { | ||
| return &v2.ResourceId{ResourceType: servicePrincipalResourceType.Id, Resource: spID}, nil | ||
| } else if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return nil, nil | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: this runs 1-3 uncached SCIM list calls per principal per securable. Grants() is invoked for every catalog, schema and table, so a metastore with a few thousand tables and a handful of grantees each turns into tens of thousands of SCIM requests per sync — slow and very likely to hit Databricks rate limits. A per-workspace map[string]*v2.ResourceId cache (plus a negative cache for unresolved names) on the builder or client would collapse almost all of it.
| return nil, err | ||
| } | ||
|
|
||
| if groupID, _, err := c.FindGroupID(ctx, workspaceId, principal); err == nil && groupID != "" { |
There was a problem hiding this comment.
🟡 Suggestion: these Find*ID helpers interpolate the principal straight into a SCIM filter (displayName eq '<value>' in client.go:371). A Unity Catalog group named e.g. Bob's Team produces a malformed filter, the SCIM call 400s, and the error propagates up through securableGrants and fails the sync. Worth escaping/quoting the value in the Find* helpers (or treating an invalid-filter 400 as unresolved) now that arbitrary UC principal strings reach this path.
Degrade gracefully when Unity Catalog is unavailable or the service principal lacks UC access: catalog/schema/table List and grant enumeration log a warning and skip instead of failing the whole sync, while context cancellation still propagates. Regenerate baton_capabilities.json and update the docs capabilities table for the new catalog, schema, and table resource types.
| name, _, err := c.FindUsername(ctx, workspaceId, principal.Resource) | ||
| return name, err | ||
| case groupResourceType.Id: | ||
| name, _, err := c.FindGroupDisplayName(ctx, workspaceId, principal.Resource) |
There was a problem hiding this comment.
🟠 Bug: Group principals in this connector carry a composite resource ID (groupResourceId() builds account/<accountId>/group/<groupId> or workspace/<ws>/group/<groupId>), and securableGrant emits group grants through groupGrantExpansion, so principal.Resource here is that composite string — not a SCIM group ID. FindGroupDisplayName then filters id eq 'account/acc-x/group/123', matches nothing, and returns "", so securableGrantChange fails with "could not resolve principal ..." for every group Grant/Revoke on a catalog/schema/table. groupBuilder.Grant/Revoke and servicePrincipalBuilder handle this by calling parseResourceId(principal.Id.Resource) first; do the same here (and derive the SCIM scope from the parsed parent, since an account-parented group must be looked up against the account API).
Confidence: high.
| rv = append(rv, ent.NewPermissionEntitlement( | ||
| resource, | ||
| ownerEntitlement, | ||
| ent.WithGrantableTo(userResourceType, groupResourceType, servicePrincipalResourceType), |
There was a problem hiding this comment.
🟡 Suggestion: The owner entitlement is declared grantable to users, groups and service principals, but securableGrantChange unconditionally rejects it ("ownership cannot be provisioned via the permissions API"). C1 will offer this entitlement for access requests and every grant attempt will fail. Drop ent.WithGrantableTo(...) for the owner entitlement so it stays read-only, matching the comment on ownerEntitlement.
| // connector resource ID. Returns nil when the principal cannot be matched to any | ||
| // known identity so the caller can skip it. | ||
| func resolvePrincipalResourceID(ctx context.Context, c *databricks.Client, workspaceId, principal string) (*v2.ResourceId, error) { | ||
| if userID, _, err := c.FindUserID(ctx, workspaceId, principal); err == nil && userID != "" { |
There was a problem hiding this comment.
🟡 Suggestion: Resolution is scoped to the securable's workspace SCIM API only. Unity Catalog privileges can be held by account-level users/groups/service principals that are not assigned to that workspace, so those assignments resolve to nil and are dropped with a Warn — silent grant loss rather than a visible failure. Consider falling back to an account-scoped lookup (workspaceId == "") when c.IsAccountAPIAvailable() and the workspace lookup finds nothing.
Confidence: medium.
| } | ||
| // Degrade gracefully: a securable whose grants cannot be read (UC access | ||
| // missing, securable deleted mid-sync) must not fail the whole sync. | ||
| l.Warn("databricks-connector: unable to list unity catalog permissions, skipping securable grants", |
There was a problem hiding this comment.
🟡 Suggestion: This Warn (and the unresolved-principal Warns below) fires once per securable, so a metastore where UC permissions are not readable will emit one warning per table — thousands per sync. Consider logarithmic sampling (1, 10, 100, every 1000) with a total_occurrences field, or aggregating to a single warning per catalog.
| NextPageToken string `json:"next_page_token"` | ||
| } | ||
|
|
||
| type permissionsResponse struct { |
There was a problem hiding this comment.
🟡 Suggestion: permissionsResponse omits next_page_token, which the UC "Get permissions" endpoint does return. It is safe today because ListPermissions sends no max_results (the API then returns all assignments), but if that default ever changes — or a caller adds max_results — grants will be silently truncated with no way to detect it. Worth decoding next_page_token and looping/returning a cursor.
| } | ||
|
|
||
| // ListTables returns a page of tables within a schema. | ||
| func (c *Client) ListTables( |
There was a problem hiding this comment.
🟡 Suggestion: The List Tables response carries full column and property metadata by default, none of which is used by tableResource — that is a large payload per page on wide tables. Passing omit_columns=true and omit_properties=true via ucListVars would cut response size substantially.
Connector PR Review: Add Unity Catalog resource sync (catalogs, schemas, tables)Blocking Issues: 2 | Suggestions: 7 | Threads Resolved: 0 Review SummaryThe full PR diff was scanned for security and correctness; the new commit was additionally reviewed at suggestion level. Commit Security IssuesNone found. Correctness Issues
Suggestions
Prompt for AI agents |
… limits - Gate Unity Catalog behind sync-unity-catalog (and table sync behind sync-unity-catalog-tables) so existing installs are unaffected until opted in; regenerate config schema and document the flags. - Resolve UC principals against account SCIM first (with workspace fallback) so grants match account-level identities, not just workspace-scoped ones. - Cache principal resolution per workspace to avoid a SCIM lookup storm across every catalog/schema/table. - Skip (with warning) privileges outside a securable's modeled set so no grant references an entitlement Entitlements() never produced; add EXTERNAL_USE_SCHEMA. - Return rate-limit annotations from UC List and Grants.
| var result *v2.ResourceId | ||
| for _, scope := range r.scimScopes(workspaceId) { | ||
| id, err := resolvePrincipalResourceID(ctx, r.client, scope, principal) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if id != nil { | ||
| result = id | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
🟠 Bug: the new workspace-scope fallback yields principal IDs that match no synced resource (high confidence). When the account API is available, identities are synced only from account SCIM (account.go:52-59 adds user/group/service_principal as account children; workspaceResource does not), yet scimScopes falls back to workspace SCIM. resolve discards which scope matched, so a workspace-local identity returns its workspace SCIM ID — and securableGrant then picks the group parent purely from IsAccountAPIAvailable(), producing account/<acct>/group/<workspaceGroupId>. Those grants point at principals that were never synced.
Either return the matched scope from resolve and pass it to groupGrantParent (workspace parent for a workspace-scope match), or skip the workspace fallback entirely when the account API is available, since those identities aren't in the graph anyway.
| for _, priv := range a.Privileges { | ||
| if _, ok := known[priv]; !ok { | ||
| l.Warn("databricks-connector: skipping unmodeled unity catalog privilege", | ||
| zap.String("privilege", priv), | ||
| zap.String("securable_type", securableType), | ||
| zap.String("securable", fullName), | ||
| ) | ||
| continue | ||
| } | ||
| g, err := securableGrant(ctx, r.client, resource, workspaceId, priv, principalID) |
There was a problem hiding this comment.
🟡 Suggestion: the privilege lists are now an authoritative allowlist, which turns a previously-visible problem (grant referencing an entitlement Entitlements() never emitted) into silent under-reporting. Any UC privilege outside the list — legacy USAGE/CREATE/CREATE_VIEW on migrated metastores, or any value Databricks adds to the privilege enum later — disappears from the access review with only a log line. Consider emitting the entitlement dynamically for unknown privileges, or at minimum adding a table-driven test that pins each level's list against the documented enum so drift fails CI.
Separately, this Warn fires per privilege per securable, so a metastore with thousands of tables can emit tens of thousands of identical lines. Logarithmic sampling (1, 10, 100, every 1000) with a total_occurrences field would keep it readable.
|
Closing this in favour of a stack that grew out of it, and I want to be clear up front that your research is what got the ticket moving. The resource shape, reading direct privilege assignments rather than effective ones so reviews do not over-report, surfacing securable owners as their own read-only entitlement, and resolving service principals by application ID all carried straight through into what shipped. Two things we found running it against a live account changed the shape enough that it became a different change rather than a revision of this one: A catalog's parent is the metastore, not the workspace. The same catalog is reachable from every workspace the metastore is assigned to, so a The workspace is an access path that has to be resolved, not a given. Metastores live on the account plane, but securables only answer on a workspace host the metastore is assigned to, so every call now works out which running workspace can actually reach a given catalog. That matters more than it sounds: an Scope ended up at five securable levels — metastore, catalog, schema, table and volume — with Grant/Revoke on each, so it is going in as four stacked PRs instead of one:
|
2 participants
Summary
Adds Unity Catalog data assets to the baton-databricks connector so access reviews can certify who can read/modify specific catalogs, schemas, and tables. Previously the connector synced only account-level identity resources (accounts, groups, roles, service principals, workspaces).
Resolves CXH-2341.
What's included
Resources — three new resource types synced in a
workspace → catalog → schema → tablehierarchy:catalog,schema,tablebuilders with fullList/Entitlements/Grants/Grant/Revoke.{deployment}::{full_name}so childListcalls can reconstruct the workspace and Unity Catalog full name from the parentResourceIdalone.Entitlements & grants
SELECT,MODIFY,MANAGE,ALL_PRIVILEGES,USE_*, etc.).GET /api/2.1/unity-catalog/permissions/{type}/{full_name}), not effective/inherited, to avoid over-reporting in reviews.ownerentitlement, since ownership carries implicit full control the permissions API does not return.Principal resolution — Unity Catalog references principals by name; grants map both directions:
Provisioning —
Grant/RevokeviaPATCH .../permissions/{type}/{full_name}with add/remove changes. Owner is intentionally rejected (not settable through the permissions API).Client layer (
pkg/databricks/unity_catalog.go) — cursor-paginatedListCatalogs/ListSchemas/ListTables, plusListPermissions/UpdatePermissions.Testing
go build ./...,go vet ./...,gofmt -l: clean.go test ./...: all packages pass.TestUnityCatalogRequestscaptures real requests through the client and asserts UC path assembly, dotted full-name preserved as a single path segment, query params, and the PATCH changes body.Operational note
The OAuth service principal must be assigned to each target workspace with rights to enumerate Unity Catalog securables and read grants; end-to-end sync was not run here (no live tenant).
Out of scope
Metastore-level resource and any tag-driven / just-in-time-elevation overlays are deliberately excluded.