From f4e6431da29e5af77f317d9ac7ed3097004d29b2 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Mon, 11 Feb 2019 17:05:24 -0800 Subject: [PATCH] core: Ensure context tests comply with plan/apply safety checks Prior to Terraform 0.12 there were certain behaviors we expected from providers that were actually just details of the SDK and not part of the enforced contract. For 0.12 we're now codifying some of these behaviors explicitly via safety checks in core, thus ensuring that all future providers will behave in a consistent way that users can rely on. Unfortunately, due to the hand-written nature of the mock provider implementations we use in tests, they have been getting away with some unusual behaviors that don't match our usual expectations, and our safety checks now detect those as incorrect behaviors. To address this, we make the minimal changes to each test to ensure that its mock provider behaves in a consistent way, which requires that values set in config be represented correctly in the plan and ultimately saved in the new state, without any changes along the way. In particular, the common testDiffFn implementation has historically used a number of special hidden attributes to trigger special behaviors, and our new rules require that these special settings propagate properly through the plan and into the state. --- terraform/context_apply_test.go | 188 ++++++++++++------- terraform/context_plan_test.go | 48 +++-- terraform/context_test.go | 47 +++++ terraform/terraform_test.go | 5 + terraform/test-fixtures/apply-idattr/main.tf | 1 + terraform/test-fixtures/plan-diffvar/main.tf | 4 +- 6 files changed, 209 insertions(+), 84 deletions(-) diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index 27c08369b..fc9dbb0a3 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -1878,14 +1878,21 @@ func TestContext2Apply_cancel(t *testing.T) { }, }, nil } - p.DiffFn = func(*InstanceInfo, *InstanceState, *ResourceConfig) (*InstanceDiff, error) { - return &InstanceDiff{ - Attributes: map[string]*ResourceAttrDiff{ - "value": &ResourceAttrDiff{ - New: "2", - }, - }, - }, nil + p.DiffFn = func(info *InstanceInfo, s *InstanceState, rc *ResourceConfig) (*InstanceDiff, error) { + d := &InstanceDiff{ + Attributes: map[string]*ResourceAttrDiff{}, + } + if new, ok := rc.Get("value"); ok { + d.Attributes["value"] = &ResourceAttrDiff{ + New: new.(string), + } + } + if new, ok := rc.Get("foo"); ok { + d.Attributes["foo"] = &ResourceAttrDiff{ + New: new.(string), + } + } + return d, nil } if _, diags := ctx.Plan(); diags.HasErrors() { @@ -1937,6 +1944,9 @@ func TestContext2Apply_cancelBlock(t *testing.T) { "id": &ResourceAttrDiff{ New: "foo", }, + "num": &ResourceAttrDiff{ + New: "2", + }, }, }, nil } @@ -1954,6 +1964,9 @@ func TestContext2Apply_cancelBlock(t *testing.T) { return &InstanceState{ ID: "foo", + Attributes: map[string]string{ + "num": "2", + }, }, nil } @@ -2002,6 +2015,7 @@ func TestContext2Apply_cancelBlock(t *testing.T) { aws_instance.foo: ID = foo provider = provider.aws + num = 2 `) } @@ -2130,10 +2144,6 @@ func TestContext2Apply_compute(t *testing.T) { ), }) - if _, diags := ctx.Plan(); diags.HasErrors() { - t.Fatalf("plan errors: %s", diags.Err()) - } - ctx.variables = InputValues{ "value": &InputValue{ Value: cty.NumberIntVal(1), @@ -2141,6 +2151,10 @@ func TestContext2Apply_compute(t *testing.T) { }, } + if _, diags := ctx.Plan(); diags.HasErrors() { + t.Fatalf("plan errors: %s", diags.Err()) + } + state, diags := ctx.Apply() if diags.HasErrors() { t.Fatalf("unexpected errors: %s", diags.Err()) @@ -3741,25 +3755,35 @@ func TestContext2Apply_multiVarComprehensive(t *testing.T) { p.ApplyFn = testApplyFn - p.DiffFn = func(info *InstanceInfo, s *InstanceState, c *ResourceConfig) (*InstanceDiff, error) { + p.PlanResourceChangeFn = func(req providers.PlanResourceChangeRequest) providers.PlanResourceChangeResponse { + proposed := req.ProposedNewState configsLock.Lock() defer configsLock.Unlock() - key := c.Config["key"].(string) - configs[key] = c + key := proposed.GetAttr("key").AsString() + // This test was originally written using the legacy p.DiffFn interface, + // and so the assertions below expect an old-style ResourceConfig, which + // we'll construct via our shim for now to avoid rewriting all of the + // assertions. + configs[key] = NewResourceConfigShimmed(req.Config, p.GetSchemaReturn.ResourceTypes["test_thing"]) - // Return a minimal diff to make sure this resource gets included in - // the apply graph and thus the final state, but otherwise we're just - // gathering data for assertions. - return &InstanceDiff{ - Attributes: map[string]*ResourceAttrDiff{ - "id": &ResourceAttrDiff{ - NewComputed: true, - }, - "name": &ResourceAttrDiff{ - New: key, - }, - }, - }, nil + retVals := make(map[string]cty.Value) + for it := proposed.ElementIterator(); it.Next(); { + idxVal, val := it.Element() + idx := idxVal.AsString() + + switch idx { + case "id": + retVals[idx] = cty.UnknownVal(cty.String) + case "name": + retVals[idx] = cty.StringVal(key) + default: + retVals[idx] = val + } + } + + return providers.PlanResourceChangeResponse{ + PlannedState: cty.ObjectVal(retVals), + } } p.GetSchemaReturn = &ProviderSchema{ @@ -6149,21 +6173,33 @@ func TestContext2Apply_outputDiffVars(t *testing.T) { result := s.MergeDiff(d) result.ID = "foo" - result.Attributes["foo"] = "bar" return result, nil } - p.DiffFn = func(*InstanceInfo, *InstanceState, *ResourceConfig) (*InstanceDiff, error) { - return &InstanceDiff{ - Attributes: map[string]*ResourceAttrDiff{ - "foo": &ResourceAttrDiff{ - NewComputed: true, - Type: DiffAttrOutput, - }, - "bar": &ResourceAttrDiff{ - New: "baz", - }, - }, - }, nil + p.DiffFn = func(info *InstanceInfo, s *InstanceState, rc *ResourceConfig) (*InstanceDiff, error) { + d := &InstanceDiff{ + Attributes: map[string]*ResourceAttrDiff{}, + } + if new, ok := rc.Get("value"); ok { + d.Attributes["value"] = &ResourceAttrDiff{ + New: new.(string), + } + } + if new, ok := rc.Get("foo"); ok { + d.Attributes["foo"] = &ResourceAttrDiff{ + New: new.(string), + } + } else if rc.IsComputed("foo") { + d.Attributes["foo"] = &ResourceAttrDiff{ + NewComputed: true, + Type: DiffAttrOutput, // This doesn't actually really do anything anymore, but this test originally set it. + } + } + if new, ok := rc.Get("num"); ok { + d.Attributes["num"] = &ResourceAttrDiff{ + New: fmt.Sprintf("%#v", new), + } + } + return d, nil } if _, diags := ctx.Plan(); diags.HasErrors() { @@ -6899,14 +6935,21 @@ func TestContext2Apply_destroyOrphan(t *testing.T) { result.ID = "foo" return result, nil } - p.DiffFn = func(*InstanceInfo, *InstanceState, *ResourceConfig) (*InstanceDiff, error) { - return &InstanceDiff{ - Attributes: map[string]*ResourceAttrDiff{ - "value": &ResourceAttrDiff{ - New: "bar", - }, - }, - }, nil + p.DiffFn = func(info *InstanceInfo, s *InstanceState, rc *ResourceConfig) (*InstanceDiff, error) { + d := &InstanceDiff{ + Attributes: map[string]*ResourceAttrDiff{}, + } + if new, ok := rc.Get("value"); ok { + d.Attributes["value"] = &ResourceAttrDiff{ + New: new.(string), + } + } + if new, ok := rc.Get("foo"); ok { + d.Attributes["foo"] = &ResourceAttrDiff{ + New: new.(string), + } + } + return d, nil } if _, diags := ctx.Plan(); diags.HasErrors() { @@ -7021,14 +7064,21 @@ func TestContext2Apply_error(t *testing.T) { }, }, nil } - p.DiffFn = func(*InstanceInfo, *InstanceState, *ResourceConfig) (*InstanceDiff, error) { - return &InstanceDiff{ - Attributes: map[string]*ResourceAttrDiff{ - "value": &ResourceAttrDiff{ - New: "2", - }, - }, - }, nil + p.DiffFn = func(info *InstanceInfo, s *InstanceState, rc *ResourceConfig) (*InstanceDiff, error) { + d := &InstanceDiff{ + Attributes: map[string]*ResourceAttrDiff{}, + } + if new, ok := rc.Get("value"); ok { + d.Attributes["value"] = &ResourceAttrDiff{ + New: new.(string), + } + } + if new, ok := rc.Get("foo"); ok { + d.Attributes["foo"] = &ResourceAttrDiff{ + New: new.(string), + } + } + return d, nil } if _, diags := ctx.Plan(); diags.HasErrors() { @@ -7090,14 +7140,21 @@ func TestContext2Apply_errorPartial(t *testing.T) { }, }, nil } - p.DiffFn = func(*InstanceInfo, *InstanceState, *ResourceConfig) (*InstanceDiff, error) { - return &InstanceDiff{ - Attributes: map[string]*ResourceAttrDiff{ - "value": &ResourceAttrDiff{ - New: "2", - }, - }, - }, nil + p.DiffFn = func(info *InstanceInfo, s *InstanceState, rc *ResourceConfig) (*InstanceDiff, error) { + d := &InstanceDiff{ + Attributes: map[string]*ResourceAttrDiff{}, + } + if new, ok := rc.Get("value"); ok { + d.Attributes["value"] = &ResourceAttrDiff{ + New: new.(string), + } + } + if new, ok := rc.Get("foo"); ok { + d.Attributes["foo"] = &ResourceAttrDiff{ + New: new.(string), + } + } + return d, nil } if _, diags := ctx.Plan(); diags.HasErrors() { @@ -8907,6 +8964,7 @@ func TestContext2Apply_issue5254(t *testing.T) { template_file.child: ID = foo provider = provider.template + __template_requires_new = true template = Hi type = template_file diff --git a/terraform/context_plan_test.go b/terraform/context_plan_test.go index a58035233..f7c8466ca 100644 --- a/terraform/context_plan_test.go +++ b/terraform/context_plan_test.go @@ -445,7 +445,7 @@ func TestContext2Plan_moduleCycle(t *testing.T) { Attributes: map[string]*configschema.Attribute{ "id": {Type: cty.String, Computed: true}, "some_input": {Type: cty.String, Optional: true}, - "type": {Type: cty.String, Optional: true}, + "type": {Type: cty.String, Computed: true}, }, }, }, @@ -644,9 +644,10 @@ func TestContext2Plan_moduleInputComputed(t *testing.T) { switch i := ric.Addr.String(); i { case "aws_instance.bar": checkVals(t, objectVal(t, schema, map[string]cty.Value{ - "id": cty.UnknownVal(cty.String), - "foo": cty.UnknownVal(cty.String), - "type": cty.StringVal("aws_instance"), + "id": cty.UnknownVal(cty.String), + "foo": cty.UnknownVal(cty.String), + "type": cty.StringVal("aws_instance"), + "compute": cty.StringVal("foo"), }), ric.After) case "module.child.aws_instance.foo": checkVals(t, objectVal(t, schema, map[string]cty.Value{ @@ -1401,9 +1402,10 @@ func TestContext2Plan_moduleVarComputed(t *testing.T) { }), ric.After) case "module.child.aws_instance.foo": checkVals(t, objectVal(t, schema, map[string]cty.Value{ - "id": cty.UnknownVal(cty.String), - "foo": cty.UnknownVal(cty.String), - "type": cty.StringVal("aws_instance"), + "id": cty.UnknownVal(cty.String), + "foo": cty.UnknownVal(cty.String), + "type": cty.StringVal("aws_instance"), + "compute": cty.StringVal("foo"), }), ric.After) default: t.Fatal("unknown instance:", i) @@ -1745,10 +1747,11 @@ func TestContext2Plan_computed(t *testing.T) { }), ric.After) case "aws_instance.foo": checkVals(t, objectVal(t, schema, map[string]cty.Value{ - "id": cty.UnknownVal(cty.String), - "foo": cty.UnknownVal(cty.String), - "num": cty.NumberIntVal(2), - "type": cty.StringVal("aws_instance"), + "id": cty.UnknownVal(cty.String), + "foo": cty.UnknownVal(cty.String), + "num": cty.NumberIntVal(2), + "type": cty.StringVal("aws_instance"), + "compute": cty.StringVal("foo"), }), ric.After) default: t.Fatal("unknown instance:", i) @@ -2096,6 +2099,10 @@ func TestContext2Plan_computedList(t *testing.T) { New: "", NewComputed: true, } + diff.Attributes["compute"] = &ResourceAttrDiff{ + Old: "", + New: compute, + } } fooOld := s.Attributes["foo"] @@ -2168,8 +2175,9 @@ func TestContext2Plan_computedList(t *testing.T) { }), ric.After) case "aws_instance.foo": checkVals(t, objectVal(t, schema, map[string]cty.Value{ - "list": cty.UnknownVal(cty.List(cty.String)), - "num": cty.NumberIntVal(2), + "list": cty.UnknownVal(cty.List(cty.String)), + "num": cty.NumberIntVal(2), + "compute": cty.StringVal("list.#"), }), ric.After) default: t.Fatal("unknown instance:", i) @@ -2208,6 +2216,10 @@ func TestContext2Plan_computedMultiIndex(t *testing.T) { New: "", NewComputed: true, } + diff.Attributes["compute"] = &ResourceAttrDiff{ + Old: "", + New: compute, + } } fooOld := s.Attributes["foo"] @@ -2270,13 +2282,15 @@ func TestContext2Plan_computedMultiIndex(t *testing.T) { switch i := ric.Addr.String(); i { case "aws_instance.foo[0]": checkVals(t, objectVal(t, schema, map[string]cty.Value{ - "ip": cty.UnknownVal(cty.List(cty.String)), - "foo": cty.NullVal(cty.List(cty.String)), + "ip": cty.UnknownVal(cty.List(cty.String)), + "foo": cty.NullVal(cty.List(cty.String)), + "compute": cty.StringVal("ip.#"), }), ric.After) case "aws_instance.foo[1]": checkVals(t, objectVal(t, schema, map[string]cty.Value{ - "ip": cty.UnknownVal(cty.List(cty.String)), - "foo": cty.NullVal(cty.List(cty.String)), + "ip": cty.UnknownVal(cty.List(cty.String)), + "foo": cty.NullVal(cty.List(cty.String)), + "compute": cty.StringVal("ip.#"), }), ric.After) case "aws_instance.bar[0]": checkVals(t, objectVal(t, schema, map[string]cty.Value{ diff --git a/terraform/context_test.go b/terraform/context_test.go index 1b368112f..72e3f5db7 100644 --- a/terraform/context_test.go +++ b/terraform/context_test.go @@ -4,7 +4,9 @@ import ( "bufio" "bytes" "fmt" + "github.com/davecgh/go-spew/spew" "io/ioutil" + "log" "os" "path/filepath" "sort" @@ -195,6 +197,10 @@ func testDiffFn( diff := new(InstanceDiff) diff.Attributes = make(map[string]*ResourceAttrDiff) + defer func() { + log.Printf("[TRACE] testDiffFn: generated diff is:\n%s", spew.Sdump(diff)) + }() + if s != nil { diff.DestroyTainted = s.Tainted } @@ -202,6 +208,28 @@ func testDiffFn( for k, v := range c.Raw { // Ignore __-prefixed keys since they're used for magic if k[0] == '_' && k[1] == '_' { + // ...though we do still need to include them in the diff, to + // simulate normal provider behaviors. + old := s.Attributes[k] + var new string + switch tv := v.(type) { + case string: + new = tv + default: + new = fmt.Sprintf("%#v", v) + } + if new == hil.UnknownValue { + diff.Attributes[k] = &ResourceAttrDiff{ + Old: old, + New: "", + NewComputed: true, + } + } else { + diff.Attributes[k] = &ResourceAttrDiff{ + Old: old, + New: new, + } + } continue } @@ -211,10 +239,25 @@ func testDiffFn( // This key is used for other purposes if k == "compute_value" { + if old, ok := s.Attributes["compute_value"]; !ok || old != v.(string) { + diff.Attributes["compute_value"] = &ResourceAttrDiff{ + Old: old, + New: v.(string), + } + } continue } if k == "compute" { + // The "compute" value itself must be included in the diff if it + // has changed since prior. + if old, ok := s.Attributes["compute"]; !ok || old != v.(string) { + diff.Attributes["compute"] = &ResourceAttrDiff{ + Old: old, + New: v.(string), + } + } + if v == hil.UnknownValue || v == "unknown" { // compute wasn't set in the config, so don't use these // computed values from the schema. @@ -520,6 +563,7 @@ func testProviderSchema(name string) *ProviderSchema { "foo": { Type: cty.String, Optional: true, + Computed: true, }, "bar": { Type: cty.String, @@ -538,6 +582,7 @@ func testProviderSchema(name string) *ProviderSchema { "value": { Type: cty.String, Optional: true, + Computed: true, }, "output": { Type: cty.String, @@ -600,10 +645,12 @@ func testProviderSchema(name string) *ProviderSchema { "id": { Type: cty.String, Optional: true, + Computed: true, }, "ids": { Type: cty.List(cty.String), Optional: true, + Computed: true, }, }, }, diff --git a/terraform/terraform_test.go b/terraform/terraform_test.go index dd77d127a..5b9ef6cd1 100644 --- a/terraform/terraform_test.go +++ b/terraform/terraform_test.go @@ -446,6 +446,8 @@ aws_instance.bar: aws_instance.foo: ID = foo provider = provider.aws + compute = value + compute_value = 1 num = 2 type = aws_instance value = computed_value @@ -659,6 +661,8 @@ aws_instance.bar: aws_instance.foo: ID = foo provider = provider.aws + compute = value + compute_value = 1 num = 2 type = aws_instance value = computed_value @@ -1031,6 +1035,7 @@ const testTerraformApplyUnknownAttrStr = ` aws_instance.foo: (tainted) ID = foo provider = provider.aws + compute = unknown num = 2 type = aws_instance ` diff --git a/terraform/test-fixtures/apply-idattr/main.tf b/terraform/test-fixtures/apply-idattr/main.tf index b626e60c8..1c49f3975 100644 --- a/terraform/test-fixtures/apply-idattr/main.tf +++ b/terraform/test-fixtures/apply-idattr/main.tf @@ -1,2 +1,3 @@ resource "aws_instance" "foo" { + num = 42 } diff --git a/terraform/test-fixtures/plan-diffvar/main.tf b/terraform/test-fixtures/plan-diffvar/main.tf index eadc5b71b..eccc16ff2 100644 --- a/terraform/test-fixtures/plan-diffvar/main.tf +++ b/terraform/test-fixtures/plan-diffvar/main.tf @@ -1,7 +1,7 @@ resource "aws_instance" "foo" { - num = "2" + num = "3" } resource "aws_instance" "bar" { - num = "${aws_instance.foo.num}" + num = aws_instance.foo.num }