Add pulumi state delete --target-dependents

Add comment + fix tests

We had two problems here.

1. Our graph rapid tests didn't encode the `includeChildren` flag as
transitive, which it should be. Looking at the call sites of
`DependingOn` confirms this.

2. `TestDeleteProtected` was sharing variables between threads (causing
flakey results). This was fixed.

changelog/config.yaml

@@ -5,7 +5,7 @@ types:
 scopes:
   auto: [dotnet, go, java, nodejs, python, yaml]
   backend: [service, filestate]
-  cli: [about, config, display, engine, import, new, plugin, package]
+  cli: [about, config, display, engine, import, new, plugin, package, state]
   pkg: [testing]
   sdk: [dotnet, go, java, nodejs, python, yaml]
   components: [dotnet, go, java, nodejs, python, yaml]

changelog/pending/20221026--cli-state--add-the-target-dependents-flag-to-pulumi-state-delete.yaml

@@ -0,0 +1,4 @@
+changes:
+- type: feat
+  scope: cli/state
+  description: Add the --target-dependents flag to `pulumi state delete`

pkg/cmd/pulumi/state_delete.go

@@ -31,6 +31,7 @@ func newStateDeleteCommand() *cobra.Command {
 	var force bool // Force deletion of protected resources
 	var stack string
 	var yes bool
+	var targetDepenedents bool
 
 	cmd := &cobra.Command{
 		Use:   "delete <resource URN>",
@@ -57,32 +58,31 @@ pulumi state delete 'urn:pulumi:stage::demo::eks:index:Cluster$pulumi:providers:
 			showPrompt := !yes
 
 			res := runStateEdit(ctx, stack, showPrompt, urn, func(snap *deploy.Snapshot, res *resource.State) error {
-				if !force {
-					return edit.DeleteResource(snap, res)
-				}
-
-				if res.Protect {
-					cmdutil.Diag().Warningf(diag.RawMessage("" /*urn*/, "deleting protected resource due to presence of --force"))
-					res.Protect = false
+				var handleProtected func(*resource.State) error
+				if force {
+					handleProtected = func(res *resource.State) error {
+						cmdutil.Diag().Warningf(diag.Message(res.URN,
+							"deleting protected resource %s due to presence of --force"), res.URN)
+						return edit.UnprotectResource(nil, res)
+					}
 				}
-
-				return edit.DeleteResource(snap, res)
+				return edit.DeleteResource(snap, res, handleProtected, targetDepenedents)
 			})
 			if res != nil {
 				switch e := res.Error().(type) {
 				case edit.ResourceHasDependenciesError:
-					message := "This resource can't be safely deleted because the following resources depend on it:\n"
+					message := string(e.Condemned.URN) + " can't be safely deleted because the following resources depend on it:\n"
 					for _, dependentResource := range e.Dependencies {
 						depUrn := dependentResource.URN
 						message += fmt.Sprintf(" * %-15q (%s)\n", depUrn.Name(), depUrn)
 					}
 
-					message += "\nDelete those resources first before deleting this one."
+					message += "\nDelete those resources first or pass --target-dependents."
 					return result.Error(message)
 				case edit.ResourceProtectedError:
-					return result.Error(
-						"This resource can't be safely deleted because it is protected. " +
-							"Re-run this command with --force to force deletion")
+					return result.Errorf(
+						"%s can't be safely deleted because it is protected. "+
+							"Re-run this command with --force to force deletion", string(e.Condemned.URN))
 				default:
 					return res
 				}
@@ -97,5 +97,6 @@ pulumi state delete 'urn:pulumi:stage::demo::eks:index:Cluster$pulumi:providers:
 		"The name of the stack to operate on. Defaults to the current stack")
 	cmd.Flags().BoolVar(&force, "force", false, "Force deletion of protected resources")
 	cmd.Flags().BoolVarP(&yes, "yes", "y", false, "Skip confirmation prompts")
+	cmd.Flags().BoolVar(&targetDepenedents, "target-dependents", false, "Delete the URN and all its dependents")
 	return cmd
 }

pkg/resource/edit/operations.go

@@ -1,4 +1,4 @@
-// Copyright 2016-2018, Pulumi Corporation.
+// Copyright 2016-2022, Pulumi Corporation.
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -29,43 +29,77 @@ import (
 // given snapshot and pertain to the specific passed-in resource.
 type OperationFunc func(*deploy.Snapshot, *resource.State) error
 
-// DeleteResource deletes a given resource from the snapshot, if it is possible to do so. A resource can only be deleted
-// from a stack if there do not exist any resources that depend on it or descend from it. If such a resource does exist,
-// DeleteResource will return an error instance of `ResourceHasDependenciesError`.
-func DeleteResource(snapshot *deploy.Snapshot, condemnedRes *resource.State) error {
+// DeleteResource deletes a given resource from the snapshot, if it is possible to do so.
+//
+// If targetDependents is true, dependents will also be deleted. Otherwise an error
+// instance of `ResourceHasDependenciesError` will be returned.
+//
+// If non-nil, onProtected will be called on all protected resources planed for deletion.
+//
+// If a resource is marked protected after onProtected is called, an error instance of
+// `ResourceHasDependenciesError` will be returned.
+func DeleteResource(
+	snapshot *deploy.Snapshot, condemnedRes *resource.State,
+	onProtected func(*resource.State) error, targetDependents bool,
+) error {
 	contract.Require(snapshot != nil, "snapshot")
 	contract.Require(condemnedRes != nil, "state")
 
-	if condemnedRes.Protect {
-		return ResourceProtectedError{condemnedRes}
+	handleProtected := func(res *resource.State) error {
+		if !res.Protect {
+			return nil
+		}
+		var err error
+		if onProtected != nil {
+			err = onProtected(res)
+		}
+		if err == nil && res.Protect {
+			err = ResourceProtectedError{res}
+		}
+		return err
+	}
+
+	if err := handleProtected(condemnedRes); err != nil {
+		return err
 	}
 
 	dg := graph.NewDependencyGraph(snapshot.Resources)
-	dependencies := dg.DependingOn(condemnedRes, nil, false)
-	if len(dependencies) != 0 {
-		return ResourceHasDependenciesError{Condemned: condemnedRes, Dependencies: dependencies}
+	deleteSet := map[resource.URN]bool{
+		condemnedRes.URN: true,
+	}
+
+	if dependencies := dg.DependingOn(condemnedRes, nil, true); len(dependencies) != 0 {
+		if !targetDependents {
+			return ResourceHasDependenciesError{Condemned: condemnedRes, Dependencies: dependencies}
+		}
+		for _, dep := range dependencies {
+			err := handleProtected(dep)
+			if err != nil {
+				return err
+			}
+			deleteSet[dep.URN] = true
+		}
 	}
 
 	// If there are no resources that depend on condemnedRes, iterate through the snapshot and keep everything that's
 	// not condemnedRes.
 	var newSnapshot []*resource.State
 	var children []*resource.State
 	for _, res := range snapshot.Resources {
-		// While iterating, keep track of the set of resources that are parented to our condemned resource. We'll only
-		// actually perform the deletion if this set is empty, otherwise it is not legal to delete the resource.
-		if res.Parent == condemnedRes.URN {
+		// While iterating, keep track of the set of resources that are parented to our
+		// condemned resource. This acts as a check on DependingOn, preventing a bug from
+		// introducing state corruption.
+		if res.Parent == condemnedRes.URN && !deleteSet[res.URN] {
 			children = append(children, res)
 		}
 
-		if res != condemnedRes {
+		if !deleteSet[res.URN] {
 			newSnapshot = append(newSnapshot, res)
 		}
 	}
 
 	// If there exists a resource that is the child of condemnedRes, we can't delete it.
-	if len(children) != 0 {
-		return ResourceHasDependenciesError{Condemned: condemnedRes, Dependencies: children}
-	}
+	contract.Assertf(len(children) == 0, "unexpected children in resource dependency list")
 
 	// Otherwise, we're good to go. Writing the new resource list into the snapshot persists the mutations that we have
 	// made above.

pkg/resource/edit/operations_test.go

@@ -1,4 +1,4 @@
-// Copyright 2016-2018, Pulumi Corporation.
+// Copyright 2016-2022, Pulumi Corporation.
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -27,6 +27,7 @@ import (
 	"github.com/pulumi/pulumi/sdk/v3/go/common/tokens"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func NewResource(name string, provider *resource.State, deps ...resource.URN) *resource.State {
@@ -84,12 +85,30 @@ func TestDeletion(t *testing.T) {
 		c,
 	})
 
-	err := DeleteResource(snap, b)
+	err := DeleteResource(snap, b, nil, false)
 	assert.NoError(t, err)
 	assert.Len(t, snap.Resources, 3)
 	assert.Equal(t, []*resource.State{pA, a, c}, snap.Resources)
 }
 
+func TestDeletingDependencies(t *testing.T) {
+	t.Parallel()
+
+	pA := NewProviderResource("a", "p1", "0")
+	a := NewResource("a", pA)
+	b := NewResource("b", pA)
+	c := NewResource("c", pA, a.URN)
+	d := NewResource("d", pA, c.URN)
+	snap := NewSnapshot([]*resource.State{
+		pA, a, b, c, d,
+	})
+
+	err := DeleteResource(snap, a, nil, true)
+	require.NoError(t, err)
+
+	assert.Equal(t, snap.Resources, []*resource.State{pA, b})
+}
+
 func TestFailedDeletionProviderDependency(t *testing.T) {
 	t.Parallel()
 
@@ -104,7 +123,7 @@ func TestFailedDeletionProviderDependency(t *testing.T) {
 		c,
 	})
 
-	err := DeleteResource(snap, pA)
+	err := DeleteResource(snap, pA, nil, false)
 	assert.Error(t, err)
 	depErr, ok := err.(ResourceHasDependenciesError)
 	if !assert.True(t, ok) {
@@ -132,7 +151,7 @@ func TestFailedDeletionRegularDependency(t *testing.T) {
 		c,
 	})
 
-	err := DeleteResource(snap, a)
+	err := DeleteResource(snap, a, nil, false)
 	assert.Error(t, err)
 	depErr, ok := err.(ResourceHasDependenciesError)
 	if !assert.True(t, ok) {
@@ -158,12 +177,111 @@ func TestFailedDeletionProtected(t *testing.T) {
 		a,
 	})
 
-	err := DeleteResource(snap, a)
+	err := DeleteResource(snap, a, nil, false)
 	assert.Error(t, err)
 	_, ok := err.(ResourceProtectedError)
 	assert.True(t, ok)
 }
 
+func TestDeleteProtected(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name string
+		test func(t *testing.T, pA, a, b, c *resource.State, snap *deploy.Snapshot)
+	}{
+		{
+			"root-protected",
+			func(t *testing.T, pA, a, b, c *resource.State, snap *deploy.Snapshot) {
+				a.Protect = true
+				protectedCount := 0
+				err := DeleteResource(snap, a, func(s *resource.State) error {
+					s.Protect = false
+					protectedCount++
+					return nil
+				}, false)
+				assert.NoError(t, err)
+				assert.Equal(t, protectedCount, 1)
+				assert.Equal(t, snap.Resources, []*resource.State{pA, b, c})
+			},
+		},
+		{
+			"root-and-branch",
+			func(t *testing.T, pA, a, b, c *resource.State, snap *deploy.Snapshot) {
+				a.Protect = true
+				b.Protect = true
+				c.Protect = true
+				protectedCount := 0
+				err := DeleteResource(snap, b, func(s *resource.State) error {
+					s.Protect = false
+					protectedCount++
+					return nil
+				}, true)
+				assert.NoError(t, err)
+				// 2 because we only plan to delete b and c. a is protected but not
+				// scheduled for deletion, so we don't call the onProtect handler.
+				assert.Equal(t, protectedCount, 2)
+				assert.Equal(t, snap.Resources, []*resource.State{pA, a})
+
+			},
+		},
+		{
+			"branch",
+			func(t *testing.T, pA, a, b, c *resource.State, snap *deploy.Snapshot) {
+				b.Protect = true
+				c.Protect = true
+				protectedCount := 0
+				err := DeleteResource(snap, c, func(s *resource.State) error {
+					s.Protect = false
+					protectedCount++
+					return nil
+				}, false)
+				assert.NoError(t, err)
+				assert.Equal(t, protectedCount, 1)
+				assert.Equal(t, snap.Resources, []*resource.State{pA, a, b})
+			},
+		},
+		{
+			"no-permission-root",
+			func(t *testing.T, pA, a, b, c *resource.State, snap *deploy.Snapshot) {
+				c.Protect = true
+				err := DeleteResource(snap, c, nil, false).(ResourceProtectedError)
+				assert.Equal(t, ResourceProtectedError{
+					Condemned: c,
+				}, err)
+			},
+		},
+		{
+			"no-permission-branch",
+			func(t *testing.T, pA, a, b, c *resource.State, snap *deploy.Snapshot) {
+				c.Protect = true
+				err := DeleteResource(snap, b, nil, true).(ResourceProtectedError)
+				assert.Equal(t, ResourceProtectedError{
+					Condemned: c,
+				}, err)
+			},
+		},
+	}
+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			pA := NewProviderResource("a", "p1", "0")
+			a := NewResource("a", pA)
+			b := NewResource("b", pA)
+			c := NewResource("c", pA, b.URN)
+			snap := NewSnapshot([]*resource.State{
+				pA,
+				a,
+				b,
+				c,
+			})
+
+			tt.test(t, pA, a, b, c, snap)
+		})
+	}
+}
+
 func TestFailedDeletionParentDependency(t *testing.T) {
 	t.Parallel()
 
@@ -180,7 +298,7 @@ func TestFailedDeletionParentDependency(t *testing.T) {
 		c,
 	})
 
-	err := DeleteResource(snap, a)
+	err := DeleteResource(snap, a, nil, false)
 	assert.Error(t, err)
 	depErr, ok := err.(ResourceHasDependenciesError)
 	if !assert.True(t, ok) {

pkg/resource/graph/dependency_graph.go

@@ -20,6 +20,8 @@ type DependencyGraph struct {
 // order with respect to the snapshot dependency graph.
 //
 // The time complexity of DependingOn is linear with respect to the number of resources.
+//
+// includeChildren adds children as another type of (transitive) dependency.
 func (dg *DependencyGraph) DependingOn(res *resource.State,
 	ignore map[resource.URN]bool, includeChildren bool) []*resource.State {
 	// This implementation relies on the detail that snapshots are stored in a valid
@@ -35,7 +37,7 @@ func (dg *DependencyGraph) DependingOn(res *resource.State,
 		if ignore[candidate.URN] {
 			return false
 		}
-		if includeChildren && candidate.Parent == res.URN {
+		if includeChildren && dependentSet[candidate.Parent] {
 			return true
 		}
 		for _, dependency := range candidate.Dependencies {

pkg/resource/graph/dependency_graph_rapid_test.go

@@ -127,10 +127,8 @@ func expectedDependingOn(universe []*resource.State, includeChildren bool) R {
 		return inverse(transitively(universe)(restrictedDependenciesOf))
 	}
 
-	// TODO this extends base `expectedDependingOn` with
-	// immediate children. Should it be a transitive closure?
 	dependingOn := expectedDependingOn(universe, false)
-	return func(a, b *resource.State) bool {
+	return transitively(universe)(func(a, b *resource.State) bool {
 		if dependingOn(a, b) || isParent(b, a) {
 			return true
 		}
@@ -140,7 +138,7 @@ func expectedDependingOn(universe []*resource.State, includeChildren bool) R {
 			}
 		}
 		return false
-	}
+	})
 }
 
 // Verify `DependingOn` against `expectedDependingOn`. Note that