diff --git a/internal/command/testdata/show-json-sensitive/output.json b/internal/command/testdata/show-json-sensitive/output.json index 156b12f3e3..db98e521b6 100644 --- a/internal/command/testdata/show-json-sensitive/output.json +++ b/internal/command/testdata/show-json-sensitive/output.json @@ -28,7 +28,8 @@ "password": "secret" }, "sensitive_values": { - "ami": true + "ami": true, + "password": true } }, { @@ -44,7 +45,8 @@ "password": "secret" }, "sensitive_values": { - "ami": true + "ami": true, + "password": true } }, { @@ -60,7 +62,8 @@ "password": "secret" }, "sensitive_values": { - "ami": true + "ami": true, + "password": true } } ] diff --git a/internal/command/testdata/show-json/conditions/for-refresh.tfstate b/internal/command/testdata/show-json/conditions/for-refresh.tfstate index 1b0b10196f..ec436cef0b 100644 --- a/internal/command/testdata/show-json/conditions/for-refresh.tfstate +++ b/internal/command/testdata/show-json/conditions/for-refresh.tfstate @@ -1,39 +1,55 @@ { - "version": 4, - "terraform_version": "1.2.0-dev", - "serial": 1, - "lineage": "no", - "outputs": {}, - "resources": [ + "version": 4, + "terraform_version": "1.2.0-dev", + "serial": 1, + "lineage": "no", + "outputs": {}, + "resources": [ + { + "mode": "managed", + "type": "test_instance", + "name": "foo", + "provider": "provider[\"registry.terraform.io/hashicorp/test\"]", + "instances": [ { - "mode": "managed", - "type": "test_instance", - "name": "foo", - "provider": "provider[\"registry.terraform.io/hashicorp/test\"]", - "instances": [ - { - "schema_version": 0, - "attributes": { - "ami": "ami-test", - "id": "placeholder" - } - } + "schema_version": 0, + "attributes": { + "ami": "ami-test", + "id": "placeholder" + }, + "sensitive_attributes": [ + [ + { + "type": "get_attr", + "value": "password" + } ] - }, + ] + } + ] + }, + { + "mode": "managed", + "type": "test_instance", + "name": "bar", + "provider": "provider[\"registry.terraform.io/hashicorp/test\"]", + "instances": [ { - "mode": "managed", - "type": "test_instance", - "name": "bar", - "provider": "provider[\"registry.terraform.io/hashicorp/test\"]", - "instances": [ - { - "schema_version": 0, - "attributes": { - "ami": "ami-test", - "id": "placeheld" - } - } + "schema_version": 0, + "attributes": { + "ami": "ami-test", + "id": "placeheld" + }, + "sensitive_attributes": [ + [ + { + "type": "get_attr", + "value": "password" + } ] + ] } - ] + ] + } + ] } diff --git a/internal/command/testdata/show-json/conditions/output-refresh-only.json b/internal/command/testdata/show-json/conditions/output-refresh-only.json index 7b1845f1d7..75452839f3 100644 --- a/internal/command/testdata/show-json/conditions/output-refresh-only.json +++ b/internal/command/testdata/show-json/conditions/output-refresh-only.json @@ -1,6 +1,6 @@ { - "format_version": "1.1", - "terraform_version": "1.2.0-dev", + "format_version": "1.2", + "terraform_version": "1.8.0-dev", "variables": { "ami": { "value": "bad-ami" @@ -33,13 +33,13 @@ }, "prior_state": { "format_version": "1.0", - "terraform_version": "1.1.0", + "terraform_version": "1.8.0", "values": { "outputs": { "foo_id": { "sensitive": false, - "type": "string", - "value": "placeholder" + "value": "placeholder", + "type": "string" } }, "root_module": { @@ -146,26 +146,70 @@ ] } ], - "condition_results": [ + "checks": [ { - "address": "output.foo_id", - "condition_type": "OutputPrecondition", - "result": true, - "unknown": false + "address": { + "kind": "output_value", + "name": "foo_id", + "to_display": "output.foo_id" + }, + "status": "pass", + "instances": [ + { + "address": { + "to_display": "output.foo_id" + }, + "status": "pass" + } + ] }, { - "address": "test_instance.bar", - "condition_type": "ResourcePostcondition", - "result": false, - "unknown": false, - "error_message": "Resource ID is unacceptably short (9 characters)." + "address": { + "kind": "resource", + "mode": "managed", + "name": "bar", + "to_display": "test_instance.bar", + "type": "test_instance" + }, + "status": "fail", + "instances": [ + { + "address": { + "to_display": "test_instance.bar" + }, + "status": "fail", + "problems": [ + { + "message": "Resource ID is unacceptably short (9 characters)." + } + ] + } + ] }, { - "address": "test_instance.foo", - "condition_type": "ResourcePrecondition", - "result": false, - "unknown": false, - "error_message": "Invalid AMI ID: must start with \"ami-\"." + "address": { + "kind": "resource", + "mode": "managed", + "name": "foo", + "to_display": "test_instance.foo", + "type": "test_instance" + }, + "status": "fail", + "instances": [ + { + "address": { + "to_display": "test_instance.foo" + }, + "status": "fail", + "problems": [ + { + "message": "Invalid AMI ID: must start with \"ami-\"." + } + ] + } + ] } - ] + ], + "timestamp": "2024-01-24T18:33:05Z", + "errored": false } diff --git a/internal/terraform/context_apply_test.go b/internal/terraform/context_apply_test.go index 81d61cebd7..a15a4b5655 100644 --- a/internal/terraform/context_apply_test.go +++ b/internal/terraform/context_apply_test.go @@ -11971,15 +11971,22 @@ resource "test_resource" "foo" { } verifySensitiveValue := func(pvms []cty.PathValueMarks) { - if len(pvms) != 1 { - t.Fatalf("expected 1 sensitive path, got %d", len(pvms)) + if len(pvms) != 2 { + t.Fatalf("expected 2 sensitive paths, got %d", len(pvms)) } - pvm := pvms[0] - if gotPath, wantPath := pvm.Path, cty.GetAttrPath("value"); !gotPath.Equals(wantPath) { - t.Errorf("wrong path\n got: %#v\nwant: %#v", gotPath, wantPath) - } - if gotMarks, wantMarks := pvm.Marks, cty.NewValueMarks(marks.Sensitive); !gotMarks.Equal(wantMarks) { - t.Errorf("wrong marks\n got: %#v\nwant: %#v", gotMarks, wantMarks) + + for _, pvm := range pvms { + switch { + case pvm.Path.Equals(cty.GetAttrPath("value")): + case pvm.Path.Equals(cty.GetAttrPath("sensitive_value")): + default: + t.Errorf("unexpected path mark: %#v", pvm) + return + } + + if want := cty.NewValueMarks(marks.Sensitive); !pvm.Marks.Equal(want) { + t.Errorf("wrong marks\n got: %#v\nwant: %#v", pvm.Marks, want) + } } } @@ -12034,15 +12041,19 @@ resource "test_resource" "baz" { } verifySensitiveValue := func(pvms []cty.PathValueMarks) { - if len(pvms) != 1 { - t.Fatalf("expected 1 sensitive path, got %d", len(pvms)) - } - pvm := pvms[0] - if gotPath, wantPath := pvm.Path, cty.GetAttrPath("value"); !gotPath.Equals(wantPath) { - t.Errorf("wrong path\n got: %#v\nwant: %#v", gotPath, wantPath) - } - if gotMarks, wantMarks := pvm.Marks, cty.NewValueMarks(marks.Sensitive); !gotMarks.Equal(wantMarks) { - t.Errorf("wrong marks\n got: %#v\nwant: %#v", gotMarks, wantMarks) + for _, pvm := range pvms { + switch { + case pvm.Path.Equals(cty.GetAttrPath("value")): + case pvm.Path.Equals(cty.GetAttrPath("sensitive_value")): + case pvm.Path.Equals(cty.GetAttrPath("nesting_single").GetAttr("sensitive_value")): + default: + t.Errorf("unexpected path mark: %#v", pvm) + return + } + + if want := cty.NewValueMarks(marks.Sensitive); !pvm.Marks.Equal(want) { + t.Errorf("wrong marks\n got: %#v\nwant: %#v", pvm.Marks, want) + } } } @@ -12120,17 +12131,22 @@ resource "test_resource" "foo" { fooState := state.ResourceInstance(addr) - if len(fooState.Current.AttrSensitivePaths) != 1 { - t.Fatalf("wrong number of sensitive paths, expected 1, got, %v", len(fooState.Current.AttrSensitivePaths)) - } - got := fooState.Current.AttrSensitivePaths[0] - want := cty.PathValueMarks{ - Path: cty.GetAttrPath("value"), - Marks: cty.NewValueMarks(marks.Sensitive), + if len(fooState.Current.AttrSensitivePaths) != 2 { + t.Fatalf("wrong number of sensitive paths, expected 2, got, %v", len(fooState.Current.AttrSensitivePaths)) } - if !got.Equal(want) { - t.Fatalf("wrong value marks; got:\n%#v\n\nwant:\n%#v\n", got, want) + for _, pvm := range fooState.Current.AttrSensitivePaths { + switch { + case pvm.Path.Equals(cty.GetAttrPath("value")): + case pvm.Path.Equals(cty.GetAttrPath("sensitive_value")): + default: + t.Errorf("unexpected path mark: %#v", pvm) + return + } + + if want := cty.NewValueMarks(marks.Sensitive); !pvm.Marks.Equal(want) { + t.Errorf("wrong marks\n got: %#v\nwant: %#v", pvm.Marks, want) + } } m2 := testModuleInline(t, map[string]string{ @@ -12145,33 +12161,22 @@ resource "test_resource" "foo" { }`, }) - ctx2 := testContext2(t, &ContextOpts{ + ctx = testContext2(t, &ContextOpts{ Providers: map[addrs.Provider]providers.Factory{ addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), }, }) - // NOTE: Prior to our refactoring to make the state an explicit argument - // of Plan, as opposed to hidden state inside Context, this test was - // calling ctx.Apply instead of ctx2.Apply and thus using the previous - // plan instead of this new plan. "Fixing" it to use the new plan seems - // to break the test, so we've preserved that oddity here by saving the - // old plan as oldPlan and essentially discarding the new plan entirely, - // but this seems rather suspicious and we should ideally figure out what - // this test was originally intending to do and make it do that. - oldPlan := plan - _, diags = ctx2.Plan(m2, state, SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables))) + plan, diags = ctx.Plan(m2, state, SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables))) assertNoErrors(t, diags) - stateWithoutSensitive, diags := ctx.Apply(oldPlan, m, nil) + stateWithoutSensitive, diags := ctx.Apply(plan, m2, nil) assertNoErrors(t, diags) fooState2 := stateWithoutSensitive.ResourceInstance(addr) - if len(fooState2.Current.AttrSensitivePaths) > 0 { - t.Fatalf( - "wrong number of sensitive paths, expected 0, got, %v\n%s", + if len(fooState2.Current.AttrSensitivePaths) != 1 { + t.Fatalf("wrong number of sensitive paths, expected 1, got, %v\n%#v\n", len(fooState2.Current.AttrSensitivePaths), - spew.Sdump(fooState2.Current.AttrSensitivePaths), - ) + fooState2.Current.AttrSensitivePaths) } } diff --git a/internal/terraform/context_plan2_test.go b/internal/terraform/context_plan2_test.go index 81e6094242..1109e9c6d8 100644 --- a/internal/terraform/context_plan2_test.go +++ b/internal/terraform/context_plan2_test.go @@ -2932,7 +2932,7 @@ output "output" { } if res.Addr.Resource.Resource.Mode == addrs.DataResourceMode && res.Action != plans.NoOp { - t.Errorf("unexpected %s plan for %s", res.Action, res.Addr) + t.Errorf("unexpected %s/%s plan for %s", res.Action, res.ActionReason, res.Addr) } } } diff --git a/internal/terraform/evaluate.go b/internal/terraform/evaluate.go index 45f75dd4c6..f7bd5e9c81 100644 --- a/internal/terraform/evaluate.go +++ b/internal/terraform/evaluate.go @@ -686,13 +686,9 @@ func (d *evaluationStateData) GetResource(addr addrs.Resource, rng tfdiags.Sourc continue } - // If our provider schema contains sensitive values, mark those as sensitive - afterMarks := change.AfterValMarks - if schema.ContainsSensitive() { - afterMarks = append(afterMarks, schema.ValueMarks(val, nil)...) - } - - instances[key] = val.MarkWithPaths(afterMarks) + // Unlike decoding state, decoding a change does not automatically + // mark values. + instances[key] = val.MarkWithPaths(change.AfterValMarks) continue } @@ -711,16 +707,6 @@ func (d *evaluationStateData) GetResource(addr addrs.Resource, rng tfdiags.Sourc val := ios.Value - // If our schema contains sensitive values, mark those as sensitive. - // Since decoding the instance object can also apply sensitivity marks, - // we must remove and combine those before remarking to avoid a double- - // mark error. - if schema.ContainsSensitive() { - var marks []cty.PathValueMarks - val, marks = val.UnmarkDeepWithPaths() - marks = append(marks, schema.ValueMarks(val, nil)...) - val = val.MarkWithPaths(marks) - } instances[key] = val } diff --git a/internal/terraform/evaluate_test.go b/internal/terraform/evaluate_test.go index cfd518f4b3..ce7446d153 100644 --- a/internal/terraform/evaluate_test.go +++ b/internal/terraform/evaluate_test.go @@ -236,6 +236,32 @@ func TestEvaluatorGetResource(t *testing.T) { &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{"id":"foo", "nesting_list": [{"sensitive_value":"abc"}], "nesting_map": {"foo":{"foo":"x"}}, "nesting_set": [{"baz":"abc"}], "nesting_single": {"boop":"abc"}, "nesting_nesting": {"nesting_list":[{"sensitive_value":"abc"}]}, "value":"hello"}`), + AttrSensitivePaths: []cty.PathValueMarks{ + { + Path: cty.GetAttrPath("nesting_list").IndexInt(0).GetAttr("sensitive_value"), + Marks: cty.NewValueMarks(marks.Sensitive), + }, + { + Path: cty.GetAttrPath("nesting_map").IndexString("foo").GetAttr("foo"), + Marks: cty.NewValueMarks(marks.Sensitive), + }, + { + Path: cty.GetAttrPath("nesting_nesting").GetAttr("nesting_list").IndexInt(0).GetAttr("sensitive_value"), + Marks: cty.NewValueMarks(marks.Sensitive), + }, + { + Path: cty.GetAttrPath("nesting_set"), + Marks: cty.NewValueMarks(marks.Sensitive), + }, + { + Path: cty.GetAttrPath("nesting_single").GetAttr("boop"), + Marks: cty.NewValueMarks(marks.Sensitive), + }, + { + Path: cty.GetAttrPath("value"), + Marks: cty.NewValueMarks(marks.Sensitive), + }, + }, }, addrs.AbsProviderConfig{ Provider: addrs.NewDefaultProvider("test"), @@ -434,10 +460,10 @@ func TestEvaluatorGetResource_changes(t *testing.T) { After: cty.ObjectVal(map[string]cty.Value{ "id": cty.StringVal("foo"), "to_mark_val": cty.StringVal("pizza").Mark(marks.Sensitive), - "sensitive_value": cty.StringVal("abc"), + "sensitive_value": cty.StringVal("abc").Mark(marks.Sensitive), "sensitive_collection": cty.MapVal(map[string]cty.Value{ "boop": cty.StringVal("beep"), - }), + }).Mark(marks.Sensitive), }), }, } diff --git a/internal/terraform/marks.go b/internal/terraform/marks.go index 7b0d168123..e57c4745fa 100644 --- a/internal/terraform/marks.go +++ b/internal/terraform/marks.go @@ -56,3 +56,26 @@ func marksEqual(a, b []cty.PathValueMarks) bool { return true } + +// Remove duplicate PathValueMarks from the slice. +// When adding marks from a resource schema to a value, most of the time there +// will be duplicates from a prior value already in the list of marks. While +// MarkwithPaths will accept duplicates, we need to be able to easily compare +// the PathValueMarks within this package too. +func dedupePathValueMarks(m []cty.PathValueMarks) []cty.PathValueMarks { + var res []cty.PathValueMarks + // we'll use a GoString format key to differentiate PathValueMarks, since + // the Path portion is not automagically comparable. + seen := make(map[string]bool) + + for _, pvm := range m { + key := fmt.Sprintf("%#v", pvm) + if _, ok := seen[key]; ok { + continue + } + seen[key] = true + res = append(res, pvm) + } + + return res +} diff --git a/internal/terraform/node_resource_abstract_instance.go b/internal/terraform/node_resource_abstract_instance.go index 960da8599e..696645a91b 100644 --- a/internal/terraform/node_resource_abstract_instance.go +++ b/internal/terraform/node_resource_abstract_instance.go @@ -620,9 +620,9 @@ func (n *NodeAbstractResourceInstance) refresh(ctx EvalContext, deposedKey state priorVal := state.Value // Unmarked before sending to provider - var priorPaths []cty.PathValueMarks + var priorMarks []cty.PathValueMarks if priorVal.ContainsMarked() { - priorVal, priorPaths = priorVal.UnmarkDeepWithPaths() + priorVal, priorMarks = priorVal.UnmarkDeepWithPaths() } var resp providers.ReadResourceResponse @@ -714,9 +714,14 @@ func (n *NodeAbstractResourceInstance) refresh(ctx EvalContext, deposedKey state return ret, diags } - // Mark the value if necessary - if len(priorPaths) > 0 { - ret.Value = ret.Value.MarkWithPaths(priorPaths) + // Mark the value with any prior marks from the state, and the marks from + // the schema. This ensures we capture any marks from the last + // configuration, as well as any marks from the schema which were not in + // the prior state. New marks may appear when the prior state was from an + // import operation, or if the provider added new marks to the schema. + if marks := dedupePathValueMarks(append(priorMarks, schema.ValueMarks(ret.Value, nil)...)); len(marks) > 0 { + //if marks := priorMarks; len(marks) > 0 { + ret.Value = ret.Value.MarkWithPaths(marks) } return ret, diags @@ -992,9 +997,13 @@ func (n *NodeAbstractResourceInstance) plan( } } - // Add the marks back to the planned new value -- this must happen after ignore changes - // have been processed + // Add the marks back to the planned new value -- this must happen after + // ignore changes have been processed. We add in the schema marks as well, + // to ensure that provider defined private attributes are marked correctly + // here. unmarkedPlannedNewVal := plannedNewVal + unmarkedPaths = dedupePathValueMarks(append(unmarkedPaths, schema.ValueMarks(plannedNewVal, nil)...)) + if len(unmarkedPaths) > 0 { plannedNewVal = plannedNewVal.MarkWithPaths(unmarkedPaths) }