diff --git a/plans/objchange/compatible.go b/plans/objchange/compatible.go index 6dc622dfa..6d92fca4d 100644 --- a/plans/objchange/compatible.go +++ b/plans/objchange/compatible.go @@ -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 } } diff --git a/plans/objchange/compatible_test.go b/plans/objchange/compatible_test.go index 1eacdd8b1..9a6924441 100644 --- a/plans/objchange/compatible_test.go +++ b/plans/objchange/compatible_test.go @@ -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{