Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .nextchanges/bundles/empty-grants-migrate.md
Original file line number Diff line number Diff line change
@@ -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.
1 change: 1 addition & 0 deletions acceptance/bundle/invariant/migrate/out.test.toml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 0 additions & 11 deletions acceptance/bundle/invariant/migrate/test.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"]

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
bundle:
name: schema-grants-remove-all-$UNIQUE_NAME

resources:
schemas:
grants_schema:
name: schema_remove_all_$UNIQUE_NAME
catalog_name: main
grants: # TO_EMPTY
- { principal: deco-test-user@databricks.com, privileges: [USE_SCHEMA] } # TO_EMPTY
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@

>>> [CLI] bundle plan
Plan: 0 to add, 0 to change, 0 to delete, 2 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, 2 unchanged
Original file line number Diff line number Diff line change
@@ -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

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

31 changes: 31 additions & 0 deletions acceptance/bundle/resources/grants/schemas/remove_all/output.txt
Original file line number Diff line number Diff line change
@@ -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!
27 changes: 27 additions & 0 deletions acceptance/bundle/resources/grants/schemas/remove_all/script
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
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.
grep -v 'TO_EMPTY' databricks.yml > tmp.yml && mv tmp.yml databricks.yml
echo ' grants: []' >> databricks.yml

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'

# Per-engine: direct keeps the emptied grants node in state and reports it as unchanged,
# terraform has no grants resource left to report.
{
trace $CLI bundle plan
trace $CLI bundle deploy
trace $CLI bundle plan
} &> out.plan.$DATABRICKS_BUNDLE_ENGINE.txt
11 changes: 11 additions & 0 deletions bundle/direct/bundle_plan.go
Original file line number Diff line number Diff line change
Expand Up @@ -975,6 +975,17 @@ 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.
if _, hasState := db.State[node]; !hasState {
skip, err := adapter.SkipCreate(newStateConfig)
if err != nil {
return nil, fmt.Errorf("%s: %w", prefix, err)
}
if skip {
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})
Expand Down
4 changes: 4 additions & 0 deletions bundle/direct/dresources/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,10 @@ 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.

## No-op creates: SkipCreate

If a desired state requires no API call to create (e.g. an empty grants list), implement `SkipCreate` and the planner omits the node instead of planning a create. It is only consulted for nodes without a state entry: once state exists the node stays in the plan, so emptying it still plans an update.

## State backward compatibility

The state struct is serialized to JSON and persisted between deploys. Backward incompatible changes will result in a drift, which depending
Expand Down
29 changes: 29 additions & 0 deletions bundle/direct/dresources/adapter.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,11 @@ 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] SkipCreate reports that creating newState is a no-op, so the planner omits the
// node instead of planning a create. Only consulted for nodes without a state entry.
// Example: func (*ResourceGrants) SkipCreate(state *GrantsState) bool
SkipCreate(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)
Expand Down Expand Up @@ -103,6 +108,7 @@ type Adapter struct {
doCreate *calladapt.BoundCaller

// Optional:
skipCreate *calladapt.BoundCaller
doUpdate *calladapt.BoundCaller
doUpdateWithID *calladapt.BoundCaller
waitAfterCreate *calladapt.BoundCaller
Expand Down Expand Up @@ -136,6 +142,7 @@ func NewAdapter(typedNil any, resourceType string, client *databricks.WorkspaceC
doRefresh: nil,
doDelete: nil,
doCreate: nil,
skipCreate: nil,
doUpdate: nil,
doUpdateWithID: nil,
doResize: nil,
Expand Down Expand Up @@ -205,6 +212,11 @@ func (a *Adapter) initMethods(resource any) error {

// Optional methods with varying signatures:

a.skipCreate, err = calladapt.PrepareCall(resource, reflect.TypeFor[IResource](), "SkipCreate")
if err != nil {
return err
}

a.doUpdate, err = calladapt.PrepareCall(resource, reflect.TypeFor[IResource](), "DoUpdate")
if err != nil {
return err
Expand Down Expand Up @@ -313,6 +325,10 @@ func (a *Adapter) validate() error {
}
validations = append(validations, "DoCreate remoteState return", a.doCreate.OutTypes[1], remoteType)

if a.skipCreate != nil {
validations = append(validations, "SkipCreate newState", a.skipCreate.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)
Expand Down Expand Up @@ -470,6 +486,19 @@ func (a *Adapter) DoCreate(ctx context.Context, newState any) (string, any, erro
return id, remoteState, nil
}

// SkipCreate reports whether creating newState is a no-op; false if not implemented.
func (a *Adapter) SkipCreate(newState any) (bool, error) {
if a.skipCreate == nil {
return false, nil
}

outs, err := a.skipCreate.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
Expand Down
6 changes: 6 additions & 0 deletions bundle/direct/dresources/grants.go
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,12 @@ func (*ResourceGrants) PrepareState(state *GrantsState) *GrantsState {
return state
}

// SkipCreate reports an empty grants list as a no-op: nothing to grant, and Terraform
// records no databricks_grants resource for it, so migrated bundles have no state entry.
func (*ResourceGrants) SkipCreate(state *GrantsState) bool {
return len(state.EmbeddedSlice) == 0
}

func grantKey(x catalog.PrivilegeAssignment) (string, string) {
return "principal", x.Principal
}
Expand Down
52 changes: 52 additions & 0 deletions bundle/direct/dresources/grants_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -75,6 +76,57 @@ func TestBuildGrantChanges(t *testing.T) {
}
}

// Calls through the adapter so the optional-method wiring is covered too.
func TestGrantsSkipCreate(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) {
skip, err := adapter.SkipCreate(tt.state)
require.NoError(t, err)
assert.Equal(t, tt.expected, skip)
})
}
}

// Resources without SkipCreate always need a create.
func TestSkipCreateNotImplemented(t *testing.T) {
adapter, err := NewAdapter(SupportedResources["schemas"], "schemas", nil)
require.NoError(t, err)

skip, err := adapter.SkipCreate(&catalog.CreateSchema{Name: "myschema"})
require.NoError(t, err)
assert.False(t, skip)
}

func TestNormalizeAssignments(t *testing.T) {
tests := []struct {
name string
Expand Down
Loading