diff --git a/internal/terraform/context_plan.go b/internal/terraform/context_plan.go index 248673254..7d1353c6a 100644 --- a/internal/terraform/context_plan.go +++ b/internal/terraform/context_plan.go @@ -3,6 +3,8 @@ package terraform import ( "fmt" "log" + "sort" + "strings" "github.com/zclconf/go-cty/cty" @@ -113,7 +115,7 @@ func (c *Context) Plan(config *configs.Config, prevRunState *states.State, opts tfdiags.Warning, "Resource targeting is in effect", `You are creating a plan with the -target option, which means that the result of this plan may not represent all of the changes requested by the current configuration. - + The -target option is not for routine use, and is provided only for exceptional situations such as recovering from errors or mistakes, or when Terraform specifically suggests to use it as part of an error message.`, )) } @@ -297,23 +299,75 @@ func (c *Context) destroyPlan(config *configs.Config, prevRunState *states.State func (c *Context) prePlanFindAndApplyMoves(config *configs.Config, prevRunState *states.State, targets []addrs.Targetable) ([]refactoring.MoveStatement, map[addrs.UniqueKey]refactoring.MoveResult) { moveStmts := refactoring.FindMoveStatements(config) moveResults := refactoring.ApplyMoves(moveStmts, prevRunState) - if len(targets) > 0 { - for _, result := range moveResults { - matchesTarget := false - for _, targetAddr := range targets { - if targetAddr.TargetContains(result.From) { - matchesTarget = true - break - } + return moveStmts, moveResults +} + +func (c *Context) prePlanVerifyTargetedMoves(moveResults map[addrs.UniqueKey]refactoring.MoveResult, targets []addrs.Targetable) tfdiags.Diagnostics { + if len(targets) < 1 { + return nil // the following only matters when targeting + } + + var diags tfdiags.Diagnostics + + var excluded []addrs.AbsResourceInstance + for _, result := range moveResults { + fromMatchesTarget := false + toMatchesTarget := false + for _, targetAddr := range targets { + if targetAddr.TargetContains(result.From) { + fromMatchesTarget = true } - //lint:ignore SA9003 TODO - if !matchesTarget { - // TODO: Return an error stating that a targeted plan is - // only valid if it includes this address that was moved. + if targetAddr.TargetContains(result.To) { + toMatchesTarget = true } } + if !fromMatchesTarget { + excluded = append(excluded, result.From) + } + if !toMatchesTarget { + excluded = append(excluded, result.To) + } } - return moveStmts, moveResults + if len(excluded) > 0 { + sort.Slice(excluded, func(i, j int) bool { + return excluded[i].Less(excluded[j]) + }) + + var listBuf strings.Builder + var prevResourceAddr addrs.AbsResource + for _, instAddr := range excluded { + // Targeting generally ends up selecting whole resources rather + // than individual instances, because we don't factor in + // individual instances until DynamicExpand, so we're going to + // always show whole resource addresses here, excluding any + // instance keys. (This also neatly avoids dealing with the + // different quoting styles required for string instance keys + // on different shells, which is handy.) + // + // To avoid showing duplicates when we have multiple instances + // of the same resource, we'll remember the most recent + // resource we rendered in prevResource, which is sufficient + // because we sorted the list of instance addresses above, and + // our sort order always groups together instances of the same + // resource. + resourceAddr := instAddr.ContainingResource() + if resourceAddr.Equal(prevResourceAddr) { + continue + } + fmt.Fprintf(&listBuf, "\n -target=%q", resourceAddr.String()) + prevResourceAddr = resourceAddr + } + diags = diags.Append(tfdiags.Sourceless( + tfdiags.Error, + "Moved resource instances excluded by targeting", + fmt.Sprintf( + "Resource instances in your current state have moved to new addresses in the latest configuration. Terraform must include those resource instances while planning in order to ensure a correct result, but your -target=... options to not fully cover all of those resource instances.\n\nTo create a valid plan, either remove your -target=... options altogether or add the following additional target options:%s\n\nNote that adding these options may include further additional resource instances in your plan, in order to respect object dependencies.", + listBuf.String(), + ), + )) + } + + return diags } func (c *Context) postPlanValidateMoves(config *configs.Config, stmts []refactoring.MoveStatement, allInsts instances.Set) tfdiags.Diagnostics { @@ -327,6 +381,16 @@ func (c *Context) planWalk(config *configs.Config, prevRunState *states.State, r prevRunState = prevRunState.DeepCopy() // don't modify the caller's object when we process the moves moveStmts, moveResults := c.prePlanFindAndApplyMoves(config, prevRunState, opts.Targets) + // If resource targeting is in effect then it might conflict with the + // move result. + diags = diags.Append(c.prePlanVerifyTargetedMoves(moveResults, opts.Targets)) + if diags.HasErrors() { + // We'll return early here, because if we have any moved resource + // instances excluded by targeting then planning is likely to encounter + // strange problems that may lead to confusing error messages. + return nil, diags + } + graph, walkOp, moreDiags := c.planGraph(config, prevRunState, opts, true) diags = diags.Append(moreDiags) if diags.HasErrors() { diff --git a/internal/terraform/context_plan2_test.go b/internal/terraform/context_plan2_test.go index 53053feda..a1c4e9feb 100644 --- a/internal/terraform/context_plan2_test.go +++ b/internal/terraform/context_plan2_test.go @@ -7,12 +7,14 @@ import ( "testing" "github.com/davecgh/go-spew/spew" + "github.com/google/go-cmp/cmp" "github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/configs/configschema" "github.com/hashicorp/terraform/internal/lang/marks" "github.com/hashicorp/terraform/internal/plans" "github.com/hashicorp/terraform/internal/providers" "github.com/hashicorp/terraform/internal/states" + "github.com/hashicorp/terraform/internal/tfdiags" "github.com/zclconf/go-cty/cty" ) @@ -772,6 +774,203 @@ func TestContext2Plan_movedResourceBasic(t *testing.T) { }) } +func TestContext2Plan_movedResourceUntargeted(t *testing.T) { + addrA := mustResourceInstanceAddr("test_object.a") + addrB := mustResourceInstanceAddr("test_object.b") + m := testModuleInline(t, map[string]string{ + "main.tf": ` + resource "test_object" "b" { + } + + moved { + from = test_object.a + to = test_object.b + } + + terraform { + experiments = [config_driven_move] + } + `, + }) + + state := states.BuildState(func(s *states.SyncState) { + // The prior state tracks test_object.a, which we should treat as + // test_object.b because of the "moved" block in the config. + s.SetResourceInstanceCurrent(addrA, &states.ResourceInstanceObjectSrc{ + AttrsJSON: []byte(`{}`), + Status: states.ObjectReady, + }, mustProviderConfig(`provider["registry.terraform.io/hashicorp/test"]`)) + }) + + p := simpleMockProvider() + ctx := testContext2(t, &ContextOpts{ + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), + }, + }) + + t.Run("without targeting instance A", func(t *testing.T) { + _, diags := ctx.Plan(m, state, &PlanOpts{ + Mode: plans.NormalMode, + Targets: []addrs.Targetable{ + // NOTE: addrA isn't included here, but it's pending move to addrB + // and so this plan request is invalid. + addrB, + }, + }) + diags.Sort() + + // We're semi-abusing "ForRPC" here just to get diagnostics that are + // more easily comparable than the various different diagnostics types + // tfdiags uses internally. The RPC-friendly diagnostics are also + // comparison-friendly, by discarding all of the dynamic type information. + gotDiags := diags.ForRPC() + wantDiags := tfdiags.Diagnostics{ + tfdiags.Sourceless( + tfdiags.Warning, + "Resource targeting is in effect", + `You are creating a plan with the -target option, which means that the result of this plan may not represent all of the changes requested by the current configuration. + +The -target option is not for routine use, and is provided only for exceptional situations such as recovering from errors or mistakes, or when Terraform specifically suggests to use it as part of an error message.`, + ), + tfdiags.Sourceless( + tfdiags.Error, + "Moved resource instances excluded by targeting", + `Resource instances in your current state have moved to new addresses in the latest configuration. Terraform must include those resource instances while planning in order to ensure a correct result, but your -target=... options to not fully cover all of those resource instances. + +To create a valid plan, either remove your -target=... options altogether or add the following additional target options: + -target="test_object.a" + +Note that adding these options may include further additional resource instances in your plan, in order to respect object dependencies.`, + ), + }.ForRPC() + + if diff := cmp.Diff(wantDiags, gotDiags); diff != "" { + t.Errorf("wrong diagnostics\n%s", diff) + } + }) + t.Run("without targeting instance B", func(t *testing.T) { + _, diags := ctx.Plan(m, state, &PlanOpts{ + Mode: plans.NormalMode, + Targets: []addrs.Targetable{ + addrA, + // NOTE: addrB isn't included here, but it's pending move from + // addrA and so this plan request is invalid. + }, + }) + diags.Sort() + + // We're semi-abusing "ForRPC" here just to get diagnostics that are + // more easily comparable than the various different diagnostics types + // tfdiags uses internally. The RPC-friendly diagnostics are also + // comparison-friendly, by discarding all of the dynamic type information. + gotDiags := diags.ForRPC() + wantDiags := tfdiags.Diagnostics{ + tfdiags.Sourceless( + tfdiags.Warning, + "Resource targeting is in effect", + `You are creating a plan with the -target option, which means that the result of this plan may not represent all of the changes requested by the current configuration. + +The -target option is not for routine use, and is provided only for exceptional situations such as recovering from errors or mistakes, or when Terraform specifically suggests to use it as part of an error message.`, + ), + tfdiags.Sourceless( + tfdiags.Error, + "Moved resource instances excluded by targeting", + `Resource instances in your current state have moved to new addresses in the latest configuration. Terraform must include those resource instances while planning in order to ensure a correct result, but your -target=... options to not fully cover all of those resource instances. + +To create a valid plan, either remove your -target=... options altogether or add the following additional target options: + -target="test_object.b" + +Note that adding these options may include further additional resource instances in your plan, in order to respect object dependencies.`, + ), + }.ForRPC() + + if diff := cmp.Diff(wantDiags, gotDiags); diff != "" { + t.Errorf("wrong diagnostics\n%s", diff) + } + }) + t.Run("without targeting either instance", func(t *testing.T) { + _, diags := ctx.Plan(m, state, &PlanOpts{ + Mode: plans.NormalMode, + Targets: []addrs.Targetable{ + mustResourceInstanceAddr("test_object.unrelated"), + // NOTE: neither addrA nor addrB are included here, but there's + // a pending move between them and so this is invalid. + }, + }) + diags.Sort() + + // We're semi-abusing "ForRPC" here just to get diagnostics that are + // more easily comparable than the various different diagnostics types + // tfdiags uses internally. The RPC-friendly diagnostics are also + // comparison-friendly, by discarding all of the dynamic type information. + gotDiags := diags.ForRPC() + wantDiags := tfdiags.Diagnostics{ + tfdiags.Sourceless( + tfdiags.Warning, + "Resource targeting is in effect", + `You are creating a plan with the -target option, which means that the result of this plan may not represent all of the changes requested by the current configuration. + +The -target option is not for routine use, and is provided only for exceptional situations such as recovering from errors or mistakes, or when Terraform specifically suggests to use it as part of an error message.`, + ), + tfdiags.Sourceless( + tfdiags.Error, + "Moved resource instances excluded by targeting", + `Resource instances in your current state have moved to new addresses in the latest configuration. Terraform must include those resource instances while planning in order to ensure a correct result, but your -target=... options to not fully cover all of those resource instances. + +To create a valid plan, either remove your -target=... options altogether or add the following additional target options: + -target="test_object.a" + -target="test_object.b" + +Note that adding these options may include further additional resource instances in your plan, in order to respect object dependencies.`, + ), + }.ForRPC() + + if diff := cmp.Diff(wantDiags, gotDiags); diff != "" { + t.Errorf("wrong diagnostics\n%s", diff) + } + }) + t.Run("with both addresses in the target set", func(t *testing.T) { + // The error messages in the other subtests above suggest adding + // addresses to the set of targets. This additional test makes sure that + // following that advice actually leads to a valid result. + + _, diags := ctx.Plan(m, state, &PlanOpts{ + Mode: plans.NormalMode, + Targets: []addrs.Targetable{ + // This time we're including both addresses in the target, + // to get the same effect an end-user would get if following + // the advice in our error message in the other subtests. + addrA, + addrB, + }, + }) + diags.Sort() + + // We're semi-abusing "ForRPC" here just to get diagnostics that are + // more easily comparable than the various different diagnostics types + // tfdiags uses internally. The RPC-friendly diagnostics are also + // comparison-friendly, by discarding all of the dynamic type information. + gotDiags := diags.ForRPC() + wantDiags := tfdiags.Diagnostics{ + // Still get the warning about the -target option... + tfdiags.Sourceless( + tfdiags.Warning, + "Resource targeting is in effect", + `You are creating a plan with the -target option, which means that the result of this plan may not represent all of the changes requested by the current configuration. + +The -target option is not for routine use, and is provided only for exceptional situations such as recovering from errors or mistakes, or when Terraform specifically suggests to use it as part of an error message.`, + ), + // ...but now we have no error about test_object.a + }.ForRPC() + + if diff := cmp.Diff(wantDiags, gotDiags); diff != "" { + t.Errorf("wrong diagnostics\n%s", diff) + } + + }) +} + func TestContext2Plan_refreshOnlyMode(t *testing.T) { addr := mustResourceInstanceAddr("test_object.a")