Merge pull request #5210 from apparentlymart/consul-keys-revamp

Make consul_keys behavior less surprising

builtin/providers/consul/key_client.go

@@ -0,0 +1,66 @@
+package consul
+
+import (
+	"fmt"
+	"log"
+
+	consulapi "github.com/hashicorp/consul/api"
+)
+
+// keyClient is a wrapper around the upstream Consul client that is
+// specialized for Terraform's manipulations of the key/value store.
+type keyClient struct {
+	client *consulapi.KV
+	qOpts  *consulapi.QueryOptions
+	wOpts  *consulapi.WriteOptions
+}
+
+func newKeyClient(realClient *consulapi.KV, dc, token string) *keyClient {
+	qOpts := &consulapi.QueryOptions{Datacenter: dc, Token: token}
+	wOpts := &consulapi.WriteOptions{Datacenter: dc, Token: token}
+
+	return &keyClient{
+		client: realClient,
+		qOpts:  qOpts,
+		wOpts:  wOpts,
+	}
+}
+
+func (c *keyClient) Get(path string) (string, error) {
+	log.Printf(
+		"[DEBUG] Reading key '%s' in %s",
+		path, c.qOpts.Datacenter,
+	)
+	pair, _, err := c.client.Get(path, c.qOpts)
+	if err != nil {
+		return "", fmt.Errorf("Failed to read Consul key '%s': %s", path, err)
+	}
+	value := ""
+	if pair != nil {
+		value = string(pair.Value)
+	}
+	return value, nil
+}
+
+func (c *keyClient) Put(path, value string) error {
+	log.Printf(
+		"[DEBUG] Setting key '%s' to '%v' in %s",
+		path, value, c.wOpts.Datacenter,
+	)
+	pair := consulapi.KVPair{Key: path, Value: []byte(value)}
+	if _, err := c.client.Put(&pair, c.wOpts); err != nil {
+		return fmt.Errorf("Failed to write Consul key '%s': %s", path, err)
+	}
+	return nil
+}
+
+func (c *keyClient) Delete(path string) error {
+	log.Printf(
+		"[DEBUG] Deleting key '%s' in %s",
+		path, c.wOpts.Datacenter,
+	)
+	if _, err := c.client.Delete(path, c.wOpts); err != nil {
+		return fmt.Errorf("Failed to delete Consul key '%s': %s", path, err)
+	}
+	return nil
+}

builtin/providers/consul/resource_consul_keys.go

@@ -2,7 +2,6 @@ package consul
 
 import (
 	"fmt"
-	"log"
 	"strconv"
 
 	consulapi "github.com/hashicorp/consul/api"
@@ -12,7 +11,7 @@ import (
 func resourceConsulKeys() *schema.Resource {
 	return &schema.Resource{
 		Create: resourceConsulKeysCreate,
-		Update: resourceConsulKeysCreate,
+		Update: resourceConsulKeysUpdate,
 		Read:   resourceConsulKeysRead,
 		Delete: resourceConsulKeysDelete,
 
@@ -84,37 +83,22 @@ func resourceConsulKeysCreate(d *schema.ResourceData, meta interface{}) error {
 		return err
 	}
 
-	// Setup the operations using the datacenter
-	qOpts := consulapi.QueryOptions{Datacenter: dc, Token: token}
-	wOpts := consulapi.WriteOptions{Datacenter: dc, Token: token}
+	keyClient := newKeyClient(kv, dc, token)
 
-	// Store the computed vars
-	vars := make(map[string]string)
-
-	// Extract the keys
 	keys := d.Get("key").(*schema.Set).List()
 	for _, raw := range keys {
-		key, path, sub, err := parseKey(raw)
+		_, path, sub, err := parseKey(raw)
 		if err != nil {
 			return err
 		}
 
 		value := sub["value"].(string)
-		if value != "" {
-			log.Printf("[DEBUG] Setting key '%s' to '%v' in %s", path, value, dc)
-			pair := consulapi.KVPair{Key: path, Value: []byte(value)}
-			if _, err := kv.Put(&pair, &wOpts); err != nil {
-				return fmt.Errorf("Failed to set Consul key '%s': %v", path, err)
-			}
-			vars[key] = value
-		} else {
-			log.Printf("[DEBUG] Getting key '%s' in %s", path, dc)
-			pair, _, err := kv.Get(path, &qOpts)
-			if err != nil {
-				return fmt.Errorf("Failed to get Consul key '%s': %v", path, err)
-			}
-			value := attributeValue(sub, key, pair)
-			vars[key] = value
+		if value == "" {
+			continue
+		}
+
+		if err := keyClient.Put(path, value); err != nil {
+			return err
 		}
 	}
 
@@ -123,15 +107,91 @@ func resourceConsulKeysCreate(d *schema.ResourceData, meta interface{}) error {
 	// 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 resourceConsulKeysRead(d, meta)
+}
+
+func resourceConsulKeysUpdate(d *schema.ResourceData, meta interface{}) error {
+	client := meta.(*consulapi.Client)
+	kv := client.KV()
+	token := d.Get("token").(string)
+	dc, err := getDC(d, client)
+	if err != nil {
 		return err
 	}
+
+	keyClient := newKeyClient(kv, dc, token)
+
+	if d.HasChange("key") {
+		o, n := d.GetChange("key")
+		if o == nil {
+			o = new(schema.Set)
+		}
+		if n == nil {
+			n = new(schema.Set)
+		}
+
+		os := o.(*schema.Set)
+		ns := n.(*schema.Set)
+
+		remove := os.Difference(ns).List()
+		add := ns.Difference(os).List()
+
+		// We'll keep track of what keys we add so that if a key is
+		// in both the "remove" and "add" sets -- which will happen if
+		// its value is changed in-place -- we will avoid writing the
+		// value and then immediately removing it.
+		addedPaths := make(map[string]bool)
+
+		// We add before we remove because then it's possible to change
+		// a key name (which will result in both an add and a remove)
+		// without very temporarily having *neither* value in the store.
+		// Instead, both will briefly be present, which should be less
+		// disruptive in most cases.
+		for _, raw := range add {
+			_, path, sub, err := parseKey(raw)
+			if err != nil {
+				return err
+			}
+
+			value := sub["value"].(string)
+			if value == "" {
+				continue
+			}
+
+			if err := keyClient.Put(path, value); err != nil {
+				return err
+			}
+			addedPaths[path] = true
+		}
+
+		for _, raw := range remove {
+			_, path, sub, err := parseKey(raw)
+			if err != nil {
+				return err
+			}
+
+			// Don't delete something we've just added.
+			// (See explanation at the declaration of this variable above.)
+			if addedPaths[path] {
+				continue
+			}
+
+			shouldDelete, ok := sub["delete"].(bool)
+			if !ok || !shouldDelete {
+				continue
+			}
+
+			if err := keyClient.Delete(path); 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)
 
-	return nil
+	return resourceConsulKeysRead(d, meta)
 }
 
 func resourceConsulKeysRead(d *schema.ResourceData, meta interface{}) error {
@@ -143,13 +203,10 @@ func resourceConsulKeysRead(d *schema.ResourceData, meta interface{}) error {
 		return err
 	}
 
-	// Setup the operations using the datacenter
-	qOpts := consulapi.QueryOptions{Datacenter: dc, Token: token}
+	keyClient := newKeyClient(kv, dc, token)
 
-	// Store the computed vars
 	vars := make(map[string]string)
 
-	// Extract the keys
 	keys := d.Get("key").(*schema.Set).List()
 	for _, raw := range keys {
 		key, path, sub, err := parseKey(raw)
@@ -157,20 +214,33 @@ func resourceConsulKeysRead(d *schema.ResourceData, meta interface{}) error {
 			return err
 		}
 
-		log.Printf("[DEBUG] Refreshing value of key '%s' in %s", path, dc)
-		pair, _, err := kv.Get(path, &qOpts)
+		value, err := keyClient.Get(path)
 		if err != nil {
-			return fmt.Errorf("Failed to get value for path '%s' from Consul: %v", path, err)
+			return err
 		}
 
-		value := attributeValue(sub, key, pair)
+		value = attributeValue(sub, value)
 		vars[key] = value
+
+		// If there is already a "value" attribute present for this key
+		// then it was created as a "write" block. We need to update the
+		// given value within the block itself so that Terraform can detect
+		// when the Consul-stored value has drifted from what was most
+		// recently written by Terraform.
+		// We don't do this for "read" blocks; that causes confusing diffs
+		// because "value" should not be set for read-only key blocks.
+		if oldValue := sub["value"]; oldValue != "" {
+			sub["value"] = value
+		}
 	}
 
-	// Update the resource
 	if err := d.Set("var", vars); err != nil {
 		return err
 	}
+	if err := d.Set("key", keys); 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)
@@ -187,10 +257,9 @@ func resourceConsulKeysDelete(d *schema.ResourceData, meta interface{}) error {
 		return err
 	}
 
-	// Setup the operations using the datacenter
-	wOpts := consulapi.WriteOptions{Datacenter: dc, Token: token}
+	keyClient := newKeyClient(kv, dc, token)
 
-	// Extract the keys
+	// Clean up any keys that we're explicitly managing
 	keys := d.Get("key").(*schema.Set).List()
 	for _, raw := range keys {
 		_, path, sub, err := parseKey(raw)
@@ -198,15 +267,14 @@ func resourceConsulKeysDelete(d *schema.ResourceData, meta interface{}) error {
 			return err
 		}
 
-		// Ignore if the key is non-managed
+		// Skip if the key is non-managed
 		shouldDelete, ok := sub["delete"].(bool)
 		if !ok || !shouldDelete {
 			continue
 		}
 
-		log.Printf("[DEBUG] Deleting key '%s' in %s", path, dc)
-		if _, err := kv.Delete(path, &wOpts); err != nil {
-			return fmt.Errorf("Failed to delete Consul key '%s': %v", path, err)
+		if err := keyClient.Delete(path); err != nil {
+			return err
 		}
 	}
 
@@ -236,10 +304,10 @@ func parseKey(raw interface{}) (string, string, map[string]interface{}, error) {
 
 // attributeValue determines the value for a key, potentially
 // using a default value if provided.
-func attributeValue(sub map[string]interface{}, key string, pair *consulapi.KVPair) string {
+func attributeValue(sub map[string]interface{}, readValue string) string {
 	// Use the value if given
-	if pair != nil {
-		return string(pair.Value)
+	if readValue != "" {
+		return readValue
 	}
 
 	// Use a default if given

builtin/providers/consul/resource_consul_keys_test.go

@@ -21,6 +21,7 @@ func TestAccConsulKeys_basic(t *testing.T) {
 					testAccCheckConsulKeysExists(),
 					testAccCheckConsulKeysValue("consul_keys.app", "enabled", "true"),
 					testAccCheckConsulKeysValue("consul_keys.app", "set", "acceptance"),
+					testAccCheckConsulKeysValue("consul_keys.app", "remove_one", "hello"),
 				),
 			},
 			resource.TestStep{
@@ -29,6 +30,7 @@ func TestAccConsulKeys_basic(t *testing.T) {
 					testAccCheckConsulKeysExists(),
 					testAccCheckConsulKeysValue("consul_keys.app", "enabled", "true"),
 					testAccCheckConsulKeysValue("consul_keys.app", "set", "acceptanceUpdated"),
+					testAccCheckConsulKeysRemoved("consul_keys.app", "remove_one"),
 				),
 			},
 		},
@@ -83,6 +85,20 @@ func testAccCheckConsulKeysValue(n, attr, val string) resource.TestCheckFunc {
 	}
 }
 
+func testAccCheckConsulKeysRemoved(n, attr string) resource.TestCheckFunc {
+	return func(s *terraform.State) error {
+		rn, ok := s.RootModule().Resources[n]
+		if !ok {
+			return fmt.Errorf("Resource not found")
+		}
+		_, ok = rn.Primary.Attributes["var."+attr]
+		if ok {
+			return fmt.Errorf("Attribute '%s' still present: %#v", attr, rn.Primary.Attributes)
+		}
+		return nil
+	}
+}
+
 const testAccConsulKeysConfig = `
 resource "consul_keys" "app" {
 	datacenter = "dc1"
@@ -97,6 +113,12 @@ resource "consul_keys" "app" {
 		value = "acceptance"
 		delete = true
 	}
+	key {
+		name = "remove_one"
+		path = "test/remove_one"
+		value = "hello"
+		delete = true
+	}
 }
 `
 

website/source/docs/providers/consul/r/keys.html.markdown

@@ -71,7 +71,8 @@ The `key` block supports the following:
   This allows a key to be written to.
 
 * `delete` - (Optional) If true, then the key will be deleted when
-  the resource is destroyed. Otherwise, it will be left in Consul.
+  either its configuration block is removed from the configuration or
+  the entire resource is destroyed. Otherwise, it will be left in Consul.
   Defaults to false.
 
 ## Attributes Reference
@@ -81,4 +82,3 @@ The following attributes are exported:
 * `datacenter` - The datacenter the keys are being read/written to.
 * `var.<name>` - For each name given, the corresponding attribute
   has the value of the key.
-