Merge pull request #20874 from hashicorp/jbardin/shim-unknown-values

Better handling of unknowns in sdk shims

builtin/providers/test/resource.go

@@ -18,6 +18,13 @@ func testResource() *schema.Resource {
 			State: schema.ImportStatePassthrough,
 		},
 
+		CustomizeDiff: func(d *schema.ResourceDiff, _ interface{}) error {
+			if d.HasChange("required") {
+				d.SetNewComputed("planned_computed")
+			}
+			return nil
+		},
+
 		Schema: map[string]*schema.Schema{
 			"required": {
 				Type:     schema.TypeString,
@@ -129,6 +136,11 @@ func testResource() *schema.Resource {
 				Optional:    true,
 				Description: "return and error during apply",
 			},
+			"planned_computed": {
+				Type:        schema.TypeString,
+				Computed:    true,
+				Description: "copied the required field during apply, and plans computed when changed",
+			},
 		},
 	}
 }
@@ -164,6 +176,8 @@ func testResourceRead(d *schema.ResourceData, meta interface{}) error {
 	d.Set("computed_list", []string{"listval1", "listval2"})
 	d.Set("computed_set", []string{"setval1", "setval2"})
 
+	d.Set("planned_computed", d.Get("required"))
+
 	// if there is no "set" value, erroneously set it to an empty set. This
 	// might change a null value to an empty set, but we should be able to
 	// ignore that.

builtin/providers/test/resource_list.go

@@ -11,6 +11,13 @@ func testResourceList() *schema.Resource {
 		Update: testResourceListUpdate,
 		Delete: testResourceListDelete,
 
+		CustomizeDiff: func(d *schema.ResourceDiff, _ interface{}) error {
+			if d.HasChange("dependent_list") {
+				d.SetNewComputed("computed_list")
+			}
+			return nil
+		},
+
 		Schema: map[string]*schema.Schema{
 			"list_block": {
 				Type:     schema.TypeList,
@@ -55,6 +62,19 @@ func testResourceList() *schema.Resource {
 								},
 							},
 						},
+						"sublist_block_optional": {
+							Type:     schema.TypeList,
+							Optional: true,
+							Elem: &schema.Resource{
+								Schema: map[string]*schema.Schema{
+									"list": {
+										Type:     schema.TypeList,
+										Optional: true,
+										Elem:     &schema.Schema{Type: schema.TypeString},
+									},
+								},
+							},
+						},
 					},
 				},
 			},

builtin/providers/test/resource_list_test.go

@@ -318,3 +318,132 @@ resource "test_resource_list" "foo" {
 		},
 	})
 }
+
+func TestResourceList_planUnknownInterpolation(t *testing.T) {
+	resource.UnitTest(t, resource.TestCase{
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckResourceDestroy,
+		Steps: []resource.TestStep{
+			resource.TestStep{
+				Config: strings.TrimSpace(`
+resource "test_resource_list" "foo" {
+	list_block {
+		string = "x"
+	}
+}
+resource "test_resource_list" "bar" {
+	list_block {
+		sublist = [
+			test_resource_list.foo.list_block[0].string,
+		]
+	}
+}
+				`),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr(
+						"test_resource_list.bar", "list_block.0.sublist.0", "x",
+					),
+				),
+			},
+			resource.TestStep{
+				Config: strings.TrimSpace(`
+resource "test_resource_list" "foo" {
+	list_block {
+		string = "x"
+	}
+	dependent_list {
+		val = "y"
+	}
+}
+resource "test_resource_list" "bar" {
+	list_block {
+		sublist = [
+			test_resource_list.foo.computed_list[0],
+		]
+	}
+}
+				`),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr(
+						"test_resource_list.bar", "list_block.0.sublist.0", "y",
+					),
+				),
+			},
+			resource.TestStep{
+				Config: strings.TrimSpace(`
+resource "test_resource_list" "foo" {
+	list_block {
+		string = "x"
+	}
+	dependent_list {
+		val = "z"
+	}
+}
+resource "test_resource_list" "bar" {
+	list_block {
+		sublist = [
+			test_resource_list.foo.computed_list[0],
+		]
+	}
+}
+				`),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr(
+						"test_resource_list.bar", "list_block.0.sublist.0", "z",
+					),
+				),
+			},
+		},
+	})
+}
+
+func TestResourceList_planUnknownInterpolationList(t *testing.T) {
+	resource.UnitTest(t, resource.TestCase{
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckResourceDestroy,
+		Steps: []resource.TestStep{
+			resource.TestStep{
+				Config: strings.TrimSpace(`
+resource "test_resource_list" "foo" {
+	dependent_list {
+		val = "y"
+	}
+}
+resource "test_resource_list" "bar" {
+	list_block {
+		sublist_block_optional {
+			list = test_resource_list.foo.computed_list
+		}
+	}
+}
+				`),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr(
+						"test_resource_list.bar", "list_block.0.sublist_block_optional.0.list.0", "y",
+					),
+				),
+			},
+			resource.TestStep{
+				Config: strings.TrimSpace(`
+resource "test_resource_list" "foo" {
+	dependent_list {
+		val = "z"
+	}
+}
+resource "test_resource_list" "bar" {
+	list_block {
+		sublist_block_optional {
+			list = test_resource_list.foo.computed_list
+		}
+	}
+}
+				`),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr(
+						"test_resource_list.bar", "list_block.0.sublist_block_optional.0.list.0", "z",
+					),
+				),
+			},
+		},
+	})
+}

builtin/providers/test/resource_test.go

@@ -790,3 +790,129 @@ resource "test_resource" "foo" {
 		},
 	})
 }
+
+func TestResource_optionalComputedMap(t *testing.T) {
+	resource.UnitTest(t, resource.TestCase{
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckResourceDestroy,
+		Steps: []resource.TestStep{
+			resource.TestStep{
+				Config: strings.TrimSpace(`
+resource "test_resource" "foo" {
+	required           = "yep"
+	required_map = {
+	  key = "value"
+	}
+	optional_computed_map = {
+		foo = "bar"
+		baz = ""
+	}
+}
+				`),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr(
+						"test_resource.foo", "optional_computed_map.foo", "bar",
+					),
+					resource.TestCheckResourceAttr(
+						"test_resource.foo", "optional_computed_map.baz", "",
+					),
+				),
+			},
+			resource.TestStep{
+				Config: strings.TrimSpace(`
+resource "test_resource" "foo" {
+	required           = "yep"
+	required_map = {
+	  key = "value"
+	}
+	optional_computed_map = {}
+}
+				`),
+				// removing the map from the config should still leave an empty computed map
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr(
+						"test_resource.foo", "optional_computed_map.%", "0",
+					),
+				),
+			},
+		},
+	})
+}
+
+func TestResource_plannedComputed(t *testing.T) {
+	resource.UnitTest(t, resource.TestCase{
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckResourceDestroy,
+		Steps: []resource.TestStep{
+			resource.TestStep{
+				Config: strings.TrimSpace(`
+resource "test_resource" "foo" {
+	required           = "ok"
+	required_map = {
+	  key = "value"
+	}
+}
+				`),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr(
+						"test_resource.foo", "planned_computed", "ok",
+					),
+				),
+			},
+			resource.TestStep{
+				Config: strings.TrimSpace(`
+resource "test_resource" "foo" {
+	required           = "changed"
+	required_map = {
+	  key = "value"
+	}
+}
+				`),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr(
+						"test_resource.foo", "planned_computed", "changed",
+					),
+				),
+			},
+		},
+	})
+}
+
+func TestDiffApply_map(t *testing.T) {
+	resSchema := map[string]*schema.Schema{
+		"map": {
+			Type:     schema.TypeMap,
+			Optional: true,
+			Computed: true,
+			Elem:     &schema.Schema{Type: schema.TypeString},
+		},
+	}
+
+	priorAttrs := map[string]string{
+		"id":      "ok",
+		"map.%":   "2",
+		"map.foo": "bar",
+		"map.bar": "",
+	}
+
+	diff := &terraform.InstanceDiff{
+		Attributes: map[string]*terraform.ResourceAttrDiff{
+			"map.foo": &terraform.ResourceAttrDiff{Old: "bar", New: "", NewRemoved: true},
+			"map.bar": &terraform.ResourceAttrDiff{Old: "", New: "", NewRemoved: true},
+		},
+	}
+
+	newAttrs, err := diff.Apply(priorAttrs, (&schema.Resource{Schema: resSchema}).CoreConfigSchema())
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	expect := map[string]string{
+		"id":    "ok",
+		"map.%": "0",
+	}
+
+	if !reflect.DeepEqual(newAttrs, expect) {
+		t.Fatalf("expected:%#v got:%#v", expect, newAttrs)
+	}
+}

helper/plugin/grpc_provider.go

@@ -1144,6 +1144,14 @@ func normalizeNullValues(dst, src cty.Value, preferDst bool) cty.Value {
 	ty := dst.Type()
 
 	if !src.IsNull() && !src.IsKnown() {
+		// While this seems backwards to return src when preferDst is set, it
+		// means this might be a plan scenario, and it must retain unknown
+		// interpolated placeholders, which could be lost if we're only updating
+		// a resource. If this is a read scenario, then there shouldn't be any
+		// unknowns all.
+		if dst.IsNull() && preferDst {
+			return src
+		}
 		return dst
 	}
 
@@ -1176,11 +1184,8 @@ func normalizeNullValues(dst, src cty.Value, preferDst bool) cty.Value {
 			dstMap = map[string]cty.Value{}
 		}
 
-		ei := src.ElementIterator()
+		srcMap := src.AsValueMap()
-		for ei.Next() {
+		for key, v := range srcMap {
-			k, v := ei.Element()
-			key := k.AsString()
-
 			dstVal := dstMap[key]
 			if dstVal == cty.NilVal {
 				if preferDst && ty.IsMapType() {
@@ -1203,6 +1208,24 @@ func normalizeNullValues(dst, src cty.Value, preferDst bool) cty.Value {
 		}
 
 		if ty.IsMapType() {
+			// helper/schema will populate an optional+computed map with
+			// unknowns which we have to fixup here.
+			// It would be preferable to simply prevent any known value from
+			// becoming unknown, but concessions have to be made to retain the
+			// broken legacy behavior when possible.
+			for k, srcVal := range srcMap {
+				if !srcVal.IsNull() && srcVal.IsKnown() {
+					dstVal, ok := dstMap[k]
+					if !ok {
+						continue
+					}
+
+					if !dstVal.IsNull() && !dstVal.IsKnown() {
+						dstMap[k] = srcVal
+					}
+				}
+			}
+
 			return cty.MapVal(dstMap)
 		}
 
@@ -1217,12 +1240,29 @@ func normalizeNullValues(dst, src cty.Value, preferDst bool) cty.Value {
 		}
 
 	case ty.IsListType(), ty.IsTupleType():
-		// If the dst is nil, and the src is known, then we lost an empty value
+		// If the dst is null, and the src is known, then we lost an empty value
 		// so take the original.
 		if dst.IsNull() {
 			if src.IsWhollyKnown() && src.LengthInt() == 0 && !preferDst {
 				return src
 			}
+
+			// if dst is null and src only contains unknown values, then we lost
+			// those during a plan (which is when preferDst is set, there would
+			// be no unknowns during read).
+			if preferDst && !src.IsNull() {
+				allUnknown := true
+				for _, v := range src.AsValueSlice() {
+					if v.IsKnown() {
+						allUnknown = false
+						break
+					}
+				}
+				if allUnknown {
+					return src
+				}
+			}
+
 			return dst
 		}
 

helper/plugin/grpc_provider_test.go

@@ -909,7 +909,7 @@ func TestNormalizeNullValues(t *testing.T) {
 				}),
 			}),
 		},
-		// the empty list should be transferred, but the new unknown show not be overridden
+		// the empty list should be transferred, but the new unknown should not be overridden
 		{
 			Src: cty.ObjectVal(map[string]cty.Value{
 				"network_interface": cty.ListVal([]cty.Value{
@@ -942,6 +942,65 @@ func TestNormalizeNullValues(t *testing.T) {
 			}),
 			Plan: true,
 		},
+		{
+			// fix unknowns added to a map
+			Src: cty.ObjectVal(map[string]cty.Value{
+				"map": cty.MapVal(map[string]cty.Value{
+					"a": cty.StringVal("a"),
+					"b": cty.StringVal(""),
+				}),
+			}),
+			Dst: cty.ObjectVal(map[string]cty.Value{
+				"map": cty.MapVal(map[string]cty.Value{
+					"a": cty.StringVal("a"),
+					"b": cty.UnknownVal(cty.String),
+				}),
+			}),
+			Expect: cty.ObjectVal(map[string]cty.Value{
+				"map": cty.MapVal(map[string]cty.Value{
+					"a": cty.StringVal("a"),
+					"b": cty.StringVal(""),
+				}),
+			}),
+			Plan: true,
+		},
+		{
+			// fix unknowns lost from a list
+			Src: cty.ObjectVal(map[string]cty.Value{
+				"top": cty.ListVal([]cty.Value{
+					cty.ObjectVal(map[string]cty.Value{
+						"list": cty.ListVal([]cty.Value{
+							cty.ObjectVal(map[string]cty.Value{
+								"values": cty.ListVal([]cty.Value{cty.UnknownVal(cty.String)}),
+							}),
+						}),
+					}),
+				}),
+			}),
+			Dst: cty.ObjectVal(map[string]cty.Value{
+				"top": cty.ListVal([]cty.Value{
+					cty.ObjectVal(map[string]cty.Value{
+						"list": cty.ListVal([]cty.Value{
+							cty.ObjectVal(map[string]cty.Value{
+								"values": cty.NullVal(cty.List(cty.String)),
+							}),
+						}),
+					}),
+				}),
+			}),
+			Expect: cty.ObjectVal(map[string]cty.Value{
+				"top": cty.ListVal([]cty.Value{
+					cty.ObjectVal(map[string]cty.Value{
+						"list": cty.ListVal([]cty.Value{
+							cty.ObjectVal(map[string]cty.Value{
+								"values": cty.ListVal([]cty.Value{cty.UnknownVal(cty.String)}),
+							}),
+						}),
+					}),
+				}),
+			}),
+			Plan: true,
+		},
 	} {
 		t.Run(fmt.Sprintf("%d", i), func(t *testing.T) {
 			got := normalizeNullValues(tc.Dst, tc.Src, tc.Plan)

terraform/diff.go

@@ -699,14 +699,6 @@ func (d *InstanceDiff) applySingleAttrDiff(path []string, attrs map[string]strin
 		return result, nil
 	}
 
-	if diff.Old == diff.New && diff.New == "" {
-		// this can only be a valid empty string
-		if attrSchema.Type == cty.String {
-			result[attr] = ""
-		}
-		return result, nil
-	}
-
 	// check for missmatched diff values
 	if exists &&
 		old != diff.Old &&
@@ -715,13 +707,21 @@ func (d *InstanceDiff) applySingleAttrDiff(path []string, attrs map[string]strin
 		return result, fmt.Errorf("diff apply conflict for %s: diff expects %q, but prior value has %q", attr, diff.Old, old)
 	}
 
-	if attrSchema.Computed && diff.NewComputed {
+	if diff.NewRemoved {
-		result[attr] = config.UnknownVariableValue
+		// don't set anything in the new value
+		return map[string]string{}, nil
+	}
+
+	if diff.Old == diff.New && diff.New == "" {
+		// this can only be a valid empty string
+		if attrSchema.Type == cty.String {
+			result[attr] = ""
+		}
 		return result, nil
 	}
 
-	if diff.NewRemoved {
+	if attrSchema.Computed && diff.NewComputed {
-		// don't set anything in the new value
+		result[attr] = config.UnknownVariableValue
 		return result, nil
 	}
 
@@ -863,8 +863,10 @@ func (d *InstanceDiff) applyCollectionDiff(path []string, attrs map[string]strin
 		}
 	}
 
-	// Fill in the count value if it was missing for some reason:
+	// Fill in the count value if it wasn't present in the diff for some reason,
-	if result[countKey] == "" {
+	// or if there is no count at all.
+	_, countDiff := d.Attributes[countKey]
+	if result[countKey] == "" || (!countDiff && len(keys) != len(result)) {
 		result[countKey] = countFlatmapContainerValues(countKey, result)
 	}