From df2ce588bce27ba4faba1f8dcfb4de24a3acbedc Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Thu, 18 Feb 2016 19:20:20 -0800 Subject: [PATCH 1/2] Specialized client for interacting with Consul keys This deals with some of the quirks of interacting with the Consul API, with the goal of making the consul_keys resource implementation, and later the consul_keys data source, less noisy to read. --- builtin/providers/consul/key_client.go | 66 ++++++++++++++++++++++++++ 1 file changed, 66 insertions(+) create mode 100644 builtin/providers/consul/key_client.go diff --git a/builtin/providers/consul/key_client.go b/builtin/providers/consul/key_client.go new file mode 100644 index 000000000000..5f41c8df78e4 --- /dev/null +++ b/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 +} From 2e33f5311c3ff19c7a5b9b737236f241764e4c23 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Thu, 10 Mar 2016 07:52:43 -0800 Subject: [PATCH 2/2] 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. --- .../providers/consul/resource_consul_keys.go | 162 +++++++++++++----- .../consul/resource_consul_keys_test.go | 22 +++ .../providers/consul/r/keys.html.markdown | 4 +- 3 files changed, 139 insertions(+), 49 deletions(-) diff --git a/builtin/providers/consul/resource_consul_keys.go b/builtin/providers/consul/resource_consul_keys.go index a2d965785c36..fe1db4ec1755 100644 --- a/builtin/providers/consul/resource_consul_keys.go +++ b/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 diff --git a/builtin/providers/consul/resource_consul_keys_test.go b/builtin/providers/consul/resource_consul_keys_test.go index a820ff3314b2..f6045658eb35 100644 --- a/builtin/providers/consul/resource_consul_keys_test.go +++ b/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 + } } ` diff --git a/website/source/docs/providers/consul/r/keys.html.markdown b/website/source/docs/providers/consul/r/keys.html.markdown index f0f854a3e80d..c1c806961681 100644 --- a/website/source/docs/providers/consul/r/keys.html.markdown +++ b/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.` - For each name given, the corresponding attribute has the value of the key. -