direct: revoke grants when the grants node is deleted and the parent stays

.nextchanges/bundles/grants-revoked-on-delete.md

@@ -0,0 +1 @@
+direct: Removing a `grants` block now revokes the grants instead of leaving them in place ([#6474](https://github.com/databricks/cli/pull/6474)).

acceptance/bundle/resources/grants/schemas/delete_semantics/databricks.empty.yml.tmpl

@@ -0,0 +1,2 @@
+bundle:
+  name: schema-grants-delete-semantics-$UNIQUE_NAME

acceptance/bundle/resources/grants/schemas/delete_semantics/databricks.yml.tmpl

@@ -0,0 +1,9 @@
+bundle:
+  name: schema-grants-delete-semantics-$UNIQUE_NAME
+
+resources:
+  schemas:
+    grants_schema:
+      name: schema_delete_semantics_$UNIQUE_NAME
+      catalog_name: main
+      grants: [{ principal: deco-test-user@databricks.com, privileges: [USE_SCHEMA] }] # GRANTS

acceptance/bundle/resources/grants/schemas/delete_semantics/out.direct.txt

@@ -0,0 +1,50 @@
+=== only the grants node is deleted
+delete schemas.grants_schema.grants
+
+Plan: 0 to add, 0 to change, 1 to delete, 1 unchanged
+{
+  "method": "PATCH",
+  "path": "/api/2.1/unity-catalog/permissions/schema/main.schema_delete_semantics_[UNIQUE_NAME]",
+  "body": {
+    "changes": [
+      {
+        "principal": "deco-test-user@databricks.com",
+        "remove": [
+          "ALL_PRIVILEGES"
+        ]
+      }
+    ]
+  }
+}
+json = {};
+=== the grants list is empty
+delete schemas.grants_schema.grants
+
+Plan: 0 to add, 0 to change, 1 to delete, 1 unchanged
+{
+  "method": "PATCH",
+  "path": "/api/2.1/unity-catalog/permissions/schema/main.schema_delete_semantics_[UNIQUE_NAME]",
+  "body": {
+    "changes": [
+      {
+        "principal": "deco-test-user@databricks.com",
+        "remove": [
+          "ALL_PRIVILEGES"
+        ]
+      }
+    ]
+  }
+}
+json = {};
+=== the parent is deleted too
+delete schemas.grants_schema
+delete schemas.grants_schema.grants
+
+Plan: 0 to add, 0 to change, 2 to delete, 0 unchanged
+{
+  "method": "DELETE",
+  "path": "/api/2.1/unity-catalog/schemas/main.schema_delete_semantics_[UNIQUE_NAME]",
+  "q": {
+    "force": "true"
+  }
+}

acceptance/bundle/resources/grants/schemas/delete_semantics/out.terraform.txt

@@ -0,0 +1,64 @@
+=== only the grants node is deleted
+delete schemas.grants_schema.grants
+
+Plan: 0 to add, 0 to change, 1 to delete, 1 unchanged
+{
+  "method": "PATCH",
+  "path": "/api/2.1/unity-catalog/permissions/schema/main.schema_delete_semantics_[UNIQUE_NAME]",
+  "body": {
+    "changes": [
+      {
+        "principal": "deco-test-user@databricks.com",
+        "remove": [
+          "USE_SCHEMA"
+        ]
+      }
+    ]
+  }
+}
+json = {};
+=== the grants list is empty
+delete schemas.grants_schema.grants
+
+Plan: 0 to add, 0 to change, 1 to delete, 1 unchanged
+{
+  "method": "PATCH",
+  "path": "/api/2.1/unity-catalog/permissions/schema/main.schema_delete_semantics_[UNIQUE_NAME]",
+  "body": {
+    "changes": [
+      {
+        "principal": "deco-test-user@databricks.com",
+        "remove": [
+          "USE_SCHEMA"
+        ]
+      }
+    ]
+  }
+}
+json = {};
+=== the parent is deleted too
+delete schemas.grants_schema
+delete schemas.grants_schema.grants
+
+Plan: 0 to add, 0 to change, 2 to delete, 0 unchanged
+{
+  "method": "DELETE",
+  "path": "/api/2.1/unity-catalog/schemas/main.schema_delete_semantics_[UNIQUE_NAME]",
+  "q": {
+    "force": "true"
+  }
+}
+{
+  "method": "PATCH",
+  "path": "/api/2.1/unity-catalog/permissions/schema/main.schema_delete_semantics_[UNIQUE_NAME]",
+  "body": {
+    "changes": [
+      {
+        "principal": "deco-test-user@databricks.com",
+        "remove": [
+          "USE_SCHEMA"
+        ]
+      }
+    ]
+  }
+}

acceptance/bundle/resources/grants/schemas/delete_semantics/output.txt

@@ -0,0 +1,27 @@
+
+=== Deploy the schema with one grant
+>>> [CLI] bundle deploy -qq
+
+=== Case 1: the grants block is removed, the schema stays
+>>> [CLI] bundle deploy -qq
+
+=== Put the grant back
+>>> [CLI] bundle deploy -qq
+
+=== Case 2: the grants list is set to []
+>>> [CLI] bundle deploy -qq
+
+=== Put the grant back
+>>> [CLI] bundle deploy -qq
+
+=== Case 3: the schema and its grants are deleted together
+>>> [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-delete-semantics-[UNIQUE_NAME]/default
+
+Destroy: 1 deleted

acceptance/bundle/resources/grants/schemas/delete_semantics/script

@@ -0,0 +1,54 @@
+SCHEMA_FULL_NAME=main.schema_delete_semantics_$UNIQUE_NAME
+GRANTS_LINE='grants: [{ principal: deco-test-user@databricks.com, privileges: [USE_SCHEMA] }] # GRANTS'
+
+# The plan, the requests and the leftover grants all go to one per-engine file: the engines
+# disagree on how a dropped grants node is classified and on what they send for it.
+ENGINE_OUT=out.$DATABRICKS_BUNDLE_ENGINE.txt
+
+envsubst < databricks.yml.tmpl > databricks.yml
+cp databricks.yml databricks.yml.saved
+
+cleanup() {
+    errcode $CLI bundle destroy --auto-approve &> LOG.cleanup
+    rm -f out.requests.txt databricks.yml.saved tmp.yml
+}
+trap cleanup EXIT
+
+record() {
+    echo "=== $1" >> $ENGINE_OUT
+    $CLI bundle plan >> $ENGINE_OUT
+    trace $CLI bundle deploy -qq
+    print_requests.py //unity-catalog --sort >> $ENGINE_OUT
+    $CLI grants get schema "$SCHEMA_FULL_NAME" | gron.py --noindex | sort_lines.py --repl >> $ENGINE_OUT
+}
+
+restore_grant() {
+    title "Put the grant back"
+    cp databricks.yml.saved databricks.yml
+    trace $CLI bundle deploy -qq
+    rm -f out.requests.txt
+}
+
+title "Deploy the schema with one grant"
+trace $CLI bundle deploy -qq
+rm -f out.requests.txt
+
+title "Case 1: the grants block is removed, the schema stays"
+grep -v GRANTS databricks.yml > tmp.yml && mv tmp.yml databricks.yml
+record "only the grants node is deleted"
+
+restore_grant
+title "Case 2: the grants list is set to []"
+update_file.py databricks.yml "$GRANTS_LINE" 'grants: []'
+record "the grants list is empty"
+
+restore_grant
+title "Case 3: the schema and its grants are deleted together"
+echo "=== the parent is deleted too" >> $ENGINE_OUT
+# Planned against a config with the schema gone, so the plan names both nodes and the
+# action each one gets. `bundle destroy` applies the same shape but its banner hides
+# child nodes, so it never shows what happens to the grants.
+envsubst < databricks.empty.yml.tmpl > databricks.yml
+$CLI bundle plan >> $ENGINE_OUT
+trace $CLI bundle destroy --auto-approve
+print_requests.py //unity-catalog --sort >> $ENGINE_OUT

acceptance/bundle/resources/grants/schemas/delete_semantics/test.toml

@@ -0,0 +1,2 @@
+# Six deploys plus a destroy; extra headroom for heavy parallel runs.
+Timeout = '2m'

bundle/deployplan/action.go

@@ -25,6 +25,17 @@ func (a Action) IsChildResource() bool {
 	return len(items) == 4
 }
 
+// ParentKey returns the key of the resource a child node hangs off, e.g.
+// "resources.schemas.foo" for "resources.schemas.foo.grants". Returns "" if
+// resourceKey is not a child node.
+func ParentKey(resourceKey string) string {
+	items := strings.Split(resourceKey, ".")
+	if len(items) != 4 {
+		return ""
+	}
+	return strings.Join(items[:3], ".")
+}
+
 type ActionType string
 
 // Actions are ordered in increasing severity.

bundle/direct/apply.go

@@ -159,24 +159,9 @@ func (d *DeploymentUnit) Update(ctx context.Context, db *dstate.DeploymentState,
 		return err
 	}
 
-	empty, err := d.Adapter.IsEmptyState(newState)
+	err = d.saveState(db, id, newState, d.DependsOn)
 	if err != nil {
-		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 = d.saveState(db, id, newState, d.DependsOn)
-		if err != nil {
-			return fmt.Errorf("saving state id=%s: %w", id, err)
-		}
+		return fmt.Errorf("saving state id=%s: %w", id, err)
 	}
 
 	waitRemoteState, err := retryOnTransient(ctx, func() (any, error) {

bundle/direct/bundle_apply.go

@@ -43,6 +43,10 @@ func (b *DeploymentBundle) Apply(ctx context.Context, client *databricks.Workspa
 		return
 	}
 
+	// Computed up front: the callback below holds a write lock on its own entry, so it
+	// cannot read another node's entry without risking a lock error.
+	parentDeleted := willDeleteParent(plan)
+
 	g.Run(defaultParallelism, func(resourceKey string, failedDependency *string) bool {
 		entry, err := plan.WriteLockEntry(resourceKey)
 		if err != nil {
@@ -100,9 +104,10 @@ func (b *DeploymentBundle) Apply(ctx context.Context, client *databricks.Workspa
 		}
 
 		if action == deployplan.Delete {
-			if entry.Gone {
-				// Planning confirmed the resource is already deleted remotely; only
-				// remove it from the state, without calling the delete API.
+			if entry.Gone || parentDeleted[resourceKey] {
+				// Planning confirmed the resource is already deleted remotely, or the parent
+				// resource is going away and takes this node with it; only remove it from
+				// the state, without calling the delete API.
 				err = b.StateDB.DeleteState(resourceKey)
 			} else {
 				err = d.Destroy(ctx, &b.StateDB)
@@ -164,6 +169,29 @@ func (b *DeploymentBundle) Apply(ctx context.Context, client *databricks.Workspa
 	})
 }
 
+// willDeleteParent returns the child nodes (grants, permissions) whose parent resource the
+// same plan deletes: the parent takes them with it, so the child delete is applied as a
+// state-only cleanup.
+func willDeleteParent(plan *deployplan.Plan) map[string]bool {
+	result := make(map[string]bool)
+
+	for key, entry := range plan.Plan {
+		if entry.Action != deployplan.Delete {
+			continue
+		}
+		parent := deployplan.ParentKey(key)
+		if parent == "" {
+			continue
+		}
+		parentEntry := plan.Plan[parent]
+		if parentEntry != nil && parentEntry.Action == deployplan.Delete {
+			result[key] = true
+		}
+	}
+
+	return result
+}
+
 func (b *DeploymentBundle) LookupReferencePostDeploy(ctx context.Context, path *structpath.PathNode) (any, error) {
 	targetResourceKey, fieldPath := splitResourcePath(path)
 	targetGroup := config.GetResourceTypeFromKey(targetResourceKey)

bundle/direct/bundle_plan.go

@@ -992,16 +992,19 @@ 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
+		// An empty state describes no resource, so the node is not planned as one. A node with
+		// no state is simply left out: there is nothing to create. A node that has state goes
+		// back to existingKeys and is planned as a delete, which revokes and stops tracking it -
+		// emptying the list is the same request as removing the block.
+		empty, err := adapter.IsEmptyState(newStateConfig)
+		if err != nil {
+			return nil, fmt.Errorf("%s: %w", prefix, err)
+		}
+		if empty {
+			if stateEntry, hasState := db.State[node]; hasState {
+				existingKeys[node] = stateEntry
 			}
+			continue
 		}
 
 		// Note, we're extracting references in input config but resolving them in newState.Config which is PrepareState(inputConfig)

bundle/direct/dresources/grants.go

@@ -141,10 +141,38 @@ func (r *ResourceGrants) DoUpdate(ctx context.Context, _ string, state *GrantsSt
 	return nil, err
 }
 
+// DoDelete revokes every privilege currently assigned on the securable, including any
+// granted out of band: dropping the grants node means the empty list is what the bundle
+// enforces. Apply only reaches this when the securable itself stays — when the parent is
+// deleted too, deleting it takes the grants with it and the node is a state-only cleanup.
 func (r *ResourceGrants) DoDelete(ctx context.Context, id string, _ *GrantsState) error {
-	// Similar to permissions, we do nothing there.
-	// We could delete all grants there, but it would be confusing to explain wrt permissions.
-	return nil
+	securableType, fullName, err := parseGrantsID(id)
+	if err != nil {
+		return err
+	}
+
+	assignments, err := r.listGrants(ctx, securableType, fullName)
+	if err != nil {
+		return err
+	}
+	if len(assignments) == 0 {
+		return nil
+	}
+
+	principals := make([]string, 0, len(assignments))
+	for _, a := range assignments {
+		principals = append(principals, a.Principal)
+	}
+	slices.Sort(principals)
+
+	_, err = r.client.Grants.Update(ctx, catalog.UpdatePermissions{
+		SecurableType:             securableType,
+		FullName:                  fullName,
+		Changes:                   buildGrantChanges(nil, principals),
+		OmitPermissionsInResponse: false,
+		ForceSendFields:           nil,
+	})
+	return err
 }
 
 func buildGrantChanges(desiredAssignments []catalog.PrivilegeAssignment, removedPrincipals []string) []catalog.PermissionsChange {