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,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.
-