Merge pull request #20977 from hashicorp/jbardin/set-elem-get-ok

Don't allow unknown values in the config when generating the apply Diff

builtin/providers/test/resource_state_func.go

@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"math/rand"
 
+	"github.com/hashicorp/terraform/helper/hashcode"
 	"github.com/hashicorp/terraform/helper/schema"
 )
 
@@ -35,6 +36,26 @@ func testResourceStateFunc() *schema.Resource {
 				Type:     schema.TypeString,
 				Optional: true,
 			},
+
+			// set block with computed elements
+			"set_block": {
+				Type:     schema.TypeSet,
+				Optional: true,
+				Set:      setBlockHash,
+				Elem: &schema.Resource{
+					Schema: map[string]*schema.Schema{
+						"required": {
+							Type:     schema.TypeString,
+							Required: true,
+						},
+						"optional": {
+							Type:     schema.TypeString,
+							Optional: true,
+							Computed: true,
+						},
+					},
+				},
+			},
 		},
 	}
 }
@@ -44,6 +65,13 @@ func stateFuncHash(v interface{}) string {
 	return hex.EncodeToString(hash[:])
 }
 
+func setBlockHash(v interface{}) int {
+	m := v.(map[string]interface{})
+	required, _ := m["required"].(string)
+	optional, _ := m["optional"].(string)
+	return hashcode.String(fmt.Sprintf("%s|%s", required, optional))
+}
+
 func testResourceStateFuncCreate(d *schema.ResourceData, meta interface{}) error {
 	d.SetId(fmt.Sprintf("%x", rand.Int63()))
 
@@ -57,6 +85,22 @@ func testResourceStateFuncCreate(d *schema.ResourceData, meta interface{}) error
 		}
 	}
 
+	// Check that we can lookup set elements by our computed hash.
+	// This is not advised, but we can use this to make sure the final diff was
+	// prepared with the correct values.
+	setBlock, ok := d.GetOk("set_block")
+	if ok {
+		set := setBlock.(*schema.Set)
+		for _, obj := range set.List() {
+			idx := setBlockHash(obj)
+			requiredAddr := fmt.Sprintf("%s.%d.%s", "set_block", idx, "required")
+			_, ok := d.GetOkExists(requiredAddr)
+			if !ok {
+				return fmt.Errorf("failed to get attr %q from %#v", fmt.Sprintf(requiredAddr), d.State().Attributes)
+			}
+		}
+	}
+
 	return testResourceStateFuncRead(d, meta)
 }
 

builtin/providers/test/resource_state_func_test.go

@@ -58,3 +58,28 @@ resource "test_resource_state_func" "foo" {
 		},
 	})
 }
+
+func TestResourceStateFunc_getOkSetElem(t *testing.T) {
+	resource.UnitTest(t, resource.TestCase{
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckResourceDestroy,
+		Steps: []resource.TestStep{
+			resource.TestStep{
+				Config: strings.TrimSpace(`
+resource "test_resource_state_func" "foo" {
+}
+
+resource "test_resource_state_func" "bar" {
+	set_block {
+		required = "foo"
+		optional = test_resource_state_func.foo.id
+	}
+	set_block {
+		required = test_resource_state_func.foo.id
+	}
+}
+				`),
+			},
+		},
+	})
+}

helper/plugin/grpc_provider.go

@@ -5,7 +5,6 @@ import (
 	"errors"
 	"fmt"
 	"strconv"
-	"strings"
 
 	"github.com/zclconf/go-cty/cty"
 	ctyconvert "github.com/zclconf/go-cty/cty/convert"
@@ -777,17 +776,6 @@ func (s *GRPCProviderServer) ApplyResourceChange(_ context.Context, req *proto.A
 		}
 	}
 
-	// We need to fix any sets that may be using the "~" index prefix to
-	// indicate partially computed. The special sigil isn't really used except
-	// as a clue to visually indicate that the set isn't wholly known.
-	for k, d := range diff.Attributes {
-		if strings.Contains(k, ".~") {
-			delete(diff.Attributes, k)
-			k = strings.Replace(k, ".~", ".", -1)
-			diff.Attributes[k] = d
-		}
-	}
-
 	// add NewExtra Fields that may have been stored in the private data
 	if newExtra := private[newExtraKey]; newExtra != nil {
 		for k, v := range newExtra.(map[string]interface{}) {
@@ -806,16 +794,14 @@ func (s *GRPCProviderServer) ApplyResourceChange(_ context.Context, req *proto.A
 		diff.Meta = private
 	}
 
-	// We need to turn off any RequiresNew. There could be attributes
-	// without changes in here inserted by helper/schema, but if they have
-	// RequiresNew then the state will will be dropped from the ResourceData.
-	for k := range diff.Attributes {
-		diff.Attributes[k].RequiresNew = false
-	}
-
-	// check that any "removed" attributes actually exist in the prior state, or
-	// helper/schema will confuse itself
 	for k, d := range diff.Attributes {
+		// We need to turn off any RequiresNew. There could be attributes
+		// without changes in here inserted by helper/schema, but if they have
+		// RequiresNew then the state will be dropped from the ResourceData.
+		d.RequiresNew = false
+
+		// Check that any "removed" attributes that don't actually exist in the
+		// prior state, or helper/schema will confuse itself
 		if d.NewRemoved {
 			if _, ok := priorState.Attributes[k]; !ok {
 				delete(diff.Attributes, k)
@@ -845,9 +831,6 @@ func (s *GRPCProviderServer) ApplyResourceChange(_ context.Context, req *proto.A
 		return resp, nil
 	}
 
-	// here we use the planned state to check for unknown/zero containers values
-	// when normalizing the flatmap.
-
 	// We keep the null val if we destroyed the resource, otherwise build the
 	// entire object, even if the new state was nil.
 	newStateVal, err = schema.StateValueFromInstanceState(newInstanceState, blockForShimming.ImpliedType())

helper/schema/shims.go

@@ -6,6 +6,7 @@ import (
 	"github.com/zclconf/go-cty/cty"
 	ctyjson "github.com/zclconf/go-cty/cty/json"
 
+	"github.com/hashicorp/terraform/config"
 	"github.com/hashicorp/terraform/configs/configschema"
 	"github.com/hashicorp/terraform/terraform"
 )
@@ -31,6 +32,8 @@ func diffFromValues(prior, planned cty.Value, res *Resource, cust CustomizeDiffF
 	configSchema := res.CoreConfigSchema()
 
 	cfg := terraform.NewResourceConfigShimmed(planned, configSchema)
+	removeConfigUnknowns(cfg.Config)
+	removeConfigUnknowns(cfg.Raw)
 
 	diff, err := schemaMap(res.Schema).Diff(instanceState, cfg, cust, nil, false)
 	if err != nil {
@@ -40,6 +43,28 @@ func diffFromValues(prior, planned cty.Value, res *Resource, cust CustomizeDiffF
 	return diff, err
 }
 
+// During apply the only unknown values are those which are to be computed by
+// the resource itself. These may have been marked as unknown config values, and
+// need to be removed to prevent the UnknownVariableValue from appearing the diff.
+func removeConfigUnknowns(cfg map[string]interface{}) {
+	for k, v := range cfg {
+		switch v := v.(type) {
+		case string:
+			if v == config.UnknownVariableValue {
+				cfg[k] = ""
+			}
+		case []interface{}:
+			for _, i := range v {
+				if m, ok := i.(map[string]interface{}); ok {
+					removeConfigUnknowns(m)
+				}
+			}
+		case map[string]interface{}:
+			removeConfigUnknowns(v)
+		}
+	}
+}
+
 // ApplyDiff takes a cty.Value state and applies a terraform.InstanceDiff to
 // get a new cty.Value state. This is used to convert the diff returned from
 // the legacy provider Diff method to the state required for the new

helper/schema/shims_test.go

@@ -3590,6 +3590,14 @@ func TestShimSchemaMap_Diff(t *testing.T) {
 				}
 			}
 
+			// there would be no unknown config variables during apply, so
+			// return early here.
+			for _, v := range tc.ConfigVariables {
+				if s, ok := v.Value.(string); ok && s == config.UnknownVariableValue {
+					return
+				}
+			}
+
 			// our diff function can't set DestroyTainted, but match the
 			// expected value here for the test fixtures
 			if tainted {
@@ -3610,3 +3618,55 @@ func TestShimSchemaMap_Diff(t *testing.T) {
 func resourceSchemaToBlock(s map[string]*Schema) *configschema.Block {
 	return (&Resource{Schema: s}).CoreConfigSchema()
 }
+
+func TestRemoveConfigUnknowns(t *testing.T) {
+	cfg := map[string]interface{}{
+		"id": "74D93920-ED26-11E3-AC10-0800200C9A66",
+		"route_rules": []interface{}{
+			map[string]interface{}{
+				"cidr_block":        "74D93920-ED26-11E3-AC10-0800200C9A66",
+				"destination":       "0.0.0.0/0",
+				"destination_type":  "CIDR_BLOCK",
+				"network_entity_id": "1",
+			},
+			map[string]interface{}{
+				"cidr_block":       "74D93920-ED26-11E3-AC10-0800200C9A66",
+				"destination":      "0.0.0.0/0",
+				"destination_type": "CIDR_BLOCK",
+				"sub_block": []interface{}{
+					map[string]interface{}{
+						"computed": "74D93920-ED26-11E3-AC10-0800200C9A66",
+					},
+				},
+			},
+		},
+	}
+
+	expect := map[string]interface{}{
+		"id": "",
+		"route_rules": []interface{}{
+			map[string]interface{}{
+				"cidr_block":        "",
+				"destination":       "0.0.0.0/0",
+				"destination_type":  "CIDR_BLOCK",
+				"network_entity_id": "1",
+			},
+			map[string]interface{}{
+				"cidr_block":       "",
+				"destination":      "0.0.0.0/0",
+				"destination_type": "CIDR_BLOCK",
+				"sub_block": []interface{}{
+					map[string]interface{}{
+						"computed": "",
+					},
+				},
+			},
+		},
+	}
+
+	removeConfigUnknowns(cfg)
+
+	if !reflect.DeepEqual(cfg, expect) {
+		t.Fatalf("\nexpected: %#v\ngot: %#v", expect, cfg)
+	}
+}