From b9cc9c0ab9241922b0ecc11e389d79260e7dad81 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Thu, 16 Jun 2022 17:41:07 -0700 Subject: [PATCH 1/2] core: Create apply graph nodes even for no-op "changes" We previously would optimize away the graph nodes for any resource instance without a real change pending, but that means we don't get an opportunity to re-check any invariants associated with the instance, such as preconditions and postconditions. Other upstream changes during apply can potentially decide the outcome of a condition even if the instance itself isn't being changed, so we do still need to revisit these during apply or else we might skip running certain checks altogether, if they yielded unknown results during planning and then don't get run during apply. --- .../terraform/node_resource_apply_instance.go | 10 +++- internal/terraform/transform_diff.go | 6 +- internal/terraform/transform_diff_test.go | 56 +++++++++++++++++++ 3 files changed, 69 insertions(+), 3 deletions(-) diff --git a/internal/terraform/node_resource_apply_instance.go b/internal/terraform/node_resource_apply_instance.go index 8940810d3f32..b59f494a8bdb 100644 --- a/internal/terraform/node_resource_apply_instance.go +++ b/internal/terraform/node_resource_apply_instance.go @@ -153,9 +153,12 @@ func (n *NodeApplyableResourceInstance) dataResourceExecute(ctx EvalContext) (di return diags } // Stop early if we don't actually have a diff - if change == nil { + if change == nil || change.Action == plans.NoOp { return diags } + if change.Action != plans.Read { + diags = diags.Append(fmt.Errorf("nonsensical planned action %#v for %s; this is a bug in Terraform", change.Action, n.Addr)) + } // In this particular call to applyDataSource we include our planned // change, which signals that we expect this read to complete fully @@ -214,9 +217,12 @@ func (n *NodeApplyableResourceInstance) managedResourceExecute(ctx EvalContext) // We don't want to do any destroys // (these are handled by NodeDestroyResourceInstance instead) - if diffApply == nil || diffApply.Action == plans.Delete { + if diffApply == nil || diffApply.Action == plans.NoOp || diffApply.Action == plans.Delete { return diags } + if diffApply.Action == plans.Read { + diags = diags.Append(fmt.Errorf("nonsensical planned action %#v for %s; this is a bug in Terraform", diffApply.Action, n.Addr)) + } destroy := (diffApply.Action == plans.Delete || diffApply.Action.IsReplace()) // Get the stored action for CBD if we have a plan already diff --git a/internal/terraform/transform_diff.go b/internal/terraform/transform_diff.go index c65d5af96c6e..894db4c51901 100644 --- a/internal/terraform/transform_diff.go +++ b/internal/terraform/transform_diff.go @@ -65,7 +65,11 @@ func (t *DiffTransformer) Transform(g *Graph) error { var update, delete, createBeforeDestroy bool switch rc.Action { case plans.NoOp: - continue + // For a no-op change we don't take any action but we still + // run any condition checks associated with the object, to + // make sure that they still hold when considering the + // results of other changes. + update = true case plans.Delete: delete = true case plans.DeleteThenCreate, plans.CreateThenDelete: diff --git a/internal/terraform/transform_diff_test.go b/internal/terraform/transform_diff_test.go index 56608e500867..c7cddc452fb2 100644 --- a/internal/terraform/transform_diff_test.go +++ b/internal/terraform/transform_diff_test.go @@ -67,6 +67,62 @@ func TestDiffTransformer(t *testing.T) { } } +func TestDiffTransformer_noOpChange(t *testing.T) { + // "No-op" changes are how we record explicitly in a plan that we did + // indeed visit a particular resource instance during the planning phase + // and concluded that no changes were needed, as opposed to the resource + // instance not existing at all or having been excluded from planning + // entirely. + // + // We must include nodes for resource instances with no-op changes in the + // apply graph, even though they won't take any external actions, because + // there are some secondary effects such as precondition/postcondition + // checks that can refer to objects elsewhere and so might have their + // results changed even if the resource instance they are attached to + // didn't actually change directly itself. + + g := Graph{Path: addrs.RootModuleInstance} + + beforeVal, err := plans.NewDynamicValue(cty.StringVal(""), cty.String) + if err != nil { + t.Fatal(err) + } + + tf := &DiffTransformer{ + Changes: &plans.Changes{ + Resources: []*plans.ResourceInstanceChangeSrc{ + { + Addr: addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "aws_instance", + Name: "foo", + }.Instance(addrs.NoKey).Absolute(addrs.RootModuleInstance), + ProviderAddr: addrs.AbsProviderConfig{ + Provider: addrs.NewDefaultProvider("aws"), + Module: addrs.RootModule, + }, + ChangeSrc: plans.ChangeSrc{ + // A "no-op" change has the no-op action and has the + // same object as both Before and After. + Action: plans.NoOp, + Before: beforeVal, + After: beforeVal, + }, + }, + }, + }, + } + if err := tf.Transform(&g); err != nil { + t.Fatalf("err: %s", err) + } + + actual := strings.TrimSpace(g.String()) + expected := strings.TrimSpace(testTransformDiffBasicStr) + if actual != expected { + t.Fatalf("bad:\n\n%s", actual) + } +} + const testTransformDiffBasicStr = ` aws_instance.foo ` From 91cb440aaaee63999066421a4cebce5050f7c77f Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Fri, 17 Jun 2022 09:26:00 -0700 Subject: [PATCH 2/2] core: Do everything except the actual action for plans.NoOp Previously we tried to early-exit before doing anything at all for any no-op changes, but that means we also skip some ancillary steps like evaluating any preconditions/postconditions. Now we'll skip only the main action itself for plans.NoOp, and still run through all of the other side-steps. Since one of those other steps is emitting events through the hooks interface, this means that now no-op actions are visible to hooks, whereas before we always filtered them out before calling. I therefore added some additional logic to the hooks to filter them out at the UI layer instead; the decision for whether or not to report that we visited a particular object and found no action required seems defensible as a UI-level concern anyway. --- internal/command/views/hook_json.go | 17 +- internal/command/views/hook_ui.go | 24 ++- internal/terraform/context_apply2_test.go | 161 +++++++++++++++++- .../node_resource_abstract_instance.go | 27 ++- .../terraform/node_resource_apply_instance.go | 27 +-- 5 files changed, 230 insertions(+), 26 deletions(-) diff --git a/internal/command/views/hook_json.go b/internal/command/views/hook_json.go index 733e62d94855..38e24de39d6c 100644 --- a/internal/command/views/hook_json.go +++ b/internal/command/views/hook_json.go @@ -7,13 +7,14 @@ import ( "time" "unicode" + "github.com/zclconf/go-cty/cty" + "github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/command/format" "github.com/hashicorp/terraform/internal/command/views/json" "github.com/hashicorp/terraform/internal/plans" "github.com/hashicorp/terraform/internal/states" "github.com/hashicorp/terraform/internal/terraform" - "github.com/zclconf/go-cty/cty" ) // How long to wait between sending heartbeat/progress messages @@ -59,8 +60,10 @@ type applyProgress struct { } func (h *jsonHook) PreApply(addr addrs.AbsResourceInstance, gen states.Generation, action plans.Action, priorState, plannedNewState cty.Value) (terraform.HookAction, error) { - idKey, idValue := format.ObjectValueIDOrName(priorState) - h.view.Hook(json.NewApplyStart(addr, action, idKey, idValue)) + if action != plans.NoOp { + idKey, idValue := format.ObjectValueIDOrName(priorState) + h.view.Hook(json.NewApplyStart(addr, action, idKey, idValue)) + } progress := applyProgress{ addr: addr, @@ -73,7 +76,9 @@ func (h *jsonHook) PreApply(addr addrs.AbsResourceInstance, gen states.Generatio h.applying[addr.String()] = progress h.applyingLock.Unlock() - go h.applyingHeartbeat(progress) + if action != plans.NoOp { + go h.applyingHeartbeat(progress) + } return terraform.HookActionContinue, nil } @@ -101,6 +106,10 @@ func (h *jsonHook) PostApply(addr addrs.AbsResourceInstance, gen states.Generati delete(h.applying, key) h.applyingLock.Unlock() + if progress.action == plans.NoOp { + return terraform.HookActionContinue, nil + } + elapsed := h.timeNow().Round(time.Second).Sub(progress.start) if err != nil { diff --git a/internal/command/views/hook_ui.go b/internal/command/views/hook_ui.go index 9f410519962a..2c5c0f5704fc 100644 --- a/internal/command/views/hook_ui.go +++ b/internal/command/views/hook_ui.go @@ -65,6 +65,7 @@ const ( uiResourceModify uiResourceDestroy uiResourceRead + uiResourceNoOp ) func (h *UiHook) PreApply(addr addrs.AbsResourceInstance, gen states.Generation, action plans.Action, priorState, plannedNewState cty.Value) (terraform.HookAction, error) { @@ -89,6 +90,8 @@ func (h *UiHook) PreApply(addr addrs.AbsResourceInstance, gen states.Generation, case plans.Read: operation = "Reading..." op = uiResourceRead + case plans.NoOp: + op = uiResourceNoOp default: // We don't expect any other actions in here, so anything else is a // bug in the caller but we'll ignore it in order to be robust. @@ -106,12 +109,14 @@ func (h *UiHook) PreApply(addr addrs.AbsResourceInstance, gen states.Generation, idValue = "" } - h.println(fmt.Sprintf( - h.view.colorize.Color("[reset][bold]%s: %s%s[reset]"), - dispAddr, - operation, - stateIdSuffix, - )) + if operation != "" { + h.println(fmt.Sprintf( + h.view.colorize.Color("[reset][bold]%s: %s%s[reset]"), + dispAddr, + operation, + stateIdSuffix, + )) + } key := addr.String() uiState := uiResourceState{ @@ -129,7 +134,9 @@ func (h *UiHook) PreApply(addr addrs.AbsResourceInstance, gen states.Generation, h.resourcesLock.Unlock() // Start goroutine that shows progress - go h.stillApplying(uiState) + if op != uiResourceNoOp { + go h.stillApplying(uiState) + } return terraform.HookActionContinue, nil } @@ -201,6 +208,9 @@ func (h *UiHook) PostApply(addr addrs.AbsResourceInstance, gen states.Generation msg = "Creation complete" case uiResourceRead: msg = "Read complete" + case uiResourceNoOp: + // We don't make any announcements about no-op changes + return terraform.HookActionContinue, nil case uiResourceUnknown: return terraform.HookActionContinue, nil } diff --git a/internal/terraform/context_apply2_test.go b/internal/terraform/context_apply2_test.go index e957a8bc0796..9637a6951e34 100644 --- a/internal/terraform/context_apply2_test.go +++ b/internal/terraform/context_apply2_test.go @@ -1,6 +1,7 @@ package terraform import ( + "bytes" "errors" "fmt" "strings" @@ -8,14 +9,16 @@ import ( "testing" "time" + "github.com/davecgh/go-spew/spew" "github.com/google/go-cmp/cmp" + "github.com/zclconf/go-cty/cty" + "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/zclconf/go-cty/cty" ) // Test that the PreApply hook is called with the correct deposed key @@ -914,6 +917,162 @@ resource "test_resource" "c" { }) } +func TestContext2Apply_resourceConditionApplyTimeFail(t *testing.T) { + // This tests the less common situation where a condition fails due to + // a change in a resource other than the one the condition is attached to, + // and the condition result is unknown during planning. + // + // This edge case is a tricky one because it relies on Terraform still + // visiting test_resource.b (in the configuration below) to evaluate + // its conditions even though there aren't any changes directly planned + // for it, so that we can consider whether changes to test_resource.a + // have changed the outcome. + + m := testModuleInline(t, map[string]string{ + "main.tf": ` + variable "input" { + type = string + } + + resource "test_resource" "a" { + value = var.input + } + + resource "test_resource" "b" { + value = "beep" + + lifecycle { + postcondition { + condition = test_resource.a.output == self.output + error_message = "Outputs must match." + } + } + } + `, + }) + + p := testProvider("test") + p.GetProviderSchemaResponse = getProviderSchemaResponseFromProviderSchema(&ProviderSchema{ + ResourceTypes: map[string]*configschema.Block{ + "test_resource": { + Attributes: map[string]*configschema.Attribute{ + "value": { + Type: cty.String, + Required: true, + }, + "output": { + Type: cty.String, + Computed: true, + }, + }, + }, + }, + }) + p.PlanResourceChangeFn = func(req providers.PlanResourceChangeRequest) (resp providers.PlanResourceChangeResponse) { + // Whenever "value" changes, "output" follows it during the apply step, + // but is initially unknown during the plan step. + + m := req.ProposedNewState.AsValueMap() + priorVal := cty.NullVal(cty.String) + if !req.PriorState.IsNull() { + priorVal = req.PriorState.GetAttr("value") + } + if m["output"].IsNull() || !priorVal.RawEquals(m["value"]) { + m["output"] = cty.UnknownVal(cty.String) + } + + resp.PlannedState = cty.ObjectVal(m) + resp.LegacyTypeSystem = true + return resp + } + p.ApplyResourceChangeFn = func(req providers.ApplyResourceChangeRequest) (resp providers.ApplyResourceChangeResponse) { + m := req.PlannedState.AsValueMap() + m["output"] = m["value"] + resp.NewState = cty.ObjectVal(m) + return resp + } + ctx := testContext2(t, &ContextOpts{ + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), + }, + }) + instA := mustResourceInstanceAddr("test_resource.a") + instB := mustResourceInstanceAddr("test_resource.b") + + // Preparation: an initial plan and apply with a correct input variable + // should succeed and give us a valid and complete state to use for the + // subsequent plan and apply that we'll expect to fail. + var prevRunState *states.State + { + plan, diags := ctx.Plan(m, states.NewState(), &PlanOpts{ + Mode: plans.NormalMode, + SetVariables: InputValues{ + "input": &InputValue{ + Value: cty.StringVal("beep"), + SourceType: ValueFromCLIArg, + }, + }, + }) + assertNoErrors(t, diags) + planA := plan.Changes.ResourceInstance(instA) + if planA == nil || planA.Action != plans.Create { + t.Fatalf("incorrect initial plan for instance A\nwant a 'create' change\ngot: %s", spew.Sdump(planA)) + } + planB := plan.Changes.ResourceInstance(instB) + if planB == nil || planB.Action != plans.Create { + t.Fatalf("incorrect initial plan for instance B\nwant a 'create' change\ngot: %s", spew.Sdump(planB)) + } + + state, diags := ctx.Apply(plan, m) + assertNoErrors(t, diags) + + stateA := state.ResourceInstance(instA) + if stateA == nil || stateA.Current == nil || !bytes.Contains(stateA.Current.AttrsJSON, []byte(`"beep"`)) { + t.Fatalf("incorrect initial state for instance A\ngot: %s", spew.Sdump(stateA)) + } + stateB := state.ResourceInstance(instB) + if stateB == nil || stateB.Current == nil || !bytes.Contains(stateB.Current.AttrsJSON, []byte(`"beep"`)) { + t.Fatalf("incorrect initial state for instance B\ngot: %s", spew.Sdump(stateB)) + } + prevRunState = state + } + + // Now we'll run another plan and apply with a different value for + // var.input that should cause the test_resource.b condition to be unknown + // during planning and then fail during apply. + { + plan, diags := ctx.Plan(m, prevRunState, &PlanOpts{ + Mode: plans.NormalMode, + SetVariables: InputValues{ + "input": &InputValue{ + Value: cty.StringVal("boop"), // NOTE: This has changed + SourceType: ValueFromCLIArg, + }, + }, + }) + assertNoErrors(t, diags) + planA := plan.Changes.ResourceInstance(instA) + if planA == nil || planA.Action != plans.Update { + t.Fatalf("incorrect initial plan for instance A\nwant an 'update' change\ngot: %s", spew.Sdump(planA)) + } + planB := plan.Changes.ResourceInstance(instB) + if planB == nil || planB.Action != plans.NoOp { + t.Fatalf("incorrect initial plan for instance B\nwant a 'no-op' change\ngot: %s", spew.Sdump(planB)) + } + + _, diags = ctx.Apply(plan, m) + if !diags.HasErrors() { + t.Fatal("final apply succeeded, but should've failed with a postcondition error") + } + if len(diags) != 1 { + t.Fatalf("expected exactly one diagnostic, but got: %s", diags.Err().Error()) + } + if got, want := diags[0].Description().Summary, "Resource postcondition failed"; got != want { + t.Fatalf("wrong diagnostic summary\ngot: %s\nwant: %s", got, want) + } + } +} + // pass an input through some expanded values, and back to a provider to make // sure we can fully evaluate a provider configuration during a destroy plan. func TestContext2Apply_destroyWithConfiguredProvider(t *testing.T) { diff --git a/internal/terraform/node_resource_abstract_instance.go b/internal/terraform/node_resource_abstract_instance.go index da678f9f71a1..7ffc310d42c0 100644 --- a/internal/terraform/node_resource_abstract_instance.go +++ b/internal/terraform/node_resource_abstract_instance.go @@ -6,6 +6,8 @@ import ( "strings" "github.com/hashicorp/hcl/v2" + "github.com/zclconf/go-cty/cty" + "github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/configs" "github.com/hashicorp/terraform/internal/configs/configschema" @@ -16,7 +18,6 @@ import ( "github.com/hashicorp/terraform/internal/provisioners" "github.com/hashicorp/terraform/internal/states" "github.com/hashicorp/terraform/internal/tfdiags" - "github.com/zclconf/go-cty/cty" ) // NodeAbstractResourceInstance represents a resource instance with no @@ -1710,7 +1711,7 @@ func (n *NodeAbstractResourceInstance) applyDataSource(ctx EvalContext, planned return nil, keyData, diags.Append(fmt.Errorf("provider schema not available for %s", n.Addr)) } - if planned != nil && planned.Action != plans.Read { + if planned != nil && planned.Action != plans.Read && planned.Action != plans.NoOp { // If any other action gets in here then that's always a bug; this // EvalNode only deals with reading. diags = diags.Append(fmt.Errorf( @@ -1745,6 +1746,13 @@ func (n *NodeAbstractResourceInstance) applyDataSource(ctx EvalContext, planned return nil, keyData, diags // failed preconditions prevent further evaluation } + if planned.Action == plans.NoOp { + // If we didn't actually plan to read this then we have nothing more + // to do; we're evaluating this only for incidentals like the + // precondition/postcondition checks. + return nil, keyData, diags + } + configVal, _, configDiags := ctx.EvaluateBlock(config.Config, schema, nil, keyData) diags = diags.Append(configDiags) if configDiags.HasErrors() { @@ -2042,6 +2050,19 @@ func (n *NodeAbstractResourceInstance) apply( state = &states.ResourceInstanceObject{} } + if applyConfig != nil { + forEach, _ := evaluateForEachExpression(applyConfig.ForEach, ctx) + keyData = EvalDataForInstanceKey(n.ResourceInstanceAddr().Resource.Key, forEach) + } + + if change.Action == plans.NoOp { + // If this is a no-op change then we don't want to actually change + // anything, so we'll just echo back the state we were given and + // let our internal checks and updates proceed. + log.Printf("[TRACE] NodeAbstractResourceInstance.apply: skipping %s because it has no planned action", n.Addr) + return state, keyData, diags + } + provider, providerSchema, err := getProvider(ctx, n.ResolvedProvider) if err != nil { return nil, keyData, diags.Append(err) @@ -2058,8 +2079,6 @@ func (n *NodeAbstractResourceInstance) apply( configVal := cty.NullVal(cty.DynamicPseudoType) if applyConfig != nil { var configDiags tfdiags.Diagnostics - forEach, _ := evaluateForEachExpression(applyConfig.ForEach, ctx) - keyData = EvalDataForInstanceKey(n.ResourceInstanceAddr().Resource.Key, forEach) configVal, _, configDiags = ctx.EvaluateBlock(applyConfig.Config, schema, nil, keyData) diags = diags.Append(configDiags) if configDiags.HasErrors() { diff --git a/internal/terraform/node_resource_apply_instance.go b/internal/terraform/node_resource_apply_instance.go index b59f494a8bdb..7fc6e6fc2e28 100644 --- a/internal/terraform/node_resource_apply_instance.go +++ b/internal/terraform/node_resource_apply_instance.go @@ -153,10 +153,10 @@ func (n *NodeApplyableResourceInstance) dataResourceExecute(ctx EvalContext) (di return diags } // Stop early if we don't actually have a diff - if change == nil || change.Action == plans.NoOp { + if change == nil { return diags } - if change.Action != plans.Read { + if change.Action != plans.Read && change.Action != plans.NoOp { diags = diags.Append(fmt.Errorf("nonsensical planned action %#v for %s; this is a bug in Terraform", change.Action, n.Addr)) } @@ -169,9 +169,16 @@ func (n *NodeApplyableResourceInstance) dataResourceExecute(ctx EvalContext) (di return diags } - diags = diags.Append(n.writeResourceInstanceState(ctx, state, workingState)) - if diags.HasErrors() { - return diags + if state != nil { + // If n.applyDataSource returned a nil state object with no accompanying + // errors then it determined that the given change doesn't require + // actually reading the data (e.g. because it was already read during + // the plan phase) and so we're only running through here to get the + // extra details like precondition/postcondition checks. + diags = diags.Append(n.writeResourceInstanceState(ctx, state, workingState)) + if diags.HasErrors() { + return diags + } } diags = diags.Append(n.writeChange(ctx, nil, "")) @@ -217,7 +224,7 @@ func (n *NodeApplyableResourceInstance) managedResourceExecute(ctx EvalContext) // We don't want to do any destroys // (these are handled by NodeDestroyResourceInstance instead) - if diffApply == nil || diffApply.Action == plans.NoOp || diffApply.Action == plans.Delete { + if diffApply == nil || diffApply.Action == plans.Delete { return diags } if diffApply.Action == plans.Read { @@ -273,10 +280,10 @@ func (n *NodeApplyableResourceInstance) managedResourceExecute(ctx EvalContext) diffApply = reducePlan(addr, diffApply, false) // reducePlan may have simplified our planned change // into a NoOp if it only requires destroying, since destroying - // is handled by NodeDestroyResourceInstance. - if diffApply == nil || diffApply.Action == plans.NoOp { - return diags - } + // is handled by NodeDestroyResourceInstance. If so, we'll + // still run through most of the logic here because we do still + // need to deal with other book-keeping such as marking the + // change as "complete", and running the author's postconditions. diags = diags.Append(n.preApplyHook(ctx, diffApply)) if diags.HasErrors() {