consul: Fix several problems w/ consul_keys update

Implementation notes:

 * The hash implementation was not considering key value, causing "diffs
   did not match" errors when a value was updated. Switching to default
   HashResource implementation fixes this
 * Using HashResource as a default exposed a bug in helper/schema that
   needed to be fixed so the Set function is picked up properly during
   Read
 * Stop writing back values into the `key` attribute; it triggers extra
   diffs when `default` is used. Computed values all just go into `var`.
 * Includes a state migration to prevent unnecessary diffs based on
   "key" attribute hashcodes changing.

In the tests:

 * Switch from leaning on the public demo Consul instance to requiring a
   CONSUL_HTTP_ADDR variable be set pointing to a `consul agent -dev`
   instance to be used only for testing.
 * Add a test that exposes the updating issues and covers the fixes

Fixes #774
Fixes #1866
Fixes #3023

builtin/providers/consul/resource_consul_keys.go

@@ -1,13 +1,11 @@
 package consul
 
 import (
-	"bytes"
 	"fmt"
 	"log"
 	"strconv"
 
 	consulapi "github.com/hashicorp/consul/api"
-	"github.com/hashicorp/terraform/helper/hashcode"
 	"github.com/hashicorp/terraform/helper/schema"
 )
 
@@ -18,6 +16,9 @@ func resourceConsulKeys() *schema.Resource {
 		Read:   resourceConsulKeysRead,
 		Delete: resourceConsulKeysDelete,
 
+		SchemaVersion: 1,
+		MigrateState:  resourceConsulKeysMigrateState,
+
 		Schema: map[string]*schema.Schema{
 			"datacenter": &schema.Schema{
 				Type:     schema.TypeString,
@@ -64,7 +65,6 @@ func resourceConsulKeys() *schema.Resource {
 						},
 					},
 				},
-				Set: resourceConsulKeysHash,
 			},
 
 			"var": &schema.Schema{
@@ -75,36 +75,14 @@ func resourceConsulKeys() *schema.Resource {
 	}
 }
 
-func resourceConsulKeysHash(v interface{}) int {
-	var buf bytes.Buffer
-	m := v.(map[string]interface{})
-	buf.WriteString(fmt.Sprintf("%s-", m["name"].(string)))
-	buf.WriteString(fmt.Sprintf("%s-", m["path"].(string)))
-	return hashcode.String(buf.String())
-}
-
 func resourceConsulKeysCreate(d *schema.ResourceData, meta interface{}) error {
 	client := meta.(*consulapi.Client)
 	kv := client.KV()
-
+	token := d.Get("token").(string)
-	// Resolve the datacenter first, all the other keys are dependent
+	dc, err := getDC(d, client)
-	// on this.
-	var dc string
-	if v, ok := d.GetOk("datacenter"); ok {
-		dc = v.(string)
-		log.Printf("[DEBUG] Consul datacenter: %s", dc)
-	} else {
-		log.Printf("[DEBUG] Resolving Consul datacenter...")
-		var err error
-		dc, err = getDC(client)
 	if err != nil {
 		return err
 	}
-	}
-	var token string
-	if v, ok := d.GetOk("token"); ok {
-		token = v.(string)
-	}
 
 	// Setup the operations using the datacenter
 	qOpts := consulapi.QueryOptions{Datacenter: dc, Token: token}
@@ -129,8 +107,6 @@ func resourceConsulKeysCreate(d *schema.ResourceData, meta interface{}) error {
 				return fmt.Errorf("Failed to set Consul key '%s': %v", path, err)
 			}
 			vars[key] = value
-			sub["value"] = value
-
 		} else {
 			log.Printf("[DEBUG] Getting key '%s' in %s", path, dc)
 			pair, _, err := kv.Get(path, &qOpts)
@@ -142,29 +118,29 @@ func resourceConsulKeysCreate(d *schema.ResourceData, meta interface{}) error {
 		}
 	}
 
-	// Update the resource
+	// The ID doesn't matter, since we use provider config, datacenter,
+	// and key paths to address consul properly. So we just need to fill it in
+	// with some value to indicate the resource has been created.
 	d.SetId("consul")
+
+	// Set the vars we collected above
+	if err := d.Set("var", vars); err != nil {
+		return err
+	}
+	// Store the datacenter on this resource, which can be helpful for reference
+	// in case it was read from the provider
 	d.Set("datacenter", dc)
-	d.Set("key", keys)
+
-	d.Set("var", vars)
 	return nil
 }
 
 func resourceConsulKeysRead(d *schema.ResourceData, meta interface{}) error {
 	client := meta.(*consulapi.Client)
 	kv := client.KV()
-
+	token := d.Get("token").(string)
-	// Get the DC, error if not available.
+	dc, err := getDC(d, client)
-	var dc string
+	if err != nil {
-	if v, ok := d.GetOk("datacenter"); ok {
+		return err
-		dc = v.(string)
-		log.Printf("[DEBUG] Consul datacenter: %s", dc)
-	} else {
-		return fmt.Errorf("Missing datacenter configuration")
-	}
-	var token string
-	if v, ok := d.GetOk("token"); ok {
-		token = v.(string)
 	}
 
 	// Setup the operations using the datacenter
@@ -189,30 +165,26 @@ func resourceConsulKeysRead(d *schema.ResourceData, meta interface{}) error {
 
 		value := attributeValue(sub, key, pair)
 		vars[key] = value
-		sub["value"] = value
 	}
 
 	// Update the resource
-	d.Set("key", keys)
+	if err := d.Set("var", vars); err != nil {
-	d.Set("var", vars)
+		return err
+	}
+	// Store the datacenter on this resource, which can be helpful for reference
+	// in case it was read from the provider
+	d.Set("datacenter", dc)
+
 	return nil
 }
 
 func resourceConsulKeysDelete(d *schema.ResourceData, meta interface{}) error {
 	client := meta.(*consulapi.Client)
 	kv := client.KV()
-
+	token := d.Get("token").(string)
-	// Get the DC, error if not available.
+	dc, err := getDC(d, client)
-	var dc string
+	if err != nil {
-	if v, ok := d.GetOk("datacenter"); ok {
+		return err
-		dc = v.(string)
-		log.Printf("[DEBUG] Consul datacenter: %s", dc)
-	} else {
-		return fmt.Errorf("Missing datacenter configuration")
-	}
-	var token string
-	if v, ok := d.GetOk("token"); ok {
-		token = v.(string)
 	}
 
 	// Setup the operations using the datacenter
@@ -285,11 +257,13 @@ func attributeValue(sub map[string]interface{}, key string, pair *consulapi.KVPa
 }
 
 // getDC is used to get the datacenter of the local agent
-func getDC(client *consulapi.Client) (string, error) {
+func getDC(d *schema.ResourceData, client *consulapi.Client) (string, error) {
+	if v, ok := d.GetOk("datacenter"); ok {
+		return v.(string), nil
+	}
 	info, err := client.Agent().Self()
 	if err != nil {
 		return "", fmt.Errorf("Failed to get datacenter from Consul agent: %v", err)
 	}
-	dc := info["Config"]["Datacenter"].(string)
+	return info["Config"]["Datacenter"].(string), nil
-	return dc, nil
 }

builtin/providers/consul/resource_consul_keys_migrate.go

@@ -0,0 +1,92 @@
+package consul
+
+import (
+	"fmt"
+	"log"
+	"strings"
+
+	"github.com/hashicorp/terraform/helper/schema"
+	"github.com/hashicorp/terraform/terraform"
+)
+
+func resourceConsulKeysMigrateState(
+	v int, is *terraform.InstanceState, meta interface{}) (*terraform.InstanceState, error) {
+	switch v {
+	case 0:
+		log.Println("[INFO] Found consul_keys State v0; migrating to v1")
+		return resourceConsulKeysMigrateStateV0toV1(is)
+	default:
+		return is, fmt.Errorf("Unexpected schema version: %d", v)
+	}
+}
+
+func resourceConsulKeysMigrateStateV0toV1(is *terraform.InstanceState) (*terraform.InstanceState, error) {
+	if is.Empty() || is.Attributes == nil {
+		log.Println("[DEBUG] Empty InstanceState; nothing to migrate.")
+		return is, nil
+	}
+
+	log.Printf("[DEBUG] Attributes before migration: %#v", is.Attributes)
+
+	res := resourceConsulKeys()
+	keys, err := readV0Keys(is, res)
+	if err != nil {
+		return is, err
+	}
+	if err := clearV0Keys(is); err != nil {
+		return is, err
+	}
+	if err := writeV1Keys(is, res, keys); err != nil {
+		return is, err
+	}
+
+	log.Printf("[DEBUG] Attributes after migration: %#v", is.Attributes)
+	return is, nil
+}
+
+func readV0Keys(
+	is *terraform.InstanceState,
+	res *schema.Resource,
+) (*schema.Set, error) {
+	reader := &schema.MapFieldReader{
+		Schema: res.Schema,
+		Map:    schema.BasicMapReader(is.Attributes),
+	}
+	result, err := reader.ReadField([]string{"key"})
+	if err != nil {
+		return nil, err
+	}
+
+	oldKeys, ok := result.Value.(*schema.Set)
+	if !ok {
+		return nil, fmt.Errorf("Got unexpected value from state: %#v", result.Value)
+	}
+	return oldKeys, nil
+}
+
+func clearV0Keys(is *terraform.InstanceState) error {
+	for k := range is.Attributes {
+		if strings.HasPrefix(k, "key.") {
+			delete(is.Attributes, k)
+		}
+	}
+	return nil
+}
+
+func writeV1Keys(
+	is *terraform.InstanceState,
+	res *schema.Resource,
+	keys *schema.Set,
+) error {
+	writer := schema.MapFieldWriter{
+		Schema: res.Schema,
+	}
+	if err := writer.WriteField([]string{"key"}, keys); err != nil {
+		return err
+	}
+	for k, v := range writer.Map() {
+		is.Attributes[k] = v
+	}
+
+	return nil
+}

builtin/providers/consul/resource_consul_keys_migrate_test.go

@@ -0,0 +1,90 @@
+package consul
+
+import (
+	"testing"
+
+	"github.com/hashicorp/terraform/terraform"
+)
+
+func TestConsulKeysMigrateState(t *testing.T) {
+	cases := map[string]struct {
+		StateVersion int
+		Attributes   map[string]string
+		Expected     map[string]string
+		Meta         interface{}
+	}{
+		"v0.6.9 and earlier, with old values hash function": {
+			StateVersion: 0,
+			Attributes: map[string]string{
+				"key.#":             "2",
+				"key.12345.name":    "hello",
+				"key.12345.path":    "foo/bar",
+				"key.12345.value":   "world",
+				"key.12345.default": "",
+				"key.12345.delete":  "false",
+				"key.6789.name":     "temp",
+				"key.6789.path":     "foo/foo",
+				"key.6789.value":    "",
+				"key.6789.default":  "",
+				"key.6789.delete":   "true",
+			},
+			Expected: map[string]string{
+				"key.#":                  "2",
+				"key.2401383718.default": "",
+				"key.2401383718.delete":  "true",
+				"key.2401383718.name":    "temp",
+				"key.2401383718.path":    "foo/foo",
+				"key.2401383718.value":   "",
+				"key.3116955509.path":    "foo/bar",
+				"key.3116955509.default": "",
+				"key.3116955509.delete":  "false",
+				"key.3116955509.name":    "hello",
+				"key.3116955509.value":   "world",
+			},
+		},
+	}
+
+	for tn, tc := range cases {
+		is := &terraform.InstanceState{
+			ID:         "consul",
+			Attributes: tc.Attributes,
+		}
+		is, err := resourceConsulKeys().MigrateState(
+			tc.StateVersion, is, tc.Meta)
+
+		if err != nil {
+			t.Fatalf("bad: %s, err: %#v", tn, err)
+		}
+
+		for k, v := range tc.Expected {
+			if is.Attributes[k] != v {
+				t.Fatalf(
+					"bad: %s\n\n expected: %#v -> %#v\n got: %#v -> %#v\n in: %#v",
+					tn, k, v, k, is.Attributes[k], is.Attributes)
+			}
+		}
+	}
+}
+
+func TestConsulKeysMigrateState_empty(t *testing.T) {
+	var is *terraform.InstanceState
+	var meta interface{}
+
+	// should handle nil
+	is, err := resourceConsulKeys().MigrateState(0, is, meta)
+
+	if err != nil {
+		t.Fatalf("err: %#v", err)
+	}
+	if is != nil {
+		t.Fatalf("expected nil instancestate, got: %#v", is)
+	}
+
+	// should handle non-nil but empty
+	is = &terraform.InstanceState{}
+	is, err = resourceConsulKeys().MigrateState(0, is, meta)
+
+	if err != nil {
+		t.Fatalf("err: %#v", err)
+	}
+}

builtin/providers/consul/resource_consul_keys_test.go

@@ -11,7 +11,7 @@ import (
 
 func TestAccConsulKeys_basic(t *testing.T) {
 	resource.Test(t, resource.TestCase{
-		PreCheck:     func() {},
+		PreCheck:     func() { testAccPreCheck(t) },
 		Providers:    testAccProviders,
 		CheckDestroy: testAccCheckConsulKeysDestroy,
 		Steps: []resource.TestStep{
@@ -19,18 +19,25 @@ func TestAccConsulKeys_basic(t *testing.T) {
 				Config: testAccConsulKeysConfig,
 				Check: resource.ComposeTestCheckFunc(
 					testAccCheckConsulKeysExists(),
-					testAccCheckConsulKeysValue("consul_keys.app", "time", "<any>"),
 					testAccCheckConsulKeysValue("consul_keys.app", "enabled", "true"),
 					testAccCheckConsulKeysValue("consul_keys.app", "set", "acceptance"),
 				),
 			},
+			resource.TestStep{
+				Config: testAccConsulKeysConfig_Update,
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckConsulKeysExists(),
+					testAccCheckConsulKeysValue("consul_keys.app", "enabled", "true"),
+					testAccCheckConsulKeysValue("consul_keys.app", "set", "acceptanceUpdated"),
+				),
+			},
 		},
 	})
 }
 
 func testAccCheckConsulKeysDestroy(s *terraform.State) error {
 	kv := testAccProvider.Meta().(*consulapi.Client).KV()
-	opts := &consulapi.QueryOptions{Datacenter: "nyc3"}
+	opts := &consulapi.QueryOptions{Datacenter: "dc1"}
 	pair, _, err := kv.Get("test/set", opts)
 	if err != nil {
 		return err
@@ -44,7 +51,7 @@ func testAccCheckConsulKeysDestroy(s *terraform.State) error {
 func testAccCheckConsulKeysExists() resource.TestCheckFunc {
 	return func(s *terraform.State) error {
 		kv := testAccProvider.Meta().(*consulapi.Client).KV()
-		opts := &consulapi.QueryOptions{Datacenter: "nyc3"}
+		opts := &consulapi.QueryOptions{Datacenter: "dc1"}
 		pair, _, err := kv.Get("test/set", opts)
 		if err != nil {
 			return err
@@ -78,11 +85,7 @@ func testAccCheckConsulKeysValue(n, attr, val string) resource.TestCheckFunc {
 
 const testAccConsulKeysConfig = `
 resource "consul_keys" "app" {
-	datacenter = "nyc3"
+	datacenter = "dc1"
-	key {
-		name = "time"
-		path = "global/time"
-	}
 	key {
 		name = "enabled"
 		path = "test/enabled"
@@ -96,3 +99,20 @@ resource "consul_keys" "app" {
 	}
 }
 `
+
+const testAccConsulKeysConfig_Update = `
+resource "consul_keys" "app" {
+	datacenter = "dc1"
+	key {
+		name = "enabled"
+		path = "test/enabled"
+		default = "true"
+	}
+	key {
+		name = "set"
+		path = "test/set"
+		value = "acceptanceUpdated"
+		delete = true
+	}
+}
+`

builtin/providers/consul/resource_provider_test.go

@@ -1,6 +1,7 @@
 package consul
 
 import (
+	"os"
 	"testing"
 
 	consulapi "github.com/hashicorp/consul/api"
@@ -21,7 +22,6 @@ func init() {
 	// Use the demo address for the acceptance tests
 	testAccProvider.ConfigureFunc = func(d *schema.ResourceData) (interface{}, error) {
 		conf := consulapi.DefaultConfig()
-		conf.Address = "demo.consul.io:80"
 		return consulapi.NewClient(conf)
 	}
 }
@@ -55,3 +55,9 @@ func TestResourceProvider_Configure(t *testing.T) {
 		t.Fatalf("err: %s", err)
 	}
 }
+
+func testAccPreCheck(t *testing.T) {
+	if v := os.Getenv("CONSUL_HTTP_ADDR"); v == "" {
+		t.Fatal("CONSUL_HTTP_ADDR must be set for acceptance tests")
+	}
+}

helper/schema/field_writer_map.go

@@ -277,7 +277,7 @@ func (w *MapFieldWriter) setSet(
 		// not the `value` directly is because this forces all types
 		// to become []interface{} (generic) instead of []string, which
 		// most hash functions are expecting.
-		s := &Set{F: schema.Set}
+		s := schema.ZeroValue().(*Set)
 		tempR := &MapFieldReader{
 			Map:    BasicMapReader(tempW.Map()),
 			Schema: tempSchemaMap,