Merge pull request #32123 from hashicorp/jbardin/noop-apply-optimizations

Apply optimizations for handling of condition checks

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 {

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
 }
 

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)
+}

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

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
 	}
 }
 

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

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

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
 

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:

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,
+					},
+				},
 			},
 		},
 	}

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))
 			}
 		}