From 2c352ef18293a71dfa1eadd2421d1259e0634e26 Mon Sep 17 00:00:00 2001 From: Pam Selle <204372+pselle@users.noreply.github.com> Date: Wed, 7 Oct 2020 11:00:51 -0400 Subject: [PATCH] Apply should not communicate w provider if only sensitivity changes If sensitivity changes, we have an update plan, but should avoid communicating with the provider on the apply, as the values are equal (and otherwise a NoOp plan) --- terraform/context_apply_test.go | 102 ++++++++++++++++++++++++++++++++ terraform/eval_apply.go | 23 ++++--- terraform/eval_diff.go | 13 ++-- 3 files changed, 119 insertions(+), 19 deletions(-) diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index 640a2041b..2b96af22d 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -12088,3 +12088,105 @@ resource "test_resource" "foo" { fooState := state.ResourceInstance(addr) verifySensitiveValue(fooState.Current.AttrSensitivePaths) } + +func TestContext2Apply_variableSensitivityChange(t *testing.T) { + m := testModuleInline(t, map[string]string{ + "main.tf": ` +terraform { + experiments = [sensitive_variables] +} + +variable "sensitive_var" { + default = "hello" + sensitive = true +} + +resource "test_resource" "foo" { + value = var.sensitive_var +}`, + }) + + p := testProvider("test") + p.ApplyFn = testApplyFn + p.DiffFn = testDiffFn + + ctx := testContext2(t, &ContextOpts{ + Config: m, + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), + }, + State: states.BuildState(func(s *states.SyncState) { + s.SetResourceInstanceCurrent( + addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "test_resource", + Name: "foo", + }.Instance(addrs.NoKey).Absolute(addrs.RootModuleInstance), + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"foo", "value":"hello"}`), + // No AttrSensitivePaths present + }, + addrs.AbsProviderConfig{ + Provider: addrs.NewDefaultProvider("test"), + Module: addrs.RootModule, + }, + ) + }), + }) + + _, diags := ctx.Plan() + assertNoErrors(t, diags) + + addr := mustResourceInstanceAddr("test_resource.foo") + + state, diags := ctx.Apply() + assertNoErrors(t, diags) + + fooState := state.ResourceInstance(addr) + + got := fooState.Current.AttrSensitivePaths[0] + want := cty.PathValueMarks{ + Path: cty.GetAttrPath("value"), + Marks: cty.NewValueMarks("sensitive"), + } + + if !got.Equal(want) { + t.Fatalf("wrong value marks; got:\n%#v\n\nwant:\n%#v\n", got, want) + } + + m2 := testModuleInline(t, map[string]string{ + "main.tf": ` +terraform { + experiments = [sensitive_variables] +} + +variable "sensitive_var" { + default = "hello" + sensitive = false +} + +resource "test_resource" "foo" { + value = var.sensitive_var +}`, + }) + + ctx2 := testContext2(t, &ContextOpts{ + Config: m2, + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), + }, + State: state, + }) + + _, diags = ctx2.Plan() + assertNoErrors(t, diags) + + stateWithoutSensitive, diags := ctx.Apply() + assertNoErrors(t, diags) + + fooState2 := stateWithoutSensitive.ResourceInstance(addr) + if len(fooState2.Current.AttrSensitivePaths) > 0 { + t.Fatalf("wrong number of sensitive paths, expected 0, got, %v", len(fooState2.Current.AttrSensitivePaths)) + } +} diff --git a/terraform/eval_apply.go b/terraform/eval_apply.go index c5b6a4cad..a6f755dcb 100644 --- a/terraform/eval_apply.go +++ b/terraform/eval_apply.go @@ -109,20 +109,17 @@ func (n *EvalApply) Eval(ctx EvalContext) (interface{}, error) { // If our config, Before or After value contain any marked values, // ensure those are stripped out before sending // this to the provider - unmarkedConfigVal := configVal - if configVal.ContainsMarked() { - unmarkedConfigVal, _ = configVal.UnmarkDeep() - } + unmarkedConfigVal, _ := configVal.UnmarkDeep() + unmarkedBefore, beforePaths := change.Before.UnmarkDeepWithPaths() + unmarkedAfter, afterPaths := change.After.UnmarkDeepWithPaths() - unmarkedBefore := change.Before - if change.Before.ContainsMarked() { - unmarkedBefore, _ = change.Before.UnmarkDeep() - } - - unmarkedAfter := change.After - var afterPaths []cty.PathValueMarks - if change.After.ContainsMarked() { - unmarkedAfter, afterPaths = change.After.UnmarkDeepWithPaths() + // If we have an Update action, our before and after values are equal, + // and only differ on their sensitivity, the newVal is the after val + // and we should not communicate with the provider or perform further action. + eqV := unmarkedBefore.Equals(unmarkedAfter) + eq := eqV.IsKnown() && eqV.True() + if change.Action == plans.Update && eq && (len(beforePaths) != len(afterPaths)) { + return nil, diags.ErrWithWarnings() } resp := provider.ApplyResourceChange(providers.ApplyResourceChangeRequest{ diff --git a/terraform/eval_diff.go b/terraform/eval_diff.go index d7e43ce28..aa45563ee 100644 --- a/terraform/eval_diff.go +++ b/terraform/eval_diff.go @@ -216,7 +216,7 @@ func (n *EvalDiff) Eval(ctx EvalContext) (interface{}, error) { // Store the paths for the config val to re-markafter // we've sent things over the wire. unmarkedConfigVal, unmarkedPaths := configValIgnored.UnmarkDeepWithPaths() - unmarkedPriorVal, priorPaths := priorVal.UnmarkDeep() + unmarkedPriorVal, priorPaths := priorVal.UnmarkDeepWithPaths() proposedNewVal := objchange.ProposedNewObject(schema, unmarkedPriorVal, unmarkedConfigVal) @@ -393,11 +393,6 @@ func (n *EvalDiff) Eval(ctx EvalContext) (interface{}, error) { action = plans.Create case eq: action = plans.NoOp - // If we plan to write or delete sensitive paths from state, - // this is an Update action - if len(priorPaths) != len(unmarkedPaths) { - action = plans.Update - } case !reqRep.Empty(): // If there are any "requires replace" paths left _after our filtering // above_ then this is a replace action. @@ -487,6 +482,12 @@ func (n *EvalDiff) Eval(ctx EvalContext) (interface{}, error) { priorVal = priorValTainted } + // If we plan to write or delete sensitive paths from state, + // this is an Update action + if action == plans.NoOp && len(priorPaths) != len(unmarkedPaths) { + action = plans.Update + } + // As a special case, if we have a previous diff (presumably from the plan // phases, whereas we're now in the apply phase) and it was for a replace, // we've already deleted the original object from state by the time we