From 54cac349a3cacf3e84b23ddff6a084641c674b55 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 7 Feb 2017 10:05:53 -0500 Subject: [PATCH 1/4] Add state locking to consul backend Use consul locks to implement state locking. The lock path is state path + "/.lock" which matches the consul cli default for locks. Lockinfo is stored at path + "/.lockinfo". --- backend/remote-state/consul/client.go | 138 +++++++++++++++++++++ backend/remote-state/consul/client_test.go | 23 ++++ backend/remote-state/testing.go | 55 ++++++++ 3 files changed, 216 insertions(+) diff --git a/backend/remote-state/consul/client.go b/backend/remote-state/consul/client.go index 51df7a31d..82e55db61 100644 --- a/backend/remote-state/consul/client.go +++ b/backend/remote-state/consul/client.go @@ -2,15 +2,35 @@ package consul import ( "crypto/md5" + "encoding/json" + "errors" + "fmt" + "time" consulapi "github.com/hashicorp/consul/api" + "github.com/hashicorp/errwrap" + multierror "github.com/hashicorp/go-multierror" "github.com/hashicorp/terraform/state/remote" ) +const ( + lockSuffix = "/.lock" + lockInfoSuffix = "/.lockinfo" +) + +// TODO: use single LockInfo struct +type lockInfo struct { + Created time.Time + Info string +} + // RemoteClient is a remote client that stores data in Consul. type RemoteClient struct { Client *consulapi.Client Path string + + consulLock *consulapi.Lock + lockCh <-chan struct{} } func (c *RemoteClient) Get() (*remote.Payload, error) { @@ -43,3 +63,121 @@ func (c *RemoteClient) Delete() error { _, err := kv.Delete(c.Path, nil) return err } + +func (c *RemoteClient) putLockInfo(info string) error { + li := &lockInfo{ + Created: time.Now().UTC(), + Info: info, + } + + js, err := json.Marshal(li) + if err != nil { + return err + } + + kv := c.Client.KV() + _, err = kv.Put(&consulapi.KVPair{ + Key: c.Path + lockInfoSuffix, + Value: js, + }, nil) + + return err +} + +func (c *RemoteClient) getLockInfo() (*lockInfo, error) { + path := c.Path + lockInfoSuffix + pair, _, err := c.Client.KV().Get(path, nil) + if err != nil { + return nil, err + } + if pair == nil { + return nil, nil + } + + li := &lockInfo{} + err = json.Unmarshal(pair.Value, li) + if err != nil { + return nil, errwrap.Wrapf("error unmarshaling lock info: {{err}}", err) + } + + return li, nil +} + +func (c *RemoteClient) Lock(info string) error { + select { + case <-c.lockCh: + // We had a lock, but lost it. + // Since we typically only call lock once, we shouldn't ever see this. + return errors.New("lost consul lock") + default: + if c.lockCh != nil { + // we have an active lock already + return nil + } + } + + if c.consulLock == nil { + opts := &consulapi.LockOptions{ + Key: c.Path + lockSuffix, + // We currently don't procide any options to block terraform and + // retry lock acquisition, but we can wait briefly in case the + // lock is about to be freed. + LockWaitTime: time.Second, + LockTryOnce: true, + } + + lock, err := c.Client.LockOpts(opts) + if err != nil { + return nil + } + + c.consulLock = lock + } + + lockCh, err := c.consulLock.Lock(make(chan struct{})) + if err != nil { + return err + } + + if lockCh == nil { + lockInfo, e := c.getLockInfo() + if e != nil { + return e + } + return fmt.Errorf("state locked: created:%s, info:%q", + lockInfo.Created, lockInfo.Info) + } + + c.lockCh = lockCh + + err = c.putLockInfo(info) + if err != nil { + err = multierror.Append(err, c.Unlock()) + return err + } + + return nil +} + +func (c *RemoteClient) Unlock() error { + if c.consulLock == nil || c.lockCh == nil { + return nil + } + + select { + case <-c.lockCh: + return errors.New("consul lock was lost") + default: + } + + err := c.consulLock.Unlock() + c.lockCh = nil + + kv := c.Client.KV() + _, delErr := kv.Delete(c.Path+lockInfoSuffix, nil) + if delErr != nil { + err = multierror.Append(err, delErr) + } + + return err +} diff --git a/backend/remote-state/consul/client_test.go b/backend/remote-state/consul/client_test.go index ce726fd5d..ed330015c 100644 --- a/backend/remote-state/consul/client_test.go +++ b/backend/remote-state/consul/client_test.go @@ -2,6 +2,7 @@ package consul import ( "fmt" + "os" "testing" "time" @@ -27,3 +28,25 @@ func TestRemoteClient(t *testing.T) { // Test remotestate.TestClient(t, b) } + +func TestConsul_stateLock(t *testing.T) { + addr := os.Getenv("CONSUL_HTTP_ADDR") + if addr == "" { + t.Log("consul lock tests require a running consul instance") + t.Skip() + } + + path := "testing" //fmt.Sprintf("tf-unit/%s", time.Now().String()) + + // create 2 instances to get 2 remote.Clients + a := backend.TestBackendConfig(t, New(), map[string]interface{}{ + "address": addr, + "path": path, + }) + b := backend.TestBackendConfig(t, New(), map[string]interface{}{ + "address": addr, + "path": path, + }) + + remotestate.TestRemoteLocks(t, a, b) +} diff --git a/backend/remote-state/testing.go b/backend/remote-state/testing.go index 17d9b80f5..db0894ac7 100644 --- a/backend/remote-state/testing.go +++ b/backend/remote-state/testing.go @@ -4,6 +4,7 @@ import ( "testing" "github.com/hashicorp/terraform/backend" + "github.com/hashicorp/terraform/state" "github.com/hashicorp/terraform/state/remote" ) @@ -15,3 +16,57 @@ func TestClient(t *testing.T, raw backend.Backend) { remote.TestClient(t, b.client) } + +// Test the lock implementation for a remote.Client. +// This test requires 2 backend instances, in oder to have multiple remote +// clients since some implementations may tie the client to the lock, or may +// have reentrant locks. +func TestRemoteLocks(t *testing.T, a, b backend.Backend) { + sA, err := a.State() + if err != nil { + t.Fatal("failed to get state from backend A:", err) + } + + sB, err := b.State() + if err != nil { + t.Fatal("failed to get state from backend B:", err) + } + + lockerA, ok := sA.(state.Locker) + if !ok { + t.Fatal("client A not a state.Locker") + } + + lockerB, ok := sB.(state.Locker) + if !ok { + t.Fatal("client B not a state.Locker") + } + + if err := lockerA.Lock("test client A"); err != nil { + t.Fatal("unable to get initial lock:", err) + } + + if err := lockerB.Lock("test client B"); err == nil { + lockerA.Unlock() + t.Fatal("client B obtained lock while held by client A") + } else { + t.Log("lock info error:", err) + } + + if err := lockerA.Unlock(); err != nil { + t.Fatal("error unlocking client A", err) + } + + if err := lockerB.Lock("test client B"); err != nil { + t.Fatal("unable to obtain lock from client B") + } + + if err := lockerB.Unlock(); err != nil { + t.Fatal("error unlocking client B:", err) + } + + // unlock should be repeatable + if err := lockerA.Unlock(); err != nil { + t.Fatal("Unlock error from client A when state was not locked:", err) + } +} From 9b76f6e138bd3d25634aa47e945bbc756346892c Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 7 Feb 2017 10:33:05 -0500 Subject: [PATCH 2/4] Move TestRemoteLocks to state/remote This was legacy remote state client and backends can use this test function without an import cycle. --- backend/remote-state/consul/client_test.go | 25 ++++++---- backend/remote-state/testing.go | 55 ---------------------- state/remote/remote_test.go | 32 ------------- state/remote/s3_test.go | 2 +- state/remote/testing.go | 44 +++++++++++++++++ 5 files changed, 61 insertions(+), 97 deletions(-) diff --git a/backend/remote-state/consul/client_test.go b/backend/remote-state/consul/client_test.go index ed330015c..09d018fda 100644 --- a/backend/remote-state/consul/client_test.go +++ b/backend/remote-state/consul/client_test.go @@ -32,21 +32,28 @@ func TestRemoteClient(t *testing.T) { func TestConsul_stateLock(t *testing.T) { addr := os.Getenv("CONSUL_HTTP_ADDR") if addr == "" { - t.Log("consul lock tests require a running consul instance") + t.Log("consul lock tests require CONSUL_HTTP_ADDR") t.Skip() } - path := "testing" //fmt.Sprintf("tf-unit/%s", time.Now().String()) + path := fmt.Sprintf("tf-unit/%s", time.Now().String()) // create 2 instances to get 2 remote.Clients - a := backend.TestBackendConfig(t, New(), map[string]interface{}{ + sA, err := backend.TestBackendConfig(t, New(), map[string]interface{}{ "address": addr, "path": path, - }) - b := backend.TestBackendConfig(t, New(), map[string]interface{}{ - "address": addr, - "path": path, - }) + }).State() + if err != nil { + t.Fatal(err) + } - remotestate.TestRemoteLocks(t, a, b) + sB, err := backend.TestBackendConfig(t, New(), map[string]interface{}{ + "address": addr, + "path": path, + }).State() + if err != nil { + t.Fatal(err) + } + + remote.TestRemoteLocks(t, sA.(*remote.State).Client, sB.(*remote.State).Client) } diff --git a/backend/remote-state/testing.go b/backend/remote-state/testing.go index db0894ac7..17d9b80f5 100644 --- a/backend/remote-state/testing.go +++ b/backend/remote-state/testing.go @@ -4,7 +4,6 @@ import ( "testing" "github.com/hashicorp/terraform/backend" - "github.com/hashicorp/terraform/state" "github.com/hashicorp/terraform/state/remote" ) @@ -16,57 +15,3 @@ func TestClient(t *testing.T, raw backend.Backend) { remote.TestClient(t, b.client) } - -// Test the lock implementation for a remote.Client. -// This test requires 2 backend instances, in oder to have multiple remote -// clients since some implementations may tie the client to the lock, or may -// have reentrant locks. -func TestRemoteLocks(t *testing.T, a, b backend.Backend) { - sA, err := a.State() - if err != nil { - t.Fatal("failed to get state from backend A:", err) - } - - sB, err := b.State() - if err != nil { - t.Fatal("failed to get state from backend B:", err) - } - - lockerA, ok := sA.(state.Locker) - if !ok { - t.Fatal("client A not a state.Locker") - } - - lockerB, ok := sB.(state.Locker) - if !ok { - t.Fatal("client B not a state.Locker") - } - - if err := lockerA.Lock("test client A"); err != nil { - t.Fatal("unable to get initial lock:", err) - } - - if err := lockerB.Lock("test client B"); err == nil { - lockerA.Unlock() - t.Fatal("client B obtained lock while held by client A") - } else { - t.Log("lock info error:", err) - } - - if err := lockerA.Unlock(); err != nil { - t.Fatal("error unlocking client A", err) - } - - if err := lockerB.Lock("test client B"); err != nil { - t.Fatal("unable to obtain lock from client B") - } - - if err := lockerB.Unlock(); err != nil { - t.Fatal("error unlocking client B:", err) - } - - // unlock should be repeatable - if err := lockerA.Unlock(); err != nil { - t.Fatal("Unlock error from client A when state was not locked:", err) - } -} diff --git a/state/remote/remote_test.go b/state/remote/remote_test.go index 5fa20f4eb..db3c795c8 100644 --- a/state/remote/remote_test.go +++ b/state/remote/remote_test.go @@ -44,38 +44,6 @@ func testClient(t *testing.T, c Client) { } } -func testClientLocks(t *testing.T, c Client) { - s3Client := c.(*S3Client) - - // initial lock - if err := s3Client.Lock("test"); err != nil { - t.Fatal(err) - } - - // second lock should fail - if err := s3Client.Lock("test"); err == nil { - t.Fatal("expected error, got nil") - } - - // unlock should work - if err := s3Client.Unlock(); err != nil { - t.Fatal(err) - } - - // now we should be able to lock again - if err := s3Client.Lock("test"); err != nil { - t.Fatal(err) - } - - // unlock should be idempotent - if err := s3Client.Unlock(); err != nil { - t.Fatal(err) - } - if err := s3Client.Unlock(); err != nil { - t.Fatal(err) - } -} - func TestRemoteClient_noPayload(t *testing.T) { s := &State{ Client: nilClient{}, diff --git a/state/remote/s3_test.go b/state/remote/s3_test.go index f29426fa7..7a0983589 100644 --- a/state/remote/s3_test.go +++ b/state/remote/s3_test.go @@ -169,7 +169,7 @@ func TestS3ClientLocks(t *testing.T) { createDynamoDBTable(t, s3Client, bucketName) - testClientLocks(t, client) + TestRemoteLocks(t, client, client) } // create the dynamoDB table, and wait until we can query it. diff --git a/state/remote/testing.go b/state/remote/testing.go index bc874a458..d7de10b16 100644 --- a/state/remote/testing.go +++ b/state/remote/testing.go @@ -41,3 +41,47 @@ func TestClient(t *testing.T, c Client) { t.Fatalf("bad: %#v", p) } } + +// Test the lock implementation for a remote.Client. +// This test requires 2 client instances, in oder to have multiple remote +// clients since some implementations may tie the client to the lock, or may +// have reentrant locks. +func TestRemoteLocks(t *testing.T, a, b Client) { + lockerA, ok := a.(state.Locker) + if !ok { + t.Fatal("client A not a state.Locker") + } + + lockerB, ok := b.(state.Locker) + if !ok { + t.Fatal("client B not a state.Locker") + } + + if err := lockerA.Lock("test client A"); err != nil { + t.Fatal("unable to get initial lock:", err) + } + + if err := lockerB.Lock("test client B"); err == nil { + lockerA.Unlock() + t.Fatal("client B obtained lock while held by client A") + } else { + t.Log("lock info error:", err) + } + + if err := lockerA.Unlock(); err != nil { + t.Fatal("error unlocking client A", err) + } + + if err := lockerB.Lock("test client B"); err != nil { + t.Fatal("unable to obtain lock from client B") + } + + if err := lockerB.Unlock(); err != nil { + t.Fatal("error unlocking client B:", err) + } + + // unlock should be repeatable + if err := lockerA.Unlock(); err != nil { + t.Fatal("Unlock error from client A when state was not locked:", err) + } +} From 14d965722e32b21d96ca93d4209b004da1874213 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 7 Feb 2017 11:12:02 -0500 Subject: [PATCH 3/4] Use single state.LockInfo struct Remove redundant structures --- backend/remote-state/consul/client.go | 18 ++++++---------- state/local.go | 28 +++++++++++++++--------- state/local_test.go | 2 +- state/remote/s3.go | 31 ++++++++++++++++----------- 4 files changed, 44 insertions(+), 35 deletions(-) diff --git a/backend/remote-state/consul/client.go b/backend/remote-state/consul/client.go index 82e55db61..580789522 100644 --- a/backend/remote-state/consul/client.go +++ b/backend/remote-state/consul/client.go @@ -4,12 +4,12 @@ import ( "crypto/md5" "encoding/json" "errors" - "fmt" "time" consulapi "github.com/hashicorp/consul/api" "github.com/hashicorp/errwrap" multierror "github.com/hashicorp/go-multierror" + "github.com/hashicorp/terraform/state" "github.com/hashicorp/terraform/state/remote" ) @@ -18,12 +18,6 @@ const ( lockInfoSuffix = "/.lockinfo" ) -// TODO: use single LockInfo struct -type lockInfo struct { - Created time.Time - Info string -} - // RemoteClient is a remote client that stores data in Consul. type RemoteClient struct { Client *consulapi.Client @@ -65,7 +59,8 @@ func (c *RemoteClient) Delete() error { } func (c *RemoteClient) putLockInfo(info string) error { - li := &lockInfo{ + li := &state.LockInfo{ + Path: c.Path, Created: time.Now().UTC(), Info: info, } @@ -84,7 +79,7 @@ func (c *RemoteClient) putLockInfo(info string) error { return err } -func (c *RemoteClient) getLockInfo() (*lockInfo, error) { +func (c *RemoteClient) getLockInfo() (*state.LockInfo, error) { path := c.Path + lockInfoSuffix pair, _, err := c.Client.KV().Get(path, nil) if err != nil { @@ -94,7 +89,7 @@ func (c *RemoteClient) getLockInfo() (*lockInfo, error) { return nil, nil } - li := &lockInfo{} + li := &state.LockInfo{} err = json.Unmarshal(pair.Value, li) if err != nil { return nil, errwrap.Wrapf("error unmarshaling lock info: {{err}}", err) @@ -144,8 +139,7 @@ func (c *RemoteClient) Lock(info string) error { if e != nil { return e } - return fmt.Errorf("state locked: created:%s, info:%q", - lockInfo.Created, lockInfo.Info) + return lockInfo.Err() } c.lockCh = lockCh diff --git a/state/local.go b/state/local.go index b34d04837..62454e257 100644 --- a/state/local.go +++ b/state/local.go @@ -14,19 +14,27 @@ import ( ) // lock metadata structure for local locks -type lockInfo struct { +type LockInfo struct { // Path to the state file Path string // The time the lock was taken Created time.Time // Extra info passed to State.Lock - Reason string + Info string } // return the lock info formatted in an error -func (l *lockInfo) Err() error { - return fmt.Errorf("state file %q locked. created:%s, reason:%s", - l.Path, l.Created, l.Reason) +func (l *LockInfo) Err() error { + return fmt.Errorf("state locked. path:%q, created:%s, info:%q", + l.Path, l.Created, l.Info) +} + +func (l *LockInfo) String() string { + js, err := json.Marshal(l) + if err != nil { + panic(err) + } + return string(js) } // LocalState manages a state storage that is local to the filesystem. @@ -224,14 +232,14 @@ func (s *LocalState) lockInfoPath() string { } // lockInfo returns the data in a lock info file -func (s *LocalState) lockInfo() (*lockInfo, error) { +func (s *LocalState) lockInfo() (*LockInfo, error) { path := s.lockInfoPath() infoData, err := ioutil.ReadFile(path) if err != nil { return nil, err } - info := lockInfo{} + info := LockInfo{} err = json.Unmarshal(infoData, &info) if err != nil { return nil, fmt.Errorf("state file %q locked, but could not unmarshal lock info: %s", s.Path, err) @@ -240,13 +248,13 @@ func (s *LocalState) lockInfo() (*lockInfo, error) { } // write a new lock info file -func (s *LocalState) writeLockInfo(reason string) error { +func (s *LocalState) writeLockInfo(info string) error { path := s.lockInfoPath() - lockInfo := &lockInfo{ + lockInfo := &LockInfo{ Path: s.Path, Created: time.Now().UTC(), - Reason: reason, + Info: info, } infoData, err := json.Marshal(lockInfo) diff --git a/state/local_test.go b/state/local_test.go index 5049d043e..cfd327f8d 100644 --- a/state/local_test.go +++ b/state/local_test.go @@ -40,7 +40,7 @@ func TestLocalStateLocks(t *testing.T) { t.Fatal(err) } - if lockInfo.Reason != "test" { + if lockInfo.Info != "test" { t.Fatalf("invalid lock info %#v\n", lockInfo) } diff --git a/state/remote/s3.go b/state/remote/s3.go index 367335865..ab101d2b7 100644 --- a/state/remote/s3.go +++ b/state/remote/s3.go @@ -2,6 +2,7 @@ package remote import ( "bytes" + "encoding/json" "fmt" "io" "log" @@ -17,6 +18,7 @@ import ( "github.com/hashicorp/go-cleanhttp" "github.com/hashicorp/go-multierror" terraformAws "github.com/hashicorp/terraform/builtin/providers/aws" + "github.com/hashicorp/terraform/state" ) func s3Factory(conf map[string]string) (Client, error) { @@ -196,18 +198,22 @@ func (c *S3Client) Delete() error { return err } -func (c *S3Client) Lock(reason string) error { +func (c *S3Client) Lock(info string) error { if c.lockTable == "" { return nil } stateName := fmt.Sprintf("%s/%s", c.bucketName, c.keyName) + lockInfo := &state.LockInfo{ + Path: stateName, + Created: time.Now().UTC(), + Info: info, + } putParams := &dynamodb.PutItemInput{ Item: map[string]*dynamodb.AttributeValue{ - "LockID": {S: aws.String(stateName)}, - "Created": {S: aws.String(time.Now().UTC().Format(time.RFC3339))}, - "Info": {S: aws.String(reason)}, + "LockID": {S: aws.String(stateName)}, + "Info": {S: aws.String(lockInfo.String())}, }, TableName: aws.String(c.lockTable), ConditionExpression: aws.String("attribute_not_exists(LockID)"), @@ -225,20 +231,21 @@ func (c *S3Client) Lock(reason string) error { resp, err := c.dynClient.GetItem(getParams) if err != nil { - return fmt.Errorf("s3 state file %q locked, cfailed to retrive info: %s", stateName, err) + return fmt.Errorf("s3 state file %q locked, failed to retrive info: %s", stateName, err) } - var created, info string - if v, ok := resp.Item["Created"]; ok && v.S != nil { - created = *v.S - } + var infoData string if v, ok := resp.Item["Info"]; ok && v.S != nil { - info = *v.S + infoData = *v.S } - return fmt.Errorf("state file %q locked. created:%s, reason:%s", - stateName, created, info) + lockInfo = &state.LockInfo{} + err = json.Unmarshal([]byte(infoData), lockInfo) + if err != nil { + return fmt.Errorf("s3 state file %q locked, failed get lock info: %s", stateName, err) + } + return lockInfo.Err() } return nil } From 80fab23e04f2dc03f96c51746411ecb318d67af8 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 8 Feb 2017 10:51:51 -0500 Subject: [PATCH 4/4] Don't test consul using demo.consul.io We shoudn't require an external service for unit test. TODO: create some proper acceptance tests for consul --- backend/remote-state/consul/client_test.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/backend/remote-state/consul/client_test.go b/backend/remote-state/consul/client_test.go index 09d018fda..bd52092ca 100644 --- a/backend/remote-state/consul/client_test.go +++ b/backend/remote-state/consul/client_test.go @@ -8,7 +8,6 @@ import ( "github.com/hashicorp/terraform/backend" "github.com/hashicorp/terraform/backend/remote-state" - "github.com/hashicorp/terraform/helper/acctest" "github.com/hashicorp/terraform/state/remote" ) @@ -17,11 +16,15 @@ func TestRemoteClient_impl(t *testing.T) { } func TestRemoteClient(t *testing.T) { - acctest.RemoteTestPrecheck(t) + addr := os.Getenv("CONSUL_HTTP_ADDR") + if addr == "" { + t.Log("consul tests require CONSUL_HTTP_ADDR") + t.Skip() + } // Get the backend b := backend.TestBackendConfig(t, New(), map[string]interface{}{ - "address": "demo.consul.io:80", + "address": addr, "path": fmt.Sprintf("tf-unit/%s", time.Now().String()), })