[mateoHernandez123]

Description

• Bug fix
• New feature

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.

flowchart TD
  account[account]
  metastore[metastore]
  catalog[catalog]
  schema[schema]
  other["user, group, service principal, role, workspace"]
  account --> other
  account --> metastore
  metastore --> catalog
  catalog --> schema

Loading

After this PR. The tree reaches the data assets.

flowchart TD
  account[account]
  metastore[metastore]
  catalog[catalog]
  schema[schema]
  table[table]
  volume[volume]
  other["user, group, service principal, role, workspace"]
  account --> other
  account --> metastore
  metastore --> catalog
  catalog --> schema
  schema --> table
  schema --> volume
  style table fill:#1f6feb,color:#fff,stroke:#1f6feb
  style volume fill:#1f6feb,color:#fff,stroke:#1f6feb

Loading

Order	PR	Delivers	Base	State
1	#70	Client, routing, metastore	staging	Merged
2	#71	Catalog	staging	Open
3	#72	Schema	#71	Open
4	#73	Table and volume	#72	This PR

Sync:

• Tables (table, NEW) — child of schema. Unity Catalog uses one table securable for tables, views, materialized views, metric views and streaming tables. table_type picks the privilege set, so entitlements are per instance.
• Volumes (volume, NEW) — child of schema, read with GET /api/2.1/unity-catalog/volumes. The privilege set is static, plus a read-only owner.
• The types above are unchanged from [CXH-2720] - Add Unity Catalog client, access-path routing, and metastore sync - Databricks Connector #70, [CXH-2721] - Add Unity Catalog catalog sync - Databricks Connector #71 and [CXH-2722] - Add Unity Catalog schema sync - Databricks Connector #72.

table_type decides which privileges the connector offers on each table:

flowchart TD
  table[table securable]
  table --> managed[MANAGED / EXTERNAL / shallow clones]
  table --> view[VIEW / METRIC_VIEW]
  table --> mv[MATERIALIZED_VIEW]
  table --> st[STREAMING_TABLE]
  table --> foreign[FOREIGN]
  table --> empty[missing or unrecognised table_type]
  managed --> tpriv[table privileges]
  view --> vpriv["view set; ALL_PRIVILEGES is not requestable"]
  mv --> mvpriv["view set + REFRESH, requestable"]
  st --> stpriv["SELECT, READ_METADATA, APPLY_TAG, MANAGE, ALL_PRIVILEGES, REFRESH, MODIFY; REFRESH requestable"]
  foreign --> fpriv["read-only; no DELETE, INSERT, UPDATE, or MODIFY"]
  empty --> anypriv["union; REFRESH is not requestable"]

Loading

Provisioning:

• Grant/Revoke table privileges — the requestable set follows table_type, so ConductorOne does not offer an entitlement the grants API rejects.
  • VIEW / METRIC_VIEW: the view set. ALL_PRIVILEGES is emitted when Databricks already holds it and is not requestable. Grant returns InvalidArgument before any PATCH. Revoke still removes it.
  • MATERIALIZED_VIEW: the view set plus REFRESH. REFRESH stays requestable.
  • STREAMING_TABLE: SELECT, READ_METADATA, APPLY_TAG, MANAGE, ALL_PRIVILEGES, REFRESH and MODIFY. REFRESH stays requestable. The table DML trio (DELETE, INSERT, UPDATE) is not offered.
  • MANAGED, EXTERNAL, MANAGED_SHALLOW_CLONE and EXTERNAL_SHALLOW_CLONE: the table set, which does not include REFRESH.
  • FOREIGN: read-only. DELETE, INSERT, UPDATE and MODIFY are not offered.
  • Missing or unrecognised table_type: the union of every variant, so a held REFRESH is not dropped. That REFRESH is not requestable, and Grant returns InvalidArgument before any PATCH.
• Grant/Revoke volume privileges — the same PATCH path and the same echo-confirmed idempotency: GrantAlreadyExists and GrantAlreadyRevoked.
• Owner is read-only.
• A revoke whose table, volume, schema or catalog Databricks reports as gone (TABLE_DOES_NOT_EXIST, VOLUME_DOES_NOT_EXIST, SCHEMA_DOES_NOT_EXIST, CATALOG_DOES_NOT_EXIST) is GrantAlreadyRevoked. Any other 404 still fails, because an unbound or unprivileged workspace answers 404 too.

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 PermissionDenied unless that listing is complete, and tables and volumes are read through the same workspace. information_schema is 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:

• Table listing sends omit_columns and omit_properties. It does not send omit_username, because that one also drops owner, and every securable's profile and owner grant depend on it. The client test asserts the exact query set, so adding it fails there.
• The tables endpoint rejects max_results above 50. The client clamps to 50.
• Shared trust probe: holdsPrivilegeOn is 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 levels name none, so every 404 there leaves the privilege unproven rather than claiming an absence.
• Table and volume Grants use the same principal path as [CXH-2722] - Add Unity Catalog schema sync - Databricks Connector #72. They read the full account SCIM only when assignments exist, and an owner-only page uses quoted 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:

• Linear ticket CXH-2723 — table and volume sync
• List tables
• List volumes
• Unity Catalog privileges
• Update permissions on a securable

[linear-code]

CXH-2723

[FeliLucero1]

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]

[FeliLucero1]

Round 4. Nice rebuild: the generic securable flow + route in the page token fixes most of what we flagged (view ALL_PRIVILEGES, table_type sets, perf on the Grants side). Two things below I'd want before merge: the children completeness proof (:186) and the empty list when usable[0] doesn't see the catalog (:163). Also the revoke-on-dropped mapping got lost in the rebuild (replied on that thread).

(out of scope, #71/#72) CI test is red here, on #71 and on #72: cannot list every schema in catalog system (built-in catalog, exists on every tenant), and on #71 cannot read every privilege on catalog test_workspace. Fail-closed is the right call per c1, but right now any SP that isn't metastore owner/admin aborts the whole sync, including the types that already worked. Probably worth skipping system and fixing the CI tenant perms down in #71/#72.

[sergiocorral-conductorone]

Automated review (follow-up/first pass) — drafted 3 → confirmed 3, low-confidence 0, dropped 0, added 0 (panel: opus-5.5 / gpt-6-astra / gemini-4-argon, all high).

[FeliLucero1]

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 way system did
• the docs don't say samples is SYSTEM_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.

[ggreer]

Are all the new resource types apps? I think it's fine if they have no trait. A profile can go on any resource now.

[ggreer]

This was a little hard for me to follow, but it might be simpler to change these to:

	volumeResourceType = &v2.ResourceType{
		Id:          "volume",
		DisplayName: "Volume",
		Traits:      []v2.ResourceType_Trait{v2.ResourceType_TRAIT_APP},
		Annotations: annotations.New(
			&v2.OptInRequired{},
			&v2.SkipEntitlements{},
			capabilityPermissions(securablePermissions(permissionUseCatalog, permissionUseSchema)...),
		),
	}

That way anyone reading the code can quickly tell which resource types are opt in, which ones skip entitlements/grants, etc.

[ggreer]

Nitpick: I don't think this comment is necessary. Child resource types always work this way.

[FeliLucero1]

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).

[FeliLucero1]

[medium] careful with this one. RESOURCE_DOES_NOT_EXIST is the generic 404 code (error codes), and our own tests treat it as a route miss that must still fail: table_test.go:417 ("a bare 404 also comes back from a workspace the table is not reachable through, where the privilege is still held") and catalog_test.go:781. For volumes it now means GrantAlreadyRevoked with no PATCH. So if a stale/unreachable workspace answers with it, the revoke "succeeds" and the access stays. Since f2fca90 there's also no volume revoke test. Suggest: on RESOURCE_DOES_NOT_EXIST, confirm with GET /unity-catalog/volumes/{full_name} through the same workspace and only treat it as gone if that 404s too (200 = route problem → error). Plus a test for both cases.

[FeliLucero1]

nit: VOLUME_DOES_NOT_EXIST isn't in the documented error_code enum (TABLE/SCHEMA/CATALOG/METASTORE_DOES_NOT_EXIST are, and so is generic RESOURCE_DOES_NOT_EXIST), so this is probably never returned. Harmless, but the comment makes it sound real.

[sergiocorral-conductorone]

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] catalogThrough used to answer (nil, nil) for both CATALOG_DOES_NOT_EXIST and a 401/403/404 deny, and every caller's walk treated catalog == nil as "try the next workspace". Now it returns errCatalogGone or the raw deny error, but not every caller was updated:
• pushSchemaChildRoute (schema_child.go:169-172) still does if err != nil { return err }. A deny or a gone code on usable[0] ends the table/volume walk there.
• schema pushRoute (schema.go:305-311) continues on a deny, but returns errCatalogGone as 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 the seen dedupe 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 old seen map 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.

[sergiocorral-conductorone]

[major] RESOURCE_DOES_NOT_EXIST is Databricks' generic 404 code, not a volume-specific one. Adding it to volumeLevel.goneErrorCodes now has two effects. In revokeSecurable, any 404 carrying it answers GrantAlreadyRevoked. In the new syncSecurableGrants branch (securable_helpers.go:168-175), it returns zero grants with a nil error. The PR's stated rule is "any other 404 still fails, because an unbound or unprivileged workspace answers 404 too". This code is the one most likely to be shared with those non-deletion 404s, such as a volume read through a workspace that cannot serve it, or a routing miss. When that happens, Revoke tells C1 the access is gone while it stays in Databricks, and Grants silently reports an empty volume. Measuring "missing volume → RESOURCE_DOES_NOT_EXIST" on one tenant shows the code is necessary, not that it is sufficient. Suggest: on RESOURCE_DOES_NOT_EXIST, confirm with GET /api/2.1/unity-catalog/volumes/{full_name}, and treat the volume as gone only when that answers VOLUME_DOES_NOT_EXIST (or the parent codes). Otherwise refuse the read. Please also add a volume Revoke test and a Grants test for this code. Neither path is tested in this diff.