Skip to content

Commit f622110

Browse files
jbardinjbardin
authored
Merge pull request #24149 from mlafeldt/fix-oss-state-locking
Fix & improve state locking of OSS backend
2 parents 51bdcbb + 1f3a2c0 commit f622110

File tree

6 files changed

+89
-87
lines changed

6 files changed

+89
-87
lines changed

‎backend/remote-state/oss/backend.go‎

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,11 @@ import (
44
"context"
55
"encoding/json"
66
"fmt"
7+
"io/ioutil"
8+
"os"
9+
"runtime"
10+
"strings"
11+
712
"github.com/aliyun/alibaba-cloud-sdk-go/sdk/requests"
813
"github.com/aliyun/alibaba-cloud-sdk-go/sdk/responses"
914
"github.com/aliyun/alibaba-cloud-sdk-go/services/sts"
@@ -12,10 +17,11 @@ import (
1217
"github.com/hashicorp/terraform/helper/schema"
1318
"github.com/hashicorp/terraform/helper/validation"
1419
"github.com/jmespath/go-jmespath"
15-
"io/ioutil"
16-
"os"
17-
"runtime"
18-
"strings"
20+
21+
"log"
22+
"net/http"
23+
"strconv"
24+
"time"
1925

2026
"github.com/aliyun/alibaba-cloud-sdk-go/sdk"
2127
"github.com/aliyun/alibaba-cloud-sdk-go/sdk/auth/credentials"
@@ -24,10 +30,6 @@ import (
2430
"github.com/hashicorp/go-cleanhttp"
2531
"github.com/hashicorp/terraform/version"
2632
"github.com/mitchellh/go-homedir"
27-
"log"
28-
"net/http"
29-
"strconv"
30-
"time"
3133
)
3234

3335
// New creates a new backend for OSS remote state.

‎backend/remote-state/oss/backend_state.go‎

Lines changed: 3 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -12,9 +12,10 @@ import (
1212
"github.com/hashicorp/terraform/state/remote"
1313
"github.com/hashicorp/terraform/states"
1414

15-
"github.com/aliyun/aliyun-tablestore-go-sdk/tablestore"
1615
"log"
1716
"path"
17+
18+
"github.com/aliyun/aliyun-tablestore-go-sdk/tablestore"
1819
)
1920

2021
const (
@@ -38,28 +39,12 @@ func (b *Backend) remoteClient(name string) (*RemoteClient, error) {
3839
otsClient: b.otsClient,
3940
}
4041
if b.otsEndpoint != "" && b.otsTable != "" {
41-
table, err := b.otsClient.DescribeTable(&tablestore.DescribeTableRequest{
42+
_, err := b.otsClient.DescribeTable(&tablestore.DescribeTableRequest{
4243
TableName: b.otsTable,
4344
})
4445
if err != nil {
4546
return client, fmt.Errorf("Error describing table store %s: %#v", b.otsTable, err)
4647
}
47-
for _, t := range table.TableMeta.SchemaEntry {
48-
pkMeta := TableStorePrimaryKeyMeta{
49-
PKName: *t.Name,
50-
}
51-
if *t.Type == tablestore.PrimaryKeyType_INTEGER {
52-
pkMeta.PKType = "Integer"
53-
} else if *t.Type == tablestore.PrimaryKeyType_STRING {
54-
pkMeta.PKType = "String"
55-
} else if *t.Type == tablestore.PrimaryKeyType_BINARY {
56-
pkMeta.PKType = "Binary"
57-
} else {
58-
return client, fmt.Errorf("Unsupported PrimaryKey type: %d.", *t.Type)
59-
}
60-
client.otsTabkePK = pkMeta
61-
break
62-
}
6348
}
6449

6550
return client, nil

‎backend/remote-state/oss/backend_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -177,11 +177,11 @@ func deleteOSSBucket(t *testing.T, ossClient *oss.Client, bucketName string) {
177177
}
178178
}
179179

180-
// create the dynamoDB table, and wait until we can query it.
180+
// create the tablestore table, and wait until we can query it.
181181
func createTablestoreTable(t *testing.T, otsClient *tablestore.TableStoreClient, tableName string) {
182182
tableMeta := new(tablestore.TableMeta)
183183
tableMeta.TableName = tableName
184-
tableMeta.AddPrimaryKeyColumn("testbackend", tablestore.PrimaryKeyType_STRING)
184+
tableMeta.AddPrimaryKeyColumn(pkName, tablestore.PrimaryKeyType_STRING)
185185

186186
tableOption := new(tablestore.TableOption)
187187
tableOption.TimeToAlive = -1

‎backend/remote-state/oss/client.go‎

Lines changed: 25 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -8,23 +8,24 @@ import (
88
"io"
99

1010
"encoding/hex"
11+
"log"
12+
"sync"
13+
"time"
14+
1115
"github.com/aliyun/aliyun-oss-go-sdk/oss"
1216
"github.com/aliyun/aliyun-tablestore-go-sdk/tablestore"
1317
"github.com/hashicorp/go-multierror"
1418
uuid "github.com/hashicorp/go-uuid"
15-
"github.com/hashicorp/terraform/helper/hashcode"
1619
"github.com/hashicorp/terraform/state"
1720
"github.com/hashicorp/terraform/state/remote"
1821
"github.com/pkg/errors"
19-
"log"
20-
"sync"
21-
"time"
2222
)
2323

24-
// Store the last saved serial in tablestore with this suffix for consistency checks.
2524
const (
25+
// Store the last saved serial in tablestore with this suffix for consistency checks.
2626
stateIDSuffix = "-md5"
27-
statePKValue = "terraform-remote-state-lock"
27+
28+
pkName = "LockID"
2829
)
2930

3031
var (
@@ -39,11 +40,6 @@ var (
3940
// test hook called when checksums don't match
4041
var testChecksumHook func()
4142

42-
type TableStorePrimaryKeyMeta struct {
43-
PKName string
44-
PKType string
45-
}
46-
4743
type RemoteClient struct {
4844
ossClient *oss.Client
4945
otsClient *tablestore.TableStoreClient
@@ -55,7 +51,6 @@ type RemoteClient struct {
5551
info *state.LockInfo
5652
mu sync.Mutex
5753
otsTable string
58-
otsTabkePK TableStorePrimaryKeyMeta
5954
}
6055

6156
func (c *RemoteClient) Get() (payload *remote.Payload, err error) {
@@ -157,6 +152,8 @@ func (c *RemoteClient) Lock(info *state.LockInfo) (string, error) {
157152
return "", nil
158153
}
159154

155+
info.Path = c.lockPath()
156+
160157
if info.ID == "" {
161158
lockID, err := uuid.GenerateUUID()
162159
if err != nil {
@@ -170,16 +167,12 @@ func (c *RemoteClient) Lock(info *state.LockInfo) (string, error) {
170167
PrimaryKey: &tablestore.PrimaryKey{
171168
PrimaryKeys: []*tablestore.PrimaryKeyColumn{
172169
{
173-
ColumnName: c.otsTabkePK.PKName,
174-
Value: c.getPKValue(),
170+
ColumnName: pkName,
171+
Value: c.lockPath(),
175172
},
176173
},
177174
},
178175
Columns: []tablestore.AttributeColumn{
179-
{
180-
ColumnName: "LockID",
181-
Value: c.lockFile,
182-
},
183176
{
184177
ColumnName: "Info",
185178
Value: string(info.Marshal()),
@@ -190,7 +183,7 @@ func (c *RemoteClient) Lock(info *state.LockInfo) (string, error) {
190183
},
191184
}
192185

193-
log.Printf("[DEBUG] Recoring state lock in tablestore: %#v", putParams)
186+
log.Printf("[DEBUG] Recording state lock in tablestore: %#v", putParams)
194187

195188
_, err := c.otsClient.PutRow(&tablestore.PutRowRequest{
196189
PutRowChange: putParams,
@@ -223,12 +216,12 @@ func (c *RemoteClient) getMD5() ([]byte, error) {
223216
PrimaryKey: &tablestore.PrimaryKey{
224217
PrimaryKeys: []*tablestore.PrimaryKeyColumn{
225218
{
226-
ColumnName: c.otsTabkePK.PKName,
227-
Value: c.getPKValue(),
219+
ColumnName: pkName,
220+
Value: c.lockPath() + stateIDSuffix,
228221
},
229222
},
230223
},
231-
ColumnsToGet: []string{"LockID", "Digest"},
224+
ColumnsToGet: []string{pkName, "Digest"},
232225
MaxVersion: 1,
233226
}
234227

@@ -270,23 +263,19 @@ func (c *RemoteClient) putMD5(sum []byte) error {
270263
PrimaryKey: &tablestore.PrimaryKey{
271264
PrimaryKeys: []*tablestore.PrimaryKeyColumn{
272265
{
273-
ColumnName: c.otsTabkePK.PKName,
274-
Value: c.getPKValue(),
266+
ColumnName: pkName,
267+
Value: c.lockPath() + stateIDSuffix,
275268
},
276269
},
277270
},
278271
Columns: []tablestore.AttributeColumn{
279-
{
280-
ColumnName: "LockID",
281-
Value: c.lockPath() + stateIDSuffix,
282-
},
283272
{
284273
ColumnName: "Digest",
285274
Value: hex.EncodeToString(sum),
286275
},
287276
},
288277
Condition: &tablestore.RowCondition{
289-
RowExistenceExpectation: tablestore.RowExistenceExpectation_EXPECT_NOT_EXIST,
278+
RowExistenceExpectation: tablestore.RowExistenceExpectation_IGNORE,
290279
},
291280
}
292281

@@ -315,8 +304,8 @@ func (c *RemoteClient) deleteMD5() error {
315304
PrimaryKey: &tablestore.PrimaryKey{
316305
PrimaryKeys: []*tablestore.PrimaryKeyColumn{
317306
{
318-
ColumnName: c.otsTabkePK.PKName,
319-
Value: c.getPKValue(),
307+
ColumnName: pkName,
308+
Value: c.lockPath() + stateIDSuffix,
320309
},
321310
},
322311
},
@@ -341,12 +330,12 @@ func (c *RemoteClient) getLockInfo() (*state.LockInfo, error) {
341330
PrimaryKey: &tablestore.PrimaryKey{
342331
PrimaryKeys: []*tablestore.PrimaryKeyColumn{
343332
{
344-
ColumnName: c.otsTabkePK.PKName,
345-
Value: c.getPKValue(),
333+
ColumnName: pkName,
334+
Value: c.lockPath(),
346335
},
347336
},
348337
},
349-
ColumnsToGet: []string{"LockID", "Info"},
338+
ColumnsToGet: []string{pkName, "Info"},
350339
MaxVersion: 1,
351340
}
352341

@@ -394,8 +383,8 @@ func (c *RemoteClient) Unlock(id string) error {
394383
PrimaryKey: &tablestore.PrimaryKey{
395384
PrimaryKeys: []*tablestore.PrimaryKeyColumn{
396385
{
397-
ColumnName: c.otsTabkePK.PKName,
398-
Value: c.getPKValue(),
386+
ColumnName: pkName,
387+
Value: c.lockPath(),
399388
},
400389
},
401390
},
@@ -457,23 +446,6 @@ func (c *RemoteClient) getObj() (*remote.Payload, error) {
457446
return payload, nil
458447
}
459448

460-
func (c *RemoteClient) getPKValue() (value interface{}) {
461-
value = statePKValue
462-
if c.otsTabkePK.PKType == "Integer" {
463-
value = hashcode.String(statePKValue)
464-
} else if c.otsTabkePK.PKType == "Binary" {
465-
value = stringToBin(statePKValue)
466-
}
467-
return
468-
}
469-
470-
func stringToBin(s string) (binString string) {
471-
for _, c := range s {
472-
binString = fmt.Sprintf("%s%b", binString, c)
473-
}
474-
return
475-
}
476-
477449
const errBadChecksumFmt = `state data in OSS does not have the expected content.
478450
479451
This may be caused by unusually long delays in OSS processing a previous state

‎backend/remote-state/oss/client_test.go‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88

99
"bytes"
1010
"crypto/md5"
11+
1112
"github.com/hashicorp/terraform/backend"
1213
"github.com/hashicorp/terraform/state"
1314
"github.com/hashicorp/terraform/state/remote"
@@ -84,6 +85,52 @@ func TestRemoteClientLocks(t *testing.T) {
8485
remote.TestRemoteLocks(t, s1.(*remote.State).Client, s2.(*remote.State).Client)
8586
}
8687

88+
// verify that the backend can handle more than one state in the same table
89+
func TestRemoteClientLocks_multipleStates(t *testing.T) {
90+
testACC(t)
91+
bucketName := fmt.Sprintf("tf-remote-oss-test-force-%x", time.Now().Unix())
92+
tableName := fmt.Sprintf("tfRemoteTestForce%x", time.Now().Unix())
93+
path := "testState"
94+
95+
b1 := backend.TestBackendConfig(t, New(), backend.TestWrapConfig(map[string]interface{}{
96+
"bucket": bucketName,
97+
"prefix": path,
98+
"encrypt": true,
99+
"tablestore_table": tableName,
100+
"tablestore_endpoint": RemoteTestUsedOTSEndpoint,
101+
})).(*Backend)
102+
103+
b2 := backend.TestBackendConfig(t, New(), backend.TestWrapConfig(map[string]interface{}{
104+
"bucket": bucketName,
105+
"prefix": path,
106+
"encrypt": true,
107+
"tablestore_table": tableName,
108+
"tablestore_endpoint": RemoteTestUsedOTSEndpoint,
109+
})).(*Backend)
110+
111+
createOSSBucket(t, b1.ossClient, bucketName)
112+
defer deleteOSSBucket(t, b1.ossClient, bucketName)
113+
createTablestoreTable(t, b1.otsClient, tableName)
114+
defer deleteTablestoreTable(t, b1.otsClient, tableName)
115+
116+
s1, err := b1.StateMgr("s1")
117+
if err != nil {
118+
t.Fatal(err)
119+
}
120+
if _, err := s1.Lock(state.NewLockInfo()); err != nil {
121+
t.Fatal("failed to get lock for s1:", err)
122+
}
123+
124+
// s1 is now locked, s2 should not be locked as it's a different state file
125+
s2, err := b2.StateMgr("s2")
126+
if err != nil {
127+
t.Fatal(err)
128+
}
129+
if _, err := s2.Lock(state.NewLockInfo()); err != nil {
130+
t.Fatal("failed to get lock for s2:", err)
131+
}
132+
}
133+
87134
// verify that we can unlock a state with an existing lock
88135
func TestRemoteForceUnlock(t *testing.T) {
89136
testACC(t)

‎website/docs/backends/types/oss.html.md‎

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -18,9 +18,6 @@ the `tablestore_table` field to an existing TableStore table name.
1818

1919
-> **Note:** The OSS backend is available from terraform version 0.12.2.
2020

21-
!> **Warning:** If you set `tablestore_table`, please ensure the table does not contain primary key named
22-
`LockID`, `Info` and `Digest`. Otherwise, there will throw an error `OTSParameterInvalid Duplicated attribute column ...`.
23-
2421
## Example Configuration
2522

2623
```hcl
@@ -39,7 +36,7 @@ terraform {
3936
This assumes we have a [OSS Bucket](https://www.terraform.io/docs/providers/alicloud/r/oss_bucket.html) created called `bucket-for-terraform-state`,
4037
a [OTS Instance](https://www.terraform.io/docs/providers/alicloud/r/ots_instance.html) called `terraform-remote` and
4138
a [OTS TableStore](https://www.terraform.io/docs/providers/alicloud/r/ots_table.html) called `statelock`. The
42-
Terraform state will be written into the file `path/mystate/version-1.tfstate`. The `TableStore` must have a primary key of type `string`.
39+
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`.
4340

4441

4542
## Data Source Configuration
@@ -90,7 +87,7 @@ The following configuration options or environment variables are supported:
9087
* `prefix` - (Opeional) The path directory of the state file will be stored. Default to "env:".
9188
* `key` - (Optional) The name of the state file. Defaults to `terraform.tfstate`.
9289
* `tablestore_endpoint` / `ALICLOUD_TABLESTORE_ENDPOINT` - (Optional) A custom endpoint for the TableStore API.
93-
* `tablestore_table` - (Optional) A TableStore table for state locking and consistency.
90+
* `tablestore_table` - (Optional) A TableStore table for state locking and consistency. The table must have a primary key named `LockID` of type `String`.
9491
* `encrypt` - (Optional) Whether to enable server side
9592
encryption of the state file. If it is true, OSS will use 'AES256' encryption algorithm to encrypt state file.
9693
* `acl` - (Optional) [Object
@@ -113,4 +110,3 @@ The nested `assume_role` block supports the following:
113110
* `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`.
114111

115112
-> **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"
116-

0 commit comments

Comments
 (0)