From ea077cbd90b3e079ef4df5659a6ac06ecab49d91 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 19 Jul 2021 14:54:48 -0400 Subject: [PATCH 1/4] handle marks within ignore_changes Up until now marks were not considered by `ignore_changes`, that however means changes to sensitivity within a configuration cannot ignored, even though they are planned as changes. Rather than separating the marks and tracking their paths, we can easily update the processIgnoreChanges routine to handle the marked values directly. Moving the `processIgnoreChanges` call also cleans up some of the variable naming, making it more consistent through the body of the function. --- .../node_resource_abstract_instance.go | 83 ++++++++++++------- internal/terraform/reduce_plan_test.go | 2 +- 2 files changed, 56 insertions(+), 29 deletions(-) diff --git a/internal/terraform/node_resource_abstract_instance.go b/internal/terraform/node_resource_abstract_instance.go index c3c28e2c6..d2279f88f 100644 --- a/internal/terraform/node_resource_abstract_instance.go +++ b/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. - // The config was statically validated early on, but there may have been - // unknown values which the provider could not validate at the time. + // 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. + + // 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) { - // When we walk below we will be using cty.Path values for comparison, so - // we'll convert our traversals here so we can compare more easily. - ignoreChangesPath := make([]cty.Path, len(ignoreChanges)) - for i, traversal := range ignoreChanges { +// Convert the hcl.Traversal values we get form the configuration to the +// cty.Path values we need to operate on the cty.Values +func traversalsToPaths(traversals []hcl.Traversal) []cty.Path { + paths := make([]cty.Path, len(traversals)) + 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 !eq.IsKnown() || eq.False() { + if !p.RawEquals(c) { // 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 } diff --git a/internal/terraform/reduce_plan_test.go b/internal/terraform/reduce_plan_test.go index 30d819aad..2d134d2c2 100644 --- a/internal/terraform/reduce_plan_test.go +++ b/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()) } From e574f73f61ba604f9de2761cfdc51852e7818b35 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 19 Jul 2021 15:33:28 -0400 Subject: [PATCH 2/4] test with marked, unknown, planned values --- internal/terraform/context_apply2_test.go | 56 +++++++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/internal/terraform/context_apply2_test.go b/internal/terraform/context_apply2_test.go index 5d90e8f09..e172b4dd2 100644 --- a/internal/terraform/context_apply2_test.go +++ b/internal/terraform/context_apply2_test.go @@ -508,3 +508,59 @@ output "out" { } } } + +func TestContext2Apply_ignoreImpureFunctionChanges(t *testing.T) { + // Ensure we're not trying to double-mark values decoded from state + 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"] ] + } +} + +`, + }) + + 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) + } + } +} From 66a5950acbec653aff0e91cc41c7c84932e73f0b Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 19 Jul 2021 15:33:38 -0400 Subject: [PATCH 3/4] ignored value in this test should not be marked --- internal/terraform/context_plan_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/terraform/context_plan_test.go b/internal/terraform/context_plan_test.go index 4efc2f136..8a5bc9e6f 100644 --- a/internal/terraform/context_plan_test.go +++ b/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) } From 07ee689eff36e193c54374048566a68361878a34 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 20 Jul 2021 16:09:46 -0400 Subject: [PATCH 4/4] update comment and extend test --- internal/terraform/context_apply2_test.go | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/internal/terraform/context_apply2_test.go b/internal/terraform/context_apply2_test.go index e172b4dd2..681595959 100644 --- a/internal/terraform/context_apply2_test.go +++ b/internal/terraform/context_apply2_test.go @@ -510,7 +510,8 @@ output "out" { } func TestContext2Apply_ignoreImpureFunctionChanges(t *testing.T) { - // Ensure we're not trying to double-mark values decoded from state + // The impure function call should not cause a planned change with + // ignore_changes m := testModuleInline(t, map[string]string{ "main.tf": ` variable "pw" { @@ -527,6 +528,15 @@ resource "test_object" "x" { } } +resource "test_object" "y" { + test_map = { + string = "X${bcrypt(var.pw)}" + } + lifecycle { + ignore_changes = [ test_map ] + } +} + `, })