From bbd2d46ac89962ed7d3bd8e5c7e489a96861dc36 Mon Sep 17 00:00:00 2001 From: Mathias Lafeldt Date: Tue, 18 Feb 2020 10:36:57 +0100 Subject: [PATCH 1/7] backend/remote-state/oss: Format code using goimports --- backend/remote-state/oss/backend.go | 18 ++++++++++-------- backend/remote-state/oss/backend_state.go | 3 ++- backend/remote-state/oss/client.go | 7 ++++--- backend/remote-state/oss/client_test.go | 1 + 4 files changed, 17 insertions(+), 12 deletions(-) diff --git a/backend/remote-state/oss/backend.go b/backend/remote-state/oss/backend.go index 8e868c176..5e7268470 100644 --- a/backend/remote-state/oss/backend.go +++ b/backend/remote-state/oss/backend.go @@ -4,6 +4,11 @@ import ( "context" "encoding/json" "fmt" + "io/ioutil" + "os" + "runtime" + "strings" + "github.com/aliyun/alibaba-cloud-sdk-go/sdk/requests" "github.com/aliyun/alibaba-cloud-sdk-go/sdk/responses" "github.com/aliyun/alibaba-cloud-sdk-go/services/sts" @@ -12,10 +17,11 @@ import ( "github.com/hashicorp/terraform/helper/schema" "github.com/hashicorp/terraform/helper/validation" "github.com/jmespath/go-jmespath" - "io/ioutil" - "os" - "runtime" - "strings" + + "log" + "net/http" + "strconv" + "time" "github.com/aliyun/alibaba-cloud-sdk-go/sdk" "github.com/aliyun/alibaba-cloud-sdk-go/sdk/auth/credentials" @@ -24,10 +30,6 @@ import ( "github.com/hashicorp/go-cleanhttp" "github.com/hashicorp/terraform/version" "github.com/mitchellh/go-homedir" - "log" - "net/http" - "strconv" - "time" ) // New creates a new backend for OSS remote state. diff --git a/backend/remote-state/oss/backend_state.go b/backend/remote-state/oss/backend_state.go index 28c40ee4a..af6f45628 100644 --- a/backend/remote-state/oss/backend_state.go +++ b/backend/remote-state/oss/backend_state.go @@ -12,9 +12,10 @@ import ( "github.com/hashicorp/terraform/state/remote" "github.com/hashicorp/terraform/states" - "github.com/aliyun/aliyun-tablestore-go-sdk/tablestore" "log" "path" + + "github.com/aliyun/aliyun-tablestore-go-sdk/tablestore" ) const ( diff --git a/backend/remote-state/oss/client.go b/backend/remote-state/oss/client.go index e50f801e8..e40faf5df 100644 --- a/backend/remote-state/oss/client.go +++ b/backend/remote-state/oss/client.go @@ -8,6 +8,10 @@ import ( "io" "encoding/hex" + "log" + "sync" + "time" + "github.com/aliyun/aliyun-oss-go-sdk/oss" "github.com/aliyun/aliyun-tablestore-go-sdk/tablestore" "github.com/hashicorp/go-multierror" @@ -16,9 +20,6 @@ import ( "github.com/hashicorp/terraform/state" "github.com/hashicorp/terraform/state/remote" "github.com/pkg/errors" - "log" - "sync" - "time" ) // Store the last saved serial in tablestore with this suffix for consistency checks. diff --git a/backend/remote-state/oss/client_test.go b/backend/remote-state/oss/client_test.go index 174976526..486217080 100644 --- a/backend/remote-state/oss/client_test.go +++ b/backend/remote-state/oss/client_test.go @@ -8,6 +8,7 @@ import ( "bytes" "crypto/md5" + "github.com/hashicorp/terraform/backend" "github.com/hashicorp/terraform/state" "github.com/hashicorp/terraform/state/remote" From 3b3739b0ca6f04a39696f868dd2ff4dea69f110f Mon Sep 17 00:00:00 2001 From: Mathias Lafeldt Date: Tue, 18 Feb 2020 10:40:44 +0100 Subject: [PATCH 2/7] backend/remote-state/oss: Add missing lock path to lock info So that is shows up in lock errors, etc. --- backend/remote-state/oss/client.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/backend/remote-state/oss/client.go b/backend/remote-state/oss/client.go index e40faf5df..47073fb72 100644 --- a/backend/remote-state/oss/client.go +++ b/backend/remote-state/oss/client.go @@ -158,6 +158,8 @@ func (c *RemoteClient) Lock(info *state.LockInfo) (string, error) { return "", nil } + info.Path = c.lockPath() + if info.ID == "" { lockID, err := uuid.GenerateUUID() if err != nil { From b4a735779c02d3ef33f902a1d934129ff250c5e1 Mon Sep 17 00:00:00 2001 From: Mathias Lafeldt Date: Tue, 18 Feb 2020 10:48:53 +0100 Subject: [PATCH 3/7] backend/remote-state/oss: Prepend bucket name to LockID To allow using the same Tablestore table with multiple OSS buckets. e.g. instead of env:/some/path/terraform.tfstate the LockID now becomes some-bucket/env:/some/path/terraform.tfstate --- backend/remote-state/oss/client.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/backend/remote-state/oss/client.go b/backend/remote-state/oss/client.go index 47073fb72..86ae4dc49 100644 --- a/backend/remote-state/oss/client.go +++ b/backend/remote-state/oss/client.go @@ -181,7 +181,7 @@ func (c *RemoteClient) Lock(info *state.LockInfo) (string, error) { Columns: []tablestore.AttributeColumn{ { ColumnName: "LockID", - Value: c.lockFile, + Value: c.lockPath(), }, { ColumnName: "Info", From 6bb22907a1527b3fe37c92531d42ebdbcfb0001c Mon Sep 17 00:00:00 2001 From: Mathias Lafeldt Date: Tue, 18 Feb 2020 14:45:06 +0100 Subject: [PATCH 4/7] backend/remote-state/oss: Fix state locking by using LockID as PK --- backend/remote-state/oss/backend_state.go | 18 +------ backend/remote-state/oss/backend_test.go | 4 +- backend/remote-state/oss/client.go | 61 ++++++----------------- 3 files changed, 17 insertions(+), 66 deletions(-) diff --git a/backend/remote-state/oss/backend_state.go b/backend/remote-state/oss/backend_state.go index af6f45628..7f7778012 100644 --- a/backend/remote-state/oss/backend_state.go +++ b/backend/remote-state/oss/backend_state.go @@ -39,28 +39,12 @@ func (b *Backend) remoteClient(name string) (*RemoteClient, error) { otsClient: b.otsClient, } if b.otsEndpoint != "" && b.otsTable != "" { - table, err := b.otsClient.DescribeTable(&tablestore.DescribeTableRequest{ + _, err := b.otsClient.DescribeTable(&tablestore.DescribeTableRequest{ TableName: b.otsTable, }) if err != nil { return client, fmt.Errorf("Error describing table store %s: %#v", b.otsTable, err) } - for _, t := range table.TableMeta.SchemaEntry { - pkMeta := TableStorePrimaryKeyMeta{ - PKName: *t.Name, - } - if *t.Type == tablestore.PrimaryKeyType_INTEGER { - pkMeta.PKType = "Integer" - } else if *t.Type == tablestore.PrimaryKeyType_STRING { - pkMeta.PKType = "String" - } else if *t.Type == tablestore.PrimaryKeyType_BINARY { - pkMeta.PKType = "Binary" - } else { - return client, fmt.Errorf("Unsupported PrimaryKey type: %d.", *t.Type) - } - client.otsTabkePK = pkMeta - break - } } return client, nil diff --git a/backend/remote-state/oss/backend_test.go b/backend/remote-state/oss/backend_test.go index c71ed2325..e292bdd7d 100644 --- a/backend/remote-state/oss/backend_test.go +++ b/backend/remote-state/oss/backend_test.go @@ -177,11 +177,11 @@ func deleteOSSBucket(t *testing.T, ossClient *oss.Client, bucketName string) { } } -// create the dynamoDB table, and wait until we can query it. +// create the tablestore table, and wait until we can query it. func createTablestoreTable(t *testing.T, otsClient *tablestore.TableStoreClient, tableName string) { tableMeta := new(tablestore.TableMeta) tableMeta.TableName = tableName - tableMeta.AddPrimaryKeyColumn("testbackend", tablestore.PrimaryKeyType_STRING) + tableMeta.AddPrimaryKeyColumn("LockID", tablestore.PrimaryKeyType_STRING) tableOption := new(tablestore.TableOption) tableOption.TimeToAlive = -1 diff --git a/backend/remote-state/oss/client.go b/backend/remote-state/oss/client.go index 86ae4dc49..0b3c5ad51 100644 --- a/backend/remote-state/oss/client.go +++ b/backend/remote-state/oss/client.go @@ -16,7 +16,6 @@ import ( "github.com/aliyun/aliyun-tablestore-go-sdk/tablestore" "github.com/hashicorp/go-multierror" uuid "github.com/hashicorp/go-uuid" - "github.com/hashicorp/terraform/helper/hashcode" "github.com/hashicorp/terraform/state" "github.com/hashicorp/terraform/state/remote" "github.com/pkg/errors" @@ -25,7 +24,6 @@ import ( // Store the last saved serial in tablestore with this suffix for consistency checks. const ( stateIDSuffix = "-md5" - statePKValue = "terraform-remote-state-lock" ) var ( @@ -40,11 +38,6 @@ var ( // test hook called when checksums don't match var testChecksumHook func() -type TableStorePrimaryKeyMeta struct { - PKName string - PKType string -} - type RemoteClient struct { ossClient *oss.Client otsClient *tablestore.TableStoreClient @@ -56,7 +49,6 @@ type RemoteClient struct { info *state.LockInfo mu sync.Mutex otsTable string - otsTabkePK TableStorePrimaryKeyMeta } func (c *RemoteClient) Get() (payload *remote.Payload, err error) { @@ -173,16 +165,12 @@ func (c *RemoteClient) Lock(info *state.LockInfo) (string, error) { PrimaryKey: &tablestore.PrimaryKey{ PrimaryKeys: []*tablestore.PrimaryKeyColumn{ { - ColumnName: c.otsTabkePK.PKName, - Value: c.getPKValue(), + ColumnName: "LockID", + Value: c.lockPath(), }, }, }, Columns: []tablestore.AttributeColumn{ - { - ColumnName: "LockID", - Value: c.lockPath(), - }, { ColumnName: "Info", Value: string(info.Marshal()), @@ -193,7 +181,7 @@ func (c *RemoteClient) Lock(info *state.LockInfo) (string, error) { }, } - log.Printf("[DEBUG] Recoring state lock in tablestore: %#v", putParams) + log.Printf("[DEBUG] Recording state lock in tablestore: %#v", putParams) _, err := c.otsClient.PutRow(&tablestore.PutRowRequest{ PutRowChange: putParams, @@ -226,8 +214,8 @@ func (c *RemoteClient) getMD5() ([]byte, error) { PrimaryKey: &tablestore.PrimaryKey{ PrimaryKeys: []*tablestore.PrimaryKeyColumn{ { - ColumnName: c.otsTabkePK.PKName, - Value: c.getPKValue(), + ColumnName: "LockID", + Value: c.lockPath() + stateIDSuffix, }, }, }, @@ -273,23 +261,19 @@ func (c *RemoteClient) putMD5(sum []byte) error { PrimaryKey: &tablestore.PrimaryKey{ PrimaryKeys: []*tablestore.PrimaryKeyColumn{ { - ColumnName: c.otsTabkePK.PKName, - Value: c.getPKValue(), + ColumnName: "LockID", + Value: c.lockPath() + stateIDSuffix, }, }, }, Columns: []tablestore.AttributeColumn{ - { - ColumnName: "LockID", - Value: c.lockPath() + stateIDSuffix, - }, { ColumnName: "Digest", Value: hex.EncodeToString(sum), }, }, Condition: &tablestore.RowCondition{ - RowExistenceExpectation: tablestore.RowExistenceExpectation_EXPECT_NOT_EXIST, + RowExistenceExpectation: tablestore.RowExistenceExpectation_IGNORE, }, } @@ -318,8 +302,8 @@ func (c *RemoteClient) deleteMD5() error { PrimaryKey: &tablestore.PrimaryKey{ PrimaryKeys: []*tablestore.PrimaryKeyColumn{ { - ColumnName: c.otsTabkePK.PKName, - Value: c.getPKValue(), + ColumnName: "LockID", + Value: c.lockPath() + stateIDSuffix, }, }, }, @@ -344,8 +328,8 @@ func (c *RemoteClient) getLockInfo() (*state.LockInfo, error) { PrimaryKey: &tablestore.PrimaryKey{ PrimaryKeys: []*tablestore.PrimaryKeyColumn{ { - ColumnName: c.otsTabkePK.PKName, - Value: c.getPKValue(), + ColumnName: "LockID", + Value: c.lockPath(), }, }, }, @@ -397,8 +381,8 @@ func (c *RemoteClient) Unlock(id string) error { PrimaryKey: &tablestore.PrimaryKey{ PrimaryKeys: []*tablestore.PrimaryKeyColumn{ { - ColumnName: c.otsTabkePK.PKName, - Value: c.getPKValue(), + ColumnName: "LockID", + Value: c.lockPath(), }, }, }, @@ -460,23 +444,6 @@ func (c *RemoteClient) getObj() (*remote.Payload, error) { return payload, nil } -func (c *RemoteClient) getPKValue() (value interface{}) { - value = statePKValue - if c.otsTabkePK.PKType == "Integer" { - value = hashcode.String(statePKValue) - } else if c.otsTabkePK.PKType == "Binary" { - value = stringToBin(statePKValue) - } - return -} - -func stringToBin(s string) (binString string) { - for _, c := range s { - binString = fmt.Sprintf("%s%b", binString, c) - } - return -} - const errBadChecksumFmt = `state data in OSS does not have the expected content. This may be caused by unusually long delays in OSS processing a previous state From a4178d12d6a591468f32dd2ee27ed519a6b5ebdb Mon Sep 17 00:00:00 2001 From: Mathias Lafeldt Date: Tue, 18 Feb 2020 16:32:34 +0100 Subject: [PATCH 5/7] Update website documentation for OSS backend --- website/docs/backends/types/oss.html.md | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/website/docs/backends/types/oss.html.md b/website/docs/backends/types/oss.html.md index 4cd04dda3..360765526 100644 --- a/website/docs/backends/types/oss.html.md +++ b/website/docs/backends/types/oss.html.md @@ -18,9 +18,6 @@ the `tablestore_table` field to an existing TableStore table name. -> **Note:** The OSS backend is available from terraform version 0.12.2. -!> **Warning:** If you set `tablestore_table`, please ensure the table does not contain primary key named -`LockID`, `Info` and `Digest`. Otherwise, there will throw an error `OTSParameterInvalid Duplicated attribute column ...`. - ## Example Configuration ```hcl @@ -39,7 +36,7 @@ terraform { This assumes we have a [OSS Bucket](https://www.terraform.io/docs/providers/alicloud/r/oss_bucket.html) created called `bucket-for-terraform-state`, a [OTS Instance](https://www.terraform.io/docs/providers/alicloud/r/ots_instance.html) called `terraform-remote` and a [OTS TableStore](https://www.terraform.io/docs/providers/alicloud/r/ots_table.html) called `statelock`. The -Terraform state will be written into the file `path/mystate/version-1.tfstate`. The `TableStore` must have a primary key of type `string`. +Terraform state will be written into the file `path/mystate/version-1.tfstate`. The `TableStore` must have a primary key named `LockID` of type `String`. ## Using the OSS remote state @@ -90,7 +87,7 @@ The following configuration options or environment variables are supported: * `prefix` - (Opeional) The path directory of the state file will be stored. Default to "env:". * `key` - (Optional) The name of the state file. Defaults to `terraform.tfstate`. * `tablestore_endpoint` / `ALICLOUD_TABLESTORE_ENDPOINT` - (Optional) A custom endpoint for the TableStore API. - * `tablestore_table` - (Optional) A TableStore table for state locking and consistency. + * `tablestore_table` - (Optional) A TableStore table for state locking and consistency. The table must have a primary key named `LockID` of type `String`. * `encrypt` - (Optional) Whether to enable server side encryption of the state file. If it is true, OSS will use 'AES256' encryption algorithm to encrypt state file. * `acl` - (Optional) [Object @@ -113,4 +110,3 @@ The nested `assume_role` block supports the following: * `session_expiration` - (Optional) The time after which the established session for assuming role expires. Valid value range: [900-3600] seconds. Default to 3600 (in this case Alibaba Cloud use own default value). It supports environment variable `ALICLOUD_ASSUME_ROLE_SESSION_EXPIRATION`. -> **Note:** If you want to store state in the custom OSS endpoint, you can specify a environment variable `OSS_ENDPOINT`, like "oss-cn-beijing-internal.aliyuncs.com" - From 07139e453ad5576171b2e1fbcee36f6d38773fc8 Mon Sep 17 00:00:00 2001 From: Mathias Lafeldt Date: Tue, 18 Feb 2020 18:47:15 +0100 Subject: [PATCH 6/7] backend/remote-state/oss: extract pkName constant --- backend/remote-state/oss/backend_test.go | 2 +- backend/remote-state/oss/client.go | 20 +++++++++++--------- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/backend/remote-state/oss/backend_test.go b/backend/remote-state/oss/backend_test.go index e292bdd7d..c81367bb4 100644 --- a/backend/remote-state/oss/backend_test.go +++ b/backend/remote-state/oss/backend_test.go @@ -181,7 +181,7 @@ func deleteOSSBucket(t *testing.T, ossClient *oss.Client, bucketName string) { func createTablestoreTable(t *testing.T, otsClient *tablestore.TableStoreClient, tableName string) { tableMeta := new(tablestore.TableMeta) tableMeta.TableName = tableName - tableMeta.AddPrimaryKeyColumn("LockID", tablestore.PrimaryKeyType_STRING) + tableMeta.AddPrimaryKeyColumn(pkName, tablestore.PrimaryKeyType_STRING) tableOption := new(tablestore.TableOption) tableOption.TimeToAlive = -1 diff --git a/backend/remote-state/oss/client.go b/backend/remote-state/oss/client.go index 0b3c5ad51..cd28458cb 100644 --- a/backend/remote-state/oss/client.go +++ b/backend/remote-state/oss/client.go @@ -21,9 +21,11 @@ import ( "github.com/pkg/errors" ) -// Store the last saved serial in tablestore with this suffix for consistency checks. const ( + // Store the last saved serial in tablestore with this suffix for consistency checks. stateIDSuffix = "-md5" + + pkName = "LockID" ) var ( @@ -165,7 +167,7 @@ func (c *RemoteClient) Lock(info *state.LockInfo) (string, error) { PrimaryKey: &tablestore.PrimaryKey{ PrimaryKeys: []*tablestore.PrimaryKeyColumn{ { - ColumnName: "LockID", + ColumnName: pkName, Value: c.lockPath(), }, }, @@ -214,12 +216,12 @@ func (c *RemoteClient) getMD5() ([]byte, error) { PrimaryKey: &tablestore.PrimaryKey{ PrimaryKeys: []*tablestore.PrimaryKeyColumn{ { - ColumnName: "LockID", + ColumnName: pkName, Value: c.lockPath() + stateIDSuffix, }, }, }, - ColumnsToGet: []string{"LockID", "Digest"}, + ColumnsToGet: []string{pkName, "Digest"}, MaxVersion: 1, } @@ -261,7 +263,7 @@ func (c *RemoteClient) putMD5(sum []byte) error { PrimaryKey: &tablestore.PrimaryKey{ PrimaryKeys: []*tablestore.PrimaryKeyColumn{ { - ColumnName: "LockID", + ColumnName: pkName, Value: c.lockPath() + stateIDSuffix, }, }, @@ -302,7 +304,7 @@ func (c *RemoteClient) deleteMD5() error { PrimaryKey: &tablestore.PrimaryKey{ PrimaryKeys: []*tablestore.PrimaryKeyColumn{ { - ColumnName: "LockID", + ColumnName: pkName, Value: c.lockPath() + stateIDSuffix, }, }, @@ -328,12 +330,12 @@ func (c *RemoteClient) getLockInfo() (*state.LockInfo, error) { PrimaryKey: &tablestore.PrimaryKey{ PrimaryKeys: []*tablestore.PrimaryKeyColumn{ { - ColumnName: "LockID", + ColumnName: pkName, Value: c.lockPath(), }, }, }, - ColumnsToGet: []string{"LockID", "Info"}, + ColumnsToGet: []string{pkName, "Info"}, MaxVersion: 1, } @@ -381,7 +383,7 @@ func (c *RemoteClient) Unlock(id string) error { PrimaryKey: &tablestore.PrimaryKey{ PrimaryKeys: []*tablestore.PrimaryKeyColumn{ { - ColumnName: "LockID", + ColumnName: pkName, Value: c.lockPath(), }, }, From 1f3a2c0e0217c1a7f032f59e522247dc63eaa5bc Mon Sep 17 00:00:00 2001 From: Mathias Lafeldt Date: Wed, 19 Feb 2020 11:41:56 +0100 Subject: [PATCH 7/7] backend/remote-state/oss: Add test revealing bug in state locking --- backend/remote-state/oss/client_test.go | 46 +++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/backend/remote-state/oss/client_test.go b/backend/remote-state/oss/client_test.go index 486217080..d3f0febec 100644 --- a/backend/remote-state/oss/client_test.go +++ b/backend/remote-state/oss/client_test.go @@ -85,6 +85,52 @@ func TestRemoteClientLocks(t *testing.T) { remote.TestRemoteLocks(t, s1.(*remote.State).Client, s2.(*remote.State).Client) } +// verify that the backend can handle more than one state in the same table +func TestRemoteClientLocks_multipleStates(t *testing.T) { + testACC(t) + bucketName := fmt.Sprintf("tf-remote-oss-test-force-%x", time.Now().Unix()) + tableName := fmt.Sprintf("tfRemoteTestForce%x", time.Now().Unix()) + path := "testState" + + b1 := backend.TestBackendConfig(t, New(), backend.TestWrapConfig(map[string]interface{}{ + "bucket": bucketName, + "prefix": path, + "encrypt": true, + "tablestore_table": tableName, + "tablestore_endpoint": RemoteTestUsedOTSEndpoint, + })).(*Backend) + + b2 := backend.TestBackendConfig(t, New(), backend.TestWrapConfig(map[string]interface{}{ + "bucket": bucketName, + "prefix": path, + "encrypt": true, + "tablestore_table": tableName, + "tablestore_endpoint": RemoteTestUsedOTSEndpoint, + })).(*Backend) + + createOSSBucket(t, b1.ossClient, bucketName) + defer deleteOSSBucket(t, b1.ossClient, bucketName) + createTablestoreTable(t, b1.otsClient, tableName) + defer deleteTablestoreTable(t, b1.otsClient, tableName) + + s1, err := b1.StateMgr("s1") + if err != nil { + t.Fatal(err) + } + if _, err := s1.Lock(state.NewLockInfo()); err != nil { + t.Fatal("failed to get lock for s1:", err) + } + + // s1 is now locked, s2 should not be locked as it's a different state file + s2, err := b2.StateMgr("s2") + if err != nil { + t.Fatal(err) + } + if _, err := s2.Lock(state.NewLockInfo()); err != nil { + t.Fatal("failed to get lock for s2:", err) + } +} + // verify that we can unlock a state with an existing lock func TestRemoteForceUnlock(t *testing.T) { testACC(t)