Merge pull request #29193 from hashicorp/jbardin/ignore-changes-marks

Handle marks on ignore_changes values

internal/terraform/context_apply2_test.go

@@ -508,3 +508,69 @@ output "out" {
 		}
 	}
 }
+
+func TestContext2Apply_ignoreImpureFunctionChanges(t *testing.T) {
+	// The impure function call should not cause a planned change with
+	// ignore_changes
+	m := testModuleInline(t, map[string]string{
+		"main.tf": `
+variable "pw" {
+  sensitive = true
+  default = "foo"
+}
+
+resource "test_object" "x" {
+  test_map = {
+	string = "X${bcrypt(var.pw)}"
+  }
+  lifecycle {
+    ignore_changes = [ test_map["string"] ]
+  }
+}
+
+resource "test_object" "y" {
+  test_map = {
+	string = "X${bcrypt(var.pw)}"
+  }
+  lifecycle {
+    ignore_changes = [ test_map ]
+  }
+}
+
+`,
+	})
+
+	p := simpleMockProvider()
+
+	ctx := testContext2(t, &ContextOpts{
+		Config: m,
+		Providers: map[addrs.Provider]providers.Factory{
+			addrs.NewDefaultProvider("test"): testProviderFuncFixed(p),
+		},
+	})
+
+	_, diags := ctx.Plan()
+	if diags.HasErrors() {
+		t.Fatal(diags.ErrWithWarnings())
+	}
+
+	_, diags = ctx.Apply()
+	if diags.HasErrors() {
+		t.Fatal(diags.ErrWithWarnings())
+	}
+
+	// FINAL PLAN:
+	plan, diags := ctx.Plan()
+	if diags.HasErrors() {
+		t.Fatal(diags.ErrWithWarnings())
+	}
+
+	// make sure the same marks are compared in the next plan as well
+	for _, c := range plan.Changes.Resources {
+		if c.Action != plans.NoOp {
+			t.Logf("marks before: %#v", c.BeforeValMarks)
+			t.Logf("marks after:  %#v", c.AfterValMarks)
+			t.Errorf("Unexpcetd %s change for %s", c.Action, c.Addr)
+		}
+	}
+}

internal/terraform/context_plan_test.go

@@ -4794,7 +4794,7 @@ func TestContext2Plan_ignoreChangesSensitive(t *testing.T) {
 
 	checkVals(t, objectVal(t, schema, map[string]cty.Value{
 		"id":   cty.StringVal("bar"),
-		"ami":  cty.StringVal("ami-abcd1234").Mark(marks.Sensitive),
+		"ami":  cty.StringVal("ami-abcd1234"),
 		"type": cty.StringVal("aws_instance"),
 	}), ric.After)
 }

internal/terraform/node_resource_abstract_instance.go

@@ -687,26 +687,24 @@ func (n *NodeAbstractResourceInstance) plan(
 		priorVal = cty.NullVal(schema.ImpliedType())
 	}
 
-	// Create an unmarked version of our config val and our prior val.
-	// Store the paths for the config val to re-markafter
-	// we've sent things over the wire.
-	unmarkedConfigVal, unmarkedPaths := origConfigVal.UnmarkDeepWithPaths()
-	unmarkedPriorVal, priorPaths := priorVal.UnmarkDeepWithPaths()
-
 	log.Printf("[TRACE] Re-validating config for %q", n.Addr)
-	// Allow the provider to validate the final set of values.
+	// Allow the provider to validate the final set of values.  The config was
-	// The config was statically validated early on, but there may have been
+	// statically validated early on, but there may have been unknown values
-	// unknown values which the provider could not validate at the time.
+	// which the provider could not validate at the time.
+	//
 	// TODO: It would be more correct to validate the config after
 	// ignore_changes has been applied, but the current implementation cannot
 	// exclude computed-only attributes when given the `all` option.
+
+	// we must unmark and use the original config, since the ignore_changes
+	// handling below needs access to the marks.
+	unmarkedConfigVal, _ := origConfigVal.UnmarkDeep()
 	validateResp := provider.ValidateResourceConfig(
 		providers.ValidateResourceConfigRequest{
 			TypeName: n.Addr.Resource.Resource.Type,
 			Config:   unmarkedConfigVal,
 		},
 	)
-
 	diags = diags.Append(validateResp.Diagnostics.InConfigBody(config.Config, n.Addr.String()))
 	if diags.HasErrors() {
 		return plan, state, diags
@@ -717,13 +715,21 @@ func (n *NodeAbstractResourceInstance) plan(
 	// the proposed value, the proposed value itself, and the config presented
 	// to the provider in the PlanResourceChange request all agree on the
 	// starting values.
-	configValIgnored, ignoreChangeDiags := n.processIgnoreChanges(unmarkedPriorVal, unmarkedConfigVal)
+	// Here we operate on the marked values, so as to revert any changes to the
+	// marks as well as the value.
+	configValIgnored, ignoreChangeDiags := n.processIgnoreChanges(priorVal, origConfigVal)
 	diags = diags.Append(ignoreChangeDiags)
 	if ignoreChangeDiags.HasErrors() {
 		return plan, state, diags
 	}
 
-	proposedNewVal := objchange.ProposedNew(schema, unmarkedPriorVal, configValIgnored)
+	// Create an unmarked version of our config val and our prior val.
+	// Store the paths for the config val to re-mark after we've sent things
+	// over the wire.
+	unmarkedConfigVal, unmarkedPaths := configValIgnored.UnmarkDeepWithPaths()
+	unmarkedPriorVal, priorPaths := priorVal.UnmarkDeepWithPaths()
+
+	proposedNewVal := objchange.ProposedNew(schema, unmarkedPriorVal, unmarkedConfigVal)
 
 	// Call pre-diff hook
 	diags = diags.Append(ctx.Hook(func(h Hook) (HookAction, error) {
@@ -735,7 +741,7 @@ func (n *NodeAbstractResourceInstance) plan(
 
 	resp := provider.PlanResourceChange(providers.PlanResourceChangeRequest{
 		TypeName:         n.Addr.Resource.Resource.Type,
-		Config:           configValIgnored,
+		Config:           unmarkedConfigVal,
 		PriorState:       unmarkedPriorVal,
 		ProposedNewState: proposedNewVal,
 		PriorPrivate:     priorPrivate,
@@ -774,7 +780,7 @@ func (n *NodeAbstractResourceInstance) plan(
 		return plan, state, diags
 	}
 
-	if errs := objchange.AssertPlanValid(schema, unmarkedPriorVal, configValIgnored, plannedNewVal); len(errs) > 0 {
+	if errs := objchange.AssertPlanValid(schema, unmarkedPriorVal, unmarkedConfigVal, plannedNewVal); len(errs) > 0 {
 		if resp.LegacyTypeSystem {
 			// The shimming of the old type system in the legacy SDK is not precise
 			// enough to pass this consistency check, so we'll give it a pass here,
@@ -1085,7 +1091,7 @@ func (n *NodeAbstractResource) processIgnoreChanges(prior, config cty.Value) (ct
 		return config, nil
 	}
 
-	ignoreChanges := n.Config.Managed.IgnoreChanges
+	ignoreChanges := traversalsToPaths(n.Config.Managed.IgnoreChanges)
 	ignoreAll := n.Config.Managed.IgnoreAllChanges
 
 	if len(ignoreChanges) == 0 && !ignoreAll {
@@ -1100,14 +1106,16 @@ func (n *NodeAbstractResource) processIgnoreChanges(prior, config cty.Value) (ct
 		return config, nil
 	}
 
-	return processIgnoreChangesIndividual(prior, config, ignoreChanges)
+	ret, diags := processIgnoreChangesIndividual(prior, config, ignoreChanges)
+
+	return ret, diags
 }
 
-func processIgnoreChangesIndividual(prior, config cty.Value, ignoreChanges []hcl.Traversal) (cty.Value, tfdiags.Diagnostics) {
+// Convert the hcl.Traversal values we get form the configuration to the
-	// When we walk below we will be using cty.Path values for comparison, so
+// cty.Path values we need to operate on the cty.Values
-	// we'll convert our traversals here so we can compare more easily.
+func traversalsToPaths(traversals []hcl.Traversal) []cty.Path {
-	ignoreChangesPath := make([]cty.Path, len(ignoreChanges))
+	paths := make([]cty.Path, len(traversals))
-	for i, traversal := range ignoreChanges {
+	for i, traversal := range traversals {
 		path := make(cty.Path, len(traversal))
 		for si, step := range traversal {
 			switch ts := step.(type) {
@@ -1127,9 +1135,12 @@ func processIgnoreChangesIndividual(prior, config cty.Value, ignoreChanges []hcl
 				panic(fmt.Sprintf("unsupported traversal step %#v", step))
 			}
 		}
-		ignoreChangesPath[i] = path
+		paths[i] = path
 	}
+	return paths
+}
 
+func processIgnoreChangesIndividual(prior, config cty.Value, ignoreChangesPath []cty.Path) (cty.Value, tfdiags.Diagnostics) {
 	type ignoreChange struct {
 		// Path is the full path, minus any trailing map index
 		path cty.Path
@@ -1175,8 +1186,7 @@ func processIgnoreChangesIndividual(prior, config cty.Value, ignoreChanges []hcl
 		// here even if our ignored key doesn't change. That is OK since it
 		// won't cause any changes in the transformation, but allows us to skip
 		// breaking up the maps and checking for key existence here too.
-		eq := p.Equals(c)
+		if !p.RawEquals(c) {
-		if !eq.IsKnown() || eq.False() {
 			// there a change to ignore at this path, store the prior value
 			ignoredValues = append(ignoredValues, ignoreChange{icPath, p, key})
 		}
@@ -1205,6 +1215,10 @@ func processIgnoreChangesIndividual(prior, config cty.Value, ignoreChanges []hcl
 			return v, nil
 		}
 
+		// The map values will remain as cty values, so we only need to store
+		// the marks from the outer map itself
+		v, vMarks := v.Unmark()
+
 		// The configMap is the current configuration value, which we will
 		// mutate based on the ignored paths and the prior map value.
 		var configMap map[string]cty.Value
@@ -1234,11 +1248,17 @@ func processIgnoreChangesIndividual(prior, config cty.Value, ignoreChanges []hcl
 			// return null for a key with a null value and for a non-existent
 			// key.
 			var priorMap map[string]cty.Value
+
+			// We need to drop the marks from the ignored map for handling. We
+			// don't need to store these, as we now know the ignored value is
+			// only within the map, not the map itself.
+			ignoredVal, _ := ignored.value.Unmark()
+
 			switch {
-			case ignored.value.IsNull() || ignored.value.LengthInt() == 0:
+			case ignored.value.IsNull() || ignoredVal.LengthInt() == 0:
 				priorMap = map[string]cty.Value{}
 			default:
-				priorMap = ignored.value.AsValueMap()
+				priorMap = ignoredVal.AsValueMap()
 			}
 
 			key := ignored.key.AsString()
@@ -1254,11 +1274,18 @@ func processIgnoreChangesIndividual(prior, config cty.Value, ignoreChanges []hcl
 			}
 		}
 
+		var newVal cty.Value
 		if len(configMap) == 0 {
-			return cty.MapValEmpty(v.Type().ElementType()), nil
+			newVal = cty.MapValEmpty(v.Type().ElementType())
+		} else {
+			newVal = cty.MapVal(configMap)
 		}
 
-		return cty.MapVal(configMap), nil
+		if len(vMarks) > 0 {
+			newVal = v.WithMarks(vMarks)
+		}
+
+		return newVal, nil
 	})
 	return ret, nil
 }

internal/terraform/reduce_plan_test.go

@@ -382,7 +382,7 @@ func TestProcessIgnoreChangesIndividual(t *testing.T) {
 				ignore[i] = trav
 			}
 
-			ret, diags := processIgnoreChangesIndividual(test.Old, test.New, ignore)
+			ret, diags := processIgnoreChangesIndividual(test.Old, test.New, traversalsToPaths(ignore))
 			if diags.HasErrors() {
 				t.Fatal(diags.Err())
 			}