CXH-2164: add workspace exclude flag to scope sync - #52
Conversation
Add --databricks-exclude-workspaces (BATON_DATABRICKS_EXCLUDE_WORKSPACES), a comma-separated denylist of workspaces to omit from sync, matched by workspace name, deployment name, or numeric workspace ID. Filtering happens in client.ListWorkspaces so both Validate and sync honor it; excluded workspaces and their roles are skipped entirely.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| for _, w := range excludeWorkspaces { | ||
| if w == "" { | ||
| continue | ||
| } | ||
| excludeSet[w] = struct{}{} | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Entries aren't trimmed before being added to the exclude set, and matching is exact. The README/docs advertise a "comma-separated list," so a user setting BATON_DATABRICKS_EXCLUDE_WORKSPACES=ws-a, ws-b will get " ws-b" (pflag's StringSlice does not trim whitespace), which silently fails to exclude ws-b. Consider strings.TrimSpace(w) before the empty check. (medium confidence)
| for _, w := range excludeWorkspaces { | |
| if w == "" { | |
| continue | |
| } | |
| excludeSet[w] = struct{}{} | |
| } | |
| for _, w := range excludeWorkspaces { | |
| w = strings.TrimSpace(w) | |
| if w == "" { | |
| continue | |
| } | |
| excludeSet[w] = struct{}{} | |
| } |
Connector PR Review: CXH-2164: add workspace exclude flag to scope syncBlocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0 Review SummaryScanned the full PR diff for security and correctness. The change adds a Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agents |
Comma-separated --databricks-exclude-workspaces values arrive space-padded (pflag CSV parsing does not trim), and isWorkspaceExcluded does exact map lookups, so an entry like " ws-b" silently never matched. TrimSpace each entry when building the exclude set. Adds a table-driven test covering exclusion by name, deployment name, and numeric ID, plus empty/whitespace entries being ignored.
baton-sdk v0.20.5 deprecated WithGroupProfile/WithRoleProfile/ WithUserProfile/WithStatus and the trait-level Profile fields (staticcheck SA1019), which failed verify/lint. Move profile to WithResourceProfile and user status to WithResourceStatus, and read profile via resource.GetProfile() instead of the deprecated trait.Profile. The SDK already mirrored the trait profile/status onto the resource, so the synced resource output is unchanged.
| func (c *Client) isWorkspaceExcluded(w Workspace) bool { | ||
| if len(c.excludeWorkspaces) == 0 { | ||
| return false | ||
| } | ||
| for _, key := range []string{w.Name, w.DeploymentName, strconv.Itoa(w.ID)} { | ||
| if _, ok := c.excludeWorkspaces[key]; ok { | ||
| return true | ||
| } | ||
| } | ||
| return false |
There was a problem hiding this comment.
🟡 Suggestion: Exclusion matching is exact and case-sensitive, so an entry like Prod (vs. actual name prod) silently fails to exclude the workspace. For a scoping feature this fails open. Consider lowercasing both the exclude set (in NewClient) and these keys before comparing, and/or noting the case-sensitivity in the flag description. (low confidence)
Lowercase both the exclude entries and the workspace name/deployment name at comparison time so a case-mismatched entry (e.g. "Prod" vs "prod") no longer silently fails to exclude a workspace. Numeric ID matching is unaffected.
| matched := make(map[string]struct{}, len(c.excludeWorkspaces)) | ||
| filtered := make([]Workspace, 0, len(res)) | ||
| for _, w := range res { | ||
| if key, ok := c.isWorkspaceExcluded(w); ok { | ||
| matched[key] = struct{}{} | ||
| continue | ||
| } | ||
| filtered = append(filtered, w) | ||
| } | ||
|
|
||
| unmatched := make([]string, 0, len(c.excludeWorkspaces)) | ||
| for key := range c.excludeWorkspaces { | ||
| if _, ok := matched[key]; !ok { | ||
| unmatched = append(unmatched, key) | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: isWorkspaceExcluded returns only the first matching key, so if two exclude entries both match the same workspace (e.g. its name and its deployment name), only one is recorded in matched and the other is falsely reported in unmatched_exclude_entries. Low impact since this is Debug-only diagnostics, but if you want the "unmatched" list to be accurate, mark every matching key rather than short-circuiting. (confidence: medium)
| } | ||
|
|
||
| parentType, parentID, err := getParentInfoFromProfile(roleTrait.Profile) | ||
| parentType, parentID, err := getParentInfoFromProfile(entitlement.Resource.GetProfile()) |
There was a problem hiding this comment.
these provisioning reads use the raw resource.GetProfile() (also service-principals.go:226/236/304/314, workspaces.go:192/222), which only returns the resource-level Profile — no fallback to the deprecated trait profile that older connector versions wrote. provisioning runs on the C1-stored entitlement.Resource from the last sync, so in the post-upgrade / pre-resync window the profile may live only on the trait → raw getter returns nil → Grant/Revoke fail (graceful, self-heals after a full resync). fix: use rs.GetProfile(entitlement.Resource) (the fallback-aware helper) — which PR #51 already does at these exact call sites. heads up: I actually trial-merged #51 into this branch locally and it produces real conflicts in roles.go, service-principals.go, workspaces.go — and also groups.go, which has the same raw-GetProfile pattern but isn't touched by this comment. worth reconciling all four files on rs.GetProfile and picking a landing order (land #51 first, probably, since it already has the fallback-aware version).
…matched exclude key Grant/Revoke read entitlement.Resource as stored by C1 from the last sync, so a resource written before the trait->resource profile migration (commit 1217bb7) only carries Profile on the deprecated trait. Read it via rs.GetProfile, which falls back to the trait, instead of the raw resource-level getter. isWorkspaceExcluded also only recorded the first exclude-set key a workspace matched, so a workspace matched by two entries (e.g. both name and deployment name) misreported the second as unmatched in the Debug diagnostic. Record every matching key.
4 participants
Customers can now exclude specific Databricks workspaces from a sync by name or ID, so an inaccessible or unwanted workspace no longer has to be synced or granted access to.