diff --git a/plans/objchange/compatible.go b/plans/objchange/compatible.go index 0ef4b01e7..e4ece3f97 100644 --- a/plans/objchange/compatible.go +++ b/plans/objchange/compatible.go @@ -75,20 +75,12 @@ func assertObjectCompatible(schema *configschema.Block, planned, actual cty.Valu plannedV, _ := planned.GetAttr(name).Unmark() actualV, _ := actual.GetAttr(name).Unmark() - // As a special case, if there were any blocks whose leaf attributes - // are all unknown then we assume (possibly incorrectly) that the - // HCL dynamic block extension is in use with an unknown for_each - // argument, and so we will do looser validation here that allows - // for those blocks to have expanded into a different number of blocks - // if the for_each value is now known. - maybeUnknownBlocks := couldHaveUnknownBlockPlaceholder(plannedV, blockS, false) - path := append(path, cty.GetAttrStep{Name: name}) switch blockS.Nesting { case configschema.NestingSingle, configschema.NestingGroup: // If an unknown block placeholder was present then the placeholder // may have expanded out into zero blocks, which is okay. - if maybeUnknownBlocks && actualV.IsNull() { + if !plannedV.IsKnown() && actualV.IsNull() { continue } moreErrs := assertObjectCompatible(&blockS.Block, plannedV, actualV, path) @@ -102,14 +94,6 @@ func assertObjectCompatible(schema *configschema.Block, planned, actual cty.Valu continue } - if maybeUnknownBlocks { - // When unknown blocks are present the final blocks may be - // at different indices than the planned blocks, so unfortunately - // we can't do our usual checks in this case without generating - // false negatives. - continue - } - plannedL := plannedV.LengthInt() actualL := actualV.LengthInt() if plannedL != actualL { @@ -144,7 +128,7 @@ func assertObjectCompatible(schema *configschema.Block, planned, actual cty.Valu moreErrs := assertObjectCompatible(&blockS.Block, plannedEV, actualEV, append(path, cty.GetAttrStep{Name: k})) errs = append(errs, moreErrs...) } - if !maybeUnknownBlocks { // new blocks may appear if unknown blocks were present in the plan + if plannedV.IsKnown() { // new blocks may appear if unknown blocks were present in the plan for k := range actualAtys { if _, ok := plannedAtys[k]; !ok { errs = append(errs, path.NewErrorf("new block key %q has appeared", k)) @@ -158,7 +142,7 @@ func assertObjectCompatible(schema *configschema.Block, planned, actual cty.Valu } plannedL := plannedV.LengthInt() actualL := actualV.LengthInt() - if plannedL != actualL && !maybeUnknownBlocks { // new blocks may appear if unknown blocks were persent in the plan + if plannedL != actualL && plannedV.IsKnown() { // new blocks may appear if unknown blocks were persent in the plan errs = append(errs, path.NewErrorf("block count changed from %d to %d", plannedL, actualL)) continue } @@ -177,7 +161,7 @@ func assertObjectCompatible(schema *configschema.Block, planned, actual cty.Valu continue } - if maybeUnknownBlocks { + if !plannedV.IsKnown() { // When unknown blocks are present the final number of blocks // may be different, either because the unknown set values // become equal and are collapsed, or the count is unknown due @@ -328,96 +312,6 @@ func indexStrForErrors(v cty.Value) string { } } -// couldHaveUnknownBlockPlaceholder is a heuristic that recognizes how the -// HCL dynamic block extension behaves when it's asked to expand a block whose -// for_each argument is unknown. In such cases, it generates a single placeholder -// block with all leaf attribute values unknown, and once the for_each -// expression becomes known the placeholder may be replaced with any number -// of blocks, so object compatibility checks would need to be more liberal. -// -// Set "nested" if testing a block that is nested inside a candidate block -// placeholder; this changes the interpretation of there being no blocks of -// a type to allow for there being zero nested blocks. -func couldHaveUnknownBlockPlaceholder(v cty.Value, blockS *configschema.NestedBlock, nested bool) bool { - switch blockS.Nesting { - case configschema.NestingSingle, configschema.NestingGroup: - 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) - default: - // These situations should be impossible for correct providers, but - // we permit the legacy SDK to produce some incorrect outcomes - // for compatibility with its existing logic, and so we must be - // tolerant here. - if !v.IsKnown() { - return true - } - if v.IsNull() { - return false // treated as if the list were empty, so we would see zero iterations below - } - - // Unmark before we call ElementIterator in case this iterable is marked sensitive. - // This can arise in the case where a member of a Set is sensitive, and thus the - // whole Set is marked sensitive - v, _ := v.Unmark() - // For all other nesting modes, our value should be something iterable. - for it := v.ElementIterator(); it.Next(); { - _, ev := it.Element() - if couldBeUnknownBlockPlaceholderElement(ev, blockS) { - return true - } - } - - // Our default changes depending on whether we're testing the candidate - // block itself or something nested inside of it: zero blocks of a type - // can never contain a dynamic block placeholder, but a dynamic block - // placeholder might contain zero blocks of one of its own nested block - // types, if none were set in the config at all. - return nested - } -} - -func couldBeUnknownBlockPlaceholderElement(v cty.Value, schema *configschema.NestedBlock) bool { - if v.IsNull() { - return false // null value can never be a placeholder element - } - if !v.IsKnown() { - return true // this should never happen for well-behaved providers, but can happen with the legacy SDK opt-outs - } - for name := range schema.Attributes { - av := v.GetAttr(name) - - // Unknown block placeholders contain only unknown or null attribute - // values, depending on whether or not a particular attribute was set - // explicitly inside the content block. Note that this is imprecise: - // 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 - } - } - for name, blockS := range schema.BlockTypes { - if !couldHaveUnknownBlockPlaceholder(v.GetAttr(name), blockS, true) { - return false - } - } - return true -} - // assertSetValuesCompatible checks that each of the elements in a can // be correlated with at least one equivalent element in b and vice-versa, // using the given correlation function. diff --git a/plans/objchange/compatible_test.go b/plans/objchange/compatible_test.go index 678ce8ce3..942a477ea 100644 --- a/plans/objchange/compatible_test.go +++ b/plans/objchange/compatible_test.go @@ -30,10 +30,6 @@ func TestAssertObjectCompatible(t *testing.T) { "foo": cty.StringVal("bar"), "bar": cty.NullVal(cty.String), // simulating the situation where bar isn't set in the config at all }) - fooBarBlockDynamicPlaceholder := cty.ObjectVal(map[string]cty.Value{ - "foo": cty.UnknownVal(cty.String), - "bar": cty.NullVal(cty.String), // simulating the situation where bar isn't set in the config at all - }) tests := []struct { Schema *configschema.Block @@ -919,13 +915,11 @@ func TestAssertObjectCompatible(t *testing.T) { }, }, }, - cty.ObjectVal(map[string]cty.Value{ - "key": cty.ObjectVal(map[string]cty.Value{ - // One wholly unknown block is what "dynamic" blocks - // generate when the for_each expression is unknown. - "foo": cty.UnknownVal(cty.String), + cty.UnknownVal(cty.Object(map[string]cty.Type{ + "key": cty.Object(map[string]cty.Type{ + "foo": cty.String, }), - }), + })), cty.ObjectVal(map[string]cty.Value{ "key": cty.NullVal(cty.Object(map[string]cty.Type{ "foo": cty.String, @@ -1011,11 +1005,9 @@ func TestAssertObjectCompatible(t *testing.T) { }, }, }, - cty.ObjectVal(map[string]cty.Value{ - "key": cty.ListVal([]cty.Value{ - fooBarBlockDynamicPlaceholder, // the presence of this disables some of our checks - }), - }), + cty.UnknownVal(cty.Object(map[string]cty.Type{ + "key": cty.List(fooBarBlockValue.Type()), + })), cty.ObjectVal(map[string]cty.Value{ "key": cty.ListVal([]cty.Value{ cty.ObjectVal(map[string]cty.Value{ @@ -1026,35 +1018,7 @@ func TestAssertObjectCompatible(t *testing.T) { }), }), }), - nil, // a single block whose attrs are all unknown is allowed to expand into multiple, because that's how dynamic blocks behave when for_each is unknown - }, - { - &configschema.Block{ - BlockTypes: map[string]*configschema.NestedBlock{ - "key": { - Nesting: configschema.NestingList, - Block: schemaWithFooBar, - }, - }, - }, - cty.ObjectVal(map[string]cty.Value{ - "key": cty.ListVal([]cty.Value{ - fooBarBlockValue, // the presence of one static block does not negate that the following element looks like a dynamic placeholder - fooBarBlockDynamicPlaceholder, // the presence of this disables some of our checks - }), - }), - 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"), - }), - }), - }), - 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 + nil, // an unknown block is allowed to expand into multiple, because that's how dynamic blocks behave when for_each is unknown }, { &configschema.Block{ @@ -1195,14 +1159,11 @@ func TestAssertObjectCompatible(t *testing.T) { }, }, cty.ObjectVal(map[string]cty.Value{ - "block": cty.SetVal([]cty.Value{ - cty.ObjectVal(map[string]cty.Value{ - "foo": cty.UnknownVal(cty.String), + "block": cty.UnknownVal(cty.Set( + cty.Object(map[string]cty.Type{ + "foo": cty.String, }), - cty.ObjectVal(map[string]cty.Value{ - "foo": cty.UnknownVal(cty.String), - }), - }), + )), }), cty.ObjectVal(map[string]cty.Value{ "block": cty.SetVal([]cty.Value{ @@ -1221,47 +1182,6 @@ 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{ @@ -1335,11 +1255,7 @@ func TestAssertObjectCompatible(t *testing.T) { }, }, cty.ObjectVal(map[string]cty.Value{ - "block": cty.SetVal([]cty.Value{ - cty.ObjectVal(map[string]cty.Value{ - "foo": cty.UnknownVal(cty.String), - }), - }), + "block": cty.UnknownVal(cty.Set(fooBlockValue.Type())), }), cty.ObjectVal(map[string]cty.Value{ "block": cty.SetVal([]cty.Value{ @@ -1364,11 +1280,7 @@ func TestAssertObjectCompatible(t *testing.T) { }, }, cty.ObjectVal(map[string]cty.Value{ - "block2": cty.SetVal([]cty.Value{ - cty.ObjectVal(map[string]cty.Value{ - "foo": cty.UnknownVal(cty.String), - }), - }), + "block2": cty.UnknownVal(cty.Set(fooBlockValue.Type())), }), cty.ObjectVal(map[string]cty.Value{ "block2": cty.SetValEmpty(cty.Object(map[string]cty.Type{ @@ -1406,37 +1318,6 @@ 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{ diff --git a/plans/objchange/objchange.go b/plans/objchange/objchange.go index fade668c2..6bfc06e22 100644 --- a/plans/objchange/objchange.go +++ b/plans/objchange/objchange.go @@ -93,6 +93,12 @@ func proposedNew(schema *configschema.Block, prior, config cty.Value) cty.Value } func proposedNewNestedBlock(schema *configschema.NestedBlock, prior, config cty.Value) cty.Value { + // The only time we should encounter an entirely unknown block is from the + // use of dynamic with an unknown for_each expression. + if !config.IsKnown() { + return config + } + var newV cty.Value switch schema.Nesting { @@ -103,7 +109,7 @@ func proposedNewNestedBlock(schema *configschema.NestedBlock, prior, config cty. case configschema.NestingList: // Nested blocks are correlated by index. configVLen := 0 - if config.IsKnown() && !config.IsNull() { + if !config.IsNull() { configVLen = config.LengthInt() } if configVLen > 0 {