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.
This commit is contained in:
Martin Atkins 2019-02-11 17:05:24 -08:00
parent e831182c8d
commit f4e6431da2
6 changed files with 209 additions and 84 deletions

View File

@ -1878,14 +1878,21 @@ func TestContext2Apply_cancel(t *testing.T) {
}, },
}, nil }, nil
} }
p.DiffFn = func(*InstanceInfo, *InstanceState, *ResourceConfig) (*InstanceDiff, error) { p.DiffFn = func(info *InstanceInfo, s *InstanceState, rc *ResourceConfig) (*InstanceDiff, error) {
return &InstanceDiff{ d := &InstanceDiff{
Attributes: map[string]*ResourceAttrDiff{ Attributes: map[string]*ResourceAttrDiff{},
"value": &ResourceAttrDiff{ }
New: "2", if new, ok := rc.Get("value"); ok {
}, d.Attributes["value"] = &ResourceAttrDiff{
}, New: new.(string),
}, nil }
}
if new, ok := rc.Get("foo"); ok {
d.Attributes["foo"] = &ResourceAttrDiff{
New: new.(string),
}
}
return d, nil
} }
if _, diags := ctx.Plan(); diags.HasErrors() { if _, diags := ctx.Plan(); diags.HasErrors() {
@ -1937,6 +1944,9 @@ func TestContext2Apply_cancelBlock(t *testing.T) {
"id": &ResourceAttrDiff{ "id": &ResourceAttrDiff{
New: "foo", New: "foo",
}, },
"num": &ResourceAttrDiff{
New: "2",
},
}, },
}, nil }, nil
} }
@ -1954,6 +1964,9 @@ func TestContext2Apply_cancelBlock(t *testing.T) {
return &InstanceState{ return &InstanceState{
ID: "foo", ID: "foo",
Attributes: map[string]string{
"num": "2",
},
}, nil }, nil
} }
@ -2002,6 +2015,7 @@ func TestContext2Apply_cancelBlock(t *testing.T) {
aws_instance.foo: aws_instance.foo:
ID = foo ID = foo
provider = provider.aws 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{ ctx.variables = InputValues{
"value": &InputValue{ "value": &InputValue{
Value: cty.NumberIntVal(1), 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() state, diags := ctx.Apply()
if diags.HasErrors() { if diags.HasErrors() {
t.Fatalf("unexpected errors: %s", diags.Err()) t.Fatalf("unexpected errors: %s", diags.Err())
@ -3741,25 +3755,35 @@ func TestContext2Apply_multiVarComprehensive(t *testing.T) {
p.ApplyFn = testApplyFn 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() configsLock.Lock()
defer configsLock.Unlock() defer configsLock.Unlock()
key := c.Config["key"].(string) key := proposed.GetAttr("key").AsString()
configs[key] = c // 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 retVals := make(map[string]cty.Value)
// the apply graph and thus the final state, but otherwise we're just for it := proposed.ElementIterator(); it.Next(); {
// gathering data for assertions. idxVal, val := it.Element()
return &InstanceDiff{ idx := idxVal.AsString()
Attributes: map[string]*ResourceAttrDiff{
"id": &ResourceAttrDiff{ switch idx {
NewComputed: true, case "id":
}, retVals[idx] = cty.UnknownVal(cty.String)
"name": &ResourceAttrDiff{ case "name":
New: key, retVals[idx] = cty.StringVal(key)
}, default:
}, retVals[idx] = val
}, nil }
}
return providers.PlanResourceChangeResponse{
PlannedState: cty.ObjectVal(retVals),
}
} }
p.GetSchemaReturn = &ProviderSchema{ p.GetSchemaReturn = &ProviderSchema{
@ -6149,21 +6173,33 @@ func TestContext2Apply_outputDiffVars(t *testing.T) {
result := s.MergeDiff(d) result := s.MergeDiff(d)
result.ID = "foo" result.ID = "foo"
result.Attributes["foo"] = "bar"
return result, nil return result, nil
} }
p.DiffFn = func(*InstanceInfo, *InstanceState, *ResourceConfig) (*InstanceDiff, error) { p.DiffFn = func(info *InstanceInfo, s *InstanceState, rc *ResourceConfig) (*InstanceDiff, error) {
return &InstanceDiff{ d := &InstanceDiff{
Attributes: map[string]*ResourceAttrDiff{ Attributes: map[string]*ResourceAttrDiff{},
"foo": &ResourceAttrDiff{ }
NewComputed: true, if new, ok := rc.Get("value"); ok {
Type: DiffAttrOutput, d.Attributes["value"] = &ResourceAttrDiff{
}, New: new.(string),
"bar": &ResourceAttrDiff{ }
New: "baz", }
}, if new, ok := rc.Get("foo"); ok {
}, d.Attributes["foo"] = &ResourceAttrDiff{
}, nil 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() { if _, diags := ctx.Plan(); diags.HasErrors() {
@ -6899,14 +6935,21 @@ func TestContext2Apply_destroyOrphan(t *testing.T) {
result.ID = "foo" result.ID = "foo"
return result, nil return result, nil
} }
p.DiffFn = func(*InstanceInfo, *InstanceState, *ResourceConfig) (*InstanceDiff, error) { p.DiffFn = func(info *InstanceInfo, s *InstanceState, rc *ResourceConfig) (*InstanceDiff, error) {
return &InstanceDiff{ d := &InstanceDiff{
Attributes: map[string]*ResourceAttrDiff{ Attributes: map[string]*ResourceAttrDiff{},
"value": &ResourceAttrDiff{ }
New: "bar", if new, ok := rc.Get("value"); ok {
}, d.Attributes["value"] = &ResourceAttrDiff{
}, New: new.(string),
}, nil }
}
if new, ok := rc.Get("foo"); ok {
d.Attributes["foo"] = &ResourceAttrDiff{
New: new.(string),
}
}
return d, nil
} }
if _, diags := ctx.Plan(); diags.HasErrors() { if _, diags := ctx.Plan(); diags.HasErrors() {
@ -7021,14 +7064,21 @@ func TestContext2Apply_error(t *testing.T) {
}, },
}, nil }, nil
} }
p.DiffFn = func(*InstanceInfo, *InstanceState, *ResourceConfig) (*InstanceDiff, error) { p.DiffFn = func(info *InstanceInfo, s *InstanceState, rc *ResourceConfig) (*InstanceDiff, error) {
return &InstanceDiff{ d := &InstanceDiff{
Attributes: map[string]*ResourceAttrDiff{ Attributes: map[string]*ResourceAttrDiff{},
"value": &ResourceAttrDiff{ }
New: "2", if new, ok := rc.Get("value"); ok {
}, d.Attributes["value"] = &ResourceAttrDiff{
}, New: new.(string),
}, nil }
}
if new, ok := rc.Get("foo"); ok {
d.Attributes["foo"] = &ResourceAttrDiff{
New: new.(string),
}
}
return d, nil
} }
if _, diags := ctx.Plan(); diags.HasErrors() { if _, diags := ctx.Plan(); diags.HasErrors() {
@ -7090,14 +7140,21 @@ func TestContext2Apply_errorPartial(t *testing.T) {
}, },
}, nil }, nil
} }
p.DiffFn = func(*InstanceInfo, *InstanceState, *ResourceConfig) (*InstanceDiff, error) { p.DiffFn = func(info *InstanceInfo, s *InstanceState, rc *ResourceConfig) (*InstanceDiff, error) {
return &InstanceDiff{ d := &InstanceDiff{
Attributes: map[string]*ResourceAttrDiff{ Attributes: map[string]*ResourceAttrDiff{},
"value": &ResourceAttrDiff{ }
New: "2", if new, ok := rc.Get("value"); ok {
}, d.Attributes["value"] = &ResourceAttrDiff{
}, New: new.(string),
}, nil }
}
if new, ok := rc.Get("foo"); ok {
d.Attributes["foo"] = &ResourceAttrDiff{
New: new.(string),
}
}
return d, nil
} }
if _, diags := ctx.Plan(); diags.HasErrors() { if _, diags := ctx.Plan(); diags.HasErrors() {
@ -8907,6 +8964,7 @@ func TestContext2Apply_issue5254(t *testing.T) {
template_file.child: template_file.child:
ID = foo ID = foo
provider = provider.template provider = provider.template
__template_requires_new = true
template = Hi template = Hi
type = template_file type = template_file

View File

@ -445,7 +445,7 @@ func TestContext2Plan_moduleCycle(t *testing.T) {
Attributes: map[string]*configschema.Attribute{ Attributes: map[string]*configschema.Attribute{
"id": {Type: cty.String, Computed: true}, "id": {Type: cty.String, Computed: true},
"some_input": {Type: cty.String, Optional: 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 { switch i := ric.Addr.String(); i {
case "aws_instance.bar": case "aws_instance.bar":
checkVals(t, objectVal(t, schema, map[string]cty.Value{ checkVals(t, objectVal(t, schema, map[string]cty.Value{
"id": cty.UnknownVal(cty.String), "id": cty.UnknownVal(cty.String),
"foo": cty.UnknownVal(cty.String), "foo": cty.UnknownVal(cty.String),
"type": cty.StringVal("aws_instance"), "type": cty.StringVal("aws_instance"),
"compute": cty.StringVal("foo"),
}), ric.After) }), ric.After)
case "module.child.aws_instance.foo": case "module.child.aws_instance.foo":
checkVals(t, objectVal(t, schema, map[string]cty.Value{ checkVals(t, objectVal(t, schema, map[string]cty.Value{
@ -1401,9 +1402,10 @@ func TestContext2Plan_moduleVarComputed(t *testing.T) {
}), ric.After) }), ric.After)
case "module.child.aws_instance.foo": case "module.child.aws_instance.foo":
checkVals(t, objectVal(t, schema, map[string]cty.Value{ checkVals(t, objectVal(t, schema, map[string]cty.Value{
"id": cty.UnknownVal(cty.String), "id": cty.UnknownVal(cty.String),
"foo": cty.UnknownVal(cty.String), "foo": cty.UnknownVal(cty.String),
"type": cty.StringVal("aws_instance"), "type": cty.StringVal("aws_instance"),
"compute": cty.StringVal("foo"),
}), ric.After) }), ric.After)
default: default:
t.Fatal("unknown instance:", i) t.Fatal("unknown instance:", i)
@ -1745,10 +1747,11 @@ func TestContext2Plan_computed(t *testing.T) {
}), ric.After) }), ric.After)
case "aws_instance.foo": case "aws_instance.foo":
checkVals(t, objectVal(t, schema, map[string]cty.Value{ checkVals(t, objectVal(t, schema, map[string]cty.Value{
"id": cty.UnknownVal(cty.String), "id": cty.UnknownVal(cty.String),
"foo": cty.UnknownVal(cty.String), "foo": cty.UnknownVal(cty.String),
"num": cty.NumberIntVal(2), "num": cty.NumberIntVal(2),
"type": cty.StringVal("aws_instance"), "type": cty.StringVal("aws_instance"),
"compute": cty.StringVal("foo"),
}), ric.After) }), ric.After)
default: default:
t.Fatal("unknown instance:", i) t.Fatal("unknown instance:", i)
@ -2096,6 +2099,10 @@ func TestContext2Plan_computedList(t *testing.T) {
New: "", New: "",
NewComputed: true, NewComputed: true,
} }
diff.Attributes["compute"] = &ResourceAttrDiff{
Old: "",
New: compute,
}
} }
fooOld := s.Attributes["foo"] fooOld := s.Attributes["foo"]
@ -2168,8 +2175,9 @@ func TestContext2Plan_computedList(t *testing.T) {
}), ric.After) }), ric.After)
case "aws_instance.foo": case "aws_instance.foo":
checkVals(t, objectVal(t, schema, map[string]cty.Value{ checkVals(t, objectVal(t, schema, map[string]cty.Value{
"list": cty.UnknownVal(cty.List(cty.String)), "list": cty.UnknownVal(cty.List(cty.String)),
"num": cty.NumberIntVal(2), "num": cty.NumberIntVal(2),
"compute": cty.StringVal("list.#"),
}), ric.After) }), ric.After)
default: default:
t.Fatal("unknown instance:", i) t.Fatal("unknown instance:", i)
@ -2208,6 +2216,10 @@ func TestContext2Plan_computedMultiIndex(t *testing.T) {
New: "", New: "",
NewComputed: true, NewComputed: true,
} }
diff.Attributes["compute"] = &ResourceAttrDiff{
Old: "",
New: compute,
}
} }
fooOld := s.Attributes["foo"] fooOld := s.Attributes["foo"]
@ -2270,13 +2282,15 @@ func TestContext2Plan_computedMultiIndex(t *testing.T) {
switch i := ric.Addr.String(); i { switch i := ric.Addr.String(); i {
case "aws_instance.foo[0]": case "aws_instance.foo[0]":
checkVals(t, objectVal(t, schema, map[string]cty.Value{ checkVals(t, objectVal(t, schema, map[string]cty.Value{
"ip": cty.UnknownVal(cty.List(cty.String)), "ip": cty.UnknownVal(cty.List(cty.String)),
"foo": cty.NullVal(cty.List(cty.String)), "foo": cty.NullVal(cty.List(cty.String)),
"compute": cty.StringVal("ip.#"),
}), ric.After) }), ric.After)
case "aws_instance.foo[1]": case "aws_instance.foo[1]":
checkVals(t, objectVal(t, schema, map[string]cty.Value{ checkVals(t, objectVal(t, schema, map[string]cty.Value{
"ip": cty.UnknownVal(cty.List(cty.String)), "ip": cty.UnknownVal(cty.List(cty.String)),
"foo": cty.NullVal(cty.List(cty.String)), "foo": cty.NullVal(cty.List(cty.String)),
"compute": cty.StringVal("ip.#"),
}), ric.After) }), ric.After)
case "aws_instance.bar[0]": case "aws_instance.bar[0]":
checkVals(t, objectVal(t, schema, map[string]cty.Value{ checkVals(t, objectVal(t, schema, map[string]cty.Value{

View File

@ -4,7 +4,9 @@ import (
"bufio" "bufio"
"bytes" "bytes"
"fmt" "fmt"
"github.com/davecgh/go-spew/spew"
"io/ioutil" "io/ioutil"
"log"
"os" "os"
"path/filepath" "path/filepath"
"sort" "sort"
@ -195,6 +197,10 @@ func testDiffFn(
diff := new(InstanceDiff) diff := new(InstanceDiff)
diff.Attributes = make(map[string]*ResourceAttrDiff) diff.Attributes = make(map[string]*ResourceAttrDiff)
defer func() {
log.Printf("[TRACE] testDiffFn: generated diff is:\n%s", spew.Sdump(diff))
}()
if s != nil { if s != nil {
diff.DestroyTainted = s.Tainted diff.DestroyTainted = s.Tainted
} }
@ -202,6 +208,28 @@ func testDiffFn(
for k, v := range c.Raw { for k, v := range c.Raw {
// Ignore __-prefixed keys since they're used for magic // Ignore __-prefixed keys since they're used for magic
if k[0] == '_' && k[1] == '_' { 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 continue
} }
@ -211,10 +239,25 @@ func testDiffFn(
// This key is used for other purposes // This key is used for other purposes
if k == "compute_value" { 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 continue
} }
if k == "compute" { 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" { if v == hil.UnknownValue || v == "unknown" {
// compute wasn't set in the config, so don't use these // compute wasn't set in the config, so don't use these
// computed values from the schema. // computed values from the schema.
@ -520,6 +563,7 @@ func testProviderSchema(name string) *ProviderSchema {
"foo": { "foo": {
Type: cty.String, Type: cty.String,
Optional: true, Optional: true,
Computed: true,
}, },
"bar": { "bar": {
Type: cty.String, Type: cty.String,
@ -538,6 +582,7 @@ func testProviderSchema(name string) *ProviderSchema {
"value": { "value": {
Type: cty.String, Type: cty.String,
Optional: true, Optional: true,
Computed: true,
}, },
"output": { "output": {
Type: cty.String, Type: cty.String,
@ -600,10 +645,12 @@ func testProviderSchema(name string) *ProviderSchema {
"id": { "id": {
Type: cty.String, Type: cty.String,
Optional: true, Optional: true,
Computed: true,
}, },
"ids": { "ids": {
Type: cty.List(cty.String), Type: cty.List(cty.String),
Optional: true, Optional: true,
Computed: true,
}, },
}, },
}, },

View File

@ -446,6 +446,8 @@ aws_instance.bar:
aws_instance.foo: aws_instance.foo:
ID = foo ID = foo
provider = provider.aws provider = provider.aws
compute = value
compute_value = 1
num = 2 num = 2
type = aws_instance type = aws_instance
value = computed_value value = computed_value
@ -659,6 +661,8 @@ aws_instance.bar:
aws_instance.foo: aws_instance.foo:
ID = foo ID = foo
provider = provider.aws provider = provider.aws
compute = value
compute_value = 1
num = 2 num = 2
type = aws_instance type = aws_instance
value = computed_value value = computed_value
@ -1031,6 +1035,7 @@ const testTerraformApplyUnknownAttrStr = `
aws_instance.foo: (tainted) aws_instance.foo: (tainted)
ID = foo ID = foo
provider = provider.aws provider = provider.aws
compute = unknown
num = 2 num = 2
type = aws_instance type = aws_instance
` `

View File

@ -1,2 +1,3 @@
resource "aws_instance" "foo" { resource "aws_instance" "foo" {
num = 42
} }

View File

@ -1,7 +1,7 @@
resource "aws_instance" "foo" { resource "aws_instance" "foo" {
num = "2" num = "3"
} }
resource "aws_instance" "bar" { resource "aws_instance" "bar" {
num = "${aws_instance.foo.num}" num = aws_instance.foo.num
} }