Treat each consul key as having its own lifecycle

Previously this resource managed the set of keys as a whole rather than
the individual keys, and so it was unable to recognize when a particular
managed key is removed and delete just that one key from Consul.

Here this is addressed by recognizing that each key actually has its own
lifecycle, and detecting when individual keys are added and removed
without replacing the entire consul_keys instance.

Additionally this restores the behavior of updating the "value" attribute
on read, but restricts it only to blocks that already had a value so as
to avoid the quirkiness seen previously when we updated blocks that were
intended to be read-only. Updating the value is important now, because we
rely on this to detect and repair discrepancies between values stored in
Consul and values given in the configuration.

This change produces a change in the handling of the "delete" attribute.
Before it was considered only when the entire consul_keys resource was
deleted, but now it is considered also when a particular key block is
removed from within a resource.

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,34 +203,44 @@ 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)
 		if err != nil {
 			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,26 +257,24 @@ 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)
 		if err != nil {
 			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.
-