Repository navigation
[CXH-2723] - Add Unity Catalog table and volume sync - Databricks Connector - #73
Conversation
920df46 to
f9142b4
Compare
0ee9040 to
38e403e
Compare
f9142b4 to
29274ee
Compare
38e403e to
7c8e7a3
Compare
29274ee to
e74210e
Compare
7c8e7a3 to
a3bd512
Compare
e74210e to
7deff05
Compare
a3bd512 to
5dce9b1
Compare
7deff05 to
1253d7e
Compare
5dce9b1 to
5effb3f
Compare
FeliLucero1
left a comment
FeliLucero1
left a comment
There was a problem hiding this comment.
Did a pass on this one. Main things: the per-table perf (the stack-wide one actually bites here), the List/Revoke 404 handling copied from #72, and the table_type mapping dropping REFRESH / ALL_PRIVILEGES grants from the sync. The rest are nits.
Comments tagged (out of scope) are inherited or stack-wide, so treat them as FYI, not blocking for this PR. Build/vet/tests pass locally.
FeliLucero1
left a comment
FeliLucero1
left a comment
There was a problem hiding this comment.
Round 5. Looks good, the main asks are fixed (children completeness proof, no silent empty list, revoke on dropped targets, route docs) and CI is green. Approvable from my side; the comments below are small follow-ups.
(out of scope, #71 catalog.go:270) built-in catalog skip:
__databricks_internal(INTERNAL_CATALOG, Lakeflow pipelines) isn't covered, so it'd fail the sync the same waysystemdid- the docs don't say
samplesisSYSTEM_CATALOG(could be Delta Sharing), so worth confirming - this commit duplicates logic already on #71's tip (
abee6af5), so watch the rebase
nits (outside the diff): schema.go:195 still says the schema List skips system (only the catalog syncer does), catalog_test.go:356 still says "never synced / deletes nothing", and the README schema-listing rule leaves out metastore owner + the admin path that docs-info has.
FeliLucero1
left a comment
FeliLucero1
left a comment
There was a problem hiding this comment.
Round 6. Looks good, CI green, and thanks for the walk/comment fixes. One thing I'd fix before merge: the RESOURCE_DOES_NOT_EXIST mapping on volumes (inline). I'm fine leaving the non-owner revoke path out as you said.
Leftovers outside the diff: schema.go:195 still says the schema List leaves system out (only the catalog syncer does). (out of scope, #71) the built-in skip still doesn't cover __databricks_internal (INTERNAL_CATALOG).
sergiocorral-conductorone
left a comment
sergiocorral-conductorone
left a comment
There was a problem hiding this comment.
Automated review (follow-up round on new commits).
Stats: drafted 3 → confirmed 3, low-confidence 0, dropped 0, added 0 (panel: opus-5.5 / gpt-6-astra / gemini-4-argon, all high).
Findings not anchorable to a diff line:
pkg/connector/catalog.go:419— [major]catalogThroughused to answer(nil, nil)for bothCATALOG_DOES_NOT_EXISTand a 401/403/404 deny, and every caller's walk treatedcatalog == nilas "try the next workspace". Now it returnserrCatalogGoneor the raw deny error, but not every caller was updated:pushSchemaChildRoute(schema_child.go:169-172) still doesif err != nil { return err }. A deny or a gone code onusable[0]ends the table/volume walk there.- schema
pushRoute(schema.go:305-311) continues on a deny, but returnserrCatalogGoneas a plain error.
This PR's own logic (workspaceForCatalog's allGone and confirmCatalogGone's "still exists but no workspace in sync scope can list it") assumes a workspace the catalog is not bound to answers CATALOG_DOES_NOT_EXIST. So for an ISOLATED catalog bound only to usable[1], the catalog syncer emits it through workspace B. Then the schema List and every table/volume List under it fail on workspace A, and the sync never completes on multi-workspace accounts. That undoes the "walk usable until one lists it" fix from the schema_child.go:163 thread. Suggest treating errors.Is(err, errCatalogGone) || deniesTrust(err) as continue in both walks, keeping lastDeny for the final error message the way schema.go now does. Please add a two-workspace test where wsa answers 404 CATALOG_DOES_NOT_EXIST and wsb serves the catalog, for both schemas and tables.
pkg/connector/catalog.go:316— [minor, perf] With theseendedupe gone, a catalog bound to W in-scope workspaces is emitted W times, and every OPEN catalog is bound to all of them. The store does collapse the catalog row on upsert. But the SDK syncer queues a child-resource sync for each resource a List returns, so the catalog's whole subtree is listed W times: schemas, then tables and volumes per schema. That subtree includes the uncached per-schema completeness proof (schemaListingIsComplete/childListingIsComplete) that the listSchemaChildren comment already calls quadratic. The live measurement here was a single-workspace tenant, so W=1 hides it. If the oldseenmap was dropped because it outlived a sync, a sync-safe alternative would be to emit an OPEN catalog only from the first route that serves it, or to carry the names already emitted in the bag state.
433e33c to
b704b02
Compare
d10a7e9 to
a03b5a6
Compare
b704b02 to
f63db64
Compare
a03b5a6 to
47a3e98
Compare
f63db64 to
068e05d
Compare
47a3e98 to
acfcba8
Compare
| newCatalogBuilder(d.client, d.clientID, workspaces, principals), | ||
| newSchemaBuilder(d.client, d.clientID, workspaces, principals), | ||
| newTableBuilder(d.client, d.clientID, workspaces, principals), | ||
| newVolumeBuilder(d.client, d.clientID, workspaces, principals), |
There was a problem hiding this comment.
O(schemas^2) API calls per catalog (schema_child.go:127-229). pushSchemaChildRoute runs on page 1 of every schema, once for tables and once for volumes. Each run lists every schema of the catalog through schemaThrough (50 per page) and repeats the uncached privilege probes. A catalog with 1,000 schemas makes about 2,000 x 20 schema-list pages, around 40k calls before any table is listed, which will hit rate limits. Fix: a single GET schemas/{catalog}.{schema}, or reuse the owner data the schema syncer already wrote onto the schema resource.
Per-table cost in Grants (securable_helpers.go, schema_child.go:356). Each table does its own permissions read, and a non-owner connector adds an uncached effective-privileges read per table. Entitlements run at roughly 10 per table (table.go:285). With 100k tables that is 200k+ extra calls per sync. Consider trusting the route-level proof carried in the profile rather than re-reading per resource.
I am a bit worried about the performance. Could you please confirm with it?
There was a problem hiding this comment.
You are right on the first one. Sergio flagged the same shape earlier in this PR and I answered that the uhttp GET cache kept it cheap. That was too comfortable: the cache is 5 MB shared by the whole sync, so it only holds while a catalog's entire schema listing fits in it.
Measured against a simulated tenant, one catalog with 1,000 schemas: 43,024 requests with the cache off (your ~40k is right), 5,027 warm, and with 30 KB schema rows the listing count goes from 4 to 108 at only 200 schemas, with the default cache untouched.
Fixed in 01bab61. schemaThrough now reads the one schema through GET /schemas/{catalog}.{schema}, the move catalogThrough already made. Linear in the schema count now. Error classification is unchanged and the branch that matters has a test: a 404 with no code still fails the sync instead of listing nothing.
Per-table cost: correct for a connector that does not own the metastore — 600 extra uncached effective-permissions reads over 200 schemas. This account owns the metastore, so it costs nothing today, and removing it needs the route memoized per catalog, which I would rather do separately.
Entitlements does not apply: tableBuilder.Entitlements reads table_type off the profile and makes no API call. The measurement produced 20,000 entitlements with 0 requests.
Listing a schema's tables or volumes resolved its route on the first page, once per child type, and schemaThrough found the schema by paging the whole catalog's schema listing. That is one full listing walk per schema per child type: a catalog with 1,000 schemas spends about 21,000 listing requests, and 43,000 requests in all, before the first table is listed. The uhttp GET cache hid it while a catalog's listing fit in the 5 MB it holds by default, which a catalog of 30 KB schema rows does not. schemaThrough now reads the one schema it needs. The same catalog costs 2,000 schema reads instead, and the cost is linear in the schema count rather than quadratic. Error classification is unchanged. A 404 naming SCHEMA_DOES_NOT_EXIST is read exactly like a schema absent from the listing was: proof of deletion only for a caller that sees the whole catalog, since Databricks answers the same way for a schema the caller merely cannot see. Every other refusal, including a 404 carrying no code, still leaves the schema unknown rather than absent -- read as an empty result the SDK would skip the children and ConductorOne would delete every table and volume under the schema. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Listing a schema's tables or volumes returned the catalog read's error for every refusal, where the schema syncer walks on to the next workspace. A catalog bound to one workspace of several is answered by the others with a 404, and which of the two answers Databricks gives decides which way this breaks. A 404 carrying no error code reached the SDK as codes.NotFound, which it reads as a warning: the listing finished green with no resources and ConductorOne deleted every table and volume under the schema. A 404 naming CATALOG_DOES_NOT_EXIST raised errCatalogGone, which failed the whole sync. Both now continue the walk, matching the schema syncer. A catalog every workspace names as gone, with none refusing the read, empties the listing: the catalog syncer drops it in the same sync. A refusal still fails the sync and keeps what ConductorOne already has. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
FeliLucero1
left a comment
FeliLucero1
left a comment
There was a problem hiding this comment.
Round 7. Nice: the single-schema GET fixes the O(schemas²), the walks now continue (with two-workspace tests), and volumes confirm the generic 404. CI green. One thing I'd still want before merge: the gone codes trusted from a single workspace (inline on volume.go:61/81).
(out of scope, #71) the catalog is still emitted once per bound workspace (catalog.go:225-238, no dedupe), so its whole subtree gets listed W times, Sergio's perf point. FYI on the trait split: per c1 (uplift_app.go), only TRAIT_APP resources become child apps, so not having it on 100k tables is actually a plus. Dropping it from all five like you asked ggreer sounds good to me.
| trusts: volumeGrantsAreTrustworthy, | ||
| // Dropping a parent drops the volume with it, and both parent codes name | ||
| // what is missing. The generic code does not, so it is confirmed first. | ||
| goneErrorCodes: []string{errorCodeSchemaGone, errorCodeCatalogGone}, |
There was a problem hiding this comment.
[medium] CATALOG_DOES_NOT_EXIST is taken as proof here (and in table.go:164) that the target is gone → GrantAlreadyRevoked on revoke, zero grants on Grants. But the catalog walk assumes the opposite: an unbound workspace answers that same code, which is why the catalog level runs confirmCatalogGone with include_unbound=true before believing it. Here we read through the workspace saved in the profile with no cross-check. So if the binding changed after the sync, or that workspace stops serving the catalog, the revoke "succeeds" while access stays, and Grants empties the table/volume in C1. Suggest routing CATALOG_DOES_NOT_EXIST through confirmCatalogGone (or an equivalent cross-workspace check) the same way the catalog level does.
| if err == nil { | ||
| return volume == nil, nil | ||
| } | ||
| if databricks.IsSecurableGone(err, errorCodeGenericGone, errorCodeSchemaGone, errorCodeCatalogGone) { |
There was a problem hiding this comment.
[medium, same root cause] the confirm read goes through the same workspace and also accepts RESOURCE_DOES_NOT_EXIST. Per your own measurement, GET volumes/…nope gives that same generic 404, so for a route miss (a workspace that can't serve the volume) the confirmation proves nothing. Maybe confirm the parent first through the same workspace (GET /schemas/{catalog}.{schema} → 200) and only then trust a volume 404. Or drop the generic code here and keep just the schema/catalog codes (with the catalog caveat above). Also no test covers these paths yet: generic + 404 = gone, generic + 200/500 = error, Grants on a dropped volume.
| "id": "table", | ||
| "displayName": "Table", | ||
| "traits": [ | ||
| "TRAIT_APP" |
There was a problem hiding this comment.
nit: still says TRAIT_APP for table (and volume at :502), but resource_types.go dropped the trait. The capabilities workflow only regenerates on push to main, so CI won't catch it here. Worth regenerating so the file/docs match.
| const childListRemedy = "the connector's service principal must own the metastore, the catalog or the schema, " + | ||
| "or hold ALL_PRIVILEGES on the catalog" | ||
|
|
||
| // pushSchemaChildRoute records a workspace that can list every table or volume |
There was a problem hiding this comment.
nit: comment's stale after 101ce0f. It still says an error from a workspace stops the walk and that an empty result is an error, but catalogThrough denies/gone now continue, and all-gone returns nil (:253). Also a deny from schemaThrough (:213) still stops the walk instead of trying the next workspace. Fails safe, just inconsistent with the catalog step.
Description
Last of four stacked PRs for Unity Catalog (CXH-2723). Tables and volumes are the data assets an access review is about. They are siblings under a schema.
#70 is merged. #71 and #72 merge before this one.
What the connector syncs
Before, with #72 applied. The schema is a leaf.
After this PR. The tree reaches the data assets.
staginglanded onmainmainSync:
table, NEW) — child ofschema. Unity Catalog uses onetablesecurable for tables, views, materialized views, metric views and streaming tables.table_typepicks the privilege set, so entitlements are per instance.volume, NEW) — child ofschema, read withGET /api/2.1/unity-catalog/volumes. The privilege set is static, plus a read-onlyowner.table_typedecides which privileges the connector offers on each table:Provisioning:
table_type, so ConductorOne does not offer an entitlement the grants API rejects.ALL_PRIVILEGESis emitted when Databricks already holds it and is not requestable. Grant returnsInvalidArgumentbefore any PATCH. Revoke still removes it.REFRESH.REFRESHstays requestable.SELECT,READ_METADATA,APPLY_TAG,MANAGE,ALL_PRIVILEGES,REFRESHandMODIFY.REFRESHstays requestable. The table DML trio (DELETE,INSERT,UPDATE) is not offered.MANAGED,EXTERNAL,MANAGED_SHALLOW_CLONEandEXTERNAL_SHALLOW_CLONE: the table set, which does not includeREFRESH.FOREIGN: read-only.DELETE,INSERT,UPDATEandMODIFYare not offered.table_type: the union of every variant, so a heldREFRESHis not dropped. ThatREFRESHis not requestable, and Grant returnsInvalidArgumentbefore any PATCH.PATCHpath and the same echo-confirmed idempotency:GrantAlreadyExistsandGrantAlreadyRevoked.TABLE_DOES_NOT_EXIST,SCHEMA_DOES_NOT_EXIST,CATALOG_DOES_NOT_EXIST) isGrantAlreadyRevoked. Any other 404 still fails, because an unbound or unprivileged workspace answers 404 too.RESOURCE_DOES_NOT_EXISTfor a missing volume, a missing schema and a missing catalog alike, andVOLUME_DOES_NOT_EXISTis never returned at all. A workspace that cannot serve the volume answers the same generic code, so on its own it proves nothing: taking it as proof let a routing miss reportGrantAlreadyRevokedwhile the privilege was still held. The volume level now names that code as ambiguous and confirms it withGET /api/2.1/unity-catalog/volumes/{full_name}through the same workspace, which answers 200 while the volume exists. Only a second absence counts, and a confirmation that does not answer leaves it unproven.Verified end to end: a revoke on a volume that does not exist answers
GrantAlreadyRevokedand sends no PATCH, and a revoke on a volume that exists is never reported gone.Auth:
Credentials are unchanged. There is no new config field.
The schema listing from #72 decides what this PR can read. Schema List fails with
PermissionDeniedunless that listing is complete, and tables and volumes are read through the same workspace.information_schemais skipped.A direct Admin is enough when the catalog is owned by that workspace's admins group. The connector reads the workspace id off the group name
_workspace_admins_<workspace>_<id>and checks that workspace, whatever route the securable is listed through. That rule arrived from #71 and applies to the table and volume trust rules here as well.Architecture highlights:
omit_columnsandomit_properties. It does not sendomit_username, because that one also dropsowner, and every securable's profile and owner grant depend on it. The client test asserts the exact query set, so adding it fails there.max_resultsabove 50. The client clamps to 50.holdsPrivilegeOnis the one uncached read of the connector's own effective privileges. Each level passes the privileges it accepts and the error codes that prove the securable is gone. The table and volume probes carry the codes of the securable they read, so a 404 naming a missing parent is an absence rather than a privilege the caller cannot see.securableLevelseparates a code that proves an absence from one that only suggests it. A level may nameambiguousGoneCodesand supply the read that settles them, which is what the volume level does. Both shared call sites go through it, soGrantsno longer reports an empty volume on the generic code either.tableandvolumecarry no trait. They hold a profile and no app semantics, and a profile goes on any resource now.tablekeeps per-instanceEntitlements()because its privilege set followstable_type, so it must not skip entitlements;volumehas a static set and does.Find*filters. A 400 on the owner SCIM filter continues to the next principal kind: user, then group, then service principal. A 500 fails Grants. The rejection is logged at Debug.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.