From aa5c8add2edb659943105769c7317b7aebe2b113 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 3 Dec 2020 16:03:49 -0500 Subject: [PATCH 1/3] fix bug in ignore_changes Transform The cty.Transform for ignore_changes could return early when building a map that had multiple ignored keys. Refactor the function to try and separate the fast-path a little better, and hopefully make it easier to follow. --- terraform/eval_diff.go | 82 +++++++++++++++++++++---------------- terraform/eval_diff_test.go | 47 +++++++++++++++++++++ 2 files changed, 93 insertions(+), 36 deletions(-) diff --git a/terraform/eval_diff.go b/terraform/eval_diff.go index 0e49346d3..7287332cb 100644 --- a/terraform/eval_diff.go +++ b/terraform/eval_diff.go @@ -664,47 +664,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) { + 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. + + if !v.IsNull() && !v.IsKnown() { + // since v is not known, we cannot ignore individual keys + return v, nil + } + + // 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 + switch { + case v.IsNull() || v.LengthInt() == 0: + configMap = map[string]cty.Value{} + default: + configMap = v.AsValueMap() + } + for _, ignored := range ignoredValues { if !path.Equals(ignored.path) { - return v, nil + continue } - // no index, so we can return the entire value if ignored.key.IsNull() { + // The map address is confirmed to match at this point, + // so if there is no key, we want the entire map and can + // stop accumulating values. return ignored.value, nil } - - // we have an index key, so make sure we have a map - if !v.Type().IsMapType() { - // 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 config map and modify, add, or remove the desired key. - var configMap map[string]cty.Value - var priorMap map[string]cty.Value - - if !v.IsNull() { - if !v.IsKnown() { - // 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{} - } // We also need to create a prior map, so we can check for - // existence while getting the value. Value.Index will always - // return null. - if !ignored.value.IsNull() { - priorMap = ignored.value.AsValueMap() - } - if priorMap == nil { + // 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 +729,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 + + if len(configMap) == 0 { + return cty.MapValEmpty(v.Type().ElementType()), nil + } + + return cty.MapVal(configMap), nil }) return ret, nil } diff --git a/terraform/eval_diff_test.go b/terraform/eval_diff_test.go index 8f2a100d1..491b18c72 100644 --- a/terraform/eval_diff_test.go +++ b/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{ From 02e7efab9ed105164e26b6a74a1bcd21e5c3c380 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 3 Dec 2020 17:42:03 -0500 Subject: [PATCH 2/3] re-apply ignore_changes after plan for legacy 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 on the planned state. --- terraform/context_plan_test.go | 66 ++++++++++++++++++++++++++++++++++ terraform/eval_diff.go | 17 +++++++++ 2 files changed, 83 insertions(+) diff --git a/terraform/context_plan_test.go b/terraform/context_plan_test.go index b7d05b418..1026e0f0b 100644 --- a/terraform/context_plan_test.go +++ b/terraform/context_plan_test.go @@ -6473,3 +6473,69 @@ 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) + } + } +} diff --git a/terraform/eval_diff.go b/terraform/eval_diff.go index 7287332cb..8ccb3b586 100644 --- a/terraform/eval_diff.go +++ b/terraform/eval_diff.go @@ -322,6 +322,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 From 4f4e8c17e0a1ec4133863ffed36636d1ea6c204b Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 3 Dec 2020 17:58:46 -0500 Subject: [PATCH 3/3] validate the configuration before ignore_changes The ignore_changes option `all` can cause computed attributes to show up in the validation configuration, which will be rejected by the provider SDK. Validate the config before applying the ignore_changes options. In the future it we should probably have a way for processIgnoreChanges to skip computed values based on the schema. Since we also want a way to more easily query the schema for "computed-ness" to validate the ignore_changes arguments for computed values, we can fix these at the same time with a future change to configschema. This will most likely require some sort of method to retrieve info from the configschema.Block via cty.Path, which we cannot easily do right now. --- terraform/context_plan_test.go | 60 ++++++++++++++++++++++++++++++++++ terraform/eval_diff.go | 33 ++++++++++--------- 2 files changed, 78 insertions(+), 15 deletions(-) diff --git a/terraform/context_plan_test.go b/terraform/context_plan_test.go index 1026e0f0b..602c227f1 100644 --- a/terraform/context_plan_test.go +++ b/terraform/context_plan_test.go @@ -6539,3 +6539,63 @@ resource "test_instance" "a" { } } } + +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()) + } +} diff --git a/terraform/eval_diff.go b/terraform/eval_diff.go index 8ccb3b586..bf2c86b31 100644 --- a/terraform/eval_diff.go +++ b/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,