Merge pull request #27141 from hashicorp/jbardin/ignore_changes

Fixes for ignore_changes with unintended provider behavior

terraform/context_plan_test.go

@@ -6473,3 +6473,129 @@ resource "test_instance" "a" {
 		}
 	}
 }
+
+// ignore_changes needs to be re-applied to the planned value for provider
+// using the LegacyTypeSystem
+func TestContext2Plan_legacyProviderIgnoreChanges(t *testing.T) {
+	m := testModuleInline(t, map[string]string{
+		"main.tf": `
+resource "test_instance" "a" {
+  lifecycle {
+    ignore_changes = [data]
+  }
+}
+`,
+	})
+
+	p := testProvider("test")
+	p.PlanResourceChangeFn = func(req providers.PlanResourceChangeRequest) (resp providers.PlanResourceChangeResponse) {
+		m := req.ProposedNewState.AsValueMap()
+		// this provider "hashes" the data attribute as bar
+		m["data"] = cty.StringVal("bar")
+
+		resp.PlannedState = cty.ObjectVal(m)
+		resp.LegacyTypeSystem = true
+		return resp
+	}
+
+	p.GetSchemaReturn = &ProviderSchema{
+		ResourceTypes: map[string]*configschema.Block{
+			"test_instance": {
+				Attributes: map[string]*configschema.Attribute{
+					"id":   {Type: cty.String, Computed: true},
+					"data": {Type: cty.String, Optional: true},
+				},
+			},
+		},
+	}
+
+	state := states.NewState()
+	root := state.EnsureModule(addrs.RootModuleInstance)
+	root.SetResourceInstanceCurrent(
+		mustResourceInstanceAddr("test_instance.a").Resource,
+		&states.ResourceInstanceObjectSrc{
+			Status:       states.ObjectReady,
+			AttrsJSON:    []byte(`{"id":"a","data":"foo"}`),
+			Dependencies: []addrs.ConfigResource{},
+		},
+		mustProviderConfig(`provider["registry.terraform.io/hashicorp/test"]`),
+	)
+
+	ctx := testContext2(t, &ContextOpts{
+		Config: m,
+		Providers: map[addrs.Provider]providers.Factory{
+			addrs.NewDefaultProvider("test"): testProviderFuncFixed(p),
+		},
+		State: state,
+	})
+	plan, diags := ctx.Plan()
+	if diags.HasErrors() {
+		t.Fatal(diags.Err())
+	}
+
+	for _, c := range plan.Changes.Resources {
+		if c.Action != plans.NoOp {
+			t.Fatalf("expected no changes, got %s for %q", c.Action, c.Addr)
+		}
+	}
+}
+
+func TestContext2Plan_validateIgnoreAll(t *testing.T) {
+	m := testModuleInline(t, map[string]string{
+		"main.tf": `
+resource "test_instance" "a" {
+  lifecycle {
+    ignore_changes = all
+  }
+}
+`,
+	})
+
+	p := testProvider("test")
+	p.GetSchemaReturn = &ProviderSchema{
+		ResourceTypes: map[string]*configschema.Block{
+			"test_instance": {
+				Attributes: map[string]*configschema.Attribute{
+					"id":   {Type: cty.String, Computed: true},
+					"data": {Type: cty.String, Optional: true},
+				},
+			},
+		},
+	}
+	p.PlanResourceChangeFn = testDiffFn
+	p.ValidateResourceTypeConfigFn = func(req providers.ValidateResourceTypeConfigRequest) providers.ValidateResourceTypeConfigResponse {
+		var diags tfdiags.Diagnostics
+		if req.TypeName == "test_instance" {
+			if !req.Config.GetAttr("id").IsNull() {
+				diags = diags.Append(errors.New("id cannot be set in config"))
+			}
+		}
+		return providers.ValidateResourceTypeConfigResponse{
+			Diagnostics: diags,
+		}
+	}
+
+	state := states.NewState()
+	root := state.EnsureModule(addrs.RootModuleInstance)
+	root.SetResourceInstanceCurrent(
+		mustResourceInstanceAddr("test_instance.a").Resource,
+		&states.ResourceInstanceObjectSrc{
+			Status:       states.ObjectReady,
+			AttrsJSON:    []byte(`{"id":"a","data":"foo"}`),
+			Dependencies: []addrs.ConfigResource{},
+		},
+		mustProviderConfig(`provider["registry.terraform.io/hashicorp/test"]`),
+	)
+
+	ctx := testContext2(t, &ContextOpts{
+		Config: m,
+		Providers: map[addrs.Provider]providers.Factory{
+			addrs.NewDefaultProvider("test"): testProviderFuncFixed(p),
+		},
+		State: state,
+	})
+	_, diags := ctx.Plan()
+	if diags.HasErrors() {
+		t.Fatal(diags.Err())
+	}
+}

terraform/eval_diff.go

@@ -212,6 +212,24 @@ func (n *EvalDiff) Eval(ctx EvalContext) tfdiags.Diagnostics {
 	unmarkedConfigVal, unmarkedPaths := origConfigVal.UnmarkDeepWithPaths()
 	unmarkedPriorVal, priorPaths := priorVal.UnmarkDeepWithPaths()
 
+	log.Printf("[TRACE] Re-validating config for %q", n.Addr.Absolute(ctx.Path()))
+	// Allow the provider to validate the final set of values.
+	// The config was statically validated early on, but there may have been
+	// unknown values 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.
+	validateResp := provider.ValidateResourceTypeConfig(
+		providers.ValidateResourceTypeConfigRequest{
+			TypeName: n.Addr.Resource.Type,
+			Config:   unmarkedConfigVal,
+		},
+	)
+	if validateResp.Diagnostics.HasErrors() {
+		diags = diags.Append(validateResp.Diagnostics.InConfigBody(config.Config))
+		return diags
+	}
+
 	// ignore_changes is meant to only apply to the configuration, so it must
 	// be applied before we generate a plan. This ensures the config used for
 	// the proposed value, the proposed value itself, and the config presented
@@ -235,21 +253,6 @@ func (n *EvalDiff) Eval(ctx EvalContext) tfdiags.Diagnostics {
 		}
 	}
 
-	log.Printf("[TRACE] Re-validating config for %q", n.Addr.Absolute(ctx.Path()))
-	// Allow the provider to validate the final set of values.
-	// The config was statically validated early on, but there may have been
-	// unknown values which the provider could not validate at the time.
-	validateResp := provider.ValidateResourceTypeConfig(
-		providers.ValidateResourceTypeConfigRequest{
-			TypeName: n.Addr.Resource.Type,
-			Config:   configValIgnored,
-		},
-	)
-	if validateResp.Diagnostics.HasErrors() {
-		diags = diags.Append(validateResp.Diagnostics.InConfigBody(config.Config))
-		return diags
-	}
-
 	resp := provider.PlanResourceChange(providers.PlanResourceChangeRequest{
 		TypeName:         n.Addr.Resource.Type,
 		Config:           configValIgnored,
@@ -322,6 +325,23 @@ func (n *EvalDiff) Eval(ctx EvalContext) tfdiags.Diagnostics {
 		}
 	}
 
+	if resp.LegacyTypeSystem {
+		// Because we allow legacy providers to depart from the contract and
+		// return changes to non-computed values, the plan response may have
+		// altered values that were already suppressed with ignore_changes.
+		// A prime example of this is where providers attempt to obfuscate
+		// config data by turning the config value into a hash and storing the
+		// hash value in the state. There are enough cases of this in existing
+		// providers that we must accommodate the behavior for now, so for
+		// ignore_changes to work at all on these values, we will revert the
+		// ignored values once more.
+		plannedNewVal, ignoreChangeDiags = n.processIgnoreChanges(unmarkedPriorVal, plannedNewVal)
+		diags = diags.Append(ignoreChangeDiags)
+		if ignoreChangeDiags.HasErrors() {
+			return diags
+		}
+	}
+
 	// Add the marks back to the planned new value -- this must happen after ignore changes
 	// have been processed
 	unmarkedPlannedNewVal := plannedNewVal
@@ -664,47 +684,58 @@ func processIgnoreChangesIndividual(prior, config cty.Value, ignoreChanges []hcl
 	}
 
 	ret, _ := cty.Transform(config, func(path cty.Path, v cty.Value) (cty.Value, error) {
+		// Easy path for when we are only matching the entire value. The only
+		// values we break up for inspection are maps.
+		if !v.Type().IsMapType() {
 			for _, ignored := range ignoredValues {
-			if !path.Equals(ignored.path) {
+				if path.Equals(ignored.path) {
-				return v, nil
-			}
-
-			// no index, so we can return the entire value
-			if ignored.key.IsNull() {
 					return ignored.value, nil
 				}
+			}
+			return v, nil
+		}
+		// We now know this must be a map, so we need to accumulate the values
+		// key-by-key.
 
-			// we have an index key, so make sure we have a map
+		if !v.IsNull() && !v.IsKnown() {
-			if !v.Type().IsMapType() {
+			// since v is not known, we cannot ignore individual keys
-				// we'll let other validation catch any type mismatch
 			return v, nil
 		}
 
-			// Now we know we are ignoring a specific index of this map, so get
+		// The configMap is the current configuration value, which we will
-			// the config map and modify, add, or remove the desired key.
+		// mutate based on the ignored paths and the prior map value.
 		var configMap map[string]cty.Value
-			var priorMap map[string]cty.Value
+		switch {
-
+		case v.IsNull() || v.LengthInt() == 0:
-			if !v.IsNull() {
+			configMap = map[string]cty.Value{}
-				if !v.IsKnown() {
+		default:
-					// if the entire map is not known, we can't ignore any
-					// specific keys yet.
-					continue
-				}
 			configMap = v.AsValueMap()
 		}
-			if configMap == nil {
+
-				configMap = map[string]cty.Value{}
+		for _, ignored := range ignoredValues {
+			if !path.Equals(ignored.path) {
+				continue
 			}
 
-			// We also need to create a prior map, so we can check for
+			if ignored.key.IsNull() {
-			// existence while getting the value. Value.Index will always
+				// The map address is confirmed to match at this point,
-			// return null.
+				// so if there is no key, we want the entire map and can
-			if !ignored.value.IsNull() {
+				// stop accumulating values.
-				priorMap = ignored.value.AsValueMap()
+				return ignored.value, nil
 			}
-			if priorMap == nil {
+			// Now we know we are ignoring a specific index of this map, so get
+			// the config map and modify, add, or remove the desired key.
+
+			// We also need to create a prior map, so we can check for
+			// existence while getting the value, because Value.Index will
+			// return null for a key with a null value and for a non-existent
+			// key.
+			var priorMap map[string]cty.Value
+			switch {
+			case ignored.value.IsNull() || ignored.value.LengthInt() == 0:
 				priorMap = map[string]cty.Value{}
+			default:
+				priorMap = ignored.value.AsValueMap()
 			}
 
 			key := ignored.key.AsString()
@@ -718,14 +749,13 @@ func processIgnoreChangesIndividual(prior, config cty.Value, ignoreChanges []hcl
 			default:
 				configMap[key] = priorElem
 			}
+		}
 
 		if len(configMap) == 0 {
 			return cty.MapValEmpty(v.Type().ElementType()), nil
 		}
 
 		return cty.MapVal(configMap), nil
-		}
-		return v, nil
 	})
 	return ret, nil
 }

terraform/eval_diff_test.go

@@ -136,6 +136,53 @@ func TestProcessIgnoreChangesIndividual(t *testing.T) {
 				"b": cty.StringVal("b value"),
 			}),
 		},
+		"map_index_multiple_keys": {
+			cty.ObjectVal(map[string]cty.Value{
+				"a": cty.MapVal(map[string]cty.Value{
+					"a0": cty.StringVal("a0 value"),
+					"a1": cty.StringVal("a1 value"),
+					"a2": cty.StringVal("a2 value"),
+					"a3": cty.StringVal("a3 value"),
+				}),
+				"b": cty.StringVal("b value"),
+			}),
+			cty.ObjectVal(map[string]cty.Value{
+				"a": cty.NullVal(cty.Map(cty.String)),
+				"b": cty.StringVal("new b value"),
+			}),
+			[]string{`a["a1"]`, `a["a2"]`, `a["a3"]`, `b`},
+			cty.ObjectVal(map[string]cty.Value{
+				"a": cty.MapVal(map[string]cty.Value{
+					"a1": cty.StringVal("a1 value"),
+					"a2": cty.StringVal("a2 value"),
+					"a3": cty.StringVal("a3 value"),
+				}),
+				"b": cty.StringVal("b value"),
+			}),
+		},
+		"map_index_redundant": {
+			cty.ObjectVal(map[string]cty.Value{
+				"a": cty.MapVal(map[string]cty.Value{
+					"a0": cty.StringVal("a0 value"),
+					"a1": cty.StringVal("a1 value"),
+					"a2": cty.StringVal("a2 value"),
+				}),
+				"b": cty.StringVal("b value"),
+			}),
+			cty.ObjectVal(map[string]cty.Value{
+				"a": cty.NullVal(cty.Map(cty.String)),
+				"b": cty.StringVal("new b value"),
+			}),
+			[]string{`a["a1"]`, `a`, `b`},
+			cty.ObjectVal(map[string]cty.Value{
+				"a": cty.MapVal(map[string]cty.Value{
+					"a0": cty.StringVal("a0 value"),
+					"a1": cty.StringVal("a1 value"),
+					"a2": cty.StringVal("a2 value"),
+				}),
+				"b": cty.StringVal("b value"),
+			}),
+		},
 		"missing_map_index": {
 			cty.ObjectVal(map[string]cty.Value{
 				"a": cty.MapVal(map[string]cty.Value{