From 41257f6ba599c15367269bf0566840ca527a972d Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Fri, 24 Jul 2026 17:54:43 +0200 Subject: [PATCH 1/3] direct: Fix perpetual update on grants with ALL_PRIVILEGES buildGrantChanges now computes a per-principal add/remove diff against remote state (Add = desired - remote, Remove = remote - desired) like the terraform provider's diffPermissions, instead of sending a blanket "remove ALL_PRIVILEGES" to wipe each principal. This lets a principal granted ALL_PRIVILEGES converge when the backend also reports concrete privileges for it, instead of reporting update forever (#6030). Also normalize privileges (uppercase, spaces to underscores) and sort them on both the config and read sides so grants do not drift on case or order, and update the grants testserver to mirror that backend behaviour. --- .nextchanges/bundles/grants-normalization.md | 1 + .../grants-remove-principal/output.txt | 25 +--- .../grants_privilege_normalization.yml.tmpl | 18 +++ .../invariant/continue_293/out.test.toml | 1 + .../bundle/invariant/migrate/out.test.toml | 1 + .../bundle/invariant/no_drift/out.test.toml | 1 + acceptance/bundle/invariant/test.toml | 1 + .../catalogs/out.deploy1.requests.direct.json | 5 +- .../catalogs/out.deploy2.requests.direct.json | 3 +- .../bundle/resources/grants/catalogs/script | 4 +- .../out.deploy1.requests.direct.json | 5 +- .../databricks.yml.tmpl | 17 +++ .../all_privileges_coexist/out.test.toml | 4 + .../schemas/all_privileges_coexist/output.txt | 38 ++++++ .../schemas/all_privileges_coexist/script | 28 ++++ .../schemas/all_privileges_coexist/test.toml | 1 + .../all_privileges_coexist/update.json | 8 ++ .../grants/schemas/change_privilege/script | 4 +- .../volumes/out.deploy1.requests.direct.json | 5 +- .../volumes/out.deploy2.requests.direct.json | 5 +- .../bundle/resources/grants/volumes/script | 4 +- bundle/direct/dresources/grants.go | 118 ++++++++++++----- bundle/direct/dresources/grants_test.go | 123 ++++++++++++------ libs/testserver/grants.go | 31 +++-- 24 files changed, 321 insertions(+), 130 deletions(-) create mode 100644 .nextchanges/bundles/grants-normalization.md create mode 100644 acceptance/bundle/invariant/configs/grants_privilege_normalization.yml.tmpl create mode 100644 acceptance/bundle/resources/grants/schemas/all_privileges_coexist/databricks.yml.tmpl create mode 100644 acceptance/bundle/resources/grants/schemas/all_privileges_coexist/out.test.toml create mode 100644 acceptance/bundle/resources/grants/schemas/all_privileges_coexist/output.txt create mode 100644 acceptance/bundle/resources/grants/schemas/all_privileges_coexist/script create mode 100644 acceptance/bundle/resources/grants/schemas/all_privileges_coexist/test.toml create mode 100644 acceptance/bundle/resources/grants/schemas/all_privileges_coexist/update.json diff --git a/.nextchanges/bundles/grants-normalization.md b/.nextchanges/bundles/grants-normalization.md new file mode 100644 index 00000000000..6a20b8f8fb0 --- /dev/null +++ b/.nextchanges/bundles/grants-normalization.md @@ -0,0 +1 @@ +Fixed spurious `update` on catalog/schema/volume grants (direct engine): grants with `ALL_PRIVILEGES` now converge when the backend also reports concrete privileges, and privilege names are normalized to match the case and ordering Unity Catalog returns ([#6030](https://github.com/databricks/cli/issues/6030)). diff --git a/acceptance/bundle/deploy/readplan/grants-remove-principal/output.txt b/acceptance/bundle/deploy/readplan/grants-remove-principal/output.txt index f7926fa9fa9..f1f02a201c8 100644 --- a/acceptance/bundle/deploy/readplan/grants-remove-principal/output.txt +++ b/acceptance/bundle/deploy/readplan/grants-remove-principal/output.txt @@ -13,21 +13,15 @@ Deployment complete! "changes": [ { "add": [ - "USE_SCHEMA" + "CREATE_TABLE" ], - "principal": "[USERNAME]", - "remove": [ - "ALL_PRIVILEGES" - ] + "principal": "extra@example.test" }, { "add": [ - "CREATE_TABLE" + "USE_SCHEMA" ], - "principal": "extra@example.test", - "remove": [ - "ALL_PRIVILEGES" - ] + "principal": "[USERNAME]" } ] } @@ -51,19 +45,10 @@ Deployment complete! "path": "/api/2.1/unity-catalog/permissions/schema/main.myschema", "body": { "changes": [ - { - "add": [ - "USE_SCHEMA" - ], - "principal": "[USERNAME]", - "remove": [ - "ALL_PRIVILEGES" - ] - }, { "principal": "extra@example.test", "remove": [ - "ALL_PRIVILEGES" + "CREATE_TABLE" ] } ] diff --git a/acceptance/bundle/invariant/configs/grants_privilege_normalization.yml.tmpl b/acceptance/bundle/invariant/configs/grants_privilege_normalization.yml.tmpl new file mode 100644 index 00000000000..3021005e550 --- /dev/null +++ b/acceptance/bundle/invariant/configs/grants_privilege_normalization.yml.tmpl @@ -0,0 +1,18 @@ +bundle: + name: test-bundle-$UNIQUE_NAME + +resources: + schemas: + foo: + catalog_name: main + name: test-schema-grantnorm-$UNIQUE_NAME + grants: + # UC normalizes privileges (uppercases, converts spaces to underscores) + # and does not guarantee their order on readback. The direct engine must + # apply the same normalization so a deploy of non-canonical privileges + # does not report perpetual drift. + - principal: account users + privileges: + - use_schema + - CREATE TABLE + - APPLY_TAG diff --git a/acceptance/bundle/invariant/continue_293/out.test.toml b/acceptance/bundle/invariant/continue_293/out.test.toml index 407bac1bf19..ade55214d37 100644 --- a/acceptance/bundle/invariant/continue_293/out.test.toml +++ b/acceptance/bundle/invariant/continue_293/out.test.toml @@ -16,6 +16,7 @@ EnvMatrix.INPUT_CONFIG = [ "database_instance.yml.tmpl", "experiment.yml.tmpl", "external_location.yml.tmpl", + "grants_privilege_normalization.yml.tmpl", "job.yml.tmpl", "job_pydabs_10_tasks.yml.tmpl", "job_run_job_ref.yml.tmpl", diff --git a/acceptance/bundle/invariant/migrate/out.test.toml b/acceptance/bundle/invariant/migrate/out.test.toml index 0ac1b789d6f..3d7867735ba 100644 --- a/acceptance/bundle/invariant/migrate/out.test.toml +++ b/acceptance/bundle/invariant/migrate/out.test.toml @@ -14,6 +14,7 @@ EnvMatrix.INPUT_CONFIG = [ "database_catalog.yml.tmpl", "database_instance.yml.tmpl", "experiment.yml.tmpl", + "grants_privilege_normalization.yml.tmpl", "job.yml.tmpl", "job_pydabs_10_tasks.yml.tmpl", "job_run_job_ref.yml.tmpl", diff --git a/acceptance/bundle/invariant/no_drift/out.test.toml b/acceptance/bundle/invariant/no_drift/out.test.toml index 1b0b5d6585a..ac63cc1bdff 100644 --- a/acceptance/bundle/invariant/no_drift/out.test.toml +++ b/acceptance/bundle/invariant/no_drift/out.test.toml @@ -17,6 +17,7 @@ EnvMatrix.INPUT_CONFIG = [ "experiment.yml.tmpl", "external_location.yml.tmpl", "genie_space.yml.tmpl", + "grants_privilege_normalization.yml.tmpl", "instance_pool.yml.tmpl", "job.yml.tmpl", "job_pydabs_10_tasks.yml.tmpl", diff --git a/acceptance/bundle/invariant/test.toml b/acceptance/bundle/invariant/test.toml index 55e77c64640..cf8553666b4 100644 --- a/acceptance/bundle/invariant/test.toml +++ b/acceptance/bundle/invariant/test.toml @@ -35,6 +35,7 @@ EnvMatrix.INPUT_CONFIG = [ "experiment.yml.tmpl", "external_location.yml.tmpl", "genie_space.yml.tmpl", + "grants_privilege_normalization.yml.tmpl", "instance_pool.yml.tmpl", "job.yml.tmpl", "job_pydabs_10_tasks.yml.tmpl", diff --git a/acceptance/bundle/resources/grants/catalogs/out.deploy1.requests.direct.json b/acceptance/bundle/resources/grants/catalogs/out.deploy1.requests.direct.json index 16f913b8a77..68245ff9fdc 100644 --- a/acceptance/bundle/resources/grants/catalogs/out.deploy1.requests.direct.json +++ b/acceptance/bundle/resources/grants/catalogs/out.deploy1.requests.direct.json @@ -8,10 +8,7 @@ "CREATE_SCHEMA", "USE_CATALOG" ], - "principal": "deco-test-user@databricks.com", - "remove": [ - "ALL_PRIVILEGES" - ] + "principal": "deco-test-user@databricks.com" } ] } diff --git a/acceptance/bundle/resources/grants/catalogs/out.deploy2.requests.direct.json b/acceptance/bundle/resources/grants/catalogs/out.deploy2.requests.direct.json index 47e1335feae..68eb137c9c2 100644 --- a/acceptance/bundle/resources/grants/catalogs/out.deploy2.requests.direct.json +++ b/acceptance/bundle/resources/grants/catalogs/out.deploy2.requests.direct.json @@ -5,12 +5,11 @@ "changes": [ { "add": [ - "USE_CATALOG", "USE_SCHEMA" ], "principal": "deco-test-user@databricks.com", "remove": [ - "ALL_PRIVILEGES" + "CREATE_SCHEMA" ] } ] diff --git a/acceptance/bundle/resources/grants/catalogs/script b/acceptance/bundle/resources/grants/catalogs/script index 067710b705e..522b7a3cbfa 100755 --- a/acceptance/bundle/resources/grants/catalogs/script +++ b/acceptance/bundle/resources/grants/catalogs/script @@ -9,7 +9,7 @@ trace print_requests.py //permissions > out.deploy1.requests.$DATABRICKS_BUNDLE_ trace $CLI bundle plan # Since we're sending different requests between terraform and direct, assert that the end result is the same: -trace $CLI grants get catalog catalog_grants_$UNIQUE_NAME | jq --sort-keys +trace $CLI grants get catalog catalog_grants_$UNIQUE_NAME | jq --sort-keys '(.privilege_assignments[].privileges) |= sort' update_file.py databricks.yml CREATE_SCHEMA USE_SCHEMA @@ -18,7 +18,7 @@ trace print_requests.py --get //permissions trace $CLI bundle deploy trace print_requests.py //permissions > out.deploy2.requests.$DATABRICKS_BUNDLE_ENGINE.json -trace $CLI grants get catalog catalog_grants_$UNIQUE_NAME | jq --sort-keys +trace $CLI grants get catalog catalog_grants_$UNIQUE_NAME | jq --sort-keys '(.privilege_assignments[].privileges) |= sort' trace $CLI bundle destroy --auto-approve trace print_requests.py //permissions > out.destroy.requests.$DATABRICKS_BUNDLE_ENGINE.json diff --git a/acceptance/bundle/resources/grants/registered_models/out.deploy1.requests.direct.json b/acceptance/bundle/resources/grants/registered_models/out.deploy1.requests.direct.json index 8491f706c62..81ca56fd5da 100644 --- a/acceptance/bundle/resources/grants/registered_models/out.deploy1.requests.direct.json +++ b/acceptance/bundle/resources/grants/registered_models/out.deploy1.requests.direct.json @@ -7,10 +7,7 @@ "add": [ "APPLY_TAG" ], - "principal": "deco-test-user@databricks.com", - "remove": [ - "ALL_PRIVILEGES" - ] + "principal": "deco-test-user@databricks.com" } ] } diff --git a/acceptance/bundle/resources/grants/schemas/all_privileges_coexist/databricks.yml.tmpl b/acceptance/bundle/resources/grants/schemas/all_privileges_coexist/databricks.yml.tmpl new file mode 100644 index 00000000000..8266f099163 --- /dev/null +++ b/acceptance/bundle/resources/grants/schemas/all_privileges_coexist/databricks.yml.tmpl @@ -0,0 +1,17 @@ +bundle: + name: schema-grants-all-privileges-coexist-$UNIQUE_NAME + +resources: + schemas: + grants_schema: + name: schema_all_priv_coexist_$UNIQUE_NAME + catalog_name: main + grants: + # Config declares only ALL_PRIVILEGES for the principal. Reproduces + # issue #6030: the backend also reports a concrete privilege for this + # principal (added out of band below / materialized for owners), and + # buildGrantChanges skips the ALL_PRIVILEGES removal, so the concrete + # privilege is never reconciled and the plan reports perpetual update. + - principal: $CURRENT_USER_NAME + privileges: + - ALL_PRIVILEGES diff --git a/acceptance/bundle/resources/grants/schemas/all_privileges_coexist/out.test.toml b/acceptance/bundle/resources/grants/schemas/all_privileges_coexist/out.test.toml new file mode 100644 index 00000000000..e849ec85ace --- /dev/null +++ b/acceptance/bundle/resources/grants/schemas/all_privileges_coexist/out.test.toml @@ -0,0 +1,4 @@ +Local = true +Cloud = true +RequiresUnityCatalog = true +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["terraform", "direct"] diff --git a/acceptance/bundle/resources/grants/schemas/all_privileges_coexist/output.txt b/acceptance/bundle/resources/grants/schemas/all_privileges_coexist/output.txt new file mode 100644 index 00000000000..64b2054b444 --- /dev/null +++ b/acceptance/bundle/resources/grants/schemas/all_privileges_coexist/output.txt @@ -0,0 +1,38 @@ + +>>> [CLI] bundle deploy +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/schema-grants-all-privileges-coexist-[UNIQUE_NAME]/default/files... +Deploying resources... +Updating deployment state... +Deployment complete! + +>>> [CLI] grants update schema main.schema_all_priv_coexist_[UNIQUE_NAME] --json @update.resolved.json + +>>> [CLI] bundle plan +update schemas.grants_schema.grants + +Plan: 0 to add, 1 to change, 0 to delete, 1 unchanged + +>>> [CLI] bundle deploy +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/schema-grants-all-privileges-coexist-[UNIQUE_NAME]/default/files... +Deploying resources... +Updating deployment state... +Deployment complete! + +>>> [CLI] bundle plan +Plan: 0 to add, 0 to change, 0 to delete, 2 unchanged + +>>> [CLI] grants get schema main.schema_all_priv_coexist_[UNIQUE_NAME] +json.privilege_assignments[].principal = "[USERNAME]"; +json.privilege_assignments[].privileges[] = "ALL_PRIVILEGES"; + +>>> errcode [CLI] bundle destroy --auto-approve +The following resources will be deleted: + delete resources.schemas.grants_schema + +This action will result in the deletion of the following UC schemas. Any underlying data may be lost: + delete resources.schemas.grants_schema + +All files and directories at the following location will be deleted: /Workspace/Users/[USERNAME]/.bundle/schema-grants-all-privileges-coexist-[UNIQUE_NAME]/default + +Deleting files... +Destroy complete! diff --git a/acceptance/bundle/resources/grants/schemas/all_privileges_coexist/script b/acceptance/bundle/resources/grants/schemas/all_privileges_coexist/script new file mode 100644 index 00000000000..a49e7bf0fd2 --- /dev/null +++ b/acceptance/bundle/resources/grants/schemas/all_privileges_coexist/script @@ -0,0 +1,28 @@ +SCHEMA_FULL_NAME=main.schema_all_priv_coexist_$UNIQUE_NAME + +envsubst < databricks.yml.tmpl > databricks.yml + +cleanup() { + trace errcode $CLI bundle destroy --auto-approve + rm -f out.requests.txt +} +trap cleanup EXIT + +trace $CLI bundle deploy + +# Out of band, grant a concrete privilege in addition to the ALL_PRIVILEGES that +# the config declares for the same principal. This reproduces issue #6030: the +# backend then reports both ALL_PRIVILEGES and the concrete privilege, and the +# deploy must revoke the concrete privilege to converge. +envsubst < update.json > update.resolved.json +trace $CLI grants update schema "$SCHEMA_FULL_NAME" --json @update.resolved.json > /dev/null + +# The plan detects the extra privilege and applies the revocation on deploy. +trace $CLI bundle plan +trace $CLI bundle deploy + +# The plan now converges to a no-op instead of reporting a perpetual update. +trace $CLI bundle plan + +# Only ALL_PRIVILEGES remains for the principal. +trace $CLI grants get schema "$SCHEMA_FULL_NAME" | gron.py --noindex | sort_lines.py --repl diff --git a/acceptance/bundle/resources/grants/schemas/all_privileges_coexist/test.toml b/acceptance/bundle/resources/grants/schemas/all_privileges_coexist/test.toml new file mode 100644 index 00000000000..3411dded007 --- /dev/null +++ b/acceptance/bundle/resources/grants/schemas/all_privileges_coexist/test.toml @@ -0,0 +1 @@ +Ignore = ['update.resolved.json'] diff --git a/acceptance/bundle/resources/grants/schemas/all_privileges_coexist/update.json b/acceptance/bundle/resources/grants/schemas/all_privileges_coexist/update.json new file mode 100644 index 00000000000..d43aec70917 --- /dev/null +++ b/acceptance/bundle/resources/grants/schemas/all_privileges_coexist/update.json @@ -0,0 +1,8 @@ +{ + "changes": [ + { + "principal": "$CURRENT_USER_NAME", + "add": ["USE_SCHEMA"] + } + ] +} diff --git a/acceptance/bundle/resources/grants/schemas/change_privilege/script b/acceptance/bundle/resources/grants/schemas/change_privilege/script index 737f7491d3a..39001e0a685 100644 --- a/acceptance/bundle/resources/grants/schemas/change_privilege/script +++ b/acceptance/bundle/resources/grants/schemas/change_privilege/script @@ -9,7 +9,7 @@ trace print_requests.py //permissions > out.deploy1.requests.$DATABRICKS_BUNDLE_ trace $CLI bundle plan # Since we're sending different requests between terraform and direct, assert that the end result is the same: -trace $CLI grants get schema main.schema_grants_$UNIQUE_NAME | jq --sort-keys +trace $CLI grants get schema main.schema_grants_$UNIQUE_NAME | jq --sort-keys '(.privilege_assignments[].privileges) |= sort' update_file.py databricks.yml USE_SCHEMA APPLY_TAG @@ -18,7 +18,7 @@ trace print_requests.py --get //permissions trace $CLI bundle deploy trace print_requests.py //permissions > out.deploy2.requests.$DATABRICKS_BUNDLE_ENGINE.json -trace $CLI grants get schema main.schema_grants_$UNIQUE_NAME | jq --sort-keys +trace $CLI grants get schema main.schema_grants_$UNIQUE_NAME | jq --sort-keys '(.privilege_assignments[].privileges) |= sort' trace $CLI bundle destroy --auto-approve trace print_requests.py //permissions > out.destroy.requests.$DATABRICKS_BUNDLE_ENGINE.json diff --git a/acceptance/bundle/resources/grants/volumes/out.deploy1.requests.direct.json b/acceptance/bundle/resources/grants/volumes/out.deploy1.requests.direct.json index 782eecac450..1e8bdf282a7 100644 --- a/acceptance/bundle/resources/grants/volumes/out.deploy1.requests.direct.json +++ b/acceptance/bundle/resources/grants/volumes/out.deploy1.requests.direct.json @@ -8,10 +8,7 @@ "READ_VOLUME", "WRITE_VOLUME" ], - "principal": "deco-test-user@databricks.com", - "remove": [ - "ALL_PRIVILEGES" - ] + "principal": "deco-test-user@databricks.com" } ] } diff --git a/acceptance/bundle/resources/grants/volumes/out.deploy2.requests.direct.json b/acceptance/bundle/resources/grants/volumes/out.deploy2.requests.direct.json index fb9d6e90ba3..5a2ad0d6eab 100644 --- a/acceptance/bundle/resources/grants/volumes/out.deploy2.requests.direct.json +++ b/acceptance/bundle/resources/grants/volumes/out.deploy2.requests.direct.json @@ -5,12 +5,11 @@ "changes": [ { "add": [ - "MANAGE", - "READ_VOLUME" + "MANAGE" ], "principal": "deco-test-user@databricks.com", "remove": [ - "ALL_PRIVILEGES" + "WRITE_VOLUME" ] } ] diff --git a/acceptance/bundle/resources/grants/volumes/script b/acceptance/bundle/resources/grants/volumes/script index 725049cda8e..a117a0b4712 100644 --- a/acceptance/bundle/resources/grants/volumes/script +++ b/acceptance/bundle/resources/grants/volumes/script @@ -5,7 +5,7 @@ trace $CLI bundle deploy trace print_requests.py //permissions > out.deploy1.requests.$DATABRICKS_BUNDLE_ENGINE.json # Since we're sending different requests between terraform and direct, assert that the end result is the same: -trace $CLI grants get volume main.schema_grants_$UNIQUE_NAME.volume_name | jq --sort-keys +trace $CLI grants get volume main.schema_grants_$UNIQUE_NAME.volume_name | jq --sort-keys '(.privilege_assignments[].privileges) |= sort' update_file.py databricks.yml WRITE_VOLUME MANAGE @@ -14,7 +14,7 @@ trace $CLI bundle plan -o json | normalize_uc_payload.py effective_predictive_op trace $CLI bundle deploy trace print_requests.py //permissions > out.deploy2.requests.$DATABRICKS_BUNDLE_ENGINE.json -trace $CLI grants get volume main.schema_grants_$UNIQUE_NAME.volume_name | jq --sort-keys +trace $CLI grants get volume main.schema_grants_$UNIQUE_NAME.volume_name | jq --sort-keys '(.privilege_assignments[].privileges) |= sort' trace $CLI bundle destroy --auto-approve trace print_requests.py //permissions > out.destroy.requests.$DATABRICKS_BUNDLE_ENGINE.json diff --git a/bundle/direct/dresources/grants.go b/bundle/direct/dresources/grants.go index bac70bc5615..4821eb9283f 100644 --- a/bundle/direct/dresources/grants.go +++ b/bundle/direct/dresources/grants.go @@ -48,10 +48,9 @@ func PrepareGrantsInputConfig(inputConfig any, node string) (*structvar.StructVa return nil, fmt.Errorf("expected *[]catalog.PrivilegeAssignment, got %T", inputConfig) } - // Backend sorts privileges, so we sort here as well. - for i := range *grantsPtr { - slices.Sort((*grantsPtr)[i].Privileges) - } + // Normalize the same way the backend does (uppercase privileges, sort) so + // the config and the value read back by DoRead compare equal. + normalizeAssignments(*grantsPtr) return &structvar.StructVar{ Value: &GrantsState{ @@ -121,11 +120,10 @@ func (r *ResourceGrants) DoUpdate(ctx context.Context, _ string, state *GrantsSt if state.FullName == "" { return nil, errors.New("internal error: grants full_name must be resolved before deployment") } - removedPrincipals := removedGrantPrincipals(state.EmbeddedSlice, entry) _, err := r.client.Grants.Update(ctx, catalog.UpdatePermissions{ SecurableType: state.SecurableType, FullName: state.FullName, - Changes: buildGrantChanges(state.EmbeddedSlice, removedPrincipals), + Changes: buildGrantChanges(state.EmbeddedSlice, remoteAssignments(entry)), OmitPermissionsInResponse: false, ForceSendFields: nil, }) @@ -138,35 +136,52 @@ func (r *ResourceGrants) DoDelete(ctx context.Context, id string, _ *GrantsState return nil } -func buildGrantChanges(desiredAssignments []catalog.PrivilegeAssignment, removedPrincipals []string) []catalog.PermissionsChange { - changes := make([]catalog.PermissionsChange, 0, len(desiredAssignments)+len(removedPrincipals)) - for _, ga := range desiredAssignments { - change := catalog.PermissionsChange{ - Principal: ga.Principal, - Add: ga.Privileges, - Remove: nil, - ForceSendFields: nil, - } - // Remove all other privileges unless ALL_PRIVILEGES is being granted - // (it would conflict with appearing in both Add and Remove). - if !slices.Contains(ga.Privileges, catalog.PrivilegeAllPrivileges) { - change.Remove = []catalog.Privilege{catalog.PrivilegeAllPrivileges} +// buildGrantChanges computes the per-principal add/remove diff between the +// desired assignments and the remote assignments, mirroring the Terraform +// provider's diffPermissions. For each principal Add is (desired - remote) and +// Remove is (remote - desired). Computing an exact diff (rather than sending a +// blanket "remove ALL_PRIVILEGES" to wipe the principal) is what lets a +// principal granted ALL_PRIVILEGES converge when the backend also reports +// concrete privileges for it: those extra privileges land in Remove instead of +// being left in place forever. A privilege can never be in both Add and Remove, +// so this also avoids the "Duplicate privileges to add and delete" API error. +func buildGrantChanges(desiredAssignments, remoteAssignments []catalog.PrivilegeAssignment) []catalog.PermissionsChange { + desired := privilegesByPrincipal(desiredAssignments) + remote := privilegesByPrincipal(remoteAssignments) + + principals := make([]string, 0, len(desired)+len(remote)) + for p := range desired { + principals = append(principals, p) + } + for p := range remote { + if _, ok := desired[p]; !ok { + principals = append(principals, p) } - changes = append(changes, change) } - for _, principal := range removedPrincipals { + slices.Sort(principals) + + changes := make([]catalog.PermissionsChange, 0, len(principals)) + for _, principal := range principals { + add := setDifference(desired[principal], remote[principal]) + remove := setDifference(remote[principal], desired[principal]) + if len(add) == 0 && len(remove) == 0 { + continue + } changes = append(changes, catalog.PermissionsChange{ Principal: principal, - Add: nil, - Remove: []catalog.Privilege{catalog.PrivilegeAllPrivileges}, + Add: add, + Remove: remove, ForceSendFields: nil, }) } return changes } -// removedGrantPrincipals returns principals present in the remote state but absent from the desired assignments. -func removedGrantPrincipals(desiredAssignments []catalog.PrivilegeAssignment, entry *PlanEntry) []string { +// remoteAssignments extracts the remote privilege assignments from the plan +// entry. It returns nil when the entry or its remote state is absent, which +// happens on create and when deploying from a serialized plan (the JSON-loaded +// state is not a *GrantsState). In those cases nothing is revoked. +func remoteAssignments(entry *PlanEntry) []catalog.PrivilegeAssignment { if entry == nil { return nil } @@ -174,18 +189,33 @@ func removedGrantPrincipals(desiredAssignments []catalog.PrivilegeAssignment, en if !ok || remote == nil { return nil } + return remote.EmbeddedSlice +} - desired := make(map[string]struct{}, len(desiredAssignments)) - for _, a := range desiredAssignments { - if a.Principal != "" { - desired[a.Principal] = struct{}{} +func privilegesByPrincipal(assignments []catalog.PrivilegeAssignment) map[string]map[catalog.Privilege]struct{} { + result := make(map[string]map[catalog.Privilege]struct{}, len(assignments)) + for _, a := range assignments { + if a.Principal == "" { + continue + } + privs := result[a.Principal] + if privs == nil { + privs = make(map[catalog.Privilege]struct{}, len(a.Privileges)) + result[a.Principal] = privs + } + for _, p := range a.Privileges { + privs[p] = struct{}{} } } + return result +} - var result []string - for _, a := range remote.EmbeddedSlice { - if _, ok := desired[a.Principal]; !ok { - result = append(result, a.Principal) +// setDifference returns the sorted privileges present in a but not in b. +func setDifference(a, b map[catalog.Privilege]struct{}) []catalog.Privilege { + var result []catalog.Privilege + for p := range a { + if _, ok := b[p]; !ok { + result = append(result, p) } } slices.Sort(result) @@ -222,9 +252,31 @@ func (r *ResourceGrants) listGrants(ctx context.Context, securableType, fullName } pageToken = resp.NextPageToken } + // The backend does not guarantee a stable privilege order across reads, so + // normalize the same way as the config side to avoid false drift. + normalizeAssignments(assignments) return assignments, nil } +// normalizePrivilege matches the backend's normalization: privileges are +// uppercased and spaces are converted to underscores (e.g. "use schema" -> +// "USE_SCHEMA"). Mirrors the Terraform provider's permissions.NormalizePrivilege. +func normalizePrivilege(p catalog.Privilege) catalog.Privilege { + return catalog.Privilege(strings.ToUpper(strings.ReplaceAll(string(p), " ", "_"))) +} + +// normalizeAssignments normalizes and sorts each assignment's privileges in +// place so that config and remote state compare equal regardless of the +// case or order the backend returns. +func normalizeAssignments(assignments []catalog.PrivilegeAssignment) { + for i := range assignments { + for j := range assignments[i].Privileges { + assignments[i].Privileges[j] = normalizePrivilege(assignments[i].Privileges[j]) + } + slices.Sort(assignments[i].Privileges) + } +} + func extractGrantResourceType(node string) (string, error) { rest, ok := strings.CutPrefix(node, "resources.") if !ok { diff --git a/bundle/direct/dresources/grants_test.go b/bundle/direct/dresources/grants_test.go index 1724eed40dc..f3875a01796 100644 --- a/bundle/direct/dresources/grants_test.go +++ b/bundle/direct/dresources/grants_test.go @@ -11,66 +11,107 @@ func TestBuildGrantChanges(t *testing.T) { tests := []struct { name string desired []catalog.PrivilegeAssignment - removed []string + remote []catalog.PrivilegeAssignment expected []catalog.PermissionsChange }{ { - name: "removes all other privileges for desired principal", + name: "adds privileges for a new principal (no remote)", desired: []catalog.PrivilegeAssignment{ - { - Principal: "alice", - Privileges: []catalog.Privilege{ - catalog.PrivilegeApplyTag, - catalog.PrivilegeCreateTable, - }, - }, + {Principal: "alice", Privileges: []catalog.Privilege{catalog.PrivilegeApplyTag, catalog.PrivilegeCreateTable}}, }, expected: []catalog.PermissionsChange{ - { - Principal: "alice", - Add: []catalog.Privilege{ - catalog.PrivilegeApplyTag, - catalog.PrivilegeCreateTable, - }, - Remove: []catalog.Privilege{ - catalog.PrivilegeAllPrivileges, - }, - }, + {Principal: "alice", Add: []catalog.Privilege{catalog.PrivilegeApplyTag, catalog.PrivilegeCreateTable}}, }, }, { - name: "skips ALL_PRIVILEGES removal when granting ALL_PRIVILEGES", + name: "removes privileges present in remote but not desired", desired: []catalog.PrivilegeAssignment{ - { - Principal: "alice", - Privileges: []catalog.Privilege{ - catalog.PrivilegeAllPrivileges, - }, - }, + {Principal: "alice", Privileges: []catalog.Privilege{catalog.PrivilegeApplyTag}}, }, - removed: []string{ - "bob", + remote: []catalog.PrivilegeAssignment{ + {Principal: "alice", Privileges: []catalog.Privilege{catalog.PrivilegeApplyTag, catalog.PrivilegeCreateTable}}, }, expected: []catalog.PermissionsChange{ - { - Principal: "alice", - Add: []catalog.Privilege{ - catalog.PrivilegeAllPrivileges, - }, - }, - { - Principal: "bob", - Remove: []catalog.Privilege{ - catalog.PrivilegeAllPrivileges, - }, - }, + {Principal: "alice", Remove: []catalog.Privilege{catalog.PrivilegeCreateTable}}, + }, + }, + { + name: "revokes a principal absent from desired", + desired: []catalog.PrivilegeAssignment{ + {Principal: "alice", Privileges: []catalog.Privilege{catalog.PrivilegeApplyTag}}, + }, + remote: []catalog.PrivilegeAssignment{ + {Principal: "alice", Privileges: []catalog.Privilege{catalog.PrivilegeApplyTag}}, + {Principal: "bob", Privileges: []catalog.Privilege{catalog.PrivilegeCreateTable}}, + }, + expected: []catalog.PermissionsChange{ + {Principal: "bob", Remove: []catalog.Privilege{catalog.PrivilegeCreateTable}}, + }, + }, + { + // Regression test for #6030: a principal granted ALL_PRIVILEGES whose + // remote also has concrete privileges must have those removed so the + // deploy converges, instead of leaving them in place forever. + name: "removes concrete privileges when desired is ALL_PRIVILEGES", + desired: []catalog.PrivilegeAssignment{ + {Principal: "alice", Privileges: []catalog.Privilege{catalog.PrivilegeAllPrivileges}}, + }, + remote: []catalog.PrivilegeAssignment{ + {Principal: "alice", Privileges: []catalog.Privilege{catalog.PrivilegeAllPrivileges, catalog.PrivilegeUseCatalog}}, + }, + expected: []catalog.PermissionsChange{ + {Principal: "alice", Remove: []catalog.Privilege{catalog.PrivilegeUseCatalog}}, + }, + }, + { + name: "no change when desired equals remote", + desired: []catalog.PrivilegeAssignment{ + {Principal: "alice", Privileges: []catalog.Privilege{catalog.PrivilegeAllPrivileges}}, + }, + remote: []catalog.PrivilegeAssignment{ + {Principal: "alice", Privileges: []catalog.Privilege{catalog.PrivilegeAllPrivileges}}, + }, + expected: []catalog.PermissionsChange{}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.expected, buildGrantChanges(tt.desired, tt.remote)) + }) + } +} + +func TestNormalizeAssignments(t *testing.T) { + tests := []struct { + name string + input []catalog.PrivilegeAssignment + expected []catalog.PrivilegeAssignment + }{ + { + name: "uppercases and sorts privileges", + input: []catalog.PrivilegeAssignment{ + {Principal: "alice", Privileges: []catalog.Privilege{"use_schema", "apply_tag"}}, + }, + expected: []catalog.PrivilegeAssignment{ + {Principal: "alice", Privileges: []catalog.Privilege{"APPLY_TAG", "USE_SCHEMA"}}, + }, + }, + { + name: "converts spaces to underscores", + input: []catalog.PrivilegeAssignment{ + {Principal: "alice", Privileges: []catalog.Privilege{"create table", "USE SCHEMA"}}, + }, + expected: []catalog.PrivilegeAssignment{ + {Principal: "alice", Privileges: []catalog.Privilege{"CREATE_TABLE", "USE_SCHEMA"}}, }, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - assert.Equal(t, tt.expected, buildGrantChanges(tt.desired, tt.removed)) + normalizeAssignments(tt.input) + assert.Equal(t, tt.expected, tt.input) }) } } diff --git a/libs/testserver/grants.go b/libs/testserver/grants.go index 8a5be42ed91..fea9204e401 100644 --- a/libs/testserver/grants.go +++ b/libs/testserver/grants.go @@ -4,7 +4,6 @@ import ( "encoding/json" "fmt" "net/http" - "slices" "strings" "github.com/databricks/databricks-sdk-go/service/catalog" @@ -14,6 +13,12 @@ func grantsKey(securableType, fullName string) string { return strings.ToUpper(securableType) + ":" + fullName } +// normalizePrivilege mirrors UC: privilege names are uppercased and spaces are +// converted to underscores (e.g. "use schema" -> "USE_SCHEMA"). +func normalizePrivilege(p catalog.Privilege) catalog.Privilege { + return catalog.Privilege(strings.ToUpper(strings.ReplaceAll(string(p), " ", "_"))) +} + func (s *FakeWorkspace) GrantsUpdate(req Request, securableType, fullName string) Response { var request catalog.UpdatePermissions if err := json.Unmarshal(req.Body, &request); err != nil { @@ -44,10 +49,10 @@ func (s *FakeWorkspace) GrantsUpdate(req Request, securableType, fullName string for _, change := range request.Changes { addSet := make(map[catalog.Privilege]bool, len(change.Add)) for _, p := range change.Add { - addSet[p] = true + addSet[normalizePrivilege(p)] = true } for _, p := range change.Remove { - if addSet[p] { + if addSet[normalizePrivilege(p)] { return Response{ StatusCode: http.StatusBadRequest, Body: map[string]string{ @@ -70,21 +75,25 @@ func (s *FakeWorkspace) GrantsUpdate(req Request, securableType, fullName string // Remove privileges for _, privilege := range change.Remove { - if privilege == catalog.PrivilegeAllPrivileges { + if normalizePrivilege(privilege) == catalog.PrivilegeAllPrivileges { principalPrivs[change.Principal] = make(map[catalog.Privilege]bool) } else { - delete(principalPrivs[change.Principal], privilege) + delete(principalPrivs[change.Principal], normalizePrivilege(privilege)) } } - // Add privileges + // Add privileges. UC uppercases privilege names and converts spaces to + // underscores, so mirror that here (e.g. "use schema" -> "USE_SCHEMA"). for _, privilege := range change.Add { - principalPrivs[change.Principal][privilege] = true + principalPrivs[change.Principal][normalizePrivilege(privilege)] = true } } - // Convert back to assignments with sorted privileges - // Note order of assignments is randomized due to map. This is intentional, azure backend behaves the same way + // Convert back to assignments. + // Note order of assignments AND privileges is randomized due to map iteration. + // This is intentional: UC does not guarantee a stable order (assignment order + // matches azure backend behavior; privilege order is unspecified across clouds), + // and the CLI must not rely on it. // (deco env run -i -n azure-prod-ucws -- go test ./acceptance -run ^TestAccept$/^bundle$/^resources$/^grants$/^schemas$/^out_of_band_principal$/direct -count=10 -failfast -timeout=1h) var assignments []catalog.PrivilegeAssignment @@ -93,14 +102,10 @@ func (s *FakeWorkspace) GrantsUpdate(req Request, securableType, fullName string continue } - // Collect and sort privileges directly privileges := make([]catalog.Privilege, 0, len(privs)) for priv := range privs { privileges = append(privileges, priv) } - slices.SortFunc(privileges, func(a, b catalog.Privilege) int { - return strings.Compare(string(a), string(b)) - }) assignments = append(assignments, catalog.PrivilegeAssignment{ Principal: principal, From 7a3ac12f70e193474451f8fab65f1e439fea4ec5 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Fri, 24 Jul 2026 18:10:50 +0200 Subject: [PATCH 2/3] direct: collapse ALL_PRIVILEGES instead of diffing grants Revert the per-principal grant diff and keep the original buildGrantChanges (ALL_PRIVILEGES still wipes other privileges). Instead fix the false drift at comparison time: normalizeAssignments collapses a principal holding ALL_PRIVILEGES down to just ALL_PRIVILEGES on both the config and read sides, so config granting only ALL_PRIVILEGES matches a backend that also reports the concrete privileges it implies (#6030). --- .nextchanges/bundles/grants-normalization.md | 2 +- .../catalogs/out.deploy1.requests.direct.json | 5 +- .../catalogs/out.deploy2.requests.direct.json | 3 +- .../out.deploy1.requests.direct.json | 5 +- .../out.plan.direct.txt | 3 + .../out.plan.terraform.txt | 5 + .../schemas/all_privileges_coexist/output.txt | 18 --- .../schemas/all_privileges_coexist/script | 16 +-- .../volumes/out.deploy1.requests.direct.json | 5 +- .../volumes/out.deploy2.requests.direct.json | 5 +- bundle/direct/dresources/grants.go | 103 +++++++----------- bundle/direct/dresources/grants_test.go | 102 +++++++++-------- 12 files changed, 125 insertions(+), 147 deletions(-) create mode 100644 acceptance/bundle/resources/grants/schemas/all_privileges_coexist/out.plan.direct.txt create mode 100644 acceptance/bundle/resources/grants/schemas/all_privileges_coexist/out.plan.terraform.txt diff --git a/.nextchanges/bundles/grants-normalization.md b/.nextchanges/bundles/grants-normalization.md index 6a20b8f8fb0..c451dec9ad1 100644 --- a/.nextchanges/bundles/grants-normalization.md +++ b/.nextchanges/bundles/grants-normalization.md @@ -1 +1 @@ -Fixed spurious `update` on catalog/schema/volume grants (direct engine): grants with `ALL_PRIVILEGES` now converge when the backend also reports concrete privileges, and privilege names are normalized to match the case and ordering Unity Catalog returns ([#6030](https://github.com/databricks/cli/issues/6030)). +Fixed spurious `update` on catalog/schema/volume grants (direct engine): a principal granted `ALL_PRIVILEGES` no longer drifts when the backend also reports the concrete privileges it implies, and privilege names are normalized to match the case and ordering Unity Catalog returns ([#6030](https://github.com/databricks/cli/issues/6030)). diff --git a/acceptance/bundle/resources/grants/catalogs/out.deploy1.requests.direct.json b/acceptance/bundle/resources/grants/catalogs/out.deploy1.requests.direct.json index 68245ff9fdc..16f913b8a77 100644 --- a/acceptance/bundle/resources/grants/catalogs/out.deploy1.requests.direct.json +++ b/acceptance/bundle/resources/grants/catalogs/out.deploy1.requests.direct.json @@ -8,7 +8,10 @@ "CREATE_SCHEMA", "USE_CATALOG" ], - "principal": "deco-test-user@databricks.com" + "principal": "deco-test-user@databricks.com", + "remove": [ + "ALL_PRIVILEGES" + ] } ] } diff --git a/acceptance/bundle/resources/grants/catalogs/out.deploy2.requests.direct.json b/acceptance/bundle/resources/grants/catalogs/out.deploy2.requests.direct.json index 68eb137c9c2..47e1335feae 100644 --- a/acceptance/bundle/resources/grants/catalogs/out.deploy2.requests.direct.json +++ b/acceptance/bundle/resources/grants/catalogs/out.deploy2.requests.direct.json @@ -5,11 +5,12 @@ "changes": [ { "add": [ + "USE_CATALOG", "USE_SCHEMA" ], "principal": "deco-test-user@databricks.com", "remove": [ - "CREATE_SCHEMA" + "ALL_PRIVILEGES" ] } ] diff --git a/acceptance/bundle/resources/grants/registered_models/out.deploy1.requests.direct.json b/acceptance/bundle/resources/grants/registered_models/out.deploy1.requests.direct.json index 81ca56fd5da..8491f706c62 100644 --- a/acceptance/bundle/resources/grants/registered_models/out.deploy1.requests.direct.json +++ b/acceptance/bundle/resources/grants/registered_models/out.deploy1.requests.direct.json @@ -7,7 +7,10 @@ "add": [ "APPLY_TAG" ], - "principal": "deco-test-user@databricks.com" + "principal": "deco-test-user@databricks.com", + "remove": [ + "ALL_PRIVILEGES" + ] } ] } diff --git a/acceptance/bundle/resources/grants/schemas/all_privileges_coexist/out.plan.direct.txt b/acceptance/bundle/resources/grants/schemas/all_privileges_coexist/out.plan.direct.txt new file mode 100644 index 00000000000..068a177d51f --- /dev/null +++ b/acceptance/bundle/resources/grants/schemas/all_privileges_coexist/out.plan.direct.txt @@ -0,0 +1,3 @@ + +>>> [CLI] bundle plan +Plan: 0 to add, 0 to change, 0 to delete, 2 unchanged diff --git a/acceptance/bundle/resources/grants/schemas/all_privileges_coexist/out.plan.terraform.txt b/acceptance/bundle/resources/grants/schemas/all_privileges_coexist/out.plan.terraform.txt new file mode 100644 index 00000000000..8c7f94dfa63 --- /dev/null +++ b/acceptance/bundle/resources/grants/schemas/all_privileges_coexist/out.plan.terraform.txt @@ -0,0 +1,5 @@ + +>>> [CLI] bundle plan +update schemas.grants_schema.grants + +Plan: 0 to add, 1 to change, 0 to delete, 1 unchanged diff --git a/acceptance/bundle/resources/grants/schemas/all_privileges_coexist/output.txt b/acceptance/bundle/resources/grants/schemas/all_privileges_coexist/output.txt index 64b2054b444..a8605d5faef 100644 --- a/acceptance/bundle/resources/grants/schemas/all_privileges_coexist/output.txt +++ b/acceptance/bundle/resources/grants/schemas/all_privileges_coexist/output.txt @@ -7,24 +7,6 @@ Deployment complete! >>> [CLI] grants update schema main.schema_all_priv_coexist_[UNIQUE_NAME] --json @update.resolved.json ->>> [CLI] bundle plan -update schemas.grants_schema.grants - -Plan: 0 to add, 1 to change, 0 to delete, 1 unchanged - ->>> [CLI] bundle deploy -Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/schema-grants-all-privileges-coexist-[UNIQUE_NAME]/default/files... -Deploying resources... -Updating deployment state... -Deployment complete! - ->>> [CLI] bundle plan -Plan: 0 to add, 0 to change, 0 to delete, 2 unchanged - ->>> [CLI] grants get schema main.schema_all_priv_coexist_[UNIQUE_NAME] -json.privilege_assignments[].principal = "[USERNAME]"; -json.privilege_assignments[].privileges[] = "ALL_PRIVILEGES"; - >>> errcode [CLI] bundle destroy --auto-approve The following resources will be deleted: delete resources.schemas.grants_schema diff --git a/acceptance/bundle/resources/grants/schemas/all_privileges_coexist/script b/acceptance/bundle/resources/grants/schemas/all_privileges_coexist/script index a49e7bf0fd2..f80ade0cedc 100644 --- a/acceptance/bundle/resources/grants/schemas/all_privileges_coexist/script +++ b/acceptance/bundle/resources/grants/schemas/all_privileges_coexist/script @@ -12,17 +12,11 @@ trace $CLI bundle deploy # Out of band, grant a concrete privilege in addition to the ALL_PRIVILEGES that # the config declares for the same principal. This reproduces issue #6030: the -# backend then reports both ALL_PRIVILEGES and the concrete privilege, and the -# deploy must revoke the concrete privilege to converge. +# backend then reports both ALL_PRIVILEGES and the concrete privilege. envsubst < update.json > update.resolved.json trace $CLI grants update schema "$SCHEMA_FULL_NAME" --json @update.resolved.json > /dev/null -# The plan detects the extra privilege and applies the revocation on deploy. -trace $CLI bundle plan -trace $CLI bundle deploy - -# The plan now converges to a no-op instead of reporting a perpetual update. -trace $CLI bundle plan - -# Only ALL_PRIVILEGES remains for the principal. -trace $CLI grants get schema "$SCHEMA_FULL_NAME" | gron.py --noindex | sort_lines.py --repl +# The plan output differs between engines: the direct engine treats ALL_PRIVILEGES +# as implying every concrete privilege, so it reports no drift (issue #6030), +# while terraform revokes the extra privilege to match the config exactly. +trace $CLI bundle plan > out.plan.$DATABRICKS_BUNDLE_ENGINE.txt 2>&1 diff --git a/acceptance/bundle/resources/grants/volumes/out.deploy1.requests.direct.json b/acceptance/bundle/resources/grants/volumes/out.deploy1.requests.direct.json index 1e8bdf282a7..782eecac450 100644 --- a/acceptance/bundle/resources/grants/volumes/out.deploy1.requests.direct.json +++ b/acceptance/bundle/resources/grants/volumes/out.deploy1.requests.direct.json @@ -8,7 +8,10 @@ "READ_VOLUME", "WRITE_VOLUME" ], - "principal": "deco-test-user@databricks.com" + "principal": "deco-test-user@databricks.com", + "remove": [ + "ALL_PRIVILEGES" + ] } ] } diff --git a/acceptance/bundle/resources/grants/volumes/out.deploy2.requests.direct.json b/acceptance/bundle/resources/grants/volumes/out.deploy2.requests.direct.json index 5a2ad0d6eab..fb9d6e90ba3 100644 --- a/acceptance/bundle/resources/grants/volumes/out.deploy2.requests.direct.json +++ b/acceptance/bundle/resources/grants/volumes/out.deploy2.requests.direct.json @@ -5,11 +5,12 @@ "changes": [ { "add": [ - "MANAGE" + "MANAGE", + "READ_VOLUME" ], "principal": "deco-test-user@databricks.com", "remove": [ - "WRITE_VOLUME" + "ALL_PRIVILEGES" ] } ] diff --git a/bundle/direct/dresources/grants.go b/bundle/direct/dresources/grants.go index 4821eb9283f..6e4ce734f2a 100644 --- a/bundle/direct/dresources/grants.go +++ b/bundle/direct/dresources/grants.go @@ -120,10 +120,11 @@ func (r *ResourceGrants) DoUpdate(ctx context.Context, _ string, state *GrantsSt if state.FullName == "" { return nil, errors.New("internal error: grants full_name must be resolved before deployment") } + removedPrincipals := removedGrantPrincipals(state.EmbeddedSlice, entry) _, err := r.client.Grants.Update(ctx, catalog.UpdatePermissions{ SecurableType: state.SecurableType, FullName: state.FullName, - Changes: buildGrantChanges(state.EmbeddedSlice, remoteAssignments(entry)), + Changes: buildGrantChanges(state.EmbeddedSlice, removedPrincipals), OmitPermissionsInResponse: false, ForceSendFields: nil, }) @@ -136,52 +137,35 @@ func (r *ResourceGrants) DoDelete(ctx context.Context, id string, _ *GrantsState return nil } -// buildGrantChanges computes the per-principal add/remove diff between the -// desired assignments and the remote assignments, mirroring the Terraform -// provider's diffPermissions. For each principal Add is (desired - remote) and -// Remove is (remote - desired). Computing an exact diff (rather than sending a -// blanket "remove ALL_PRIVILEGES" to wipe the principal) is what lets a -// principal granted ALL_PRIVILEGES converge when the backend also reports -// concrete privileges for it: those extra privileges land in Remove instead of -// being left in place forever. A privilege can never be in both Add and Remove, -// so this also avoids the "Duplicate privileges to add and delete" API error. -func buildGrantChanges(desiredAssignments, remoteAssignments []catalog.PrivilegeAssignment) []catalog.PermissionsChange { - desired := privilegesByPrincipal(desiredAssignments) - remote := privilegesByPrincipal(remoteAssignments) - - principals := make([]string, 0, len(desired)+len(remote)) - for p := range desired { - principals = append(principals, p) - } - for p := range remote { - if _, ok := desired[p]; !ok { - principals = append(principals, p) +func buildGrantChanges(desiredAssignments []catalog.PrivilegeAssignment, removedPrincipals []string) []catalog.PermissionsChange { + changes := make([]catalog.PermissionsChange, 0, len(desiredAssignments)+len(removedPrincipals)) + for _, ga := range desiredAssignments { + change := catalog.PermissionsChange{ + Principal: ga.Principal, + Add: ga.Privileges, + Remove: nil, + ForceSendFields: nil, } - } - slices.Sort(principals) - - changes := make([]catalog.PermissionsChange, 0, len(principals)) - for _, principal := range principals { - add := setDifference(desired[principal], remote[principal]) - remove := setDifference(remote[principal], desired[principal]) - if len(add) == 0 && len(remove) == 0 { - continue + // Remove all other privileges unless ALL_PRIVILEGES is being granted + // (it would conflict with appearing in both Add and Remove). + if !slices.Contains(ga.Privileges, catalog.PrivilegeAllPrivileges) { + change.Remove = []catalog.Privilege{catalog.PrivilegeAllPrivileges} } + changes = append(changes, change) + } + for _, principal := range removedPrincipals { changes = append(changes, catalog.PermissionsChange{ Principal: principal, - Add: add, - Remove: remove, + Add: nil, + Remove: []catalog.Privilege{catalog.PrivilegeAllPrivileges}, ForceSendFields: nil, }) } return changes } -// remoteAssignments extracts the remote privilege assignments from the plan -// entry. It returns nil when the entry or its remote state is absent, which -// happens on create and when deploying from a serialized plan (the JSON-loaded -// state is not a *GrantsState). In those cases nothing is revoked. -func remoteAssignments(entry *PlanEntry) []catalog.PrivilegeAssignment { +// removedGrantPrincipals returns principals present in the remote state but absent from the desired assignments. +func removedGrantPrincipals(desiredAssignments []catalog.PrivilegeAssignment, entry *PlanEntry) []string { if entry == nil { return nil } @@ -189,33 +173,18 @@ func remoteAssignments(entry *PlanEntry) []catalog.PrivilegeAssignment { if !ok || remote == nil { return nil } - return remote.EmbeddedSlice -} -func privilegesByPrincipal(assignments []catalog.PrivilegeAssignment) map[string]map[catalog.Privilege]struct{} { - result := make(map[string]map[catalog.Privilege]struct{}, len(assignments)) - for _, a := range assignments { - if a.Principal == "" { - continue - } - privs := result[a.Principal] - if privs == nil { - privs = make(map[catalog.Privilege]struct{}, len(a.Privileges)) - result[a.Principal] = privs - } - for _, p := range a.Privileges { - privs[p] = struct{}{} + desired := make(map[string]struct{}, len(desiredAssignments)) + for _, a := range desiredAssignments { + if a.Principal != "" { + desired[a.Principal] = struct{}{} } } - return result -} -// setDifference returns the sorted privileges present in a but not in b. -func setDifference(a, b map[catalog.Privilege]struct{}) []catalog.Privilege { - var result []catalog.Privilege - for p := range a { - if _, ok := b[p]; !ok { - result = append(result, p) + var result []string + for _, a := range remote.EmbeddedSlice { + if _, ok := desired[a.Principal]; !ok { + result = append(result, a.Principal) } } slices.Sort(result) @@ -265,14 +234,22 @@ func normalizePrivilege(p catalog.Privilege) catalog.Privilege { return catalog.Privilege(strings.ToUpper(strings.ReplaceAll(string(p), " ", "_"))) } -// normalizeAssignments normalizes and sorts each assignment's privileges in -// place so that config and remote state compare equal regardless of the -// case or order the backend returns. +// normalizeAssignments normalizes each assignment's privileges in place so that +// config and remote state compare equal. It uppercases privilege names, sorts +// them (the backend does not guarantee a stable order), and collapses a +// principal holding ALL_PRIVILEGES down to just ALL_PRIVILEGES. The collapse is +// applied to both sides, so config granting only ALL_PRIVILEGES matches a +// backend that reports ALL_PRIVILEGES plus the concrete privileges it implies, +// instead of reporting a perpetual update. func normalizeAssignments(assignments []catalog.PrivilegeAssignment) { for i := range assignments { for j := range assignments[i].Privileges { assignments[i].Privileges[j] = normalizePrivilege(assignments[i].Privileges[j]) } + if slices.Contains(assignments[i].Privileges, catalog.PrivilegeAllPrivileges) { + assignments[i].Privileges = []catalog.Privilege{catalog.PrivilegeAllPrivileges} + continue + } slices.Sort(assignments[i].Privileges) } } diff --git a/bundle/direct/dresources/grants_test.go b/bundle/direct/dresources/grants_test.go index f3875a01796..be647336cc4 100644 --- a/bundle/direct/dresources/grants_test.go +++ b/bundle/direct/dresources/grants_test.go @@ -11,73 +11,66 @@ func TestBuildGrantChanges(t *testing.T) { tests := []struct { name string desired []catalog.PrivilegeAssignment - remote []catalog.PrivilegeAssignment + removed []string expected []catalog.PermissionsChange }{ { - name: "adds privileges for a new principal (no remote)", + name: "removes all other privileges for desired principal", desired: []catalog.PrivilegeAssignment{ - {Principal: "alice", Privileges: []catalog.Privilege{catalog.PrivilegeApplyTag, catalog.PrivilegeCreateTable}}, + { + Principal: "alice", + Privileges: []catalog.Privilege{ + catalog.PrivilegeApplyTag, + catalog.PrivilegeCreateTable, + }, + }, }, expected: []catalog.PermissionsChange{ - {Principal: "alice", Add: []catalog.Privilege{catalog.PrivilegeApplyTag, catalog.PrivilegeCreateTable}}, + { + Principal: "alice", + Add: []catalog.Privilege{ + catalog.PrivilegeApplyTag, + catalog.PrivilegeCreateTable, + }, + Remove: []catalog.Privilege{ + catalog.PrivilegeAllPrivileges, + }, + }, }, }, { - name: "removes privileges present in remote but not desired", + name: "skips ALL_PRIVILEGES removal when granting ALL_PRIVILEGES", desired: []catalog.PrivilegeAssignment{ - {Principal: "alice", Privileges: []catalog.Privilege{catalog.PrivilegeApplyTag}}, + { + Principal: "alice", + Privileges: []catalog.Privilege{ + catalog.PrivilegeAllPrivileges, + }, + }, }, - remote: []catalog.PrivilegeAssignment{ - {Principal: "alice", Privileges: []catalog.Privilege{catalog.PrivilegeApplyTag, catalog.PrivilegeCreateTable}}, + removed: []string{ + "bob", }, expected: []catalog.PermissionsChange{ - {Principal: "alice", Remove: []catalog.Privilege{catalog.PrivilegeCreateTable}}, + { + Principal: "alice", + Add: []catalog.Privilege{ + catalog.PrivilegeAllPrivileges, + }, + }, + { + Principal: "bob", + Remove: []catalog.Privilege{ + catalog.PrivilegeAllPrivileges, + }, + }, }, }, - { - name: "revokes a principal absent from desired", - desired: []catalog.PrivilegeAssignment{ - {Principal: "alice", Privileges: []catalog.Privilege{catalog.PrivilegeApplyTag}}, - }, - remote: []catalog.PrivilegeAssignment{ - {Principal: "alice", Privileges: []catalog.Privilege{catalog.PrivilegeApplyTag}}, - {Principal: "bob", Privileges: []catalog.Privilege{catalog.PrivilegeCreateTable}}, - }, - expected: []catalog.PermissionsChange{ - {Principal: "bob", Remove: []catalog.Privilege{catalog.PrivilegeCreateTable}}, - }, - }, - { - // Regression test for #6030: a principal granted ALL_PRIVILEGES whose - // remote also has concrete privileges must have those removed so the - // deploy converges, instead of leaving them in place forever. - name: "removes concrete privileges when desired is ALL_PRIVILEGES", - desired: []catalog.PrivilegeAssignment{ - {Principal: "alice", Privileges: []catalog.Privilege{catalog.PrivilegeAllPrivileges}}, - }, - remote: []catalog.PrivilegeAssignment{ - {Principal: "alice", Privileges: []catalog.Privilege{catalog.PrivilegeAllPrivileges, catalog.PrivilegeUseCatalog}}, - }, - expected: []catalog.PermissionsChange{ - {Principal: "alice", Remove: []catalog.Privilege{catalog.PrivilegeUseCatalog}}, - }, - }, - { - name: "no change when desired equals remote", - desired: []catalog.PrivilegeAssignment{ - {Principal: "alice", Privileges: []catalog.Privilege{catalog.PrivilegeAllPrivileges}}, - }, - remote: []catalog.PrivilegeAssignment{ - {Principal: "alice", Privileges: []catalog.Privilege{catalog.PrivilegeAllPrivileges}}, - }, - expected: []catalog.PermissionsChange{}, - }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - assert.Equal(t, tt.expected, buildGrantChanges(tt.desired, tt.remote)) + assert.Equal(t, tt.expected, buildGrantChanges(tt.desired, tt.removed)) }) } } @@ -106,6 +99,19 @@ func TestNormalizeAssignments(t *testing.T) { {Principal: "alice", Privileges: []catalog.Privilege{"CREATE_TABLE", "USE_SCHEMA"}}, }, }, + { + // Regression test for #6030: ALL_PRIVILEGES implies every concrete + // privilege, so a principal holding it collapses to just + // ALL_PRIVILEGES. Applied to both config and remote, this stops the + // backend's extra concrete privileges from showing as drift. + name: "collapses ALL_PRIVILEGES with concrete privileges", + input: []catalog.PrivilegeAssignment{ + {Principal: "alice", Privileges: []catalog.Privilege{catalog.PrivilegeUseCatalog, catalog.PrivilegeAllPrivileges, catalog.PrivilegeCreateSchema}}, + }, + expected: []catalog.PrivilegeAssignment{ + {Principal: "alice", Privileges: []catalog.Privilege{catalog.PrivilegeAllPrivileges}}, + }, + }, } for _, tt := range tests { From 4ff0948a0d6a6151d20b656fc9382c9e29eaf8f5 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Fri, 24 Jul 2026 18:20:42 +0200 Subject: [PATCH 3/3] direct: dedupe grant privileges after normalization MergeGrants deduplicates the raw config strings before normalization, so distinct spellings of the same privilege (e.g. "use schema" and "USE_SCHEMA") only collide once normalizeAssignments canonicalizes them. Compact after sorting so the config side does not keep a duplicate the backend read side never returns, which would otherwise cause perpetual drift. --- bundle/direct/dresources/grants.go | 6 ++++++ bundle/direct/dresources/grants_test.go | 11 +++++++++++ 2 files changed, 17 insertions(+) diff --git a/bundle/direct/dresources/grants.go b/bundle/direct/dresources/grants.go index 6e4ce734f2a..fcec8ed2afa 100644 --- a/bundle/direct/dresources/grants.go +++ b/bundle/direct/dresources/grants.go @@ -251,6 +251,12 @@ func normalizeAssignments(assignments []catalog.PrivilegeAssignment) { continue } slices.Sort(assignments[i].Privileges) + // Dedupe here rather than relying on MergeGrants: it deduplicates the raw + // config strings before normalization, so distinct spellings of the same + // privilege (e.g. "use schema" and "USE_SCHEMA") only collide once + // normalized. Without this the config side keeps a duplicate the backend + // read side never returns, causing perpetual drift. + assignments[i].Privileges = slices.Compact(assignments[i].Privileges) } } diff --git a/bundle/direct/dresources/grants_test.go b/bundle/direct/dresources/grants_test.go index be647336cc4..aadd4122126 100644 --- a/bundle/direct/dresources/grants_test.go +++ b/bundle/direct/dresources/grants_test.go @@ -112,6 +112,17 @@ func TestNormalizeAssignments(t *testing.T) { {Principal: "alice", Privileges: []catalog.Privilege{catalog.PrivilegeAllPrivileges}}, }, }, + { + // Distinct spellings collide only after normalization, so dedupe + // here; MergeGrants deduplicates the raw config strings earlier. + name: "deduplicates privileges that collide after normalization", + input: []catalog.PrivilegeAssignment{ + {Principal: "alice", Privileges: []catalog.Privilege{"USE SCHEMA", "use_schema", "USE_SCHEMA"}}, + }, + expected: []catalog.PrivilegeAssignment{ + {Principal: "alice", Privileges: []catalog.Privilege{"USE_SCHEMA"}}, + }, + }, } for _, tt := range tests {