diff --git a/.nextchanges/bundles/grants-all-privileges.md b/.nextchanges/bundles/grants-all-privileges.md new file mode 100644 index 00000000000..d5e7b42f14c --- /dev/null +++ b/.nextchanges/bundles/grants-all-privileges.md @@ -0,0 +1 @@ +Fixes [#6030](https://github.com/databricks/cli/issues/6030): 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 ([#6064](https://github.com/databricks/cli/pull/6064)). 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.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/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..8c6cc0fcd0d --- /dev/null +++ b/acceptance/bundle/resources/grants/schemas/all_privileges_coexist/output.txt @@ -0,0 +1,29 @@ + +>>> [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 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 + +>>> 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..bf04258675f --- /dev/null +++ b/acceptance/bundle/resources/grants/schemas/all_privileges_coexist/script @@ -0,0 +1,26 @@ +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. +envsubst < update.json > update.resolved.json +trace $CLI grants update schema "$SCHEMA_FULL_NAME" --json @update.resolved.json > /dev/null + +# The first plan 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 + +# After deploying, both engines converge: the follow-up plan reports no changes. +trace $CLI bundle deploy +trace $CLI bundle plan 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/bundle/direct/dresources/grants.go b/bundle/direct/dresources/grants.go index bac70bc5615..f0a9423838c 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 as DoRead (sort, collapse ALL_PRIVILEGES) so the + // config and the value read back compare equal. + normalizeAssignments(*grantsPtr) return &structvar.StructVar{ Value: &GrantsState{ @@ -222,9 +221,28 @@ func (r *ResourceGrants) listGrants(ctx context.Context, securableType, fullName } pageToken = resp.NextPageToken } + // Normalize the same way as the config side (sort, collapse ALL_PRIVILEGES) + // so the two compare equal and we don't report false drift. + normalizeAssignments(assignments) return assignments, nil } +// normalizeAssignments sorts each assignment's privileges (the backend sorts +// them, so we match that) and collapses a principal holding ALL_PRIVILEGES down +// to just ALL_PRIVILEGES. The collapse is applied to both the config and read +// 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 { + if slices.Contains(assignments[i].Privileges, catalog.PrivilegeAllPrivileges) { + assignments[i].Privileges = []catalog.Privilege{catalog.PrivilegeAllPrivileges} + continue + } + 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..a1c5895a33d 100644 --- a/bundle/direct/dresources/grants_test.go +++ b/bundle/direct/dresources/grants_test.go @@ -74,3 +74,41 @@ func TestBuildGrantChanges(t *testing.T) { }) } } + +func TestNormalizeAssignments(t *testing.T) { + tests := []struct { + name string + input []catalog.PrivilegeAssignment + expected []catalog.PrivilegeAssignment + }{ + { + name: "sorts privileges", + input: []catalog.PrivilegeAssignment{ + {Principal: "alice", Privileges: []catalog.Privilege{catalog.PrivilegeUseSchema, catalog.PrivilegeApplyTag}}, + }, + expected: []catalog.PrivilegeAssignment{ + {Principal: "alice", Privileges: []catalog.Privilege{catalog.PrivilegeApplyTag, catalog.PrivilegeUseSchema}}, + }, + }, + { + // 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 { + t.Run(tt.name, func(t *testing.T) { + normalizeAssignments(tt.input) + assert.Equal(t, tt.expected, tt.input) + }) + } +}