From 4498299781280aded74efb82ad71eb041fac3273 Mon Sep 17 00:00:00 2001 From: Bruno Schaatsbergen Date: Wed, 27 Nov 2024 16:25:01 +0000 Subject: [PATCH 1/3] backport of commit b667207fa738c9b0daa6181d50cdc1206ac69215 --- internal/backend/remote-state/s3/client.go | 10 +++-- .../backend/remote-state/s3/client_test.go | 40 +++++++++++++++++++ 2 files changed, 47 insertions(+), 3 deletions(-) diff --git a/internal/backend/remote-state/s3/client.go b/internal/backend/remote-state/s3/client.go index e56703bef908..5014d9236170 100644 --- a/internal/backend/remote-state/s3/client.go +++ b/internal/backend/remote-state/s3/client.go @@ -346,18 +346,21 @@ func (c *RemoteClient) Lock(info *statemgr.LockInfo) (string, error) { // exist, the operation will succeed, acquiring the lock. If the lock file already exists, the operation // will fail due to a conditional write, indicating that the lock is already held by another Terraform client. func (c *RemoteClient) lockWithFile(ctx context.Context, info *statemgr.LockInfo, log hclog.Logger) error { - lockFileJson, err := json.Marshal(info) + data, err := json.Marshal(info) if err != nil { return err } input := &s3.PutObjectInput{ ContentType: aws.String("application/json"), - Body: bytes.NewReader(lockFileJson), + Body: bytes.NewReader(data), Bucket: aws.String(c.bucketName), Key: aws.String(c.lockFilePath), IfNoneMatch: aws.String("*"), } + if !c.skipS3Checksum { + input.ChecksumAlgorithm = s3types.ChecksumAlgorithmSha256 + } if c.serverSideEncryption { if c.kmsKeyID != "" { @@ -378,7 +381,8 @@ func (c *RemoteClient) lockWithFile(ctx context.Context, info *statemgr.LockInfo log.Debug("Uploading lock file") - _, err = c.s3Client.PutObject(ctx, input) + uploader := manager.NewUploader(c.s3Client, func(u *manager.Uploader) {}) + _, err = uploader.Upload(ctx, input) if err != nil { // Attempt to retrieve lock info from the file, and merge errors if it fails. lockInfo, infoErr := c.getLockInfoWithFile(ctx) diff --git a/internal/backend/remote-state/s3/client_test.go b/internal/backend/remote-state/s3/client_test.go index a1c7722ea1cf..c08729c72fcc 100644 --- a/internal/backend/remote-state/s3/client_test.go +++ b/internal/backend/remote-state/s3/client_test.go @@ -382,6 +382,46 @@ func TestRemoteClientPutLargeUploadWithObjectLock(t *testing.T) { } } +func TestRemoteClientObjectLockAndLockFile(t *testing.T) { + testACC(t) + objectLockPreCheck(t) + + ctx := context.TODO() + + bucketName := fmt.Sprintf("terraform-remote-s3-test-%x", time.Now().Unix()) + keyName := "testState" + + b := backend.TestBackendConfig(t, New(), backend.TestWrapConfig(map[string]interface{}{ + "bucket": bucketName, + "key": keyName, + "use_lockfile": true, + })).(*Backend) + + createS3Bucket(ctx, t, b.s3Client, bucketName, b.awsConfig.Region, + s3BucketWithVersioning, + s3BucketWithObjectLock(s3types.ObjectLockRetentionModeCompliance), + ) + defer deleteS3Bucket(ctx, t, b.s3Client, bucketName, b.awsConfig.Region) + + s1, err := b.StateMgr(backend.DefaultStateName) + if err != nil { + t.Fatal(err) + } + client := s1.(*remote.State).Client + + var state bytes.Buffer + dataW := io.LimitReader(neverEnding('x'), manager.DefaultUploadPartSize) + _, err = state.ReadFrom(dataW) + if err != nil { + t.Fatalf("writing dummy data: %s", err) + } + + err = client.Put(state.Bytes()) + if err != nil { + t.Fatalf("putting data: %s", err) + } +} + type neverEnding byte func (b neverEnding) Read(p []byte) (n int, err error) { From 3774cf9b9d18255042ead4c9f091dc27f6374362 Mon Sep 17 00:00:00 2001 From: Jared Baker Date: Mon, 2 Dec 2024 12:31:50 -0500 Subject: [PATCH 2/3] backend/s3: sync with main Because the S3 backend has only diverged from main with the commits to address lockfile header issues with lock enabled buckets, this commit simply copies the existing state of the `internal/backend/remote-state/s3` subfolder onto the `v1.10` branch. --- .../backend/remote-state/s3/backend_test.go | 180 ++++++++++++++++++ internal/backend/remote-state/s3/client.go | 6 +- .../backend/remote-state/s3/client_test.go | 121 +++++++++++- 3 files changed, 302 insertions(+), 5 deletions(-) diff --git a/internal/backend/remote-state/s3/backend_test.go b/internal/backend/remote-state/s3/backend_test.go index 72ed17d09712..8211060149ed 100644 --- a/internal/backend/remote-state/s3/backend_test.go +++ b/internal/backend/remote-state/s3/backend_test.go @@ -2038,6 +2038,76 @@ func TestBackendLockedWithFile(t *testing.T) { backend.TestBackendStateForceUnlock(t, b1, b2) } +func TestBackendLockedWithFile_ObjectLock_Compliance(t *testing.T) { + testACC(t) + objectLockPreCheck(t) + + ctx := context.TODO() + + bucketName := fmt.Sprintf("terraform-remote-s3-test-%x", time.Now().Unix()) + keyName := "test/state" + + b1 := backend.TestBackendConfig(t, New(), backend.TestWrapConfig(map[string]interface{}{ + "bucket": bucketName, + "key": keyName, + "encrypt": true, + "use_lockfile": true, + "region": "us-west-2", + })).(*Backend) + + b2 := backend.TestBackendConfig(t, New(), backend.TestWrapConfig(map[string]interface{}{ + "bucket": bucketName, + "key": keyName, + "encrypt": true, + "use_lockfile": true, + "region": "us-west-2", + })).(*Backend) + + createS3Bucket(ctx, t, b1.s3Client, bucketName, b1.awsConfig.Region, + s3BucketWithVersioning, + s3BucketWithObjectLock(s3types.ObjectLockRetentionModeCompliance), + ) + defer deleteS3Bucket(ctx, t, b1.s3Client, bucketName, b1.awsConfig.Region) + + backend.TestBackendStateLocks(t, b1, b2) + backend.TestBackendStateForceUnlock(t, b1, b2) +} + +func TestBackendLockedWithFile_ObjectLock_Governance(t *testing.T) { + testACC(t) + objectLockPreCheck(t) + + ctx := context.TODO() + + bucketName := fmt.Sprintf("terraform-remote-s3-test-%x", time.Now().Unix()) + keyName := "test/state" + + b1 := backend.TestBackendConfig(t, New(), backend.TestWrapConfig(map[string]interface{}{ + "bucket": bucketName, + "key": keyName, + "encrypt": true, + "use_lockfile": true, + "region": "us-west-2", + })).(*Backend) + + b2 := backend.TestBackendConfig(t, New(), backend.TestWrapConfig(map[string]interface{}{ + "bucket": bucketName, + "key": keyName, + "encrypt": true, + "use_lockfile": true, + "region": "us-west-2", + })).(*Backend) + + createS3Bucket(ctx, t, b1.s3Client, bucketName, b1.awsConfig.Region, + s3BucketWithVersioning, + s3BucketWithObjectLock(s3types.ObjectLockRetentionModeGovernance), + ) + defer deleteS3Bucket(ctx, t, b1.s3Client, bucketName, b1.awsConfig.Region) + + backend.TestBackendStateLocks(t, b1, b2) + backend.TestBackendStateForceUnlock(t, b1, b2) +} + func TestBackendLockedWithFileAndDynamoDB(t *testing.T) { testACC(t) @@ -2158,6 +2228,116 @@ func TestBackend_LockFileCleanupOnDynamoDBLock(t *testing.T) { } } +func TestBackend_LockFileCleanupOnDynamoDBLock_ObjectLock_Compliance(t *testing.T) { + testACC(t) + objectLockPreCheck(t) + + ctx := context.TODO() + + bucketName := fmt.Sprintf("terraform-remote-s3-test-%x", time.Now().Unix()) + keyName := "test/state" + + b1 := backend.TestBackendConfig(t, New(), backend.TestWrapConfig(map[string]interface{}{ + "bucket": bucketName, + "key": keyName, + "encrypt": true, + "use_lockfile": false, // Only use DynamoDB + "dynamodb_table": bucketName, + "region": "us-west-2", + })).(*Backend) + + b2 := backend.TestBackendConfig(t, New(), backend.TestWrapConfig(map[string]interface{}{ + "bucket": bucketName, + "key": keyName, + "encrypt": true, + "use_lockfile": true, // Use both DynamoDB and lockfile + "dynamodb_table": bucketName, + "region": "us-west-2", + })).(*Backend) + + createS3Bucket(ctx, t, b1.s3Client, bucketName, b1.awsConfig.Region, + s3BucketWithVersioning, + s3BucketWithObjectLock(s3types.ObjectLockRetentionModeCompliance), + ) + defer deleteS3Bucket(ctx, t, b1.s3Client, bucketName, b1.awsConfig.Region) + createDynamoDBTable(ctx, t, b1.dynClient, bucketName) + defer deleteDynamoDBTable(ctx, t, b1.dynClient, bucketName) + + backend.TestBackendStateLocks(t, b1, b2) + + // Attempt to retrieve the lock file from S3. + _, err := b1.s3Client.GetObject(ctx, &s3.GetObjectInput{ + Bucket: aws.String(b1.bucketName), + Key: aws.String(b1.keyName + ".tflock"), + }) + // We expect an error here, indicating that the lock file does not exist. + // The absence of the lock file is expected, as it should have been + // cleaned up following a failed lock acquisition due to `b1` already + // acquiring a DynamoDB lock. + if err != nil { + if !IsA[*s3types.NoSuchKey](err) { + t.Fatalf("unexpected error: %s", err) + } + } else { + t.Fatalf("expected error, got none") + } +} + +func TestBackend_LockFileCleanupOnDynamoDBLock_ObjectLock_Governance(t *testing.T) { + testACC(t) + objectLockPreCheck(t) + + ctx := context.TODO() + + bucketName := fmt.Sprintf("terraform-remote-s3-test-%x", time.Now().Unix()) + keyName := "test/state" + + b1 := backend.TestBackendConfig(t, New(), backend.TestWrapConfig(map[string]interface{}{ + "bucket": bucketName, + "key": keyName, + "encrypt": true, + "use_lockfile": false, // Only use DynamoDB + "dynamodb_table": bucketName, + "region": "us-west-2", + })).(*Backend) + + b2 := backend.TestBackendConfig(t, New(), backend.TestWrapConfig(map[string]interface{}{ + "bucket": bucketName, + "key": keyName, + "encrypt": true, + "use_lockfile": true, // Use both DynamoDB and lockfile + "dynamodb_table": bucketName, + "region": "us-west-2", + })).(*Backend) + + createS3Bucket(ctx, t, b1.s3Client, bucketName, b1.awsConfig.Region, + s3BucketWithVersioning, + s3BucketWithObjectLock(s3types.ObjectLockRetentionModeGovernance), + ) + defer deleteS3Bucket(ctx, t, b1.s3Client, bucketName, b1.awsConfig.Region) + createDynamoDBTable(ctx, t, b1.dynClient, bucketName) + defer deleteDynamoDBTable(ctx, t, b1.dynClient, bucketName) + + backend.TestBackendStateLocks(t, b1, b2) + + // Attempt to retrieve the lock file from S3. + _, err := b1.s3Client.GetObject(ctx, &s3.GetObjectInput{ + Bucket: aws.String(b1.bucketName), + Key: aws.String(b1.keyName + ".tflock"), + }) + // We expect an error here, indicating that the lock file does not exist. + // The absence of the lock file is expected, as it should have been + // cleaned up following a failed lock acquisition due to `b1` already + // acquiring a DynamoDB lock. + if err != nil { + if !IsA[*s3types.NoSuchKey](err) { + t.Fatalf("unexpected error: %s", err) + } + } else { + t.Fatalf("expected error, got none") + } +} + func TestBackend_LockDeletedOutOfBand(t *testing.T) { testACC(t) diff --git a/internal/backend/remote-state/s3/client.go b/internal/backend/remote-state/s3/client.go index 5014d9236170..85ad72bb322e 100644 --- a/internal/backend/remote-state/s3/client.go +++ b/internal/backend/remote-state/s3/client.go @@ -346,14 +346,14 @@ func (c *RemoteClient) Lock(info *statemgr.LockInfo) (string, error) { // exist, the operation will succeed, acquiring the lock. If the lock file already exists, the operation // will fail due to a conditional write, indicating that the lock is already held by another Terraform client. func (c *RemoteClient) lockWithFile(ctx context.Context, info *statemgr.LockInfo, log hclog.Logger) error { - data, err := json.Marshal(info) + lockFileJson, err := json.Marshal(info) if err != nil { return err } input := &s3.PutObjectInput{ ContentType: aws.String("application/json"), - Body: bytes.NewReader(data), + Body: bytes.NewReader(lockFileJson), Bucket: aws.String(c.bucketName), Key: aws.String(c.lockFilePath), IfNoneMatch: aws.String("*"), @@ -381,7 +381,7 @@ func (c *RemoteClient) lockWithFile(ctx context.Context, info *statemgr.LockInfo log.Debug("Uploading lock file") - uploader := manager.NewUploader(c.s3Client, func(u *manager.Uploader) {}) + uploader := manager.NewUploader(c.s3Client) _, err = uploader.Upload(ctx, input) if err != nil { // Attempt to retrieve lock info from the file, and merge errors if it fails. diff --git a/internal/backend/remote-state/s3/client_test.go b/internal/backend/remote-state/s3/client_test.go index c08729c72fcc..69368c30a4d3 100644 --- a/internal/backend/remote-state/s3/client_test.go +++ b/internal/backend/remote-state/s3/client_test.go @@ -176,6 +176,83 @@ func TestForceUnlock(t *testing.T) { } } +func TestForceUnlock_withLockfile(t *testing.T) { + testACC(t) + + ctx := context.TODO() + + bucketName := fmt.Sprintf("terraform-remote-s3-test-force-%x", time.Now().Unix()) + keyName := "testState" + + b1 := backend.TestBackendConfig(t, New(), backend.TestWrapConfig(map[string]interface{}{ + "bucket": bucketName, + "key": keyName, + "encrypt": true, + "use_lockfile": true, + })).(*Backend) + + b2 := backend.TestBackendConfig(t, New(), backend.TestWrapConfig(map[string]interface{}{ + "bucket": bucketName, + "key": keyName, + "encrypt": true, + "use_lockfile": true, + })).(*Backend) + + createS3Bucket(ctx, t, b1.s3Client, bucketName, b1.awsConfig.Region) + defer deleteS3Bucket(ctx, t, b1.s3Client, bucketName, b1.awsConfig.Region) + + // 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") + } +} + func TestRemoteClient_clientMD5(t *testing.T) { testACC(t) @@ -343,7 +420,7 @@ func TestRemoteClient_stateChecksum(t *testing.T) { } } -func TestRemoteClientPutLargeUploadWithObjectLock(t *testing.T) { +func TestRemoteClientPutLargeUploadWithObjectLock_Compliance(t *testing.T) { testACC(t) objectLockPreCheck(t) @@ -382,7 +459,7 @@ func TestRemoteClientPutLargeUploadWithObjectLock(t *testing.T) { } } -func TestRemoteClientObjectLockAndLockFile(t *testing.T) { +func TestRemoteClientLockFileWithObjectLock_Compliance(t *testing.T) { testACC(t) objectLockPreCheck(t) @@ -422,6 +499,46 @@ func TestRemoteClientObjectLockAndLockFile(t *testing.T) { } } +func TestRemoteClientLockFileWithObjectLock_Governance(t *testing.T) { + testACC(t) + objectLockPreCheck(t) + + ctx := context.TODO() + + bucketName := fmt.Sprintf("terraform-remote-s3-test-%x", time.Now().Unix()) + keyName := "testState" + + b := backend.TestBackendConfig(t, New(), backend.TestWrapConfig(map[string]interface{}{ + "bucket": bucketName, + "key": keyName, + "use_lockfile": true, + })).(*Backend) + + createS3Bucket(ctx, t, b.s3Client, bucketName, b.awsConfig.Region, + s3BucketWithVersioning, + s3BucketWithObjectLock(s3types.ObjectLockRetentionModeGovernance), + ) + defer deleteS3Bucket(ctx, t, b.s3Client, bucketName, b.awsConfig.Region) + + s1, err := b.StateMgr(backend.DefaultStateName) + if err != nil { + t.Fatal(err) + } + client := s1.(*remote.State).Client + + var state bytes.Buffer + dataW := io.LimitReader(neverEnding('x'), manager.DefaultUploadPartSize) + _, err = state.ReadFrom(dataW) + if err != nil { + t.Fatalf("writing dummy data: %s", err) + } + + err = client.Put(state.Bytes()) + if err != nil { + t.Fatalf("putting data: %s", err) + } +} + type neverEnding byte func (b neverEnding) Read(p []byte) (n int, err error) { From b9a55d2c15713bf73cb163cf59966fe8ac8a3ea4 Mon Sep 17 00:00:00 2001 From: Jared Baker Date: Mon, 2 Dec 2024 13:06:17 -0500 Subject: [PATCH 3/3] backend/s3: add changelog entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1874796ec666..35b30dbbdad6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ BUG FIXES: - config: `templatefile` would panic if given and entirely unknown map of variables [GH-36118] - config: `templatefile` would panic if the variables map contains marked values [GH-36127] - config: Remove constraint that an expanded resource block must only be used in conjunction with imports using `for_each` [GH-36119] +- backend/s3: Lock files could not be written to buckets with object locking enabled [GH-36120] ## 1.10.0 (November 27, 2024)