Fix update plans with dependent replacements

changelog/pending/20221013--engine--update-plan-dbr.yaml

@@ -0,0 +1,4 @@
+changes:
+- type: fix
+  scope: engine
+  description: Fix a bug in update plans handling resources being replaced due to other resources being deleted before replacement.

pkg/engine/lifecycletest/update_plan_test.go

@@ -17,6 +17,7 @@ package lifecycletest
 
 import (
 	"regexp"
+	"strings"
 	"testing"
 
 	"github.com/blang/semver"
@@ -1507,3 +1508,98 @@ func TestProviderDeterministicPreview(t *testing.T) {
 	assert.NotEqual(t, expectedName, snap.Resources[1].Inputs["name"])
 	assert.NotEqual(t, expectedName, snap.Resources[1].Outputs["name"])
 }
+
+func TestPlannedUpdateWithDependentDelete(t *testing.T) {
+	t.Parallel()
+
+	var diffResult *plugin.DiffResult
+
+	loaders := []*deploytest.ProviderLoader{
+		deploytest.NewProviderLoader("pkgA", semver.MustParse("1.0.0"), func() (plugin.Provider, error) {
+			return &deploytest.Provider{
+				CreateF: func(urn resource.URN, news resource.PropertyMap, timeout float64,
+					preview bool) (resource.ID, resource.PropertyMap, resource.Status, error) {
+					return resource.ID("created-id-" + urn.Name()), news, resource.StatusOK, nil
+				},
+				UpdateF: func(urn resource.URN, id resource.ID, olds, news resource.PropertyMap, timeout float64,
+					ignoreChanges []string, preview bool) (resource.PropertyMap, resource.Status, error) {
+					return news, resource.StatusOK, nil
+				},
+				CheckF: func(urn resource.URN,
+					olds, news resource.PropertyMap, _ []byte) (resource.PropertyMap, []plugin.CheckFailure, error) {
+					return news, nil, nil
+				},
+				DiffF: func(urn resource.URN,
+					id resource.ID, olds, news resource.PropertyMap, ignoreChanges []string) (plugin.DiffResult, error) {
+					if strings.Contains(string(urn), "resA") || strings.Contains(string(urn), "resB") {
+						assert.NotNil(t, diffResult, "Diff was called but diffResult wasn't set")
+						return *diffResult, nil
+					}
+					return plugin.DiffResult{}, nil
+				},
+			}, nil
+		}),
+	}
+
+	var ins resource.PropertyMap
+	program := deploytest.NewLanguageRuntime(func(_ plugin.RunInfo, monitor *deploytest.ResourceMonitor) error {
+		resA, _, outs, err := monitor.RegisterResource("pkgA:m:typA", "resA", true, deploytest.ResourceOptions{
+			Inputs: ins,
+		})
+		assert.NoError(t, err)
+
+		_, _, _, err = monitor.RegisterResource("pkgA:m:typB", "resB", true, deploytest.ResourceOptions{
+			Inputs:       outs,
+			Dependencies: []resource.URN{resA},
+		})
+		assert.NoError(t, err)
+
+		return nil
+	})
+	host := deploytest.NewPluginHost(nil, nil, program, loaders...)
+
+	p := &TestPlan{
+		Options: UpdateOptions{Host: host, GeneratePlan: true},
+	}
+
+	project := p.GetProject()
+
+	// Create an initial ResA and resB
+	ins = resource.NewPropertyMapFromMap(map[string]interface{}{
+		"foo": "bar",
+		"zed": "baz",
+	})
+	snap, res := TestOp(Update).Run(project, p.GetTarget(t, nil), p.Options, false, p.BackendClient, nil)
+	assert.NotNil(t, snap)
+	assert.Nil(t, res)
+
+	// Update the input and mark it as a replace, check that both A and B are marked as replacements
+	ins = resource.NewPropertyMapFromMap(map[string]interface{}{
+		"foo": "frob",
+		"zed": "baz",
+	})
+	diffResult = &plugin.DiffResult{
+		Changes:     plugin.DiffSome,
+		ReplaceKeys: []resource.PropertyKey{"foo"},
+		StableKeys:  []resource.PropertyKey{"zed"},
+		DetailedDiff: map[string]plugin.PropertyDiff{
+			"foo": {
+				Kind:      plugin.DiffUpdateReplace,
+				InputDiff: true,
+			},
+		},
+		DeleteBeforeReplace: true,
+	}
+	plan, res := TestOp(Update).Plan(project, p.GetTarget(t, snap), p.Options, p.BackendClient, nil)
+	assert.NotNil(t, plan)
+	assert.Nil(t, res)
+
+	assert.Equal(t, 3, len(plan.ResourcePlans["urn:pulumi:test::test::pkgA:m:typA::resA"].Ops))
+	assert.Equal(t, 3, len(plan.ResourcePlans["urn:pulumi:test::test::pkgA:m:typB::resB"].Ops))
+
+	// Now try and run with the plan
+	p.Options.Plan = plan.Clone()
+	snap, res = TestOp(Update).Run(project, p.GetTarget(t, snap), p.Options, false, p.BackendClient, nil)
+	assert.NotNil(t, snap)
+	assert.Nil(t, res)
+}

pkg/resource/deploy/deployment.go

@@ -151,6 +151,10 @@ func (m *resourcePlans) set(urn resource.URN, plan *ResourcePlan) {
 	m.m.Lock()
 	defer m.m.Unlock()
 
+	if _, ok := m.plans.ResourcePlans[urn]; ok {
+		panic(fmt.Sprintf("tried to set resource plan for %s but it's already been set", urn))
+	}
+
 	m.plans.ResourcePlans[urn] = plan
 }
 

pkg/resource/deploy/step_generator.go

@@ -253,16 +253,21 @@ func (sg *stepGenerator) GenerateSteps(event RegisterResourceEvent) ([]Step, res
 
 	// Check each proposed step against the relevant resource plan, if any
 	for _, s := range steps {
+		logging.V(5).Infof("Checking step %s for %s", s.Op(), s.URN())
+
 		if sg.deployment.plan != nil {
 			if resourcePlan, ok := sg.deployment.plan.ResourcePlans[s.URN()]; ok {
 				if len(resourcePlan.Ops) == 0 {
 					return nil, result.Errorf("%v is not allowed by the plan: no more steps were expected for this resource", s.Op())
 				}
 				constraint := resourcePlan.Ops[0]
+				// We remove the Op from the list before doing the constraint check.
+				// This is because we look at Ops at the end to see if any expected operations didn't attempt to happen.
+				// This op has been attempted, it just might fail its constraint.
+				resourcePlan.Ops = resourcePlan.Ops[1:]
 				if !ConstrainedTo(s.Op(), constraint) {
 					return nil, result.Errorf("%v is not allowed by the plan: this resource is constrained to %v", s.Op(), constraint)
 				}
-				resourcePlan.Ops = resourcePlan.Ops[1:]
 			} else {
 				if !ConstrainedTo(s.Op(), OpSame) {
 					return nil, result.Errorf("%v is not allowed by the plan: no steps were expected for this resource", s.Op())
@@ -601,11 +606,22 @@ func (sg *stepGenerator) generateSteps(event RegisterResourceEvent) ([]Step, res
 		}
 		inputDiff := oldInputs.Diff(inputs)
 
-		// Generate the output goal plan
-		newResourcePlan := &ResourcePlan{
-			Seed: randomSeed,
-			Goal: NewGoalPlan(inputDiff, goal)}
-		sg.deployment.newPlans.set(urn, newResourcePlan)
+		// Generate the output goal plan, if we're recreating this it should already exist
+		if recreating {
+			plan, ok := sg.deployment.newPlans.get(urn)
+			if !ok {
+				return nil, result.FromError(fmt.Errorf("no plan for resource %v", urn))
+			}
+			// The plan will have had it's Ops already partially filled in for the delete operation, but we
+			// now have the information needed to fill in Seed and Goal.
+			plan.Seed = randomSeed
+			plan.Goal = NewGoalPlan(inputDiff, goal)
+		} else {
+			newResourcePlan := &ResourcePlan{
+				Seed: randomSeed,
+				Goal: NewGoalPlan(inputDiff, goal)}
+			sg.deployment.newPlans.set(urn, newResourcePlan)
+		}
 	}
 
 	// If there is a plan for this resource, validate that the program goal conforms to the plan.