Repository navigation
Land staging on main - Databricks Connector - #91
Open
mateoHernandez123 wants to merge 43 commits into
Open
mateoHernandez123 wants to merge 43 commits into
mateoHernandez123 wants to merge 43 commits into
Conversation
…and metastore sync Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
…cope routing to the configured workspaces The uhttp response cache holds a GET 200 for an hour and a PATCH does not invalidate it, so Grant and Revoke could decide not to write against a page that predated the last change: Grant X then Revoke X in the same process left the principal with access while C1 recorded it as revoked. Both pre-reads and the post-PATCH confirmation now bypass the cache. Unity Catalog routing ignored the workspaces allowlist, because ListWorkspaces only applies the exclude list, so a securable could be read through a deployment the operator had scoped out of the sync. A workspace the credential cannot read answered 401 or 403 out of buildRouting, and only codes.NotFound is a warning to the SDK, so one unreadable workspace failed the whole sync including the resource types that already worked. It is now set aside the way a 404 already was. Also: a revoke against a deleted metastore reports GrantAlreadyRevoked rather than failing forever, the 403 remedy no longer names MANAGE (not assignable on a metastore), and the connector's own service principal is resolved with an applicationId filter instead of a full account SCIM walk. Co-authored-by: Cursor <cursoragent@cursor.com>
…ut of scope Filtering routing by the workspaces allowlist left a metastore that only an out-of-scope workspace is attached to still listed on the account but with no access path, so usableWorkspaces answered FailedPrecondition and, since only codes.NotFound is a warning to the SDK, scoping a sync to one workspace failed it for every metastore the others hold. Such a metastore now reads as absent, the way a filtered catalog already does, and List emits from the routing snapshot so it cannot advertise a metastore whose grants cannot be read. The remedy text names the allowlist alongside the exclude list. Co-authored-by: Cursor <cursoragent@cursor.com>
…is unreadable Dropping an unrouted metastore under a workspace allowlist assumed the only reason it could be unrouted was the workspaces left out. An in-scope workspace that answers neither its metastore assignment nor a catalog listing may be the one serving it, so the metastore would read as absent and C1 would delete it and every grant under it. Failing the sync is recoverable; that is not. fillWorkspaces now records those workspaces separately from the ones whose empty catalog listing settles that they carry nothing, and the prune only runs when none of them is unknown. Co-authored-by: Cursor <cursoragent@cursor.com>
…ed workspace serves nothing The catalogs endpoint returns only the catalogs the caller may use, so an empty listing from a credential that was also refused the workspace's metastore assignment is equally explained by a missing USE_CATALOG. That pair settled nothing, yet it left the workspace out of the unreadable set, so the allowlist prune still ran and a metastore the workspace does serve would have been reported deleted. Only a 404 on the assignment together with an empty listing now reads as a workspace outside Unity Catalog. A 401 or 403 there keeps the workspace unreadable and the metastore listed. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
…t expansion Co-authored-by: Cursor <cursoragent@cursor.com>
…esolve principals per id, and share securable provisioning Co-authored-by: Cursor <cursoragent@cursor.com>
…esh the permissions cache after a patch Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
…s and record the readable workspace Co-authored-by: Cursor <cursoragent@cursor.com>
…ip a failed profile probe Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
…nd remove the duplicate workspace probe Co-authored-by: Cursor <cursoragent@cursor.com>
… syncer Co-authored-by: Cursor <cursoragent@cursor.com>
…d and skip terminal workspaces Co-authored-by: Cursor <cursoragent@cursor.com>
…ector cannot fully read Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Both sides fixed the uhttp response cache for different reasons: this branch keys it on Host so two workspace deployments answering the same path keep separate entries, and main keys it on Cache-Control so a read-before-write no-cache read does not overwrite the shared sync entry. WithCacheKeyHeaders is variadic, so the merged client keys on both. main added getNoCache and doRequestWithOptions to reach uhttp.WithNoCache(). This branch already routes request options through doRequest and exposes GetUncached, so the merge keeps that shape and points the workspace assignment read at GetUncached. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ation branch The previous commit carried main's content but lost the second parent, so GitHub still saw the branch as conflicting. The tree is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Bring in compound resource id parsing and group workspace principals. The rule-set tests now expect the quoted SCIM filters this branch sends. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
A partial permissions read looks like an empty grant list, so List and provisioning pick the first running in-scope workspace where the connector owns the metastore or is a direct Admin. Co-authored-by: Cursor <cursoragent@cursor.com>
Keep the quoted SCIM filters and route group reads through GetUncached. Account group writes now patch, so the full-replacement displayName guard is gone. Co-authored-by: Cursor <cursoragent@cursor.com>
…B response limit (#81) * Paginate group and workspace member grants to stay under Lambda payload limit Databricks returns every group member in a single Get group response and has no member pagination. Grants() emitted one grant per member in one response, so a group with ~11k+ members (e.g. an all-users account group) exceeded the 6MB Lambda response limit and failed with HTTP 413 on every retry. Page members locally (GrantsPageSize = 2000, ~1.1MB per page) using the page token as a member offset, and emit role rule-set grants on the first page only. Databricks returns members in a different order on every call, so they are sorted before slicing so pages line up across re-fetches in separate Lambda invocations. Apply the same paging to workspace assignment grants. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Page member grants by last emitted key instead of offset An offset page token into a list that is re-fetched for every page drifts when members are added or removed between page invocations: a removal skips the first member of the next page (C1 reads that grant as revoked) and an addition grants one member twice. Store the key of the last emitted item (member Ref for groups, principal ID for workspaces) and start the next page after it, so membership churn no longer shifts page boundaries. Add churn tests for both paths, a workspace multi-page test, and mark the test-only shuffle as not security sensitive for gosec. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
…anch Co-authored-by: Cursor <cursoragent@cursor.com>
* feat: incremental sync v1 - scoped implementation * feat: extend incremental sync to role/permission-assignment changes Add audit log action mappings for account-admin, workspace-access, and SQL-access role changes, plus a coarser fallback for the cluster-create and instance-pool-create entitlements. mapAuditRowToResource now returns multiple affected resources per audit row so a single action can refresh both a principal and the role(s) it holds. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: add event_id tiebreaker to audit log pagination ORDER BY event_time ASC alone gives no deterministic ordering among rows sharing an event_time, so paging via a >= start_time filter plus a remembered ID set can stall forever if a single event_time has >= auditLogPageLimit rows. Order by (event_time, event_id) and page with a composite (event_time, event_id) > predicate instead, so the cursor always advances regardless of how many rows share a timestamp. * fix: bound sql statement polling and cancel on give-up pollStatement could block for as long as the caller's context allowed if a warehouse got stuck PENDING/RUNNING (cold start, queued, quota), hanging Validate() indefinitely when incremental sync is enabled. Cap polling at statementPollMaxWait and cancel the statement via DELETE when giving up so it stops occupying the warehouse. * fix: nil-safe parent access in user/service-principal/role Get The SDK's GetResource passes request.GetParentResourceId() straight through to Get, which is nil whenever C1 has no parent recorded for the resource (e.g. workspace-token auth, where userResource deliberately omits WithParentResourceID). That nil parent was then dereferenced directly in userResource, servicePrincipalResource, and roleResource, panicking on resync after a RESOURCE_CHANGE event. Use the nil-safe GetResourceType()/GetResource() getters instead, matching the pattern groups.go already used. * fix: mirror token-auth branch in workspaceBuilder.Get Get always called GetWorkspace, which hits the Account API via ListWorkspaces. The Account API is unreachable under workspace-token auth, so any targeted sync of a workspace (advertised via CAPABILITY_TARGETED_SYNC) always failed in that mode, even though List already builds a minimalWorkspaceResource from the configured workspace list to avoid the same call. Get now mirrors that branch. * fix: select audit event parent by Account API availability mapAuditRowToResource picked the user/group/service-principal parent from whether the audit row carried a workspace_id, not from how the resource is actually synced. Users/groups/service principals are only ever synced under the account when the Account API is reachable (accountResource declares them as children only then; groupGrantParent already encodes this rule), so a workspace-scoped row in that mode built an ID that was never synced (e.g. workspace/<deployment>/group/x instead of account/<id>/group/x), and the real resource never got refreshed. Select the parent from IsAccountAPIAvailable() instead, matching groupGrantParent. Also thread the real ctx through instead of context.Background(), now that groupResourceId's ctx parameter is actually used for something worth passing correctly. * fix: don't require the Account API for incremental sync under token auth Validate() and ListEvents() both called ListWorkspaces unconditionally to resolve the audit-log query workspace, but the Account API is unreachable under workspace-token auth — so incremental sync always failed Validate() and every ListEvents poll for token-auth customers. Add resolveSQLWorkspaces, which builds minimal workspaces from the configured deployment names under token auth (mirroring workspaceBuilder.List's token-auth branch) instead of calling ListWorkspaces, and use it in both places. auditEventFeed now carries the configured workspace list to support this. Workspace-scoped audit rows can't be resolved to a deployment name under token auth this way (no numeric workspace ID is ever learned), so they're skipped by mapAuditRowToResource rather than mis-resolved — a known limitation, not a regression, since incremental sync couldn't run under token auth at all before this. * refactor: simplify quotedInClause and ExecuteStatement's status check quotedInClause hand-rolled a strings.Join; use it directly. The two-case switch in ExecuteStatement (success vs everything else) reads clearer as a plain if. Cosmetic only, per review feedback. * feat: add sql-warehouse-workspace to pin the audit-log query workspace sqlQueryWorkspace picked whichever workspace sorted alphabetically first by deployment name to run the system.access.audit query, with no way to route it to the workspace that actually hosts sql-warehouse-id. SQL warehouses only exist in one workspace, so querying through the wrong one 404s — this broke deterministically for any account with more than one workspace, unless the warehouse happened to live in the alphabetically-smallest one. Add --sql-warehouse-workspace to pin the deployment name explicitly, validated against the resolved workspace list in both Validate() and ListEvents() via a shared resolveQueryWorkspace helper. When unset and more than one workspace is available, log a Debug line naming the arbitrarily-picked workspace and pointing at the new flag, so it's diagnosable without escalating to Warn/Error for what is, until set, a config gap rather than a connector fault. Regenerated config_schema.json and pkg/config/conf.gen.go; updated README's incremental sync section and flag list. * fix: address remaining audit-log review findings - Use RFC3339Nano for start_time so the (event_time, event_id) tiebreaker keeps sub-second precision, fixing duplicate re-emission at page boundaries. - Reject enable-incremental-sync under workspace token auth at Validate(), since no audit row can ever resolve to a synced resource that way and it would otherwise poll forever for nothing. - Add the three incremental-sync config fields to both field groups so they're selectable in the UI; regenerate config_schema.json. - Propagate rate-limit info from the SQL Statement Execution API through ExecuteStatement into ListEvents' annotations. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: log corrupt event cursors instead of resetting silently decodeEventCursor now logs at Debug when a cursor fails to decode, so the watermark reset is observable instead of invisible. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore: promote Debug logs to Warn for visibility * chore: retrigger GitHub jobs * fix: drop incremental-sync fields from the workspace-token config group Validate() rejects enable-incremental-sync under token auth, so offering those fields in the workspace-token group let the UI present an option that can never succeed. Remove them from that group (they stay in oauth2) and regenerate config_schema.json; note the OAuth-only requirement in the README. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: address remaining audit-log review findings (round 2) - Filter resolveSQLWorkspaces through --workspaces on the OAuth2 path, and mirror the same allowlist check in workspaceBuilder.Get. - Clamp intra-page cursor advances to the trailing-lag boundary so a full page of recent rows can't permanently defeat auditLogTrailingLag. - Trust an explicit earliestEvent watermark instead of clamping it to the 1h lookback default. - Bound Validate()'s audit-log probe with a dedicated timeout. - Validate parsed audit-log rows have enough columns before indexing. - Cancel timed-out SQL statements via POST .../cancel instead of DELETE, which only closes an already-terminal statement. - Note the OAuth2-only requirement for incremental sync in the docs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: address remaining audit-log review findings (round 3) - Key the audit action-name map by (service_name, action_name) instead of action_name alone, since action_name is ambiguous across services (e.g. "delete" also means cluster termination). Fixes the map along the way: adds addPrincipalsToGroup/removePrincipalsFromGroup, renames deleteGroup -> removeGroup and createUser/deleteUser -> add/delete, and stops attaching the account-admin role to removeAdmin (which actually revokes workspace admin). - Drop the write-only LatestEventSeen cursor field. - Detect a stale-but-valid cursor (older than system.access.audit's 365-day retention) and self-heal it like a corrupt one; keep routine corrupt/missing-cursor logging at Debug and log genuine staleness at Warn. - Skip and log an individual malformed audit-log row instead of failing the whole page. - Add regression coverage: a >page-limit tied-timestamp burst, an end-to-end mocked ListEvents run (also covering the service_name filter and rate-limit propagation), and a skip-malformed-row test. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: infer the SQL warehouse's workspace instead of requiring it in config sql-warehouse-workspace forced operators to manually pin the workspace hosting the configured SQL warehouse, and was unenforced for the multi-workspace case it existed to cover (see the resolveQueryWorkspace Debug-log-and-guess fallback this replaces). Databricks has no account-level "which workspace owns this warehouse" API, but each workspace's SQL warehouse endpoint is queryable directly, so the connector now probes each candidate workspace for the configured sql-warehouse-id and caches the match instead of asking for it up front. - Add Client.WarehouseExists to probe a single workspace for a warehouse ID, distinguishing 404 from a real error. - Replace resolveQueryWorkspace's pin-or-guess logic with resolveWarehouseWorkspace, which skips probing entirely for a single workspace and otherwise probes until one workspace confirms the warehouse; the resolved workspace is cached per auditEventFeed instance so it's found once, not on every poll. - Remove the sql-warehouse-workspace config field, its config_schema.json entry, and its README documentation/CLI help text. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: stop fetching group members needlessly in groupBuilder.Get Get is only used to rebuild the resource, which needs id/displayName/ parent, not membership — Grants already fetches members separately when building membership grants. Matches List, which already fetches without members. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore: clean up and comments * feat: emit CREATE_GRANT/CREATE_REVOKE for group membership audit events Group membership actions (addPrincipalToGroup, removePrincipalFromGroup, and their plural batch variants) are atomic, unambiguous grant/revoke events per Databricks' own audit log docs: the group is the entitlement's resource, targetUserId is the principal, and add/remove gives the direction. Emitting CREATE_GRANT/CREATE_REVOKE for these lets C1 apply the grant directly instead of waiting on a targeted resync. Other assignment-shaped actions (setAdmin/removeAdmin, the roleNames-driven update* actions, ACL-change actions) stay RESOURCE_CHANGE: their scope or direction isn't fully determinable from the audit row alone, and guessing would risk writing an incorrect grant straight into C1's data model. EventFeedMetadata now advertises all three event types. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: bound the audit log query by lag instead of clamping the cursor advanceEventCursor tried to enforce auditLogTrailingLag by overwriting the cursor with a synthetic now-lag timestamp after the fact, which caused three bugs: it could jump StartAt backwards past where paging already was (no floor against the incoming cursor), it wiped the (event_time, event_id) tiebreaker whenever it fired (re-emitting the whole trailing window every poll), and hasMore was computed from the post-parse row count so one malformed row on a full page caused premature draining. Bound the query itself by lag_cutoff (event_time <= :lag_cutoff) instead. Every row queryAuditLog can return is now already within the lag window, so the cursor only ever advances to a real row's boundary (hasMore) or jumps straight to lag_cutoff once drained (proven empty beyond it) - no synthetic timestamp, no clamp, no floor needed. hasMore now uses the raw statement row count. Also guards against querying an inverted range when the cursor is already past lag_cutoff (current gap: auditLogLookback < auditLogTrailingLag on bootstrap). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: remaining audit-log review findings (bootstrap lookback, log levels, exclude check) - auditLogLookback now equals auditLogTrailingLag instead of a narrower 1h default, so a fresh install's first poll isn't immediately past lagCutoff (which would otherwise skip every query until it caught up). - Downgrade the 4 remaining skip-and-continue logs from Warn to Debug (stale cursor, malformed row, statement poll timeout, cancel failure) - these are graceful degradation, not connector bugs, and Warn surfaces as a C1 incident. - workspaceBuilder.Get's token-auth branch now also checks IsWorkspaceNameExcluded, mirroring List, so a workspace in both --workspaces and --databricks-exclude-workspaces is rejected consistently by both methods. - Comment out createServicePrincipal/updateServicePrincipal/ deleteServicePrincipal and setAccountAdmin/removeAccountAdmin in auditLogActions: none of these are real action_name values per the official Databricks audit log reference (verified against the AWS, GCP, and Azure docs mirrors) - setAdmin already covers account-admin grants, and the only documented service-principal action is changeServicePrincipalAcls. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: address PR comments * fix: resolve group-membership grant principal type dynamically principalType was hardcoded to userResourceType for the four addPrincipalToGroup/removePrincipalFromGroup (singular and plural) grant mappings, but request_params.targetUserId can also be a nested group or a service principal - the audit log has no field to tell which. A wrong guess silently fails C1's grant lookup rather than corrupting data, but the connector already knows how to sync all three principal types, so it's worth resolving properly. Add resolvePrincipalType: probes GetUser, then GetGroup, then GetServicePrincipal (in that order, stopping at the first match) using the same 404-detection idiom as the existing WarehouseExists probe. A real (non-404) API error propagates up and is logged/skipped per-row rather than aborting the whole poll; rate-limit info is still recorded even on a skipped row. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: stop advanceEventCursor stall and mistranslated nested-group principal id advanceEventCursor's panic fix regressed into a stall: when hasMore is true but only the last raw row fails to parse, lastRawBoundary is zero, and the old fallback returned the cursor unchanged while ListEvents still reported HasMore true - the SDK would re-issue the identical query forever, even though the other rows on that page parsed fine. Fall back to the last successfully parsed row first, and only stop claiming HasMore when neither source can advance. Also log (instead of discarding) parseEventBoundary's error on the raw last-row extraction, so a stuck cursor is diagnosable. Separately, a grant-mapped principal that resolves to a nested group was emitted with its raw native id instead of the composite id (account/workspace + group) every other group resource in this connector uses - apply the same groupResourceId transform Grants() uses for nested-group members, so the principal actually matches what was synced. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore: fix lint errors * feat: enable incremental sync via sql-warehouse-id, drop enable-incremental-sync Incremental sync is now on exactly when sql-warehouse-id is set (whitespace counts as unset) and off when it is empty, removing the separate enable-incremental-sync flag. - config: remove EnableIncrementalSyncField; sql-warehouse-id description leads with the on/off rule; regenerate conf.gen.go and config_schema.json - Validate: no ID skips all incremental-sync checks; a set ID fails validation if no workspace is in sync scope, the warehouse can't be located, or system.access.audit can't be queried. Logs whether incremental sync is enabled - audit feed: gated on sqlWarehouseID instead of the removed bool - README and connector.mdx: document the new behavior, drop the flag - tests for disabled/enabled validation paths and the no-op feed Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix: address audit feed review comments (retention, rate limits, gRPC codes) - lower auditLogRetention to 30 days; C1 doesn't request event-feed data older than that - propagate rate limit data from ListWorkspaces in ListEvents, from GetWorkspace in workspaceBuilder.Get, from the Validate incremental-sync checks, and on error paths of the user/group/service principal Get methods - add gRPC status codes to connector-inferred errors: NotFound for missing warehouse/workspace, FailedPrecondition for no workspace in scope and failed statements, Aborted for canceled/closed statements, DeadlineExceeded for statement poll timeout Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix: bypass GET cache for statement polling and incremental-sync refreshes - add databricks.WithoutCache(ctx); doRequest sends GETs made with it with Cache-Control: no-cache so the uhttp response cache is skipped - pollStatement polls with it: the poll URL never changes, so a cached PENDING/RUNNING response froze polling until statementPollMaxWait - user/group/service principal/workspace Get (RESOURCE_CHANGE refreshes) use it so re-syncs return fresh data; regular sync callers of the same client methods stay cached - resolveWarehouseWorkspace skips workspaces that return 401/403 instead of aborting, so a service principal not assigned to every workspace can still locate the warehouse; the not-found error names skipped workspaces - tests for poll-past-cache, cache scoping, and inaccessible workspaces Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix: retry audit rows whose principal lookup fails instead of skipping them A transient principal lookup failure (429, 5xx, transport) was logged at Debug and the row skipped, so the cursor advanced past it and the grant/revoke was lost until the next full sync. ListEvents now returns the error before encoding the cursor, so the next poll retries the page. Also clarify in docs/connector.mdx that removed group memberships are picked up incrementally; only deletions wait for the next full sync. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * feat: map account-admin, user activation and workspace assignment audit events Per the official audit log reference (AWS/Azure/GCP): - setAccountAdmin/removeAccountAdmin are real `accounts` actions; map them to the account-admin role (previously commented out as nonexistent). - setAdmin is workspace admin (it pairs with removeAdmin in the workspace-level table), so it no longer refreshes the account-admin role. - Map activateUser/deactivateUser to a user refresh. - Map `workspace` add/update/deletePermissionAssignment to a workspace refresh, resolving the workspace from request_params when the row's workspace_id is 0. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix: satisfy nilerr in workspace_id request param parsing Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix: address audit feed review suggestions - Only fail the poll on retryable principal lookup errors; skip rows whose lookup fails permanently (sampled Warn) so the cursor can't get stuck. - Refresh the group when a membership row's principal can't be resolved instead of dropping the row. - Keep refreshing the account-admin role on setAdmin. - Resolve account-level updatePermissionAssignment rows' workspace from request_params. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * refactor: drop unreachable workspace-parent fallback from audit feed The account API is always available (OAuth is the only auth method and Validate fails without it), so principals are always account-parented. Remove the accountAPIAvailable parameter, the workspace-parent fallback, and the workspace-scoped principal lookup, along with the tests that covered the impossible token-auth state. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test: cover ListEvents full-page cursor advance and !advanced wiring Add a ListEvents-level test for a full audit log page: it pages forward from the last raw row's boundary, and when no boundary parses it reports HasMore=false with the cursor unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix: stop incremental sync checks from blocking connector validation Validate no longer checks workspace scope, the SQL warehouse, or system.access.audit access when sql-warehouse-id is set. Incremental sync is optional, so a misconfiguration should not block full syncs or provisioning; the event feed already reports these problems as errors on each poll. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs: document incremental sync requirements and service principal admin role Spell out what incremental sync needs (Unity Catalog, system tables, grants, warehouse access), the single-region limit of system.access.audit, and the serverless warehouse recommendation. State that the service principal needs the Account admin role, and fix the account-level Account admin wording. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix: explain SQL statement timeouts and cancel statements on every timeout When a statement doesn't finish within the poll limit, include why: the query was still running, or for a pending statement, the warehouse's state (stopped, starting, or running but at capacity). A deadline that expires mid-poll is now treated as a timeout too, so the statement is canceled instead of left running. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs: state the same-region requirement in the sql-warehouse-id description system.access.audit only exposes workspace-level events for the warehouse's cloud region, so the warehouse can't be in just any workspace. Update the field description, its comment, the regenerated config schema and the README help output to match. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * perf: cache principal type lookups within each ListEvents call resolvePrincipalType tried user, group, then service principal for every grant-mapped row, and the HTTP cache only keeps 200s, so a principal named on many rows repeated its 404 lookups each time. Memoize the resolved type (including no match) per ListEvents call; errors are not cached. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix: use a tagged switch on statement state (staticcheck QF1002) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * chore: fix lint * test: add a local test server for the incremental sync event feed Adds test-server/, a stdlib-only stand-in for the Databricks APIs that Validate and the audit-log event feed call: OAuth M2M, workspaces, account SCIM lookups, SQL warehouses and the Statement Execution API. It serves TLS with a generated CA, routes account and workspace hosts by Host header, and recognizes the connector's system.access.audit query over seeded audit rows. Each behavior is labeled DOC, ASSUMPTION or DECISION in code and README. Adds a CI job that runs the event feed against it, diffs the events with testdata/expected-events.jsonl and checks the timeout message for a warehouse that never starts. It needs no secrets. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * feat: add debug logs to trace the incremental sync event feed Log each step of ListEvents (poll window, workspaces in scope, warehouse workspace resolution, audit query, row-to-event mapping) and a per-poll summary with audit row and event counts by type. Log SQL statement submission, success and result chunks with row counts, warehouse lookups, principal type resolution, and the targeted Get refreshes. Every message names the function that logged it; all are at Debug level. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…unity-catalog-foundation-and-metastore Co-authored-by: Cursor <cursoragent@cursor.com> # Conflicts: # pkg/connector/connector.go # pkg/connector/roles.go # pkg/databricks/request.go
A 404 on the metastore's workspace list, on the account's workspace list, or on ListMetastores reached the SDK as codes.NotFound. The metastore List is a typed child of the account, so the SDK runs it with allowWarning, reads that as "no metastores" and ends the sync successfully with none. ConductorOne then deletes every metastore and its grants. Only the not-found case is re-coded to FailedPrecondition. Wrapping every failure that way, as metastore Grants did, turned a 429 or a 5xx into a code the SDK does not retry, so one throttled permissions page failed the whole sync instead of backing off. Measured against a live tenant through a proxy that injects the failure: an injected 404 on the assignment list used to exit 0 with zero metastores and now fails the sync, and an injected 429 on the permissions read used to stop after one call and now retries with backoff. Validate no longer reports a workspace that databricks-exclude-workspaces removed as absent from the account, since ListWorkspaces drops those before the check sees them. Co-authored-by: Cursor <cursoragent@cursor.com>
…ace error Co-authored-by: Cursor <cursoragent@cursor.com>
…log-foundation-and-metastore [CXH-2720] - Add Unity Catalog client, access-path routing, and metastore sync - Databricks Connector
…88) * docs: align incremental sync coverage, timeout and warehouse notes with the code Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs: tighten incremental sync scope, timeout and warehouse wording Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
staging carried 42 commits that main never received: the Unity Catalog client and metastore sync (#70), incremental sync and the audit event feed (#56), the SQL statement client, the CI workflow, the test-server, and the incremental-sync doc alignment (#88). They reach main here as one reviewable PR instead of riding along inside the Unity Catalog feature branches. #81 landed on both branches as separate commits with the same patch-id (6384cfa on staging, 0c14586 on main), so git reports a conflict in groups.go, groups_test.go and workspaces.go. staging's side is #81 plus the Get methods it added for RESOURCE_CHANGE re-sync, so it is the complete one and it is kept. The resolution is byte-identical to the one already reviewed on #71. go.mod and .versions.yaml keep main's values, which baton-admin bumped more recently than staging's. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Superseded — see the current review report for commit
|
This was referenced Nov 3, 2026
[CXH-3600] - Sync metastore privileges held by the workspace admins group - Databricks Connector
#90
Open
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
Connector PR Review: Land staging on main - Databricks ConnectorBlocking Issues: 0 | Suggestions: 5 | Threads Resolved: 0 Review SummaryThis PR lands 42
I scanned every hunk of the full diff for security and correctness. I applied the repo-local criteria in depth to the metastore
Coverage: Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agentsReviewed commit: |
Contributor
There was a problem hiding this comment.
No blocking issues found — see the full review report
Author
|
The two suggestions in the report that have no inline thread —
#91 lands the |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
stagingholds 42 commits thatmainnever received. Until now they reachedmainonly as passengers inside the Unity Catalog feature branches, which made those PRs show 50+ files and hid the layer each one actually adds. This PR lands that work onmainon its own, so every feature PR in the stack shows only its own diff.Requested by @luisina-santos on #90.
What this brings to
mainAll of this was reviewed and merged into
stagingas its own PRs. Nothing here is new code.The #81 conflict
#81 landed on both branches as separate commits:
6384cfa1onstagingand0c145865onmain. The two have the same patch-id, so they are the same change. Git still reports a conflict in three files, becausestaginglater added theGetmethods for RESOURCE_CHANGE re-sync on top of them:pkg/connector/groups.gopkg/connector/groups_test.gopkg/connector/workspaces.gostagingholds #81 plus thoseGetmethods, so its side is the complete one and it is kept. The result is byte-identical to the resolution already reviewed on #71.go.modand.versions.yamlkeep the values frommain. baton-admin bumped them afterstaginglast moved.Verification
Merge order
This PR goes first. The rest of the stack is rebased on top of it and each one then shows only its own layer:
stagingonmain#89 still targets
staging. It must retarget tomainafter this PR merges.🤖 Generated with Claude Code