aherman
/
terraform
mirror of https://github.com/hashicorp/terraform
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Merge pull request #34567 from hashicorp/jbardin/handling-instance-value-marks

Browse Source 
apply schema marks to returned instance values
pull/34575/head
James Bardin 2 years ago committed by GitHub
parent 9658f9df6b 8994e914b7
commit c4a2f74054
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
9 changed files with 238 additions and 126 deletions
Show Stats Download Patch File Download Diff File

9
internal/command/testdata/show-json-sensitive/output.json vendored
Unescape View File

@ -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
}
}
]

80
internal/command/testdata/show-json/conditions/for-refresh.tfstate vendored
Unescape View File

@ -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"
}
]
]
}
]
]
}
]
}

86
internal/command/testdata/show-json/conditions/output-refresh-only.json vendored
Unescape View File

@ -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
}

91
internal/terraform/context_apply_test.go
Unescape View File

@ -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)
}
}

2
internal/terraform/context_plan2_test.go
Unescape View File

@ -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)
}
}
}

20
internal/terraform/evaluate.go
Unescape View File

@ -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
}

30
internal/terraform/evaluate_test.go
Unescape View File

@ -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),
}),
},
}

23
internal/terraform/marks.go
Unescape View File

@ -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
}

23
internal/terraform/node_resource_abstract_instance.go
Unescape View File

@ -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)
}

Write Preview
Loading…
Cancel
Save