resilien
/
terraform
2
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

core: Fail if a moved resource instance is excluded by -target 

Because "moved" blocks produce changes that span across more than one
resource instance address at the same time, we need to take extra care
with them during planning.

The -target option allows for restricting Terraform's attention only to
a subset of resources when planning, as an escape hatch to recover from
bugs and mistakes.

However, we need to avoid any situation where only one "side" of a move
would be considered in a particular plan, because that'd create a new
situation that would be otherwise unreachable and would be difficult to
recover from.

As a compromise then, we'll reject an attempt to create a targeted plan if
the plan involves resolving a pending move and if the source address of
that move is not included in the targets.

Our error message offers the user two possible resolutions: to create an
untargeted plan, thus allowing everything to resolve, or to add additional
-target options to include just the existing resource instances that have
pending moves to resolve.

This compromise recognizes that it is possible -- though hopefully rare --
that a user could potentially both be recovering from a bug or mistake at
the same time as processing a move, if e.g. the bug was fixed by upgrading
a module and the new version includes a new "moved" block. In that edge
case, it might be necessary to just add the one additional address to
the targets rather than removing the targets altogether, if creating a
normal untargeted plan is impossible due to whatever bug they're trying to
recover from.
This commit is contained in:
Martin Atkins 2021-09-15 15:54:16 -07:00
parent b4594551f7
commit e6a76d8ba0
2 changed files with 277 additions and 14 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

92
internal/terraform/context_plan.go
View File

@ -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() {

199
internal/terraform/context_plan2_test.go
View File

@ -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")