From b71c7b859a476ce6a0d84c9948b1b8932912207b Mon Sep 17 00:00:00 2001 From: Marco Braga Date: Fri, 1 Mar 2024 08:51:40 -0300 Subject: [PATCH 1/2] =?UTF-8?q?=F0=9F=90=9B=20fix:=20s3=20bucket=20in=20us?= =?UTF-8?q?-east-1=20should=20not=20set=20locations?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `us-east-1` is invalid as a Location Contraint when creating a bucket. "Buckets in Region us-east-1 have a LocationConstraint of null."[1] [1] https://docs.aws.amazon.com/AmazonS3/latest/API/API_GetBucketLocation.html#API_GetBucketLocation_ResponseSyntax --- pkg/cloud/services/s3/s3.go | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/pkg/cloud/services/s3/s3.go b/pkg/cloud/services/s3/s3.go index a6bbf26b86..b7e9a658ac 100644 --- a/pkg/cloud/services/s3/s3.go +++ b/pkg/cloud/services/s3/s3.go @@ -38,6 +38,9 @@ import ( "sigs.k8s.io/cluster-api-provider-aws/v2/util/system" ) +// AWSDefaultRegion is the default AWS region. +const AWSDefaultRegion string = "us-east-1" + // Service holds a collection of interfaces. // The interfaces are broken down like this to group functions together. // One alternative is to have a large list of functions from the ec2 client. @@ -223,11 +226,13 @@ func (s *Service) Delete(m *scope.MachineScope) error { } func (s *Service) createBucketIfNotExist(bucketName string) error { - input := &s3.CreateBucketInput{ - Bucket: aws.String(bucketName), - CreateBucketConfiguration: &s3.CreateBucketConfiguration{ + input := &s3.CreateBucketInput{Bucket: aws.String(bucketName)} + + // See https://docs.aws.amazon.com/AmazonS3/latest/API/API_CreateBucket.html#AmazonS3-CreateBucket-request-LocationConstraint. + if s.scope.Region() != AWSDefaultRegion { + input.CreateBucketConfiguration = &s3.CreateBucketConfiguration{ LocationConstraint: aws.String(s.scope.Region()), - }, + } } _, err := s.S3Client.CreateBucket(input) From 43bded17b6ff1276d5b6cf568fd5a42375e1b29e Mon Sep 17 00:00:00 2001 From: Marco Braga Date: Fri, 1 Mar 2024 08:53:25 -0300 Subject: [PATCH 2/2] =?UTF-8?q?=F0=9F=90=9B=20e2e/s3:=20supporting=20custo?= =?UTF-8?q?m=20params=20for=20test=20service?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- pkg/cloud/services/s3/s3_test.go | 113 +++++++++++++++++++++---------- 1 file changed, 79 insertions(+), 34 deletions(-) diff --git a/pkg/cloud/services/s3/s3_test.go b/pkg/cloud/services/s3/s3_test.go index 7ce58d19d0..b74922f0f8 100644 --- a/pkg/cloud/services/s3/s3_test.go +++ b/pkg/cloud/services/s3/s3_test.go @@ -66,8 +66,10 @@ func TestReconcileBucket(t *testing.T) { expectedBucketName := "baz" - svc, s3Mock := testService(t, &infrav1.S3Bucket{ - Name: expectedBucketName, + svc, s3Mock := testService(t, &testServiceInput{ + Bucket: &infrav1.S3Bucket{ + Name: expectedBucketName, + }, }) input := &s3svc.CreateBucketInput{ @@ -168,11 +170,13 @@ func TestReconcileBucket(t *testing.T) { bucketName := "bar" - svc, s3Mock := testService(t, &infrav1.S3Bucket{ - Name: bucketName, - ControlPlaneIAMInstanceProfile: fmt.Sprintf("control-plane%s", iamv1.DefaultNameSuffix), - NodesIAMInstanceProfiles: []string{ - fmt.Sprintf("nodes%s", iamv1.DefaultNameSuffix), + svc, s3Mock := testService(t, &testServiceInput{ + Bucket: &infrav1.S3Bucket{ + Name: bucketName, + ControlPlaneIAMInstanceProfile: fmt.Sprintf("control-plane%s", iamv1.DefaultNameSuffix), + NodesIAMInstanceProfiles: []string{ + fmt.Sprintf("nodes%s", iamv1.DefaultNameSuffix), + }, }, }) @@ -218,7 +222,7 @@ func TestReconcileBucket(t *testing.T) { t.Run("is_idempotent", func(t *testing.T) { t.Parallel() - svc, s3Mock := testService(t, &infrav1.S3Bucket{}) + svc, s3Mock := testService(t, &testServiceInput{Bucket: &infrav1.S3Bucket{}}) s3Mock.EXPECT().CreateBucket(gomock.Any()).Return(nil, nil).Times(2) s3Mock.EXPECT().PutBucketTagging(gomock.Any()).Return(nil, nil).Times(2) @@ -236,7 +240,7 @@ func TestReconcileBucket(t *testing.T) { t.Run("ignores_when_bucket_already_exists_but_its_owned_by_the_same_account", func(t *testing.T) { t.Parallel() - svc, s3Mock := testService(t, &infrav1.S3Bucket{}) + svc, s3Mock := testService(t, &testServiceInput{Bucket: &infrav1.S3Bucket{}}) err := awserr.New(s3svc.ErrCodeBucketAlreadyOwnedByYou, "err", errors.New("err")) @@ -255,7 +259,7 @@ func TestReconcileBucket(t *testing.T) { t.Run("bucket_creation_fails", func(t *testing.T) { t.Parallel() - svc, s3Mock := testService(t, &infrav1.S3Bucket{}) + svc, s3Mock := testService(t, &testServiceInput{Bucket: &infrav1.S3Bucket{}}) s3Mock.EXPECT().CreateBucket(gomock.Any()).Return(nil, errors.New("error")).Times(1) @@ -267,7 +271,7 @@ func TestReconcileBucket(t *testing.T) { t.Run("bucket_creation_returns_unexpected_AWS_error", func(t *testing.T) { t.Parallel() - svc, s3Mock := testService(t, &infrav1.S3Bucket{}) + svc, s3Mock := testService(t, &testServiceInput{Bucket: &infrav1.S3Bucket{}}) s3Mock.EXPECT().CreateBucket(gomock.Any()).Return(nil, awserr.New("foo", "", nil)).Times(1) @@ -279,7 +283,7 @@ func TestReconcileBucket(t *testing.T) { t.Run("generating_bucket_policy_fails", func(t *testing.T) { t.Parallel() - svc, s3Mock := testService(t, &infrav1.S3Bucket{}) + svc, s3Mock := testService(t, &testServiceInput{Bucket: &infrav1.S3Bucket{}}) s3Mock.EXPECT().CreateBucket(gomock.Any()).Return(nil, nil).Times(1) s3Mock.EXPECT().PutBucketTagging(gomock.Any()).Return(nil, nil).Times(1) @@ -297,7 +301,7 @@ func TestReconcileBucket(t *testing.T) { t.Run("creating_bucket_policy_fails", func(t *testing.T) { t.Parallel() - svc, s3Mock := testService(t, &infrav1.S3Bucket{}) + svc, s3Mock := testService(t, &testServiceInput{Bucket: &infrav1.S3Bucket{}}) s3Mock.EXPECT().CreateBucket(gomock.Any()).Return(nil, nil).Times(1) s3Mock.EXPECT().PutBucketTagging(gomock.Any()).Return(nil, nil).Times(1) @@ -307,6 +311,27 @@ func TestReconcileBucket(t *testing.T) { t.Fatalf("Expected error") } }) + + t.Run("creates_bucket_without_location", func(t *testing.T) { + t.Parallel() + + bucketName := "test" + svc, s3Mock := testService(t, &testServiceInput{ + Region: "us-east-1", + Bucket: &infrav1.S3Bucket{Name: bucketName}, + }) + input := &s3svc.CreateBucketInput{ + Bucket: aws.String(bucketName), + } + + s3Mock.EXPECT().CreateBucket(gomock.Eq(input)).Return(nil, nil).Times(1) + s3Mock.EXPECT().PutBucketTagging(gomock.Any()).Return(nil, nil).Times(1) + s3Mock.EXPECT().PutBucketPolicy(gomock.Any()).Return(nil, nil).Times(1) + + if err := svc.ReconcileBucket(); err != nil { + t.Fatalf("Unexpected error: %v", err) + } + }) }) } @@ -328,8 +353,10 @@ func TestDeleteBucket(t *testing.T) { t.Run("deletes_bucket_with_configured_name", func(t *testing.T) { t.Parallel() - svc, s3Mock := testService(t, &infrav1.S3Bucket{ - Name: bucketName, + svc, s3Mock := testService(t, &testServiceInput{ + Bucket: &infrav1.S3Bucket{ + Name: bucketName, + }, }) input := &s3svc.DeleteBucketInput{ @@ -348,7 +375,7 @@ func TestDeleteBucket(t *testing.T) { t.Run("unexpected_error", func(t *testing.T) { t.Parallel() - svc, s3Mock := testService(t, &infrav1.S3Bucket{}) + svc, s3Mock := testService(t, &testServiceInput{Bucket: &infrav1.S3Bucket{}}) s3Mock.EXPECT().DeleteBucket(gomock.Any()).Return(nil, errors.New("err")).Times(1) @@ -360,7 +387,7 @@ func TestDeleteBucket(t *testing.T) { t.Run("unexpected_AWS_error", func(t *testing.T) { t.Parallel() - svc, s3Mock := testService(t, &infrav1.S3Bucket{}) + svc, s3Mock := testService(t, &testServiceInput{Bucket: &infrav1.S3Bucket{}}) s3Mock.EXPECT().DeleteBucket(gomock.Any()).Return(nil, awserr.New("foo", "", nil)).Times(1) @@ -373,7 +400,7 @@ func TestDeleteBucket(t *testing.T) { t.Run("ignores_when_bucket_has_already_been_removed", func(t *testing.T) { t.Parallel() - svc, s3Mock := testService(t, &infrav1.S3Bucket{}) + svc, s3Mock := testService(t, &testServiceInput{Bucket: &infrav1.S3Bucket{}}) s3Mock.EXPECT().DeleteBucket(gomock.Any()).Return(nil, awserr.New(s3svc.ErrCodeNoSuchBucket, "", nil)).Times(1) @@ -385,7 +412,7 @@ func TestDeleteBucket(t *testing.T) { t.Run("skips_bucket_removal_when_bucket_is_not_empty", func(t *testing.T) { t.Parallel() - svc, s3Mock := testService(t, &infrav1.S3Bucket{}) + svc, s3Mock := testService(t, &testServiceInput{Bucket: &infrav1.S3Bucket{}}) s3Mock.EXPECT().DeleteBucket(gomock.Any()).Return(nil, awserr.New("BucketNotEmpty", "", nil)).Times(1) @@ -406,8 +433,10 @@ func TestCreateObject(t *testing.T) { t.Run("for_machine", func(t *testing.T) { t.Parallel() - svc, s3Mock := testService(t, &infrav1.S3Bucket{ - Name: bucketName, + svc, s3Mock := testService(t, &testServiceInput{ + Bucket: &infrav1.S3Bucket{ + Name: bucketName, + }, }) machineScope := &scope.MachineScope{ @@ -487,7 +516,7 @@ func TestCreateObject(t *testing.T) { t.Run("is_idempotent", func(t *testing.T) { t.Parallel() - svc, s3Mock := testService(t, &infrav1.S3Bucket{}) + svc, s3Mock := testService(t, &testServiceInput{Bucket: &infrav1.S3Bucket{}}) machineScope := &scope.MachineScope{ Machine: &clusterv1.Machine{}, @@ -516,7 +545,7 @@ func TestCreateObject(t *testing.T) { t.Run("object_creation_fails", func(t *testing.T) { t.Parallel() - svc, s3Mock := testService(t, &infrav1.S3Bucket{}) + svc, s3Mock := testService(t, &testServiceInput{Bucket: &infrav1.S3Bucket{}}) machineScope := &scope.MachineScope{ Machine: &clusterv1.Machine{}, @@ -542,7 +571,7 @@ func TestCreateObject(t *testing.T) { t.Run("given_empty_machine_scope", func(t *testing.T) { t.Parallel() - svc, _ := testService(t, &infrav1.S3Bucket{}) + svc, _ := testService(t, &testServiceInput{Bucket: &infrav1.S3Bucket{}}) bootstrapDataURL, err := svc.Create(nil, []byte("foo")) if err == nil { @@ -558,7 +587,7 @@ func TestCreateObject(t *testing.T) { t.Run("given_empty_bootstrap_data", func(t *testing.T) { t.Parallel() - svc, _ := testService(t, &infrav1.S3Bucket{}) + svc, _ := testService(t, &testServiceInput{Bucket: &infrav1.S3Bucket{}}) machineScope := &scope.MachineScope{ Machine: &clusterv1.Machine{}, @@ -615,8 +644,10 @@ func TestDeleteObject(t *testing.T) { expectedBucketName := "foo" - svc, s3Mock := testService(t, &infrav1.S3Bucket{ - Name: expectedBucketName, + svc, s3Mock := testService(t, &testServiceInput{ + Bucket: &infrav1.S3Bucket{ + Name: expectedBucketName, + }, }) machineScope := &scope.MachineScope{ @@ -666,7 +697,7 @@ func TestDeleteObject(t *testing.T) { t.Run("succeeds_when_bucket_has_already_been_removed", func(t *testing.T) { t.Parallel() - svc, s3Mock := testService(t, &infrav1.S3Bucket{}) + svc, s3Mock := testService(t, &testServiceInput{Bucket: &infrav1.S3Bucket{}}) machineScope := &scope.MachineScope{ Machine: &clusterv1.Machine{}, @@ -690,7 +721,7 @@ func TestDeleteObject(t *testing.T) { t.Run("object_deletion_fails", func(t *testing.T) { t.Parallel() - svc, s3Mock := testService(t, &infrav1.S3Bucket{}) + svc, s3Mock := testService(t, &testServiceInput{Bucket: &infrav1.S3Bucket{}}) machineScope := &scope.MachineScope{ Machine: &clusterv1.Machine{}, @@ -712,7 +743,7 @@ func TestDeleteObject(t *testing.T) { t.Run("given_empty_machine_scope", func(t *testing.T) { t.Parallel() - svc, _ := testService(t, &infrav1.S3Bucket{}) + svc, _ := testService(t, nil) if err := svc.Delete(nil); err == nil { t.Fatalf("Expected error") @@ -742,7 +773,7 @@ func TestDeleteObject(t *testing.T) { t.Run("is_idempotent", func(t *testing.T) { t.Parallel() - svc, s3Mock := testService(t, &infrav1.S3Bucket{}) + svc, s3Mock := testService(t, &testServiceInput{Bucket: &infrav1.S3Bucket{}}) machineScope := &scope.MachineScope{ Machine: &clusterv1.Machine{}, @@ -766,7 +797,14 @@ func TestDeleteObject(t *testing.T) { }) } -func testService(t *testing.T, bucket *infrav1.S3Bucket) (*s3.Service, *mock_s3iface.MockS3API) { +type testServiceInput struct { + Bucket *infrav1.S3Bucket + Region string +} + +const testAWSRegion string = "us-west-2" + +func testService(t *testing.T, si *testServiceInput) (*s3.Service, *mock_s3iface.MockS3API) { t.Helper() mockCtrl := gomock.NewController(t) @@ -780,6 +818,13 @@ func testService(t *testing.T, bucket *infrav1.S3Bucket) (*s3.Service, *mock_s3i _ = infrav1.AddToScheme(scheme) client := fake.NewClientBuilder().WithScheme(scheme).Build() + if si == nil { + si = &testServiceInput{} + } + if si.Region == "" { + si.Region = testAWSRegion + } + scope, err := scope.NewClusterScope(scope.ClusterScopeParams{ Client: client, Cluster: &clusterv1.Cluster{ @@ -790,8 +835,8 @@ func testService(t *testing.T, bucket *infrav1.S3Bucket) (*s3.Service, *mock_s3i }, AWSCluster: &infrav1.AWSCluster{ Spec: infrav1.AWSClusterSpec{ - S3Bucket: bucket, - Region: "us-west-2", + S3Bucket: si.Bucket, + Region: si.Region, AdditionalTags: infrav1.Tags{ "additional": "from-aws-cluster", },