diff --git a/internal/states/statefile/version4.go b/internal/states/statefile/version4.go index cb3dd694dd3d..2cdde4be7107 100644 --- a/internal/states/statefile/version4.go +++ b/internal/states/statefile/version4.go @@ -414,9 +414,7 @@ func writeStateV4(file *File, w io.Writer) tfdiags.Diagnostics { } } - if file.State.CheckResults != nil { - sV4.CheckResults = encodeCheckResultsV4(file.State.CheckResults) - } + sV4.CheckResults = encodeCheckResultsV4(file.State.CheckResults) sV4.normalize() @@ -576,6 +574,11 @@ func decodeCheckResultsV4(in []checkResultsV4) (*states.CheckResults, tfdiags.Di } func encodeCheckResultsV4(in *states.CheckResults) []checkResultsV4 { + // normalize empty and nil sets in the serialized state + if in == nil || in.ConfigResults.Len() == 0 { + return nil + } + ret := make([]checkResultsV4, 0, in.ConfigResults.Len()) for _, configElem := range in.ConfigResults.Elems { diff --git a/internal/terraform/context_apply.go b/internal/terraform/context_apply.go index 4630f66a9193..8631f04c30a0 100644 --- a/internal/terraform/context_apply.go +++ b/internal/terraform/context_apply.go @@ -25,6 +25,10 @@ func (c *Context) Apply(plan *plans.Plan, config *configs.Config) (*states.State log.Printf("[DEBUG] Building and walking apply graph for %s plan", plan.UIMode) + // FIXME: refresh plans still store outputs as changes, so we can't use + // Empty() + possibleRefresh := len(plan.Changes.Resources) == 0 + graph, operation, diags := c.applyGraph(plan, config, true) if diags.HasErrors() { return nil, diags @@ -69,6 +73,18 @@ Note that the -target option is not suitable for routine use, and is provided on )) } + // FIXME: we cannot check for an empty plan for refresh-only, because root + // outputs are always stored as changes. The final condition of the state + // also depends on some cleanup which happens during the apply walk. It + // would probably make more sense if applying a refresh-only plan were + // simply just returning the planned state and checks, but some extra + // cleanup is going to be needed to make the plan state match what apply + // would do. For now we can copy the checks over which were overwritten + // during the apply walk. + if possibleRefresh { + newState.CheckResults = plan.Checks.DeepCopy() + } + return newState, diags } diff --git a/internal/terraform/context_apply2_test.go b/internal/terraform/context_apply2_test.go index 100e75def83d..9000364ed39b 100644 --- a/internal/terraform/context_apply2_test.go +++ b/internal/terraform/context_apply2_test.go @@ -1641,3 +1641,146 @@ output "from_resource" { _, diags = ctx.Apply(plan, m) assertNoErrors(t, diags) } + +// -refresh-only should update checks +func TestContext2Apply_refreshApplyUpdatesChecks(t *testing.T) { + m := testModuleInline(t, map[string]string{ + "main.tf": ` +resource "test_object" "x" { + test_string = "ok" + lifecycle { + postcondition { + condition = self.test_string == "ok" + error_message = "wrong val" + } + } +} + +output "from_resource" { + value = test_object.x.test_string + precondition { + condition = test_object.x.test_string == "ok" + error_message = "wrong val" + } +} +`}) + + p := simpleMockProvider() + p.ReadResourceResponse = &providers.ReadResourceResponse{ + NewState: cty.ObjectVal(map[string]cty.Value{ + "test_string": cty.StringVal("ok"), + }), + } + + state := states.NewState() + mod := state.EnsureModule(addrs.RootModuleInstance) + mod.SetResourceInstanceCurrent( + mustResourceInstanceAddr("test_object.x").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"test_string":"wrong val"}`), + }, + mustProviderConfig(`provider["registry.terraform.io/hashicorp/test"]`), + ) + mod.SetOutputValue("from_resource", cty.StringVal("wrong val"), false) + + ctx := testContext2(t, &ContextOpts{ + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), + }, + }) + + opts := SimplePlanOpts(plans.RefreshOnlyMode, nil) + plan, diags := ctx.Plan(m, state, opts) + assertNoErrors(t, diags) + + state, diags = ctx.Apply(plan, m) + assertNoErrors(t, diags) + + resCheck := state.CheckResults.GetObjectResult(mustResourceInstanceAddr("test_object.x")) + if resCheck.Status != checks.StatusPass { + t.Fatalf("unexpected check %s: %s\n", resCheck.Status, resCheck.FailureMessages) + } + + outAddr := addrs.AbsOutputValue{ + Module: addrs.RootModuleInstance, + OutputValue: addrs.OutputValue{ + Name: "from_resource", + }, + } + outCheck := state.CheckResults.GetObjectResult(outAddr) + if outCheck.Status != checks.StatusPass { + t.Fatalf("unexpected check %s: %s\n", outCheck.Status, outCheck.FailureMessages) + } +} + +// NoOp changes may have conditions to evaluate, but should not re-plan and +// apply the entire resource. +func TestContext2Apply_noRePlanNoOp(t *testing.T) { + m := testModuleInline(t, map[string]string{ + "main.tf": ` +resource "test_object" "x" { +} + +resource "test_object" "y" { + # test_object.w is being re-created, so this precondition must be evaluated + # during apply, however this resource should otherwise be a NoOp. + lifecycle { + precondition { + condition = test_object.x.test_string == null + error_message = "test_object.x.test_string should be null" + } + } +} +`}) + + p := simpleMockProvider() + // make sure we can compute the attr + testString := p.GetProviderSchemaResponse.ResourceTypes["test_object"].Block.Attributes["test_string"] + testString.Computed = true + testString.Optional = false + + yAddr := mustResourceInstanceAddr("test_object.y") + + state := states.NewState() + mod := state.RootModule() + mod.SetResourceInstanceCurrent( + yAddr.Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"test_string":"y"}`), + }, + mustProviderConfig(`provider["registry.terraform.io/hashicorp/test"]`), + ) + + ctx := testContext2(t, &ContextOpts{ + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), + }, + }) + + opts := SimplePlanOpts(plans.NormalMode, nil) + plan, diags := ctx.Plan(m, state, opts) + assertNoErrors(t, diags) + + for _, c := range plan.Changes.Resources { + if c.Addr.Equal(yAddr) && c.Action != plans.NoOp { + t.Fatalf("unexpected %s change for test_object.y", c.Action) + } + } + + // test_object.y is a NoOp change from the plan, but is included in the + // graph due to the conditions which must be evaluated. This however should + // not cause the resource to be re-planned. + p.PlanResourceChangeFn = func(req providers.PlanResourceChangeRequest) (resp providers.PlanResourceChangeResponse) { + testString := req.ProposedNewState.GetAttr("test_string") + if !testString.IsNull() && testString.AsString() == "y" { + resp.Diagnostics = resp.Diagnostics.Append(errors.New("Unexpected apply-time plan for test_object.y. Original plan was a NoOp")) + } + resp.PlannedState = req.ProposedNewState + return resp + } + + _, diags = ctx.Apply(plan, m) + assertNoErrors(t, diags) +} diff --git a/internal/terraform/graph_builder_apply.go b/internal/terraform/graph_builder_apply.go index 4d7763d2a5f3..22f1b8c3bc10 100644 --- a/internal/terraform/graph_builder_apply.go +++ b/internal/terraform/graph_builder_apply.go @@ -107,6 +107,7 @@ func (b *ApplyGraphBuilder) Steps() []GraphTransformer { Concrete: concreteResourceInstance, State: b.State, Changes: b.Changes, + Config: b.Config, }, // Attach the state diff --git a/internal/terraform/node_resource_abstract_instance.go b/internal/terraform/node_resource_abstract_instance.go index ba1aa3a17c60..659ff5ff5397 100644 --- a/internal/terraform/node_resource_abstract_instance.go +++ b/internal/terraform/node_resource_abstract_instance.go @@ -715,6 +715,13 @@ func (n *NodeAbstractResourceInstance) plan( return plan, state, keyData, diags // failed preconditions prevent further evaluation } + // If we have a previous plan and the action was a noop, then the only + // reason we're in this method was to evaluate the preconditions. There's + // no need to re-plan this resource. + if plannedChange != nil && plannedChange.Action == plans.NoOp { + return plannedChange, currentState.DeepCopy(), keyData, diags + } + origConfigVal, _, configDiags := ctx.EvaluateBlock(config.Config, schema, nil, keyData) diags = diags.Append(configDiags) if configDiags.HasErrors() { @@ -2059,44 +2066,38 @@ func (n *NodeAbstractResourceInstance) evalDestroyProvisionerConfig(ctx EvalCont } // apply accepts an applyConfig, instead of using n.Config, so destroy plans can -// send a nil config. Most of the errors generated in apply are returned as -// diagnostics, but if provider.ApplyResourceChange itself fails, that error is -// returned as an error and nil diags are returned. +// send a nil config. The keyData information can be empty if the config is +// nil, since it is only used to evaluate the configuration. func (n *NodeAbstractResourceInstance) apply( ctx EvalContext, state *states.ResourceInstanceObject, change *plans.ResourceInstanceChange, applyConfig *configs.Resource, - createBeforeDestroy bool) (*states.ResourceInstanceObject, instances.RepetitionData, tfdiags.Diagnostics) { + keyData instances.RepetitionData, + createBeforeDestroy bool) (*states.ResourceInstanceObject, tfdiags.Diagnostics) { var diags tfdiags.Diagnostics - var keyData instances.RepetitionData if state == nil { 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 + return state, diags } provider, providerSchema, err := getProvider(ctx, n.ResolvedProvider) if err != nil { - return nil, keyData, diags.Append(err) + return nil, diags.Append(err) } schema, _ := providerSchema.SchemaForResourceType(n.Addr.Resource.Resource.Mode, n.Addr.Resource.Resource.Type) if schema == nil { // Should be caught during validation, so we don't bother with a pretty error here diags = diags.Append(fmt.Errorf("provider does not support resource type %q", n.Addr.Resource.Resource.Type)) - return nil, keyData, diags + return nil, diags } log.Printf("[INFO] Starting apply for %s", n.Addr) @@ -2107,7 +2108,7 @@ func (n *NodeAbstractResourceInstance) apply( configVal, _, configDiags = ctx.EvaluateBlock(applyConfig.Config, schema, nil, keyData) diags = diags.Append(configDiags) if configDiags.HasErrors() { - return nil, keyData, diags + return nil, diags } } @@ -2132,13 +2133,13 @@ func (n *NodeAbstractResourceInstance) apply( strings.Join(unknownPaths, "\n"), ), )) - return nil, keyData, diags + return nil, diags } metaConfigVal, metaDiags := n.providerMetas(ctx) diags = diags.Append(metaDiags) if diags.HasErrors() { - return nil, keyData, diags + return nil, diags } log.Printf("[DEBUG] %s: applying the planned %s change", n.Addr, change.Action) @@ -2166,7 +2167,7 @@ func (n *NodeAbstractResourceInstance) apply( Status: state.Status, Value: change.After, } - return newState, keyData, diags + return newState, diags } resp := provider.ApplyResourceChange(providers.ApplyResourceChangeRequest{ @@ -2237,7 +2238,7 @@ func (n *NodeAbstractResourceInstance) apply( // Bail early in this particular case, because an object that doesn't // conform to the schema can't be saved in the state anyway -- the // serializer will reject it. - return nil, keyData, diags + return nil, diags } // After this point we have a type-conforming result object and so we @@ -2363,7 +2364,7 @@ func (n *NodeAbstractResourceInstance) apply( // prior state as the new value, making this effectively a no-op. If // the item really _has_ been deleted then our next refresh will detect // that and fix it up. - return state.DeepCopy(), keyData, diags + return state.DeepCopy(), diags case diags.HasErrors() && !newVal.IsNull(): // if we have an error, make sure we restore the object status in the new state @@ -2380,7 +2381,7 @@ func (n *NodeAbstractResourceInstance) apply( newState.Dependencies = state.Dependencies } - return newState, keyData, diags + return newState, diags case !newVal.IsNull(): // Non error case with a new state @@ -2390,11 +2391,11 @@ func (n *NodeAbstractResourceInstance) apply( Private: resp.Private, CreateBeforeDestroy: createBeforeDestroy, } - return newState, keyData, diags + return newState, diags default: // Non error case, were the object was deleted - return nil, keyData, diags + return nil, diags } } diff --git a/internal/terraform/node_resource_apply_instance.go b/internal/terraform/node_resource_apply_instance.go index f257c3f7d0c4..82aebec296c4 100644 --- a/internal/terraform/node_resource_apply_instance.go +++ b/internal/terraform/node_resource_apply_instance.go @@ -6,6 +6,7 @@ import ( "github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/configs" + "github.com/hashicorp/terraform/internal/instances" "github.com/hashicorp/terraform/internal/plans" "github.com/hashicorp/terraform/internal/plans/objchange" "github.com/hashicorp/terraform/internal/states" @@ -270,7 +271,7 @@ func (n *NodeApplyableResourceInstance) managedResourceExecute(ctx EvalContext) // Make a new diff, in case we've learned new values in the state // during apply which we can now incorporate. - diffApply, _, _, planDiags := n.plan(ctx, diff, state, false, n.forceReplace) + diffApply, _, repeatData, planDiags := n.plan(ctx, diff, state, false, n.forceReplace) diags = diags.Append(planDiags) if diags.HasErrors() { return diags @@ -295,7 +296,13 @@ func (n *NodeApplyableResourceInstance) managedResourceExecute(ctx EvalContext) return diags } - state, repeatData, applyDiags := n.apply(ctx, state, diffApply, n.Config, n.CreateBeforeDestroy()) + // If there is no change, there was nothing to apply, and we don't need to + // re-write the state, but we do need to re-evaluate postconditions. + if diffApply.Action == plans.NoOp { + return diags.Append(n.managedResourcePostconditions(ctx, repeatData)) + } + + state, applyDiags := n.apply(ctx, state, diffApply, n.Config, repeatData, n.CreateBeforeDestroy()) diags = diags.Append(applyDiags) // We clear the change out here so that future nodes don't see a change @@ -370,15 +377,18 @@ func (n *NodeApplyableResourceInstance) managedResourceExecute(ctx EvalContext) // _after_ writing the state because we want to check against // the result of the operation, and to fail on future operations // until the user makes the condition succeed. + return diags.Append(n.managedResourcePostconditions(ctx, repeatData)) +} + +func (n *NodeApplyableResourceInstance) managedResourcePostconditions(ctx EvalContext, repeatData instances.RepetitionData) (diags tfdiags.Diagnostics) { + checkDiags := evalCheckRules( addrs.ResourcePostcondition, n.Config.Postconditions, ctx, n.ResourceInstanceAddr(), repeatData, tfdiags.Error, ) - diags = diags.Append(checkDiags) - - return diags + return diags.Append(checkDiags) } // checkPlannedChange produces errors if the _actual_ expected value is not diff --git a/internal/terraform/node_resource_destroy.go b/internal/terraform/node_resource_destroy.go index d0cc7276bb2a..a468ad020ba1 100644 --- a/internal/terraform/node_resource_destroy.go +++ b/internal/terraform/node_resource_destroy.go @@ -4,6 +4,7 @@ import ( "fmt" "log" + "github.com/hashicorp/terraform/internal/instances" "github.com/hashicorp/terraform/internal/plans" "github.com/hashicorp/terraform/internal/tfdiags" @@ -210,7 +211,7 @@ func (n *NodeDestroyResourceInstance) managedResourceExecute(ctx EvalContext) (d // Managed resources need to be destroyed, while data sources // are only removed from state. // we pass a nil configuration to apply because we are destroying - s, _, d := n.apply(ctx, state, changeApply, nil, false) + s, d := n.apply(ctx, state, changeApply, nil, instances.RepetitionData{}, false) state, diags = s, diags.Append(d) // we don't return immediately here on error, so that the state can be // finalized diff --git a/internal/terraform/node_resource_destroy_deposed.go b/internal/terraform/node_resource_destroy_deposed.go index 2c042386a380..702e0596c124 100644 --- a/internal/terraform/node_resource_destroy_deposed.go +++ b/internal/terraform/node_resource_destroy_deposed.go @@ -6,6 +6,7 @@ import ( "github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/dag" + "github.com/hashicorp/terraform/internal/instances" "github.com/hashicorp/terraform/internal/plans" "github.com/hashicorp/terraform/internal/states" "github.com/hashicorp/terraform/internal/tfdiags" @@ -249,7 +250,7 @@ func (n *NodeDestroyDeposedResourceInstanceObject) Execute(ctx EvalContext, op w } // we pass a nil configuration to apply because we are destroying - state, _, applyDiags := n.apply(ctx, state, change, nil, false) + state, applyDiags := n.apply(ctx, state, change, nil, instances.RepetitionData{}, false) diags = diags.Append(applyDiags) // don't return immediately on errors, we need to handle the state diff --git a/internal/terraform/transform_diff.go b/internal/terraform/transform_diff.go index bfabf60e9fac..3bce1131f5a5 100644 --- a/internal/terraform/transform_diff.go +++ b/internal/terraform/transform_diff.go @@ -4,6 +4,8 @@ import ( "fmt" "log" + "github.com/hashicorp/terraform/internal/addrs" + "github.com/hashicorp/terraform/internal/configs" "github.com/hashicorp/terraform/internal/dag" "github.com/hashicorp/terraform/internal/plans" "github.com/hashicorp/terraform/internal/states" @@ -16,6 +18,28 @@ type DiffTransformer struct { Concrete ConcreteResourceInstanceNodeFunc State *states.State Changes *plans.Changes + Config *configs.Config +} + +// return true if the given resource instance has either Preconditions or +// Postconditions defined in the configuration. +func (t *DiffTransformer) hasConfigConditions(addr addrs.AbsResourceInstance) bool { + // unit tests may have no config + if t.Config == nil { + return false + } + + cfg := t.Config.DescendentForInstance(addr.Module) + if cfg == nil { + return false + } + + res := cfg.Module.ResourceByAddr(addr.ConfigResource().Resource) + if res == nil { + return false + } + + return len(res.Preconditions) > 0 || len(res.Postconditions) > 0 } func (t *DiffTransformer) Transform(g *Graph) error { @@ -69,7 +93,7 @@ func (t *DiffTransformer) Transform(g *Graph) error { // run any condition checks associated with the object, to // make sure that they still hold when considering the // results of other changes. - update = true + update = t.hasConfigConditions(addr) 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 c7cddc452fb2..fa1feae34d61 100644 --- a/internal/terraform/transform_diff_test.go +++ b/internal/terraform/transform_diff_test.go @@ -81,6 +81,26 @@ func TestDiffTransformer_noOpChange(t *testing.T) { // results changed even if the resource instance they are attached to // didn't actually change directly itself. + // aws_instance.foo has a precondition, so should be included in the final + // graph. aws_instance.bar has no conditions, so there is nothing to + // execute during apply and it should not be included in the graph. + m := testModuleInline(t, map[string]string{ + "main.tf": ` +resource "aws_instance" "bar" { +} + +resource "aws_instance" "foo" { + test_string = "ok" + + lifecycle { + precondition { + condition = self.test_string != "" + error_message = "resource error" + } + } +} +`}) + g := Graph{Path: addrs.RootModuleInstance} beforeVal, err := plans.NewDynamicValue(cty.StringVal(""), cty.String) @@ -89,6 +109,7 @@ func TestDiffTransformer_noOpChange(t *testing.T) { } tf := &DiffTransformer{ + Config: m, Changes: &plans.Changes{ Resources: []*plans.ResourceInstanceChangeSrc{ { @@ -109,6 +130,24 @@ func TestDiffTransformer_noOpChange(t *testing.T) { After: beforeVal, }, }, + { + Addr: addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "aws_instance", + Name: "bar", + }.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, + }, + }, }, }, } diff --git a/internal/terraform/transform_reference.go b/internal/terraform/transform_reference.go index 7c45955b86f8..fb8feb2f2a7f 100644 --- a/internal/terraform/transform_reference.go +++ b/internal/terraform/transform_reference.go @@ -136,7 +136,7 @@ func (t *ReferenceTransformer) Transform(g *Graph) error { if !graphNodesAreResourceInstancesInDifferentInstancesOfSameModule(v, parent) { g.Connect(dag.BasicEdge(v, parent)) } else { - log.Printf("[TRACE] ReferenceTransformer: skipping %s => %s inter-module-instance dependency", v, parent) + log.Printf("[TRACE] ReferenceTransformer: skipping %s => %s inter-module-instance dependency", dag.VertexName(v), dag.VertexName(parent)) } }