diff --git a/.nextchanges/bundles/empty-grants-migrate.md b/.nextchanges/bundles/empty-grants-migrate.md new file mode 100644 index 00000000000..fa6707d3688 --- /dev/null +++ b/.nextchanges/bundles/empty-grants-migrate.md @@ -0,0 +1 @@ +Fixed the direct deployment engine planning a spurious `create` for an empty `grants: []` list. Terraform records no grants resource for such a list, so `bundle plan` after `bundle deployment migrate` no longer reports an action for it. Emptying a previously deployed list still revokes the grants, after which the node is dropped from the deployment state instead of being reported as unchanged forever. diff --git a/acceptance/bundle/invariant/migrate/out.test.toml b/acceptance/bundle/invariant/migrate/out.test.toml index 0ac1b789d6f..469a1c81839 100644 --- a/acceptance/bundle/invariant/migrate/out.test.toml +++ b/acceptance/bundle/invariant/migrate/out.test.toml @@ -34,6 +34,7 @@ EnvMatrix.INPUT_CONFIG = [ "postgres_synced_table.yml.tmpl", "registered_model.yml.tmpl", "schema.yml.tmpl", + "schema_empty_grants.yml.tmpl", "schema_uppercase_name.yml.tmpl", "secret_scope.yml.tmpl", "secret_scope_default_backend_type.yml.tmpl", diff --git a/acceptance/bundle/invariant/migrate/test.toml b/acceptance/bundle/invariant/migrate/test.toml index 37dc2557922..a91f990df3b 100644 --- a/acceptance/bundle/invariant/migrate/test.toml +++ b/acceptance/bundle/invariant/migrate/test.toml @@ -26,17 +26,6 @@ EnvMatrixExclude.no_cross_resource_ref = ["INPUT_CONFIG=job_cross_resource_ref.y # Grant cross-references require the EmbeddedSlice pattern not present in terraform mode. EnvMatrixExclude.no_grant_ref = ["INPUT_CONFIG=schema_grant_ref.yml.tmpl"] -# An empty grants list plans a spurious create after migrate: Terraform records no -# databricks_grants resource for grants: [], so migrate leaves no state entry for the -# grants node, but the direct plan emits a "create" for it anyway. Found by fuzz testing. -# The plan check fails with: -# Unexpected action='create' for resources.schemas.foo.grants -# ... -# "resources.schemas.foo.grants": { "action": "create", ... } -# Exit code: 10 -# Fixed by https://github.com/databricks/cli/pull/6039; re-enable once that lands. -EnvMatrixExclude.no_empty_grants = ["INPUT_CONFIG=schema_empty_grants.yml.tmpl"] - # SQL warehouses currently failing with migration with permanent drift. TODO: fix this. EnvMatrixExclude.no_sql_warehouse = ["INPUT_CONFIG=sql_warehouse.yml.tmpl"] diff --git a/acceptance/bundle/resources/grants/schemas/remove_all/databricks.yml.tmpl b/acceptance/bundle/resources/grants/schemas/remove_all/databricks.yml.tmpl new file mode 100644 index 00000000000..00752f7b34f --- /dev/null +++ b/acceptance/bundle/resources/grants/schemas/remove_all/databricks.yml.tmpl @@ -0,0 +1,9 @@ +bundle: + name: schema-grants-remove-all-$UNIQUE_NAME + +resources: + schemas: + grants_schema: + name: schema_remove_all_$UNIQUE_NAME + catalog_name: main + grants: [{ principal: deco-test-user@databricks.com, privileges: [USE_SCHEMA] }] diff --git a/acceptance/bundle/resources/grants/schemas/remove_all/out.plan.txt b/acceptance/bundle/resources/grants/schemas/remove_all/out.plan.txt new file mode 100644 index 00000000000..52a44d17f5a --- /dev/null +++ b/acceptance/bundle/resources/grants/schemas/remove_all/out.plan.txt @@ -0,0 +1,12 @@ + +>>> [CLI] bundle plan +Plan: 0 to add, 0 to change, 0 to delete, 1 unchanged + +>>> [CLI] bundle deploy +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/schema-grants-remove-all-[UNIQUE_NAME]/default/files... +Deploying resources... +Updating deployment state... +Deployment complete! + +>>> [CLI] bundle plan +Plan: 0 to add, 0 to change, 0 to delete, 1 unchanged diff --git a/acceptance/bundle/resources/grants/schemas/remove_all/out.test.toml b/acceptance/bundle/resources/grants/schemas/remove_all/out.test.toml new file mode 100644 index 00000000000..e849ec85ace --- /dev/null +++ b/acceptance/bundle/resources/grants/schemas/remove_all/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/remove_all/output.txt b/acceptance/bundle/resources/grants/schemas/remove_all/output.txt new file mode 100644 index 00000000000..b2d5da5dca4 --- /dev/null +++ b/acceptance/bundle/resources/grants/schemas/remove_all/output.txt @@ -0,0 +1,31 @@ + +>>> [CLI] bundle deploy +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/schema-grants-remove-all-[UNIQUE_NAME]/default/files... +Deploying resources... +Updating deployment state... +Deployment complete! + +>>> [CLI] grants get schema main.schema_remove_all_[UNIQUE_NAME] +json.privilege_assignments[].principal = "deco-test-user@databricks.com"; +json.privilege_assignments[].privileges[] = "USE_SCHEMA"; + +>>> [CLI] bundle deploy +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/schema-grants-remove-all-[UNIQUE_NAME]/default/files... +Deploying resources... +Updating deployment state... +Deployment complete! + +>>> [CLI] grants get schema main.schema_remove_all_[UNIQUE_NAME] +json = {}; + +>>> 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-remove-all-[UNIQUE_NAME]/default + +Deleting files... +Destroy complete! diff --git a/acceptance/bundle/resources/grants/schemas/remove_all/script b/acceptance/bundle/resources/grants/schemas/remove_all/script new file mode 100644 index 00000000000..8d8c2f55e92 --- /dev/null +++ b/acceptance/bundle/resources/grants/schemas/remove_all/script @@ -0,0 +1,26 @@ +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 +trace $CLI grants get schema main.schema_remove_all_$UNIQUE_NAME | gron.py --noindex | sort_lines.py --repl | contains.py 'deco-test-user@databricks.com' + +# Empty the whole list. The grants node has state here, so it must stay in the plan +# and revoke; dropping it would move it to the delete branch where grants' no-op +# DoDelete silently keeps the grant. +update_file.py databricks.yml 'grants: [{ principal: deco-test-user@databricks.com, privileges: [USE_SCHEMA] }]' 'grants: []' + +trace $CLI bundle deploy +trace $CLI grants get schema main.schema_remove_all_$UNIQUE_NAME | gron.py --noindex | sort_lines.py --repl | contains.py '!deco-test-user@databricks.com' + +# The revoking deploy drops the emptied grants node from state, so from here on both +# engines plan the schema alone. +{ + trace $CLI bundle plan + trace $CLI bundle deploy + trace $CLI bundle plan +} &> out.plan.txt diff --git a/bundle/direct/apply.go b/bundle/direct/apply.go index b3c46036c53..cbb0a2d45ff 100644 --- a/bundle/direct/apply.go +++ b/bundle/direct/apply.go @@ -149,9 +149,24 @@ func (d *DeploymentUnit) Update(ctx context.Context, db *dstate.DeploymentState, return err } - err = db.SaveState(d.ResourceKey, id, newState, d.DependsOn) + empty, err := d.Adapter.IsEmptyState(newState) if err != nil { - return fmt.Errorf("saving state id=%s: %w", id, err) + return err + } + + if empty { + // The update emptied the resource out (e.g. all grants revoked). Keeping an entry + // would report the node as tracked-and-unchanged forever, while a fresh deploy of + // the same config plans no node at all; drop it so the two agree. + err = db.DeleteState(d.ResourceKey) + if err != nil { + return fmt.Errorf("deleting state id=%s: %w", id, err) + } + } else { + err = db.SaveState(d.ResourceKey, id, newState, d.DependsOn) + if err != nil { + return fmt.Errorf("saving state id=%s: %w", id, err) + } } waitRemoteState, err := retryOnTransient(ctx, func() (any, error) { diff --git a/bundle/direct/bundle_plan.go b/bundle/direct/bundle_plan.go index fda8180cfc4..739d9ae3cbd 100644 --- a/bundle/direct/bundle_plan.go +++ b/bundle/direct/bundle_plan.go @@ -975,6 +975,18 @@ func (b *DeploymentBundle) makePlan(ctx context.Context, configRoot *config.Root return nil, fmt.Errorf("%s: %w", prefix, err) } + // New nodes only: a node with state must stay in the plan, otherwise emptying it plans nothing. + // Apply drops the state entry once the node is empty, so it is skipped from then on. + if _, hasState := db.State[node]; !hasState { + empty, err := adapter.IsEmptyState(newStateConfig) + if err != nil { + return nil, fmt.Errorf("%s: %w", prefix, err) + } + if empty { + continue + } + } + // Note, we're extracting references in input config but resolving them in newState.Config which is PrepareState(inputConfig) // This means input and state must be compatible: input can have more fields, but existing fields should not be moved // This means one cannot refer to fields not present in state (e.g. ${resources.jobs.foo.permissions}) diff --git a/bundle/direct/dresources/README.md b/bundle/direct/dresources/README.md index da9e9edab28..b6c09bda4ed 100644 --- a/bundle/direct/dresources/README.md +++ b/bundle/direct/dresources/README.md @@ -42,6 +42,12 @@ For resources whose create or update is asynchronous (the resource is not immedi If the API may return a slice's elements in a different order between calls (e.g., `depends_on` in job tasks, `privileges` in grants), implement `KeyedSlices` to compare elements by a natural key rather than by index. Without this, every deploy after any reordering shows phantom diffs. +## Empty states: IsEmptyState + +If a desired state describes no resource at all (e.g. an empty grants list), implement `IsEmptyState`. The planner omits such a node instead of planning a create, and apply drops its state entry instead of persisting one. + +The planner only consults it for nodes without a state entry: once state exists the node stays in the plan, so emptying it still plans an update, and the update is what removes the entry. + ## State backward compatibility The state struct is serialized to JSON and persisted between deploys. Backward incompatible changes will result in a drift, which depending diff --git a/bundle/direct/dresources/adapter.go b/bundle/direct/dresources/adapter.go index fdaa15bfcea..ea8876c4d52 100644 --- a/bundle/direct/dresources/adapter.go +++ b/bundle/direct/dresources/adapter.go @@ -56,6 +56,12 @@ type IResource interface { // Example: func (r *ResourceVolume) DoCreate(ctx context.Context, newState *catalog.CreateVolumeRequestContent) (string, *catalog.VolumeInfo, error) DoCreate(ctx context.Context, newState any) (id string, remoteState any, e error) + // [Optional] IsEmptyState reports that newState describes no resource at all: the planner + // omits the node instead of planning a create, and apply drops the state entry instead of + // persisting one, so both engines converge on "this node does not exist". + // Example: func (*ResourceGrants) IsEmptyState(state *GrantsState) bool + IsEmptyState(newState any) bool + // [Optional] DoUpdate updates the resource. ID must not change as a result of this operation. Returns optionally remote state. // If remote state is available as part of the operation, return it; otherwise return nil. // Example: func (r *ResourceSchema) DoUpdate(ctx context.Context, id string, newState *catalog.CreateSchema, entry *PlanEntry) (*catalog.SchemaInfo, error) @@ -103,6 +109,7 @@ type Adapter struct { doCreate *calladapt.BoundCaller // Optional: + isEmptyState *calladapt.BoundCaller doUpdate *calladapt.BoundCaller doUpdateWithID *calladapt.BoundCaller waitAfterCreate *calladapt.BoundCaller @@ -136,6 +143,7 @@ func NewAdapter(typedNil any, resourceType string, client *databricks.WorkspaceC doRefresh: nil, doDelete: nil, doCreate: nil, + isEmptyState: nil, doUpdate: nil, doUpdateWithID: nil, doResize: nil, @@ -205,6 +213,11 @@ func (a *Adapter) initMethods(resource any) error { // Optional methods with varying signatures: + a.isEmptyState, err = calladapt.PrepareCall(resource, reflect.TypeFor[IResource](), "IsEmptyState") + if err != nil { + return err + } + a.doUpdate, err = calladapt.PrepareCall(resource, reflect.TypeFor[IResource](), "DoUpdate") if err != nil { return err @@ -313,6 +326,10 @@ func (a *Adapter) validate() error { } validations = append(validations, "DoCreate remoteState return", a.doCreate.OutTypes[1], remoteType) + if a.isEmptyState != nil { + validations = append(validations, "IsEmptyState newState", a.isEmptyState.InTypes[0], stateType) + } + // Validate DoUpdate: must return (remoteType, error) if implemented if a.doUpdate != nil { validations = append(validations, "DoUpdate newState", a.doUpdate.InTypes[2], stateType) @@ -470,6 +487,19 @@ func (a *Adapter) DoCreate(ctx context.Context, newState any) (string, any, erro return id, remoteState, nil } +// IsEmptyState reports whether newState describes no resource; false if not implemented. +func (a *Adapter) IsEmptyState(newState any) (bool, error) { + if a.isEmptyState == nil { + return false, nil + } + + outs, err := a.isEmptyState.Call(newState) + if err != nil { + return false, err + } + return outs[0].(bool), nil +} + // HasDoUpdate returns true if the resource implements DoUpdate method. func (a *Adapter) HasDoUpdate() bool { return a.doUpdate != nil diff --git a/bundle/direct/dresources/grants.go b/bundle/direct/dresources/grants.go index f0a9423838c..10e52446100 100644 --- a/bundle/direct/dresources/grants.go +++ b/bundle/direct/dresources/grants.go @@ -76,6 +76,13 @@ func (*ResourceGrants) PrepareState(state *GrantsState) *GrantsState { return state } +// IsEmptyState reports an empty grants list as no resource at all: nothing to grant, and +// Terraform records no databricks_grants resource for it either, so migrated bundles have +// no state entry. +func (*ResourceGrants) IsEmptyState(state *GrantsState) bool { + return len(state.EmbeddedSlice) == 0 +} + func grantKey(x catalog.PrivilegeAssignment) (string, string) { return "principal", x.Principal } diff --git a/bundle/direct/dresources/grants_test.go b/bundle/direct/dresources/grants_test.go index a1c5895a33d..faa72b02c25 100644 --- a/bundle/direct/dresources/grants_test.go +++ b/bundle/direct/dresources/grants_test.go @@ -5,6 +5,7 @@ import ( "github.com/databricks/databricks-sdk-go/service/catalog" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestBuildGrantChanges(t *testing.T) { @@ -75,6 +76,57 @@ func TestBuildGrantChanges(t *testing.T) { } } +// Calls through the adapter so the optional-method wiring is covered too. +func TestGrantsIsEmptyState(t *testing.T) { + tests := []struct { + name string + state *GrantsState + expected bool + }{ + { + name: "empty grants list", + state: &GrantsState{SecurableType: "schema", EmbeddedSlice: []catalog.PrivilegeAssignment{}}, + expected: true, + }, + { + name: "unset grants list", + state: &GrantsState{SecurableType: "schema"}, + expected: true, + }, + { + name: "one assignment", + state: &GrantsState{ + SecurableType: "schema", + EmbeddedSlice: []catalog.PrivilegeAssignment{ + {Principal: "alice", Privileges: []catalog.Privilege{catalog.PrivilegeSelect}}, + }, + }, + expected: false, + }, + } + + adapter, err := NewAdapter(SupportedResources["schemas.grants"], "schemas.grants", nil) + require.NoError(t, err) + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + empty, err := adapter.IsEmptyState(tt.state) + require.NoError(t, err) + assert.Equal(t, tt.expected, empty) + }) + } +} + +// Resources without IsEmptyState are never treated as empty. +func TestIsEmptyStateNotImplemented(t *testing.T) { + adapter, err := NewAdapter(SupportedResources["schemas"], "schemas", nil) + require.NoError(t, err) + + empty, err := adapter.IsEmptyState(&catalog.CreateSchema{Name: "myschema"}) + require.NoError(t, err) + assert.False(t, empty) +} + func TestNormalizeAssignments(t *testing.T) { tests := []struct { name string