From b667207fa738c9b0daa6181d50cdc1206ac69215 Mon Sep 17 00:00:00 2001 From: Bruno Schaatsbergen Date: Wed, 27 Nov 2024 17:25:01 +0100 Subject: [PATCH 1/3] s3: use the s3manager to write the lock file. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When S3 Object Lock is enabled on a bucket with a retention period, S3 requires the Content-MD5 or x-amz-sdk-checksum-algorithm header for object uploads (via PutObject) to ensure data integrity during the upload process. Terraform’s state writes to the S3 bucket relied on the “uploader” from aws-sdk-go-v2, which automatically appends these required headers. However, the lock file implementation did not use the “uploader,” resulting in missing headers for PutObject requests and conflicts with Object Lock requirements. This commit updates the lock file implementation to use the “uploader,” ensuring the necessary headers are included in the requests, maintaining compatibility with Object Lock-enabled buckets. --- 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 701e02c71c0a57b4a1ab68b1b2900a9a1a2789f6 Mon Sep 17 00:00:00 2001 From: Bruno Schaatsbergen Date: Wed, 27 Nov 2024 18:23:53 +0100 Subject: [PATCH 2/3] s3: add tests for Amazon S3 Object Lock --- .../backend/remote-state/s3/backend_test.go | 90 +++++++++++++++++++ .../backend/remote-state/s3/client_test.go | 2 +- 2 files changed, 91 insertions(+), 1 deletion(-) diff --git a/internal/backend/remote-state/s3/backend_test.go b/internal/backend/remote-state/s3/backend_test.go index 72ed17d09712..a82a5a403d28 100644 --- a/internal/backend/remote-state/s3/backend_test.go +++ b/internal/backend/remote-state/s3/backend_test.go @@ -2038,6 +2038,41 @@ func TestBackendLockedWithFile(t *testing.T) { backend.TestBackendStateForceUnlock(t, b1, b2) } +func TestBackendLockedWithFile_ObjectLock(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 TestBackendLockedWithFileAndDynamoDB(t *testing.T) { testACC(t) @@ -2158,6 +2193,61 @@ func TestBackend_LockFileCleanupOnDynamoDBLock(t *testing.T) { } } +func TestBackend_LockFileCleanupOnDynamoDBLock_ObjectLock(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_LockDeletedOutOfBand(t *testing.T) { testACC(t) diff --git a/internal/backend/remote-state/s3/client_test.go b/internal/backend/remote-state/s3/client_test.go index c08729c72fcc..491551f80969 100644 --- a/internal/backend/remote-state/s3/client_test.go +++ b/internal/backend/remote-state/s3/client_test.go @@ -382,7 +382,7 @@ func TestRemoteClientPutLargeUploadWithObjectLock(t *testing.T) { } } -func TestRemoteClientObjectLockAndLockFile(t *testing.T) { +func TestRemoteClientLockFileWithObjectLock(t *testing.T) { testACC(t) objectLockPreCheck(t) From 9db96476fdb2f41b66dfdda2f2cc1340e50a7598 Mon Sep 17 00:00:00 2001 From: Bruno Schaatsbergen Date: Wed, 27 Nov 2024 18:32:04 +0100 Subject: [PATCH 3/3] s3: add mix of compliance and governance tests --- .../backend/remote-state/s3/backend_test.go | 94 +++++++++++++- internal/backend/remote-state/s3/client.go | 6 +- .../backend/remote-state/s3/client_test.go | 121 +++++++++++++++++- 3 files changed, 214 insertions(+), 7 deletions(-) diff --git a/internal/backend/remote-state/s3/backend_test.go b/internal/backend/remote-state/s3/backend_test.go index a82a5a403d28..8211060149ed 100644 --- a/internal/backend/remote-state/s3/backend_test.go +++ b/internal/backend/remote-state/s3/backend_test.go @@ -2038,7 +2038,7 @@ func TestBackendLockedWithFile(t *testing.T) { backend.TestBackendStateForceUnlock(t, b1, b2) } -func TestBackendLockedWithFile_ObjectLock(t *testing.T) { +func TestBackendLockedWithFile_ObjectLock_Compliance(t *testing.T) { testACC(t) objectLockPreCheck(t) @@ -2073,6 +2073,41 @@ func TestBackendLockedWithFile_ObjectLock(t *testing.T) { 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) @@ -2193,7 +2228,7 @@ func TestBackend_LockFileCleanupOnDynamoDBLock(t *testing.T) { } } -func TestBackend_LockFileCleanupOnDynamoDBLock_ObjectLock(t *testing.T) { +func TestBackend_LockFileCleanupOnDynamoDBLock_ObjectLock_Compliance(t *testing.T) { testACC(t) objectLockPreCheck(t) @@ -2248,6 +2283,61 @@ func TestBackend_LockFileCleanupOnDynamoDBLock_ObjectLock(t *testing.T) { } } +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 491551f80969..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 TestRemoteClientLockFileWithObjectLock(t *testing.T) { +func TestRemoteClientLockFileWithObjectLock_Compliance(t *testing.T) { testACC(t) objectLockPreCheck(t) @@ -422,6 +499,46 @@ func TestRemoteClientLockFileWithObjectLock(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) {