From db132eefaca11b72860fc0a63ee9415f3000a7cc Mon Sep 17 00:00:00 2001 From: Liam Cervante Date: Fri, 20 Sep 2024 08:20:32 +0200 Subject: [PATCH] stacks: stacks destroy ordering should match Terraform (#35748) * add tests that highlight known issues in the destroy mechanism * stacks: stacks destroy ordering should match Terraform --- .../stacks/stackruntime/apply_destroy_test.go | 59 ++++++++++++++++ internal/stacks/stackruntime/apply_test.go | 68 ++++++++++++++++++- .../internal/stackeval/component_instance.go | 34 ++-------- .../internal/stackeval/main_apply.go | 13 +--- .../internal/stackeval/removed_instance.go | 12 ++-- 5 files changed, 136 insertions(+), 50 deletions(-) diff --git a/internal/stacks/stackruntime/apply_destroy_test.go b/internal/stacks/stackruntime/apply_destroy_test.go index 6c55c04d06..395bf4fd27 100644 --- a/internal/stacks/stackruntime/apply_destroy_test.go +++ b/internal/stacks/stackruntime/apply_destroy_test.go @@ -940,6 +940,65 @@ func TestApplyDestroy(t *testing.T) { }, }, }, + "destroy-with-provider-req-and-removed": { + path: path.Join("auth-provider-w-data", "removed"), + state: stackstate.NewStateBuilder(). + AddComponentInstance(stackstate.NewComponentInstanceBuilder(mustAbsComponentInstance("component.load")). + AddDependent(mustAbsComponent("component.create")). + AddOutputValue("credentials", cty.StringVal("wrong"))). // must reload the credentials + AddComponentInstance(stackstate.NewComponentInstanceBuilder(mustAbsComponentInstance("component.create")). + AddDependency(mustAbsComponent("component.load"))). + AddResourceInstance(stackstate.NewResourceInstanceBuilder(). + SetAddr(mustAbsResourceInstanceObject("component.create.testing_resource.resource")). + SetResourceInstanceObjectSrc(states.ResourceInstanceObjectSrc{ + AttrsJSON: mustMarshalJSONAttrs(map[string]interface{}{ + "id": "resource", + "value": nil, + }), + Status: states.ObjectReady, + }). + SetProviderAddr(mustDefaultRootProvider("testing"))). + Build(), + store: stacks_testing_provider.NewResourceStoreBuilder().AddResource("credentials", cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("credentials"), + // we have the wrong value in state, so this correct value must + // be loaded for this test to work. + "value": cty.StringVal("authn"), + })).Build(), + mutators: []func(store *stacks_testing_provider.ResourceStore, testContext TestContext) TestContext{ + func(store *stacks_testing_provider.ResourceStore, testContext TestContext) TestContext { + testContext.providers[addrs.NewDefaultProvider("testing")] = func() (providers.Interface, error) { + provider := stacks_testing_provider.NewProviderWithData(t, store) + provider.Authentication = "authn" // So we must reload the data source in order to authenticate. + return provider, nil + } + return testContext + }, + }, + cycles: []TestCycle{ + { + planMode: plans.DestroyMode, + wantAppliedChanges: []stackstate.AppliedChange{ + &stackstate.AppliedChangeComponentInstanceRemoved{ + ComponentAddr: mustAbsComponent("component.create"), + ComponentInstanceAddr: mustAbsComponentInstance("component.create"), + }, + &stackstate.AppliedChangeResourceInstanceObject{ + ResourceInstanceObjectAddr: mustAbsResourceInstanceObject("component.create.testing_resource.resource"), + ProviderConfigAddr: mustDefaultRootProvider("testing"), + }, + &stackstate.AppliedChangeComponentInstanceRemoved{ + ComponentAddr: mustAbsComponent("component.load"), + ComponentInstanceAddr: mustAbsComponentInstance("component.load"), + }, + &stackstate.AppliedChangeResourceInstanceObject{ + ResourceInstanceObjectAddr: mustAbsResourceInstanceObject("component.load.data.testing_data_source.credentials"), + ProviderConfigAddr: mustDefaultRootProvider("testing"), + }, + }, + }, + }, + }, } for name, tc := range tcs { t.Run(name, func(t *testing.T) { diff --git a/internal/stacks/stackruntime/apply_test.go b/internal/stacks/stackruntime/apply_test.go index 7e3dc6e091..485b9e95d2 100644 --- a/internal/stacks/stackruntime/apply_test.go +++ b/internal/stacks/stackruntime/apply_test.go @@ -1144,6 +1144,70 @@ After applying this plan, Terraform will no longer manage these objects. You wil }, }, }, + "removed block with provider-to-component dep": { + path: path.Join("auth-provider-w-data", "removed"), + state: stackstate.NewStateBuilder(). + AddComponentInstance(stackstate.NewComponentInstanceBuilder(mustAbsComponentInstance("component.load")). + AddDependent(mustAbsComponent("component.create")). + AddOutputValue("credentials", cty.StringVal("wrong"))). // must reload the credentials + AddComponentInstance(stackstate.NewComponentInstanceBuilder(mustAbsComponentInstance("component.create")). + AddDependency(mustAbsComponent("component.load"))). + AddResourceInstance(stackstate.NewResourceInstanceBuilder(). + SetAddr(mustAbsResourceInstanceObject("component.create.testing_resource.resource")). + SetResourceInstanceObjectSrc(states.ResourceInstanceObjectSrc{ + AttrsJSON: mustMarshalJSONAttrs(map[string]interface{}{ + "id": "resource", + "value": nil, + }), + Status: states.ObjectReady, + }). + SetProviderAddr(mustDefaultRootProvider("testing"))). + Build(), + store: stacks_testing_provider.NewResourceStoreBuilder().AddResource("credentials", cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("credentials"), + // we have the wrong value in state, so this correct value must + // be loaded for this test to work. + "value": cty.StringVal("authn"), + })).Build(), + cycles: []TestCycle{ + { + planMode: plans.NormalMode, + wantAppliedChanges: []stackstate.AppliedChange{ + &stackstate.AppliedChangeComponentInstanceRemoved{ + ComponentAddr: mustAbsComponent("component.create"), + ComponentInstanceAddr: mustAbsComponentInstance("component.create"), + }, + &stackstate.AppliedChangeResourceInstanceObject{ + ResourceInstanceObjectAddr: mustAbsResourceInstanceObject("component.create.testing_resource.resource"), + ProviderConfigAddr: mustDefaultRootProvider("testing"), + NewStateSrc: nil, // deleted + }, + &stackstate.AppliedChangeComponentInstance{ + ComponentAddr: mustAbsComponent("component.load"), + ComponentInstanceAddr: mustAbsComponentInstance("component.load"), + OutputValues: map[addrs.OutputValue]cty.Value{ + addrs.OutputValue{Name: "credentials"}: cty.StringVal("authn").Mark(marks.Sensitive), + }, + InputVariables: make(map[addrs.InputVariable]cty.Value), + Dependents: collections.NewSet(mustAbsComponent("component.create")), + }, + &stackstate.AppliedChangeResourceInstanceObject{ + ResourceInstanceObjectAddr: mustAbsResourceInstanceObject("component.load.data.testing_data_source.credentials"), + NewStateSrc: &states.ResourceInstanceObjectSrc{ + AttrsJSON: mustMarshalJSONAttrs(map[string]interface{}{ + "id": "credentials", + "value": "authn", + }), + AttrSensitivePaths: make([]cty.Path, 0), + Status: states.ObjectReady, + }, + ProviderConfigAddr: mustDefaultRootProvider("testing"), + Schema: stacks_testing_provider.TestingDataSourceSchema, + }, + }, + }, + }, + }, } for name, tc := range tcs { @@ -1168,7 +1232,9 @@ After applying this plan, Terraform will no longer manage these objects. You wil config: loadMainBundleConfigForTest(t, tc.path), providers: map[addrs.Provider]providers.Factory{ addrs.NewDefaultProvider("testing"): func() (providers.Interface, error) { - return stacks_testing_provider.NewProviderWithData(t, store), nil + provider := stacks_testing_provider.NewProviderWithData(t, store) + provider.Authentication = "authn" + return provider, nil }, }, dependencyLocks: *lock, diff --git a/internal/stacks/stackruntime/internal/stackeval/component_instance.go b/internal/stacks/stackruntime/internal/stackeval/component_instance.go index 054780057e..a1512ba806 100644 --- a/internal/stacks/stackruntime/internal/stackeval/component_instance.go +++ b/internal/stacks/stackruntime/internal/stackeval/component_instance.go @@ -255,16 +255,12 @@ func (c *ComponentInstance) CheckModuleTreePlan(ctx context.Context) (*plans.Pla // doesn't exist so it's fine. continue } - depComponent := depStack.Component(ctx, depAddr.Item) - if depComponent == nil { - // again, the thing we need to wait to be deleted - // doesn't exist so it's fine. - continue + depComponent, depRemoved := depStack.ApplyableComponents(ctx, depAddr.Item) + if depComponent != nil && !depComponent.PlanIsComplete(ctx) { + opts.ExternalDependencyDeferred = true + break } - if !depComponent.PlanIsComplete(ctx) { - // The other component couldn't be deleted in a single - // go, so to be safe we'll defer our deletions until - // the other one is complete. + if depRemoved != nil && !depRemoved.PlanIsComplete(ctx) { opts.ExternalDependencyDeferred = true break } @@ -299,26 +295,6 @@ func (c *ComponentInstance) CheckModuleTreePlan(ctx context.Context) (*plans.Pla opts.ExternalDependencyDeferred = true break } - - // We're also going to look through any upstream components - // that are being removed to make sure they are removed first. - for _, depAddr := range c.PlanPrevDependents(ctx).Elems() { - depStack := c.main.Stack(ctx, depAddr.Stack, PlanPhase) - if depStack == nil { - break - } - depRemoved := depStack.Removed(ctx, depAddr.Item) - if depRemoved == nil { - break - } - if !depRemoved.PlanIsComplete(ctx) { - // The other component couldn't be deleted in a single - // go, so to be safe we'll defer our deletions until - // the other one is complete. - opts.ExternalDependencyDeferred = true - break - } - } } // The instance is also upstream deferred if the for_each value for diff --git a/internal/stacks/stackruntime/internal/stackeval/main_apply.go b/internal/stacks/stackruntime/internal/stackeval/main_apply.go index c1aba53bfb..dd47aa06d1 100644 --- a/internal/stacks/stackruntime/internal/stackeval/main_apply.go +++ b/internal/stacks/stackruntime/internal/stackeval/main_apply.go @@ -197,22 +197,11 @@ func ApplyPlan(ctx context.Context, config *stackconfig.Config, plan *stackplan. // If we're being destroyed, then we're waiting for // everything that depended on us anyway. - waitForRemoveds = collections.NewSet[stackaddrs.AbsComponent]() + waitForRemoveds = dependencyAddrs } else { // For all other actions, we must wait for our // dependencies to finish applying their changes. waitForComponents = dependencyAddrs - - // If we're not being destroyed we might have some - // depdendents that are being destroyed, and we need - // to wait for them to finish before we can start. - waitForRemoveds = collections.NewSet[stackaddrs.AbsComponent]() - for _, dependent := range dependentAddrs.Elems() { - dependentStack := main.Stack(ctx, dependent.Stack, ApplyPhase) - if dependentStack.Removed(ctx, dependent.Item) != nil { - waitForRemoveds.Add(dependent) - } - } } if depCount := waitForComponents.Len(); depCount != 0 { log.Printf("[TRACE] stackeval: %s waiting for its predecessors (%d) to complete", addr, depCount) diff --git a/internal/stacks/stackruntime/internal/stackeval/removed_instance.go b/internal/stacks/stackruntime/internal/stackeval/removed_instance.go index 918373ed13..3c2993a81c 100644 --- a/internal/stacks/stackruntime/internal/stackeval/removed_instance.go +++ b/internal/stacks/stackruntime/internal/stackeval/removed_instance.go @@ -120,16 +120,12 @@ func (r *RemovedInstance) ModuleTreePlan(ctx context.Context) (*plans.Plan, tfdi // doesn't exist so it's fine. break } - depComponent := depStack.Component(ctx, depAddr.Item) - if depComponent == nil { - // again, the thing we need to wait to be deleted - // doesn't exist so it's fine. + depComponent, depRemoved := depStack.ApplyableComponents(ctx, depAddr.Item) + if depComponent != nil && !depComponent.PlanIsComplete(ctx) { + deferred = true break } - if !depComponent.PlanIsComplete(ctx) { - // The other component couldn't be deleted in a single - // go, so to be safe we'll defer our deletions until - // the other one is complete. + if depRemoved != nil && !depRemoved.PlanIsComplete(ctx) { deferred = true break }