From ebd31f5022e5bd86b590cd826a80a4f82db67f6e Mon Sep 17 00:00:00 2001 From: kmoe <5575356+kmoe@users.noreply.github.com> Date: Tue, 5 Dec 2023 16:26:08 +0000 Subject: [PATCH] core: remove import,forget targets from PlanOpts (#34355) PlanOpts should contain only static options which are known before planning. Previous to this commit, ctx.plan was modifying the opts object, which, since it is passed via pointer, could be shared by other ctx.plan calls happening concurrently, producing weird behaviour. Such weird behaviour was only noticeable during tests, in which it is common to pass a pointer to DefaultPlanOpts. When one test that modified opts.forgetResources was run at the same time as another test, DefaultPlanOpts.forgetResources would be populated when it shouldn't be. The real Terraform binary does not do this and ctx.plan is only called once per run. One fix for the race condition would be to have ctx.plan take a deep copy of the entire PlanOpts object which it can modify without touching the caller's object. Due to the multiple nested data structures in PlanOpts this would be a large amount of code, with a lot of DeepCopy methods it would be easy to miss when adding struct fields. Instead we just remove ImportTargets, forgetResources, and forgetModules from PlanOpts, since they did not belong there in the first place. These slices must be populated dynamically during planning once the entire config tree is available and passed to the graph builder. --- internal/terraform/context_apply2_test.go | 6 +-- internal/terraform/context_plan.go | 66 +++++++++++------------ 2 files changed, 34 insertions(+), 38 deletions(-) diff --git a/internal/terraform/context_apply2_test.go b/internal/terraform/context_apply2_test.go index a7f1929e90..7fe5dd4981 100644 --- a/internal/terraform/context_apply2_test.go +++ b/internal/terraform/context_apply2_test.go @@ -2483,8 +2483,7 @@ removed { }, }) - planOpts := *DefaultPlanOpts - plan, diags := ctx.Plan(m, state, &planOpts) + plan, diags := ctx.Plan(m, state, DefaultPlanOpts) if diags.HasErrors() { t.Fatalf("diags: %s", diags.Err()) } @@ -2530,8 +2529,7 @@ removed { }, }) - planOpts := *DefaultPlanOpts - plan, diags := ctx.Plan(m, state, &planOpts) + plan, diags := ctx.Plan(m, state, DefaultPlanOpts) if diags.HasErrors() { t.Fatalf("diags: %s", diags.Err()) } diff --git a/internal/terraform/context_plan.go b/internal/terraform/context_plan.go index ab61f90f10..5d1a02dcf9 100644 --- a/internal/terraform/context_plan.go +++ b/internal/terraform/context_plan.go @@ -86,18 +86,6 @@ type PlanOpts struct { // during this plan. Overrides *mocking.Overrides - // ImportTargets is a list of target resources to import. These resources - // will be added to the plan graph. - ImportTargets []*ImportTarget - - // forgetResources is a list of target resources that Terraform - // will plan to remove from state. - forgetResources []addrs.ConfigResource - - // forgetModules is a list of target modules that Terraform will plan to - // remove from state. - forgetModules []addrs.Module - // GenerateConfigPath tells Terraform where to write any generated // configuration for any ImportTargets that do not have configuration // already. @@ -360,7 +348,6 @@ func (c *Context) plan(config *configs.Config, prevRunState *states.State, opts panic(fmt.Sprintf("called Context.plan with %s", opts.Mode)) } - opts.ImportTargets = c.findImportTargets(config) plan, evalScope, walkDiags := c.planWalk(config, prevRunState, opts) diags = diags.Append(walkDiags) @@ -593,6 +580,29 @@ func (c *Context) findImportTargets(config *configs.Config) []*ImportTarget { return importTargets } +// findForgetTargets builds a list of resources and a list of modules to be +// forgotten, based on any removed blocks in config. +func (c *Context) findForgetTargets(config *configs.Config) (forgetResources []addrs.ConfigResource, forgetModules []addrs.Module, diags tfdiags.Diagnostics) { + removeStmts, diags := refactoring.FindRemoveStatements(config) + if diags.HasErrors() { + return nil, nil, diags + } + for _, rst := range removeStmts.Values() { + if rst.Destroy { + // no-op + } else { + if fr, ok := rst.From.(addrs.ConfigResource); ok { + forgetResources = append(forgetResources, fr) + } else if fm, ok := rst.From.(addrs.Module); ok { + forgetModules = append(forgetModules, fm) + } else { + panic("Invalid ConfigMoveable type in remove statement") + } + } + } + return forgetResources, forgetModules, diags +} + func (c *Context) planWalk(config *configs.Config, prevRunState *states.State, opts *PlanOpts) (*plans.Plan, *lang.Scope, tfdiags.Diagnostics) { var diags tfdiags.Diagnostics log.Printf("[DEBUG] Building and walking plan graph for %s", opts.Mode) @@ -610,24 +620,6 @@ func (c *Context) planWalk(config *configs.Config, prevRunState *states.State, o return nil, nil, diags } - removeStmts, diags := refactoring.FindRemoveStatements(config) - if diags.HasErrors() { - return nil, nil, diags - } - for _, rst := range removeStmts.Values() { - if rst.Destroy { - // no-op - } else { - if fr, ok := rst.From.(addrs.ConfigResource); ok { - opts.forgetResources = append(opts.forgetResources, fr) - } else if fm, ok := rst.From.(addrs.Module); ok { - opts.forgetModules = append(opts.forgetModules, fm) - } else { - panic("Invalid ConfigMoveable type in remove statement") - } - } - } - graph, walkOp, moreDiags := c.planGraph(config, prevRunState, opts) diags = diags.Append(moreDiags) if diags.HasErrors() { @@ -739,6 +731,12 @@ func (c *Context) planWalk(config *configs.Config, prevRunState *states.State, o func (c *Context) planGraph(config *configs.Config, prevRunState *states.State, opts *PlanOpts) (*Graph, walkOperation, tfdiags.Diagnostics) { switch mode := opts.Mode; mode { case plans.NormalMode: + // In Normal mode we need to pay attention to import and removed blocks + // in config so their targets can be added to the graph. + forgetResources, forgetModules, diags := c.findForgetTargets(config) + if diags.HasErrors() { + return nil, walkPlan, diags + } graph, diags := (&PlanGraphBuilder{ Config: config, State: prevRunState, @@ -750,9 +748,9 @@ func (c *Context) planGraph(config *configs.Config, prevRunState *states.State, preDestroyRefresh: opts.PreDestroyRefresh, Operation: walkPlan, ExternalReferences: opts.ExternalReferences, - ImportTargets: opts.ImportTargets, - forgetResources: opts.forgetResources, - forgetModules: opts.forgetModules, + ImportTargets: c.findImportTargets(config), + forgetResources: forgetResources, + forgetModules: forgetModules, GenerateConfigPath: opts.GenerateConfigPath, }).Build(addrs.RootModuleInstance) return graph, walkPlan, diags