plans/objchange: Fix handling of dynamic block placeholders

If a dynamic block (in the HCL dynamic block extension sense) has an
unknown value for its for_each argument, it gets expanded to a single
placeholder block with all of its attributes set to a unknown values.

We can use this as part of a heuristic to relax our object compatibility
checks for situations where the plan included an object that appears to
be (but isn't necessarily) such a placeholder, allowing for the fact that
the one placeholder block could be replaced with zero or more real blocks
once the for_each value is known.

Previously our heuristic was too strict: it would match only if the only
block present was a dynamic placeholder. In practice, users may mix
dynamic blocks with static blocks of the same type, so we need to be more
liberal to avoid generating incorrect incompatibility errors in such
cases.

builtin/providers/test/resource_list_test.go

@@ -447,3 +447,37 @@ resource "test_resource_list" "bar" {
 		},
 	})
 }
+
+func TestResourceList_dynamicList(t *testing.T) {
+	resource.UnitTest(t, resource.TestCase{
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckResourceDestroy,
+		Steps: []resource.TestStep{
+			resource.TestStep{
+				Config: strings.TrimSpace(`
+resource "test_resource_list" "a" {
+	dependent_list {
+		val = "a"
+	}
+
+	dependent_list {
+		val = "b"
+	}
+}
+resource "test_resource_list" "b" {
+	list_block {
+		string = "constant"
+	}
+	dynamic "list_block" {
+		for_each = test_resource_list.a.computed_list
+		content {
+		  string = list_block.value
+		}
+	}
+}
+				`),
+				Check: resource.ComposeTestCheckFunc(),
+			},
+		},
+	})
+}

plans/objchange/compatible.go

@@ -61,30 +61,22 @@ func assertObjectCompatible(schema *configschema.Block, planned, actual cty.Valu
 		plannedV := planned.GetAttr(name)
 		actualV := actual.GetAttr(name)
 
-		// As a special case, we permit a "planned" block with exactly one
+		// As a special case, if there were any blocks whose leaf attributes
-		// element where all of the "leaf" values are unknown, since that's
+		// are all unknown then we assume (possibly incorrectly) that the
-		// what HCL's dynamic block extension generates if the for_each
+		// HCL dynamic block extension is in use with an unknown for_each
-		// expression is itself unknown and thus it cannot predict how many
+		// argument, and so we will do looser validation here that allows
-		// child blocks will get created.
+		// for those blocks to have expanded into a different number of blocks
-		switch blockS.Nesting {
+		// if the for_each value is now known.
-		case configschema.NestingSingle, configschema.NestingGroup:
+		maybeUnknownBlocks := couldHaveUnknownBlockPlaceholder(plannedV, blockS, false)
-			if allLeafValuesUnknown(plannedV) && !plannedV.IsNull() {
-				return errs
-			}
-		case configschema.NestingList, configschema.NestingMap, configschema.NestingSet:
-			if plannedV.IsKnown() && !plannedV.IsNull() && plannedV.LengthInt() == 1 {
-				elemVs := plannedV.AsValueSlice()
-				if allLeafValuesUnknown(elemVs[0]) {
-					return errs
-				}
-			}
-		default:
-			panic(fmt.Sprintf("unsupported nesting mode %s", blockS.Nesting))
-		}
 
 		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() {
+				continue
+			}
 			moreErrs := assertObjectCompatible(&blockS.Block, plannedV, actualV, path)
 			errs = append(errs, moreErrs...)
 		case configschema.NestingList:
@@ -96,6 +88,14 @@ 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 {
@@ -130,19 +130,21 @@ 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
 					for k := range actualAtys {
 						if _, ok := plannedAtys[k]; !ok {
 							errs = append(errs, path.NewErrorf("new block key %q has appeared", k))
 							continue
 						}
 					}
+				}
 			} else {
 				if !plannedV.IsKnown() || plannedV.IsNull() || actualV.IsNull() {
 					continue
 				}
 				plannedL := plannedV.LengthInt()
 				actualL := actualV.LengthInt()
-				if plannedL != actualL {
+				if plannedL != actualL && !maybeUnknownBlocks { // 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
 				}
@@ -302,18 +304,77 @@ func indexStrForErrors(v cty.Value) string {
 	}
 }
 
-func allLeafValuesUnknown(v cty.Value) bool {
+// couldHaveUnknownBlockPlaceholder is a heuristic that recognizes how the
-	seenKnownValue := false
+// HCL dynamic block extension behaves when it's asked to expand a block whose
-	cty.Walk(v, func(path cty.Path, cv cty.Value) (bool, error) {
+// for_each argument is unknown. In such cases, it generates a single placeholder
-		if cv.IsNull() {
+// block with all leaf attribute values unknown, and once the for_each
-			seenKnownValue = true
+// 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
 		}
-		if cv.Type().IsPrimitiveType() && cv.IsKnown() {
+		return couldBeUnknownBlockPlaceholderElement(v, &blockS.Block)
-			seenKnownValue = true
+	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
 		}
-		return true, nil
+		if v.IsNull() {
-	})
+			return false // treated as if the list were empty, so we would see zero iterations below
-	return !seenKnownValue
+		}
+
+		// 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) {
+				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.Block) 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() {
+			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

plans/objchange/compatible_test.go

@@ -12,6 +12,29 @@ import (
 )
 
 func TestAssertObjectCompatible(t *testing.T) {
+	schemaWithFoo := configschema.Block{
+		Attributes: map[string]*configschema.Attribute{
+			"foo": {Type: cty.String, Optional: true},
+		},
+	}
+	fooBlockValue := cty.ObjectVal(map[string]cty.Value{
+		"foo": cty.StringVal("bar"),
+	})
+	schemaWithFooBar := configschema.Block{
+		Attributes: map[string]*configschema.Attribute{
+			"foo": {Type: cty.String, Optional: true},
+			"bar": {Type: cty.String, Optional: true},
+		},
+	}
+	fooBarBlockValue := cty.ObjectVal(map[string]cty.Value{
+		"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
 		Planned  cty.Value
@@ -681,11 +704,9 @@ func TestAssertObjectCompatible(t *testing.T) {
 				},
 			},
 			cty.ObjectVal(map[string]cty.Value{
-				"id":  cty.UnknownVal(cty.String),
 				"key": cty.EmptyObjectVal,
 			}),
 			cty.ObjectVal(map[string]cty.Value{
-				"id":  cty.UnknownVal(cty.String),
 				"key": cty.EmptyObjectVal,
 			}),
 			nil,
@@ -700,11 +721,9 @@ func TestAssertObjectCompatible(t *testing.T) {
 				},
 			},
 			cty.ObjectVal(map[string]cty.Value{
-				"id":  cty.UnknownVal(cty.String),
 				"key": cty.UnknownVal(cty.EmptyObject),
 			}),
 			cty.ObjectVal(map[string]cty.Value{
-				"id":  cty.UnknownVal(cty.String),
 				"key": cty.EmptyObjectVal,
 			}),
 			nil,
@@ -806,20 +825,18 @@ func TestAssertObjectCompatible(t *testing.T) {
 				BlockTypes: map[string]*configschema.NestedBlock{
 					"key": {
 						Nesting: configschema.NestingList,
-						Block:   configschema.Block{},
+						Block:   schemaWithFoo,
 					},
 				},
 			},
 			cty.ObjectVal(map[string]cty.Value{
-				"id": cty.UnknownVal(cty.String),
 				"key": cty.ListVal([]cty.Value{
-					cty.EmptyObjectVal,
+					fooBlockValue,
 				}),
 			}),
 			cty.ObjectVal(map[string]cty.Value{
-				"id": cty.UnknownVal(cty.String),
 				"key": cty.ListVal([]cty.Value{
-					cty.EmptyObjectVal,
+					fooBlockValue,
 				}),
 			}),
 			nil,
@@ -829,21 +846,19 @@ func TestAssertObjectCompatible(t *testing.T) {
 				BlockTypes: map[string]*configschema.NestedBlock{
 					"key": {
 						Nesting: configschema.NestingList,
-						Block:   configschema.Block{},
+						Block:   schemaWithFoo,
 					},
 				},
 			},
 			cty.ObjectVal(map[string]cty.Value{
-				"id": cty.UnknownVal(cty.String),
 				"key": cty.TupleVal([]cty.Value{
-					cty.EmptyObjectVal,
+					fooBlockValue,
-					cty.EmptyObjectVal,
+					fooBlockValue,
 				}),
 			}),
 			cty.ObjectVal(map[string]cty.Value{
-				"id": cty.UnknownVal(cty.String),
 				"key": cty.TupleVal([]cty.Value{
-					cty.EmptyObjectVal,
+					fooBlockValue,
 				}),
 			}),
 			[]string{
@@ -855,25 +870,77 @@ func TestAssertObjectCompatible(t *testing.T) {
 				BlockTypes: map[string]*configschema.NestedBlock{
 					"key": {
 						Nesting: configschema.NestingList,
-						Block:   configschema.Block{},
+						Block:   schemaWithFoo,
 					},
 				},
 			},
 			cty.ObjectVal(map[string]cty.Value{
-				"id":  cty.UnknownVal(cty.String),
 				"key": cty.TupleVal([]cty.Value{}),
 			}),
 			cty.ObjectVal(map[string]cty.Value{
-				"id": cty.UnknownVal(cty.String),
 				"key": cty.TupleVal([]cty.Value{
-					cty.EmptyObjectVal,
+					fooBlockValue,
-					cty.EmptyObjectVal,
+					fooBlockValue,
 				}),
 			}),
 			[]string{
 				`.key: block count changed from 0 to 2`,
 			},
 		},
+		{
+			&configschema.Block{
+				BlockTypes: map[string]*configschema.NestedBlock{
+					"key": {
+						Nesting: configschema.NestingList,
+						Block:   schemaWithFooBar,
+					},
+				},
+			},
+			cty.ObjectVal(map[string]cty.Value{
+				"key": cty.ListVal([]cty.Value{
+					fooBarBlockDynamicPlaceholder, // the presence of this disables some of our checks
+				}),
+			}),
+			cty.ObjectVal(map[string]cty.Value{
+				"key": cty.ListVal([]cty.Value{
+					cty.ObjectVal(map[string]cty.Value{
+						"foo": cty.StringVal("hello"),
+					}),
+					cty.ObjectVal(map[string]cty.Value{
+						"foo": cty.StringVal("world"),
+					}),
+				}),
+			}),
+			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
+		},
 
 		// NestingSet blocks
 		{
@@ -881,14 +948,7 @@ func TestAssertObjectCompatible(t *testing.T) {
 				BlockTypes: map[string]*configschema.NestedBlock{
 					"block": {
 						Nesting: configschema.NestingSet,
-						Block: configschema.Block{
+						Block:   schemaWithFoo,
-							Attributes: map[string]*configschema.Attribute{
-								"foo": {
-									Type:     cty.String,
-									Optional: true,
-								},
-							},
-						},
 					},
 				},
 			},
@@ -919,14 +979,7 @@ func TestAssertObjectCompatible(t *testing.T) {
 				BlockTypes: map[string]*configschema.NestedBlock{
 					"block": {
 						Nesting: configschema.NestingSet,
-						Block: configschema.Block{
+						Block:   schemaWithFoo,
-							Attributes: map[string]*configschema.Attribute{
-								"foo": {
-									Type:     cty.String,
-									Optional: true,
-								},
-							},
-						},
 					},
 				},
 			},
@@ -957,14 +1010,7 @@ func TestAssertObjectCompatible(t *testing.T) {
 				BlockTypes: map[string]*configschema.NestedBlock{
 					"block": {
 						Nesting: configschema.NestingSet,
-						Block: configschema.Block{
+						Block:   schemaWithFoo,
-							Attributes: map[string]*configschema.Attribute{
-								"foo": {
-									Type:     cty.String,
-									Optional: true,
-								},
-							},
-						},
 					},
 				},
 			},
@@ -995,14 +1041,7 @@ func TestAssertObjectCompatible(t *testing.T) {
 				BlockTypes: map[string]*configschema.NestedBlock{
 					"block": {
 						Nesting: configschema.NestingSet,
-						Block: configschema.Block{
+						Block:   schemaWithFoo,
-							Attributes: map[string]*configschema.Attribute{
-								"foo": {
-									Type:     cty.String,
-									Optional: true,
-								},
-							},
-						},
 					},
 				},
 			},
@@ -1038,14 +1077,7 @@ func TestAssertObjectCompatible(t *testing.T) {
 				BlockTypes: map[string]*configschema.NestedBlock{
 					"block": {
 						Nesting: configschema.NestingSet,
-						Block: configschema.Block{
+						Block:   schemaWithFoo,
-							Attributes: map[string]*configschema.Attribute{
-								"foo": {
-									Type:     cty.String,
-									Optional: true,
-								},
-							},
-						},
 					},
 				},
 			},
@@ -1082,14 +1114,7 @@ func TestAssertObjectCompatible(t *testing.T) {
 				BlockTypes: map[string]*configschema.NestedBlock{
 					"block": {
 						Nesting: configschema.NestingSet,
-						Block: configschema.Block{
+						Block:   schemaWithFoo,
-							Attributes: map[string]*configschema.Attribute{
-								"foo": {
-									Type:     cty.String,
-									Optional: true,
-								},
-							},
-						},
 					},
 				},
 			},
@@ -1112,8 +1137,8 @@ func TestAssertObjectCompatible(t *testing.T) {
 		},
 	}
 
-	for _, test := range tests {
+	for i, test := range tests {
-		t.Run(fmt.Sprintf("%#v and %#v", test.Planned, test.Actual), func(t *testing.T) {
+		t.Run(fmt.Sprintf("%02d: %#v and %#v", i, test.Planned, test.Actual), func(t *testing.T) {
 			errs := AssertObjectCompatible(test.Schema, test.Planned, test.Actual)
 
 			wantErrs := make(map[string]struct{})