From a7a383b0a6fcac26b07f2f644e7392efa557a76f Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Tue, 12 Sep 2023 16:00:51 -0700 Subject: [PATCH] Handles case when bucket is requested from wrong region --- .../backend/remote-state/s3/backend_state.go | 26 +++++++ .../backend/remote-state/s3/backend_test.go | 76 +++++++++++++++---- .../backend/remote-state/s3/client_test.go | 10 +-- 3 files changed, 94 insertions(+), 18 deletions(-) diff --git a/internal/backend/remote-state/s3/backend_state.go b/internal/backend/remote-state/s3/backend_state.go index a95e04192e..aa69cfa8bd 100644 --- a/internal/backend/remote-state/s3/backend_state.go +++ b/internal/backend/remote-state/s3/backend_state.go @@ -7,6 +7,7 @@ import ( "context" "errors" "fmt" + "net/http" "path" "sort" "strings" @@ -14,6 +15,7 @@ import ( "github.com/aws/aws-sdk-go-v2/aws" "github.com/aws/aws-sdk-go-v2/service/s3" s3types "github.com/aws/aws-sdk-go-v2/service/s3/types" + smithyhttp "github.com/aws/smithy-go/transport/http" baselogging "github.com/hashicorp/aws-sdk-go-base/v2/logging" "github.com/hashicorp/terraform/internal/backend" @@ -60,6 +62,13 @@ func (b *Backend) Workspaces() ([]string, error) { if IsA[*s3types.NoSuchBucket](err) { return nil, fmt.Errorf(errS3NoSuchBucket, b.bucketName, err) } + if respErr, ok := As[*smithyhttp.ResponseError](err); ok && respErr.HTTPStatusCode() == http.StatusMovedPermanently { + if resp := respErr.HTTPResponse(); resp != nil { + if bucketRegion := resp.Header.Get("X-Amz-Bucket-Region"); bucketRegion != "" { + err = newBucketRegionError(b.awsConfig.Region, bucketRegion) + } + } + } return nil, fmt.Errorf("Unable to list objects in S3 bucket %q: %w", b.bucketName, err) } @@ -247,3 +256,20 @@ Error: %s You may have to force-unlock this state in order to use it again. ` + +var _ error = bucketRegionError{} + +type bucketRegionError struct { + requestRegion, bucketRegion string +} + +func newBucketRegionError(requestRegion, bucketRegion string) bucketRegionError { + return bucketRegionError{ + requestRegion: requestRegion, + bucketRegion: bucketRegion, + } +} + +func (err bucketRegionError) Error() string { + return fmt.Sprintf("requested bucket from %q, actual location %q", err.requestRegion, err.bucketRegion) +} diff --git a/internal/backend/remote-state/s3/backend_test.go b/internal/backend/remote-state/s3/backend_test.go index 21cbc4e171..a0bd7ee94b 100644 --- a/internal/backend/remote-state/s3/backend_test.go +++ b/internal/backend/remote-state/s3/backend_test.go @@ -1358,7 +1358,7 @@ func TestBackendBasic(t *testing.T) { })).(*Backend) createS3Bucket(ctx, t, b.s3Client, bucketName, b.awsConfig.Region) - defer deleteS3Bucket(ctx, t, b.s3Client, bucketName) + defer deleteS3Bucket(ctx, t, b.s3Client, bucketName, b.awsConfig.Region) backend.TestBackendStates(t, b) } @@ -1388,7 +1388,7 @@ func TestBackendLocked(t *testing.T) { })).(*Backend) createS3Bucket(ctx, t, b1.s3Client, bucketName, b1.awsConfig.Region) - defer deleteS3Bucket(ctx, t, b1.s3Client, bucketName) + defer deleteS3Bucket(ctx, t, b1.s3Client, bucketName, b1.awsConfig.Region) createDynamoDBTable(ctx, t, b1.dynClient, bucketName) defer deleteDynamoDBTable(ctx, t, b1.dynClient, bucketName) @@ -1588,7 +1588,7 @@ func TestBackendSSECustomerKey(t *testing.T) { if !diags.HasErrors() { createS3Bucket(ctx, t, b.s3Client, bucketName, b.awsConfig.Region) - defer deleteS3Bucket(ctx, t, b.s3Client, bucketName) + defer deleteS3Bucket(ctx, t, b.s3Client, bucketName, b.awsConfig.Region) backend.TestBackendStates(t, b) } @@ -1612,7 +1612,7 @@ func TestBackendExtraPaths(t *testing.T) { })).(*Backend) createS3Bucket(ctx, t, b.s3Client, bucketName, b.awsConfig.Region) - defer deleteS3Bucket(ctx, t, b.s3Client, bucketName) + defer deleteS3Bucket(ctx, t, b.s3Client, bucketName, b.awsConfig.Region) // put multiple states in old env paths. s1 := states.NewState() @@ -1753,7 +1753,7 @@ func TestBackendPrefixInWorkspace(t *testing.T) { })).(*Backend) createS3Bucket(ctx, t, b.s3Client, bucketName, b.awsConfig.Region) - defer deleteS3Bucket(ctx, t, b.s3Client, bucketName) + defer deleteS3Bucket(ctx, t, b.s3Client, bucketName, b.awsConfig.Region) // get a state that contains the prefix as a substring sMgr, err := b.StateMgr("env-1") @@ -1769,6 +1769,46 @@ func TestBackendPrefixInWorkspace(t *testing.T) { } } +func TestBackendWrongRegion(t *testing.T) { + testACC(t) + + ctx := context.TODO() + + bucketName := fmt.Sprintf("terraform-remote-s3-test-%x", time.Now().Unix()) + keyName := "testState" + + bucketRegion := "us-west-1" + backendRegion := "us-east-1" + if backendRegion == bucketRegion { + t.Fatalf("bucket region and backend region must not be the same") + } + + b := backend.TestBackendConfig(t, New(), backend.TestWrapConfig(map[string]interface{}{ + "bucket": bucketName, + "key": keyName, + "encrypt": true, + "region": backendRegion, + })).(*Backend) + + createS3Bucket(ctx, t, b.s3Client, bucketName, bucketRegion) + defer deleteS3Bucket(ctx, t, b.s3Client, bucketName, bucketRegion) + + if _, err := b.StateMgr(backend.DefaultStateName); err == nil { + t.Fatal("expected error, got none") + } else { + if regionErr, ok := As[bucketRegionError](err); ok { + if a, e := regionErr.bucketRegion, bucketRegion; a != e { + t.Errorf("expected bucket region %q, got %q", e, a) + } + if a, e := regionErr.requestRegion, backendRegion; a != e { + t.Errorf("expected request region %q, got %q", e, a) + } + } else { + t.Fatalf("expected bucket region error, got: %v", err) + } + } +} + func TestKeyEnv(t *testing.T) { testACC(t) @@ -1785,7 +1825,7 @@ func TestKeyEnv(t *testing.T) { })).(*Backend) createS3Bucket(ctx, t, b0.s3Client, bucket0Name, b0.awsConfig.Region) - defer deleteS3Bucket(ctx, t, b0.s3Client, bucket0Name) + defer deleteS3Bucket(ctx, t, b0.s3Client, bucket0Name, b0.awsConfig.Region) bucket1Name := fmt.Sprintf("terraform-remote-s3-test-%x-1", time.Now().Unix()) b1 := backend.TestBackendConfig(t, New(), backend.TestWrapConfig(map[string]interface{}{ @@ -1796,7 +1836,7 @@ func TestKeyEnv(t *testing.T) { })).(*Backend) createS3Bucket(ctx, t, b1.s3Client, bucket1Name, b1.awsConfig.Region) - defer deleteS3Bucket(ctx, t, b1.s3Client, bucket1Name) + defer deleteS3Bucket(ctx, t, b1.s3Client, bucket1Name, b1.awsConfig.Region) bucket2Name := fmt.Sprintf("terraform-remote-s3-test-%x-2", time.Now().Unix()) b2 := backend.TestBackendConfig(t, New(), backend.TestWrapConfig(map[string]interface{}{ @@ -1806,7 +1846,7 @@ func TestKeyEnv(t *testing.T) { })).(*Backend) createS3Bucket(ctx, t, b2.s3Client, bucket2Name, b2.awsConfig.Region) - defer deleteS3Bucket(ctx, t, b2.s3Client, bucket2Name) + defer deleteS3Bucket(ctx, t, b2.s3Client, bucket2Name, b2.awsConfig.Region) if err := testGetWorkspaceForKey(b0, "some/paths/tfstate", ""); err != nil { t.Fatal(err) @@ -2025,6 +2065,8 @@ func checkStateList(b backend.Backend, expected []string) error { } func createS3Bucket(ctx context.Context, t *testing.T, s3Client *s3.Client, bucketName, region string) { + t.Helper() + createBucketReq := &s3.CreateBucketInput{ Bucket: &bucketName, } @@ -2037,34 +2079,42 @@ func createS3Bucket(ctx context.Context, t *testing.T, s3Client *s3.Client, buck // Be clear about what we're doing in case the user needs to clean // this up later. t.Logf("creating S3 bucket %s in %s", bucketName, region) - _, err := s3Client.CreateBucket(ctx, createBucketReq) + _, err := s3Client.CreateBucket(ctx, createBucketReq, s3WithRegion(region)) if err != nil { t.Fatal("failed to create test S3 bucket:", err) } } -func deleteS3Bucket(ctx context.Context, t *testing.T, s3Client *s3.Client, bucketName string) { +func deleteS3Bucket(ctx context.Context, t *testing.T, s3Client *s3.Client, bucketName, region string) { + t.Helper() + warning := "WARNING: Failed to delete the test S3 bucket. It may have been left in your AWS account and may incur storage charges. (error was %s)" // first we have to get rid of the env objects, or we can't delete the bucket - resp, err := s3Client.ListObjects(ctx, &s3.ListObjectsInput{Bucket: &bucketName}) + resp, err := s3Client.ListObjects(ctx, &s3.ListObjectsInput{Bucket: &bucketName}, s3WithRegion(region)) if err != nil { t.Logf(warning, err) return } for _, obj := range resp.Contents { - if _, err := s3Client.DeleteObject(ctx, &s3.DeleteObjectInput{Bucket: &bucketName, Key: obj.Key}); err != nil { + if _, err := s3Client.DeleteObject(ctx, &s3.DeleteObjectInput{Bucket: &bucketName, Key: obj.Key}, s3WithRegion(region)); err != nil { // this will need cleanup no matter what, so just warn and exit t.Logf(warning, err) return } } - if _, err := s3Client.DeleteBucket(ctx, &s3.DeleteBucketInput{Bucket: &bucketName}); err != nil { + if _, err := s3Client.DeleteBucket(ctx, &s3.DeleteBucketInput{Bucket: &bucketName}, s3WithRegion(region)); err != nil { t.Logf(warning, err) } } +func s3WithRegion(region string) func(o *s3.Options) { + return func(o *s3.Options) { + o.Region = region + } +} + // create the dynamoDB table, and wait until we can query it. func createDynamoDBTable(ctx context.Context, t *testing.T, dynClient *dynamodb.Client, tableName string) { createInput := &dynamodb.CreateTableInput{ diff --git a/internal/backend/remote-state/s3/client_test.go b/internal/backend/remote-state/s3/client_test.go index 5422ab2e3b..818c4fd900 100644 --- a/internal/backend/remote-state/s3/client_test.go +++ b/internal/backend/remote-state/s3/client_test.go @@ -37,7 +37,7 @@ func TestRemoteClientBasic(t *testing.T) { })).(*Backend) createS3Bucket(ctx, t, b.s3Client, bucketName, b.awsConfig.Region) - defer deleteS3Bucket(ctx, t, b.s3Client, bucketName) + defer deleteS3Bucket(ctx, t, b.s3Client, bucketName, b.awsConfig.Region) state, err := b.StateMgr(backend.DefaultStateName) if err != nil { @@ -70,7 +70,7 @@ func TestRemoteClientLocks(t *testing.T) { })).(*Backend) createS3Bucket(ctx, t, b1.s3Client, bucketName, b1.awsConfig.Region) - defer deleteS3Bucket(ctx, t, b1.s3Client, bucketName) + defer deleteS3Bucket(ctx, t, b1.s3Client, bucketName, b1.awsConfig.Region) createDynamoDBTable(ctx, t, b1.dynClient, bucketName) defer deleteDynamoDBTable(ctx, t, b1.dynClient, bucketName) @@ -111,7 +111,7 @@ func TestForceUnlock(t *testing.T) { })).(*Backend) createS3Bucket(ctx, t, b1.s3Client, bucketName, b1.awsConfig.Region) - defer deleteS3Bucket(ctx, t, b1.s3Client, bucketName) + defer deleteS3Bucket(ctx, t, b1.s3Client, bucketName, b1.awsConfig.Region) createDynamoDBTable(ctx, t, b1.dynClient, bucketName) defer deleteDynamoDBTable(ctx, t, b1.dynClient, bucketName) @@ -182,7 +182,7 @@ func TestRemoteClient_clientMD5(t *testing.T) { })).(*Backend) createS3Bucket(ctx, t, b.s3Client, bucketName, b.awsConfig.Region) - defer deleteS3Bucket(ctx, t, b.s3Client, bucketName) + defer deleteS3Bucket(ctx, t, b.s3Client, bucketName, b.awsConfig.Region) createDynamoDBTable(ctx, t, b.dynClient, bucketName) defer deleteDynamoDBTable(ctx, t, b.dynClient, bucketName) @@ -232,7 +232,7 @@ func TestRemoteClient_stateChecksum(t *testing.T) { })).(*Backend) createS3Bucket(ctx, t, b1.s3Client, bucketName, b1.awsConfig.Region) - defer deleteS3Bucket(ctx, t, b1.s3Client, bucketName) + defer deleteS3Bucket(ctx, t, b1.s3Client, bucketName, b1.awsConfig.Region) createDynamoDBTable(ctx, t, b1.dynClient, bucketName) defer deleteDynamoDBTable(ctx, t, b1.dynClient, bucketName)