From b61a0f5ff86283763ae99a8193b85de6419f5dc1 Mon Sep 17 00:00:00 2001 From: yottta Date: Wed, 19 Feb 2025 11:25:14 +0200 Subject: [PATCH] Cleaning up and reworking some implementation for better readability Signed-off-by: yottta --- internal/backend/remote-state/s3/backend.go | 2 +- .../backend/remote-state/s3/backend_test.go | 2 +- internal/backend/remote-state/s3/client.go | 112 ++++++++---------- .../backend/remote-state/s3/client_test.go | 111 +++++++++++++++-- 4 files changed, 154 insertions(+), 73 deletions(-) diff --git a/internal/backend/remote-state/s3/backend.go b/internal/backend/remote-state/s3/backend.go index 45363649d0..5532993684 100644 --- a/internal/backend/remote-state/s3/backend.go +++ b/internal/backend/remote-state/s3/backend.go @@ -457,7 +457,7 @@ See details: https://cs.opensource.google/go/x/net/+/refs/tags/v0.17.0:http/http "use_lockfile": { Type: cty.Bool, Optional: true, - Description: "Manage locking with an S3 object", + Description: "Manage locking in the same configured S3 bucket", }, }, } diff --git a/internal/backend/remote-state/s3/backend_test.go b/internal/backend/remote-state/s3/backend_test.go index 8bf8c9db90..f9de2b1966 100644 --- a/internal/backend/remote-state/s3/backend_test.go +++ b/internal/backend/remote-state/s3/backend_test.go @@ -1810,7 +1810,7 @@ func unmarshalObject(dec cty.Value, atys map[string]cty.Type, path cty.Path) (ct return cty.ObjectVal(vals), nil } -func numberOfObjects(t *testing.T, ctx context.Context, s3Client *s3.Client, bucketName string) int { +func numberOfObjectsInBucket(t *testing.T, ctx context.Context, s3Client *s3.Client, bucketName string) int { resp, err := s3Client.ListObjects(ctx, &s3.ListObjectsInput{Bucket: &bucketName}) if err != nil { t.Fatalf("error getting objects from bucket %s: %v", bucketName, err) diff --git a/internal/backend/remote-state/s3/client.go b/internal/backend/remote-state/s3/client.go index d3d6aa59f6..4ec44ec2f4 100644 --- a/internal/backend/remote-state/s3/client.go +++ b/internal/backend/remote-state/s3/client.go @@ -276,38 +276,9 @@ func (c *RemoteClient) Delete() error { } func (c *RemoteClient) Lock(info *statemgr.LockInfo) (string, error) { - s3LockID, err := c.s3Lock(info) - if err != nil { - return "", err - } - dynamoLockID, err := c.dynamoDBLock(info) - if err != nil { - // when second lock fails to get acquired, release the initially acquired one - if uErr := c.s3Unlock(s3LockID); uErr != nil { - log.Printf("[WARN] failed to release the S3 lock on after failed to acquire the dynamoDD lock: %v", uErr) - } - return "", err - } - switch { - case c.useLockfile && c.ddbTable != "": - if s3LockID != dynamoLockID { - log.Printf("[WARN] acquiring s3 and dynamodb lock resulted in different lockIds. s3 lockId: %s; dynamodb lockId; %s", s3LockID, dynamoLockID) - } - return s3LockID, nil - case c.useLockfile: - return s3LockID, nil - case c.ddbTable != "": - return dynamoLockID, nil - } - return "", nil -} - -func (c *RemoteClient) dynamoDBLock(info *statemgr.LockInfo) (string, error) { - if c.ddbTable == "" { + if !c.IsLockingEnabled() { return "", nil } - info.Path = c.lockPath() - if info.ID == "" { lockID, err := uuid.GenerateUUID() if err != nil { @@ -317,6 +288,26 @@ func (c *RemoteClient) dynamoDBLock(info *statemgr.LockInfo) (string, error) { info.ID = lockID } + if err := c.s3Lock(info); err != nil { + return "", err + } + if err := c.dynamoDBLock(info); err != nil { + // when the second lock fails from getting acquired, release the initially acquired one + if uErr := c.s3Unlock(info.ID); uErr != nil { + log.Printf("[WARN] failed to release the S3 lock after failed to acquire the dynamoDD lock: %v", uErr) + } + return "", err + } + return info.ID, nil +} + +// dynamoDBLock expects the statemgr.LockInfo#ID to be filled already +func (c *RemoteClient) dynamoDBLock(info *statemgr.LockInfo) error { + if c.ddbTable == "" { + return nil + } + info.Path = c.lockPath() + putParams := &dynamodb.PutItemInput{ Item: map[string]dtypes.AttributeValue{ "LockID": &dtypes.AttributeValueMemberS{Value: c.lockPath()}, @@ -329,7 +320,7 @@ func (c *RemoteClient) dynamoDBLock(info *statemgr.LockInfo) (string, error) { ctx := context.TODO() _, err := c.dynClient.PutItem(ctx, putParams) if err != nil { - lockInfo, infoErr := c.getLockInfo(ctx) + lockInfo, infoErr := c.getLockInfoFromDynamoDB(ctx) if infoErr != nil { err = multierror.Append(err, infoErr) } @@ -338,26 +329,19 @@ func (c *RemoteClient) dynamoDBLock(info *statemgr.LockInfo) (string, error) { Err: err, Info: lockInfo, } - return "", lockErr + return lockErr } - return info.ID, nil + return nil } -func (c *RemoteClient) s3Lock(info *statemgr.LockInfo) (string, error) { +// s3Lock expects the statemgr.LockInfo#ID to be filled already +func (c *RemoteClient) s3Lock(info *statemgr.LockInfo) error { if !c.useLockfile { - return "", nil + return nil } info.Path = c.lockPath() - if info.ID == "" { - lockID, err := uuid.GenerateUUID() - if err != nil { - return "", err - } - info.ID = lockID - } - lInfo := info.Marshal() putParams := &s3.PutObjectInput{ ContentType: aws.String(contentTypeJSON), @@ -372,7 +356,7 @@ func (c *RemoteClient) s3Lock(info *statemgr.LockInfo) (string, error) { ctx, _ = attachLoggerToContext(ctx) _, err := c.s3Client.PutObject(ctx, putParams) if err != nil { - lockInfo, infoErr := c.getS3LockInfo(ctx) + lockInfo, infoErr := c.getLockInfoFromS3(ctx) if infoErr != nil { err = multierror.Append(err, infoErr) } @@ -381,10 +365,10 @@ func (c *RemoteClient) s3Lock(info *statemgr.LockInfo) (string, error) { Err: err, Info: lockInfo, } - return "", lockErr + return lockErr } - return info.ID, nil + return nil } func (c *RemoteClient) getMD5(ctx context.Context) ([]byte, error) { @@ -464,7 +448,7 @@ func (c *RemoteClient) deleteMD5(ctx context.Context) error { return nil } -func (c *RemoteClient) getLockInfo(ctx context.Context) (*statemgr.LockInfo, error) { +func (c *RemoteClient) getLockInfoFromDynamoDB(ctx context.Context) (*statemgr.LockInfo, error) { getParams := &dynamodb.GetItemInput{ Key: map[string]dtypes.AttributeValue{ "LockID": &dtypes.AttributeValueMemberS{Value: c.lockPath()}, @@ -499,7 +483,7 @@ func (c *RemoteClient) getLockInfo(ctx context.Context) (*statemgr.LockInfo, err return lockInfo, nil } -func (c *RemoteClient) getS3LockInfo(ctx context.Context) (*statemgr.LockInfo, error) { +func (c *RemoteClient) getLockInfoFromS3(ctx context.Context) (*statemgr.LockInfo, error) { getParams := &s3.GetObjectInput{ Bucket: aws.String(c.bucketName), Key: aws.String(c.lockFilePath()), @@ -539,25 +523,31 @@ func (c *RemoteClient) getS3LockInfo(ctx context.Context) (*statemgr.LockInfo, e } func (c *RemoteClient) Unlock(id string) error { - if c.useLockfile { - if err := c.s3Unlock(id); err != nil { - return err - } - } - if c.ddbTable != "" { - if err := c.dynamoDBUnlock(id); err != nil { - return err - } + // Attempt to release the lock from both sources. + // We want to do so to be sure that we are leaving no locks unhandled + s3Err := c.s3Unlock(id) + dynamoDBErr := c.dynamoDBUnlock(id) + switch { + case s3Err != nil && dynamoDBErr != nil: + s3Err.Err = multierror.Append(s3Err.Err, dynamoDBErr.Err) + return s3Err + case s3Err != nil: + return s3Err + case dynamoDBErr != nil: + return dynamoDBErr } return nil } -func (c *RemoteClient) s3Unlock(id string) error { +func (c *RemoteClient) s3Unlock(id string) *statemgr.LockError { + if !c.useLockfile { + return nil + } lockErr := &statemgr.LockError{} ctx := context.TODO() ctx, _ = attachLoggerToContext(ctx) - lockInfo, err := c.getS3LockInfo(ctx) + lockInfo, err := c.getLockInfoFromS3(ctx) if err != nil { lockErr.Err = fmt.Errorf("failed to retrieve s3 lock info: %w", err) return lockErr @@ -582,7 +572,7 @@ func (c *RemoteClient) s3Unlock(id string) error { return nil } -func (c *RemoteClient) dynamoDBUnlock(id string) error { +func (c *RemoteClient) dynamoDBUnlock(id string) *statemgr.LockError { if c.ddbTable == "" { return nil } @@ -590,7 +580,7 @@ func (c *RemoteClient) dynamoDBUnlock(id string) error { lockErr := &statemgr.LockError{} ctx := context.TODO() - lockInfo, err := c.getLockInfo(ctx) + lockInfo, err := c.getLockInfoFromDynamoDB(ctx) if err != nil { lockErr.Err = fmt.Errorf("failed to retrieve lock info: %w", err) return lockErr diff --git a/internal/backend/remote-state/s3/client_test.go b/internal/backend/remote-state/s3/client_test.go index e3a5b9880f..56218068cc 100644 --- a/internal/backend/remote-state/s3/client_test.go +++ b/internal/backend/remote-state/s3/client_test.go @@ -59,7 +59,6 @@ func TestRemoteClientLocks(t *testing.T) { "key": keyName, "encrypt": true, "dynamodb_table": bucketName, - // "use_path_style": true, // NOTE: enable this to test against localstack (https://docs.localstack.cloud/getting-started/) })).(*Backend) b2 := backend.TestBackendConfig(t, New(encryption.StateEncryptionDisabled()), backend.TestWrapConfig(map[string]interface{}{ @@ -67,7 +66,6 @@ func TestRemoteClientLocks(t *testing.T) { "key": keyName, "encrypt": true, "dynamodb_table": bucketName, - // "use_path_style": true, // NOTE: enable this to test against localstack (https://docs.localstack.cloud/getting-started/) })).(*Backend) ctx := context.TODO() @@ -99,7 +97,6 @@ func TestRemoteS3ClientLocks(t *testing.T) { "key": keyName, "encrypt": true, "use_lockfile": true, - // "use_path_style": true, // NOTE: enable this to test against localstack (https://docs.localstack.cloud/getting-started/) })).(*Backend) b2, _ := backend.TestBackendConfig(t, New(encryption.StateEncryptionDisabled()), backend.TestWrapConfig(map[string]interface{}{ @@ -107,7 +104,6 @@ func TestRemoteS3ClientLocks(t *testing.T) { "key": keyName, "encrypt": true, "use_lockfile": true, - // "use_path_style": true, // NOTE: enable this to test against localstack (https://docs.localstack.cloud/getting-started/) })).(*Backend) ctx := context.TODO() @@ -138,7 +134,6 @@ func TestRemoteS3AndDynamoDBClientLocks(t *testing.T) { "key": keyName, "dynamodb_table": bucketName, "encrypt": true, - // "use_path_style": true, // NOTE: enable this to test against localstack (https://docs.localstack.cloud/getting-started/) })).(*Backend) b2, _ := backend.TestBackendConfig(t, New(encryption.StateEncryptionDisabled()), backend.TestWrapConfig(map[string]interface{}{ @@ -147,7 +142,6 @@ func TestRemoteS3AndDynamoDBClientLocks(t *testing.T) { "dynamodb_table": bucketName, "encrypt": true, "use_lockfile": true, - // "use_path_style": true, // NOTE: enable this to test against localstack (https://docs.localstack.cloud/getting-started/) })).(*Backend) ctx := context.TODO() @@ -188,7 +182,6 @@ func TestRemoteS3AndDynamoDBClientLocksWithNoDBInstance(t *testing.T) { "dynamodb_table": bucketName, "encrypt": true, "use_lockfile": true, - // "use_path_style": true, // NOTE: enable this to test against localstack (https://docs.localstack.cloud/getting-started/) })).(*Backend) ctx := context.TODO() @@ -209,7 +202,7 @@ func TestRemoteS3AndDynamoDBClientLocksWithNoDBInstance(t *testing.T) { } expected := 0 - if actual := numberOfObjects(t, ctx, b1.s3Client, bucketName); actual != expected { + if actual := numberOfObjectsInBucket(t, ctx, b1.s3Client, bucketName); actual != expected { t.Fatalf("expected to have %d objects but got %d", expected, actual) } } @@ -318,7 +311,6 @@ func TestForceUnlockS3Only(t *testing.T) { "key": keyName, "encrypt": true, "use_lockfile": true, - // "use_path_style": true, // NOTE: enable this to test against localstack (https://docs.localstack.cloud/getting-started/) })).(*Backend) b2, _ := backend.TestBackendConfig(t, New(encryption.StateEncryptionDisabled()), backend.TestWrapConfig(map[string]interface{}{ @@ -326,7 +318,6 @@ func TestForceUnlockS3Only(t *testing.T) { "key": keyName, "encrypt": true, "use_lockfile": true, - // "use_path_style": true, // NOTE: enable this to test against localstack (https://docs.localstack.cloud/getting-started/) })).(*Backend) ctx := context.TODO() @@ -400,6 +391,106 @@ func TestForceUnlockS3Only(t *testing.T) { } } +// verify that we can unlock a state with an existing lock +func TestForceUnlockS3AndDynamo(t *testing.T) { + testACC(t) + bucketName := fmt.Sprintf("%s-force-s3-dynamo-%x", testBucketPrefix, time.Now().Unix()) + keyName := "testState" + + b1, _ := backend.TestBackendConfig(t, New(encryption.StateEncryptionDisabled()), backend.TestWrapConfig(map[string]interface{}{ + "bucket": bucketName, + "key": keyName, + "encrypt": true, + "use_lockfile": true, + "dynamodb_table": bucketName, + })).(*Backend) + + b2, _ := backend.TestBackendConfig(t, New(encryption.StateEncryptionDisabled()), backend.TestWrapConfig(map[string]interface{}{ + "bucket": bucketName, + "key": keyName, + "encrypt": true, + "use_lockfile": true, + "dynamodb_table": bucketName, + })).(*Backend) + + ctx := context.TODO() + createS3Bucket(ctx, t, b1.s3Client, bucketName, b1.awsConfig.Region) + defer deleteS3Bucket(ctx, t, b1.s3Client, bucketName) + createDynamoDBTable(ctx, t, b1.dynClient, bucketName) + defer deleteDynamoDBTable(ctx, t, b1.dynClient, bucketName) + + // first test with default + s1, err := b1.StateMgr(backend.DefaultStateName) + if err != nil { + t.Fatal(err) + } + + info := statemgr.NewLockInfo() + info.Operation = "test" + info.Who = "clientA" + + lockID, err := s1.Lock(info) + if err != nil { + t.Fatal("unable to get initial lock:", err) + } + + // s1 is now locked, get the same state through s2 and unlock it + s2, err := b2.StateMgr(backend.DefaultStateName) + if err != nil { + t.Fatal("failed to get default state to force unlock:", err) + } + + if err = s2.Unlock(lockID); err != nil { + t.Fatal("failed to force-unlock default state") + } + + // now try the same thing with a named state + // first test with default + s1, err = b1.StateMgr("test") + if err != nil { + t.Fatal(err) + } + + info = statemgr.NewLockInfo() + info.Operation = "test" + info.Who = "clientA" + + lockID, err = s1.Lock(info) + if err != nil { + t.Fatal("unable to get initial lock:", err) + } + + // s1 is now locked, get the same state through s2 and unlock it + s2, err = b2.StateMgr("test") + if err != nil { + t.Fatal("failed to get named state to force unlock:", err) + } + + if err = s2.Unlock(lockID); err != nil { + t.Fatal("failed to force-unlock named state") + } + + // No State lock information found for the new workspace. The client should throw the appropriate error message. + secondWorkspace := "new-workspace" + s2, err = b2.StateMgr(secondWorkspace) + if err != nil { + t.Fatal(err) + } + err = s2.Unlock(lockID) + if err == nil { + t.Fatal("expected an error to occur:", err) + } + expectedErrorMsg := []error{ + fmt.Errorf("failed to retrieve s3 lock info: operation error S3: GetObject, https response error StatusCode: 404"), + fmt.Errorf("failed to retrieve lock info: no lock info found for: \"%s/env:/%s/%s\" within the DynamoDB table: %s", bucketName, secondWorkspace, keyName, bucketName), + } + for _, expectedErr := range expectedErrorMsg { + if !strings.Contains(err.Error(), expectedErr.Error()) { + t.Errorf("Unlock() should contain expected.\nactual = %v\nexpected = %v", err, expectedErr) + } + } +} + func TestRemoteClient_clientMD5(t *testing.T) { testACC(t)