resilien
/
terraform
2
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

compare empty strings as null in sets 

The Legacy SDK cannot handle missing strings from objects in sets, and
will insert an empty string when planning the missing value. This
subverts the `couldHaveUnknownBlockPlaceholder` check, and causes
errors when `dynamic` is used with NestingSet blocks.

We don't have a separate codepath to handle the internals of
AssertObjectCompatible differently for the legacy SDK, but we can treat
empty strings as null strings within set objects to avoid the failed
assertions.
This commit is contained in:
James Bardin 2020-10-19 17:57:21 -04:00
parent 5e047b0a0b
commit ef086399f9
2 changed files with 122 additions and 3 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

19
plans/objchange/compatible.go
View File

@ -344,7 +344,7 @@ func couldHaveUnknownBlockPlaceholder(v cty.Value, blockS *configschema.NestedBl
if nested && v.IsNull() {
return true // for nested blocks, a single block being unset doesn't disqualify from being an unknown block placeholder
}
return couldBeUnknownBlockPlaceholderElement(v, &blockS.Block)
return couldBeUnknownBlockPlaceholderElement(v, blockS)
default:
// These situations should be impossible for correct providers, but
// we permit the legacy SDK to produce some incorrect outcomes
@ -360,7 +360,7 @@ func couldHaveUnknownBlockPlaceholder(v cty.Value, blockS *configschema.NestedBl
// For all other nesting modes, our value should be something iterable.
for it := v.ElementIterator(); it.Next(); {
_, ev := it.Element()
if couldBeUnknownBlockPlaceholderElement(ev, &blockS.Block) {
if couldBeUnknownBlockPlaceholderElement(ev, blockS) {
return true
}
}
@ -374,7 +374,7 @@ func couldHaveUnknownBlockPlaceholder(v cty.Value, blockS *configschema.NestedBl
}
}
func couldBeUnknownBlockPlaceholderElement(v cty.Value, schema *configschema.Block) bool {
func couldBeUnknownBlockPlaceholderElement(v cty.Value, schema *configschema.NestedBlock) bool {
if v.IsNull() {
return false // null value can never be a placeholder element
}
@ -390,6 +390,19 @@ func couldBeUnknownBlockPlaceholderElement(v cty.Value, schema *configschema.Blo
// non-placeholders can also match this, so this function can generate
// false positives.
if av.IsKnown() && !av.IsNull() {
// FIXME: only required for the legacy SDK, but we don't have a
// separate codepath to switch the comparisons, and we still want
// the rest of the checks from AssertObjectCompatible to apply.
//
// The legacy SDK cannot handle missing strings from set elements,
// and will insert an empty string into the planned value.
// Skipping these treats them as null values in this case,
// preventing false alerts from AssertObjectCompatible.
if schema.Nesting == configschema.NestingSet && av.Type() == cty.String && av.AsString() == "" {
continue
}
return false
}
}

106
plans/objchange/compatible_test.go
View File

@ -991,6 +991,40 @@ func TestAssertObjectCompatible(t *testing.T) {
}),
nil, // as above, the presence of a block whose attrs are all unknown indicates dynamic block expansion, so our usual count checks don't apply
},
{
&configschema.Block{
BlockTypes: map[string]*configschema.NestedBlock{
"key": {
Nesting: configschema.NestingList,
Block: schemaWithFooBar,
},
},
},
// While we must make an exception for empty strings in sets due to
// the legacy SDK, lists should be compared more strictly.
// This does not count as a dynamic block placeholder
cty.ObjectVal(map[string]cty.Value{
"key": cty.ListVal([]cty.Value{
fooBarBlockValue,
cty.ObjectVal(map[string]cty.Value{
"foo": cty.UnknownVal(cty.String),
"bar": cty.StringVal(""),
}),
}),
}),
cty.ObjectVal(map[string]cty.Value{
"key": cty.ListVal([]cty.Value{
fooBlockValue,
cty.ObjectVal(map[string]cty.Value{
"foo": cty.StringVal("hello"),
}),
cty.ObjectVal(map[string]cty.Value{
"foo": cty.StringVal("world"),
}),
}),
}),
[]string{".key: block count changed from 2 to 3"},
},
// NestingSet blocks
{
@ -1122,6 +1156,47 @@ func TestAssertObjectCompatible(t *testing.T) {
// indicates this may be a dynamic block, and the length is unknown
nil,
},
{
&configschema.Block{
BlockTypes: map[string]*configschema.NestedBlock{
"block": {
Nesting: configschema.NestingSet,
Block: schemaWithFooBar,
},
},
},
// The legacy SDK cannot handle missing strings in sets, and will
// insert empty strings to the planned value. Empty strings should
// be handled as nulls, and this object should represent a possible
// dynamic block.
cty.ObjectVal(map[string]cty.Value{
"block": cty.SetVal([]cty.Value{
cty.ObjectVal(map[string]cty.Value{
"foo": cty.UnknownVal(cty.String),
"bar": cty.StringVal(""),
}),
}),
}),
cty.ObjectVal(map[string]cty.Value{
"block": cty.SetVal([]cty.Value{
cty.ObjectVal(map[string]cty.Value{
"foo": cty.StringVal("hello"),
"bar": cty.StringVal(""),
}),
cty.ObjectVal(map[string]cty.Value{
"foo": cty.StringVal("world"),
"bar": cty.StringVal(""),
}),
cty.ObjectVal(map[string]cty.Value{
"foo": cty.StringVal("nope"),
"bar": cty.StringVal(""),
}),
}),
}),
// there is no error here, because the presence of unknowns
// indicates this may be a dynamic block, and the length is unknown
nil,
},
{
&configschema.Block{
BlockTypes: map[string]*configschema.NestedBlock{
@ -1266,6 +1341,37 @@ func TestAssertObjectCompatible(t *testing.T) {
}),
nil,
},
{
&configschema.Block{
BlockTypes: map[string]*configschema.NestedBlock{
"block": {
Nesting: configschema.NestingSet,
Block: schemaWithFooBar,
},
},
},
cty.ObjectVal(map[string]cty.Value{
"block": cty.SetVal([]cty.Value{
cty.ObjectVal(map[string]cty.Value{
"foo": cty.UnknownVal(cty.String),
"bar": cty.NullVal(cty.String),
}),
}),
}),
cty.ObjectVal(map[string]cty.Value{
"block": cty.SetVal([]cty.Value{
cty.ObjectVal(map[string]cty.Value{
"foo": cty.StringVal("a"),
"bar": cty.StringVal(""),
}),
cty.ObjectVal(map[string]cty.Value{
"foo": cty.StringVal("b"),
"bar": cty.StringVal(""),
}),
}),
}),
nil,
},
{
&configschema.Block{
BlockTypes: map[string]*configschema.NestedBlock{