From a87470cc1522ad53a996134516e0d755a13aea11 Mon Sep 17 00:00:00 2001
From: James Bardin <j.bardin@gmail.com>
Date: Tue, 4 Sep 2018 15:03:44 -0400
Subject: [PATCH] resource ids must always have a value

The "id" field is assumed to always exist, and must have a valid value.
Set "id" to unknown when planning new resource changes to indicate that
it will be computed.
---
 helper/schema/shims.go      | 10 +++++-
 helper/schema/shims_test.go | 71 +++++++++++++++++++++++++++++++++++--
 2 files changed, 77 insertions(+), 4 deletions(-)

diff --git a/helper/schema/shims.go b/helper/schema/shims.go
index ff11a77b3..2e6777e68 100644
--- a/helper/schema/shims.go
+++ b/helper/schema/shims.go
@@ -55,12 +55,20 @@ func ApplyDiff(state cty.Value, d *terraform.InstanceDiff, schemaBlock *configsc
 	if d.Destroy || d.DestroyDeposed || d.DestroyTainted {
 		// to mark a destroy, we remove all attributes
 		attrs = map[string]string{}
+	} else if attrs["id"] == "" || d.RequiresNew() {
+		// Since "id" is always computed, make sure it always has a value. Set
+		// it as unknown to generate the correct cty.Value
+		attrs["id"] = config.UnknownVariableValue
 	}
 
 	for attr, diff := range d.Attributes {
 		old, exists := attrs[attr]
 
-		if old != diff.Old && exists {
+		if exists &&
+			old != diff.Old &&
+			// if new or old is unknown, then there's no mismatch
+			old != config.UnknownVariableValue &&
+			diff.Old != config.UnknownVariableValue {
 			return state, fmt.Errorf("mismatched diff: %q != %q", old, diff.Old)
 		}
 
diff --git a/helper/schema/shims_test.go b/helper/schema/shims_test.go
index 9b596d622..2045b63df 100644
--- a/helper/schema/shims_test.go
+++ b/helper/schema/shims_test.go
@@ -32,10 +32,9 @@ func testApplyDiff(t *testing.T,
 	resource *Resource,
 	state, expected *terraform.InstanceState,
 	diff *terraform.InstanceDiff) {
-	t.Helper()
 
 	testSchema := providers.Schema{
-		Version: resource.SchemaVersion,
+		Version: uint64(resource.SchemaVersion),
 		Block:   resourceSchemaToBlock(resource.Schema),
 	}
 
@@ -49,6 +48,27 @@ func testApplyDiff(t *testing.T,
 		t.Fatal(err)
 	}
 
+	// verify that "id" is correct
+	id := newState.AsValueMap()["id"]
+
+	switch {
+	case diff.Destroy || diff.DestroyDeposed || diff.DestroyTainted:
+		// there should be no id
+		if !id.IsNull() {
+			t.Fatalf("destroyed instance should have no id: %#v", id)
+		}
+	default:
+		// the "id" field always exists and is computed, so it must have a
+		// valid value or be unknown.
+		if id.IsNull() {
+			t.Fatal("new instance state cannot have a null id")
+		}
+
+		if id.IsKnown() && id.AsString() == "" {
+			t.Fatal("new instance id cannot be an empty string")
+		}
+	}
+
 	// Resource.Meta will be hanlded separately, so it's OK that we lose the
 	// timeout values here.
 	expectedState, err := StateValueFromInstanceState(expected, testSchema.Block.ImpliedType())
@@ -61,6 +81,46 @@ func testApplyDiff(t *testing.T,
 	}
 }
 
+func TestShimResourcePlan_destroyCreate(t *testing.T) {
+	r := &Resource{
+		SchemaVersion: 2,
+		Schema: map[string]*Schema{
+			"foo": &Schema{
+				Type:     TypeInt,
+				Optional: true,
+				ForceNew: true,
+			},
+		},
+	}
+
+	d := &terraform.InstanceDiff{
+		Attributes: map[string]*terraform.ResourceAttrDiff{
+			"foo": &terraform.ResourceAttrDiff{
+				RequiresNew: true,
+				Old:         "3",
+				New:         "42",
+			},
+		},
+	}
+
+	state := &terraform.InstanceState{
+		Attributes: map[string]string{"foo": "3"},
+	}
+
+	expected := &terraform.InstanceState{
+		ID: config.UnknownVariableValue,
+		Attributes: map[string]string{
+			"id":  config.UnknownVariableValue,
+			"foo": "42",
+		},
+		Meta: map[string]interface{}{
+			"schema_version": "2",
+		},
+	}
+
+	testApplyDiff(t, r, state, expected, d)
+}
+
 func TestShimResourceApply_create(t *testing.T) {
 	r := &Resource{
 		SchemaVersion: 2,
@@ -279,7 +339,7 @@ func TestShimResourceDiff_Timeout_diff(t *testing.T) {
 	}
 
 	testSchema := providers.Schema{
-		Version: r.SchemaVersion,
+		Version: uint64(r.SchemaVersion),
 		Block:   resourceSchemaToBlock(r.Schema),
 	}
 
@@ -351,6 +411,7 @@ func TestShimResourceApply_destroyCreate(t *testing.T) {
 			"foo": &Schema{
 				Type:     TypeInt,
 				Optional: true,
+				ForceNew: true,
 			},
 
 			"tags": &Schema{
@@ -381,6 +442,9 @@ func TestShimResourceApply_destroyCreate(t *testing.T) {
 
 	d := &terraform.InstanceDiff{
 		Attributes: map[string]*terraform.ResourceAttrDiff{
+			"id": &terraform.ResourceAttrDiff{
+				New: "foo",
+			},
 			"foo": &terraform.ResourceAttrDiff{
 				Old:         "7",
 				New:         "42",
@@ -424,6 +488,7 @@ func TestShimResourceApply_destroyCreate(t *testing.T) {
 	createdState := &terraform.InstanceState{
 		ID: "foo",
 		Attributes: map[string]string{
+			"id":        "foo",
 			"foo":       "7",
 			"tags.%":    "1",
 			"tags.Name": "foo",