Skip to content

Commit 77aed2a

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #4839 from mtulio/CORS-3285-fix-s3-loc-use1
🐛 fix: s3 bucket in us-east-1 should not set location constraint
2 parents 25a0086 + 43bded1 commit 77aed2a

File tree

2 files changed

+88
-38
lines changed

2 files changed

+88
-38
lines changed

‎pkg/cloud/services/s3/s3.go

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,9 @@ import (
3838
"sigs.k8s.io/cluster-api-provider-aws/v2/util/system"
3939
)
4040

41+
// AWSDefaultRegion is the default AWS region.
42+
const AWSDefaultRegion string = "us-east-1"
43+
4144
// Service holds a collection of interfaces.
4245
// The interfaces are broken down like this to group functions together.
4346
// 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 {
223226
}
224227

225228
func (s *Service) createBucketIfNotExist(bucketName string) error {
226-
input := &s3.CreateBucketInput{
227-
Bucket: aws.String(bucketName),
228-
CreateBucketConfiguration: &s3.CreateBucketConfiguration{
229+
input := &s3.CreateBucketInput{Bucket: aws.String(bucketName)}
230+
231+
// See https://docs.aws.amazon.com/AmazonS3/latest/API/API_CreateBucket.html#AmazonS3-CreateBucket-request-LocationConstraint.
232+
if s.scope.Region() != AWSDefaultRegion {
233+
input.CreateBucketConfiguration = &s3.CreateBucketConfiguration{
229234
LocationConstraint: aws.String(s.scope.Region()),
230-
},
235+
}
231236
}
232237

233238
_, err := s.S3Client.CreateBucket(input)

‎pkg/cloud/services/s3/s3_test.go

Lines changed: 79 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -66,8 +66,10 @@ func TestReconcileBucket(t *testing.T) {
6666

6767
expectedBucketName := "baz"
6868

69-
svc, s3Mock := testService(t, &infrav1.S3Bucket{
70-
Name: expectedBucketName,
69+
svc, s3Mock := testService(t, &testServiceInput{
70+
Bucket: &infrav1.S3Bucket{
71+
Name: expectedBucketName,
72+
},
7173
})
7274

7375
input := &s3svc.CreateBucketInput{
@@ -168,11 +170,13 @@ func TestReconcileBucket(t *testing.T) {
168170

169171
bucketName := "bar"
170172

171-
svc, s3Mock := testService(t, &infrav1.S3Bucket{
172-
Name: bucketName,
173-
ControlPlaneIAMInstanceProfile: fmt.Sprintf("control-plane%s", iamv1.DefaultNameSuffix),
174-
NodesIAMInstanceProfiles: []string{
175-
fmt.Sprintf("nodes%s", iamv1.DefaultNameSuffix),
173+
svc, s3Mock := testService(t, &testServiceInput{
174+
Bucket: &infrav1.S3Bucket{
175+
Name: bucketName,
176+
ControlPlaneIAMInstanceProfile: fmt.Sprintf("control-plane%s", iamv1.DefaultNameSuffix),
177+
NodesIAMInstanceProfiles: []string{
178+
fmt.Sprintf("nodes%s", iamv1.DefaultNameSuffix),
179+
},
176180
},
177181
})
178182

@@ -218,7 +222,7 @@ func TestReconcileBucket(t *testing.T) {
218222
t.Run("is_idempotent", func(t *testing.T) {
219223
t.Parallel()
220224

221-
svc, s3Mock := testService(t, &infrav1.S3Bucket{})
225+
svc, s3Mock := testService(t, &testServiceInput{Bucket: &infrav1.S3Bucket{}})
222226

223227
s3Mock.EXPECT().CreateBucket(gomock.Any()).Return(nil, nil).Times(2)
224228
s3Mock.EXPECT().PutBucketTagging(gomock.Any()).Return(nil, nil).Times(2)
@@ -236,7 +240,7 @@ func TestReconcileBucket(t *testing.T) {
236240
t.Run("ignores_when_bucket_already_exists_but_its_owned_by_the_same_account", func(t *testing.T) {
237241
t.Parallel()
238242

239-
svc, s3Mock := testService(t, &infrav1.S3Bucket{})
243+
svc, s3Mock := testService(t, &testServiceInput{Bucket: &infrav1.S3Bucket{}})
240244

241245
err := awserr.New(s3svc.ErrCodeBucketAlreadyOwnedByYou, "err", errors.New("err"))
242246

@@ -255,7 +259,7 @@ func TestReconcileBucket(t *testing.T) {
255259
t.Run("bucket_creation_fails", func(t *testing.T) {
256260
t.Parallel()
257261

258-
svc, s3Mock := testService(t, &infrav1.S3Bucket{})
262+
svc, s3Mock := testService(t, &testServiceInput{Bucket: &infrav1.S3Bucket{}})
259263

260264
s3Mock.EXPECT().CreateBucket(gomock.Any()).Return(nil, errors.New("error")).Times(1)
261265

@@ -267,7 +271,7 @@ func TestReconcileBucket(t *testing.T) {
267271
t.Run("bucket_creation_returns_unexpected_AWS_error", func(t *testing.T) {
268272
t.Parallel()
269273

270-
svc, s3Mock := testService(t, &infrav1.S3Bucket{})
274+
svc, s3Mock := testService(t, &testServiceInput{Bucket: &infrav1.S3Bucket{}})
271275

272276
s3Mock.EXPECT().CreateBucket(gomock.Any()).Return(nil, awserr.New("foo", "", nil)).Times(1)
273277

@@ -279,7 +283,7 @@ func TestReconcileBucket(t *testing.T) {
279283
t.Run("generating_bucket_policy_fails", func(t *testing.T) {
280284
t.Parallel()
281285

282-
svc, s3Mock := testService(t, &infrav1.S3Bucket{})
286+
svc, s3Mock := testService(t, &testServiceInput{Bucket: &infrav1.S3Bucket{}})
283287

284288
s3Mock.EXPECT().CreateBucket(gomock.Any()).Return(nil, nil).Times(1)
285289
s3Mock.EXPECT().PutBucketTagging(gomock.Any()).Return(nil, nil).Times(1)
@@ -297,7 +301,7 @@ func TestReconcileBucket(t *testing.T) {
297301
t.Run("creating_bucket_policy_fails", func(t *testing.T) {
298302
t.Parallel()
299303

300-
svc, s3Mock := testService(t, &infrav1.S3Bucket{})
304+
svc, s3Mock := testService(t, &testServiceInput{Bucket: &infrav1.S3Bucket{}})
301305

302306
s3Mock.EXPECT().CreateBucket(gomock.Any()).Return(nil, nil).Times(1)
303307
s3Mock.EXPECT().PutBucketTagging(gomock.Any()).Return(nil, nil).Times(1)
@@ -307,6 +311,27 @@ func TestReconcileBucket(t *testing.T) {
307311
t.Fatalf("Expected error")
308312
}
309313
})
314+
315+
t.Run("creates_bucket_without_location", func(t *testing.T) {
316+
t.Parallel()
317+
318+
bucketName := "test"
319+
svc, s3Mock := testService(t, &testServiceInput{
320+
Region: "us-east-1",
321+
Bucket: &infrav1.S3Bucket{Name: bucketName},
322+
})
323+
input := &s3svc.CreateBucketInput{
324+
Bucket: aws.String(bucketName),
325+
}
326+
327+
s3Mock.EXPECT().CreateBucket(gomock.Eq(input)).Return(nil, nil).Times(1)
328+
s3Mock.EXPECT().PutBucketTagging(gomock.Any()).Return(nil, nil).Times(1)
329+
s3Mock.EXPECT().PutBucketPolicy(gomock.Any()).Return(nil, nil).Times(1)
330+
331+
if err := svc.ReconcileBucket(); err != nil {
332+
t.Fatalf("Unexpected error: %v", err)
333+
}
334+
})
310335
})
311336
}
312337

@@ -328,8 +353,10 @@ func TestDeleteBucket(t *testing.T) {
328353
t.Run("deletes_bucket_with_configured_name", func(t *testing.T) {
329354
t.Parallel()
330355

331-
svc, s3Mock := testService(t, &infrav1.S3Bucket{
332-
Name: bucketName,
356+
svc, s3Mock := testService(t, &testServiceInput{
357+
Bucket: &infrav1.S3Bucket{
358+
Name: bucketName,
359+
},
333360
})
334361

335362
input := &s3svc.DeleteBucketInput{
@@ -348,7 +375,7 @@ func TestDeleteBucket(t *testing.T) {
348375
t.Run("unexpected_error", func(t *testing.T) {
349376
t.Parallel()
350377

351-
svc, s3Mock := testService(t, &infrav1.S3Bucket{})
378+
svc, s3Mock := testService(t, &testServiceInput{Bucket: &infrav1.S3Bucket{}})
352379

353380
s3Mock.EXPECT().DeleteBucket(gomock.Any()).Return(nil, errors.New("err")).Times(1)
354381

@@ -360,7 +387,7 @@ func TestDeleteBucket(t *testing.T) {
360387
t.Run("unexpected_AWS_error", func(t *testing.T) {
361388
t.Parallel()
362389

363-
svc, s3Mock := testService(t, &infrav1.S3Bucket{})
390+
svc, s3Mock := testService(t, &testServiceInput{Bucket: &infrav1.S3Bucket{}})
364391

365392
s3Mock.EXPECT().DeleteBucket(gomock.Any()).Return(nil, awserr.New("foo", "", nil)).Times(1)
366393

@@ -373,7 +400,7 @@ func TestDeleteBucket(t *testing.T) {
373400
t.Run("ignores_when_bucket_has_already_been_removed", func(t *testing.T) {
374401
t.Parallel()
375402

376-
svc, s3Mock := testService(t, &infrav1.S3Bucket{})
403+
svc, s3Mock := testService(t, &testServiceInput{Bucket: &infrav1.S3Bucket{}})
377404

378405
s3Mock.EXPECT().DeleteBucket(gomock.Any()).Return(nil, awserr.New(s3svc.ErrCodeNoSuchBucket, "", nil)).Times(1)
379406

@@ -385,7 +412,7 @@ func TestDeleteBucket(t *testing.T) {
385412
t.Run("skips_bucket_removal_when_bucket_is_not_empty", func(t *testing.T) {
386413
t.Parallel()
387414

388-
svc, s3Mock := testService(t, &infrav1.S3Bucket{})
415+
svc, s3Mock := testService(t, &testServiceInput{Bucket: &infrav1.S3Bucket{}})
389416

390417
s3Mock.EXPECT().DeleteBucket(gomock.Any()).Return(nil, awserr.New("BucketNotEmpty", "", nil)).Times(1)
391418

@@ -406,8 +433,10 @@ func TestCreateObject(t *testing.T) {
406433
t.Run("for_machine", func(t *testing.T) {
407434
t.Parallel()
408435

409-
svc, s3Mock := testService(t, &infrav1.S3Bucket{
410-
Name: bucketName,
436+
svc, s3Mock := testService(t, &testServiceInput{
437+
Bucket: &infrav1.S3Bucket{
438+
Name: bucketName,
439+
},
411440
})
412441

413442
machineScope := &scope.MachineScope{
@@ -487,7 +516,7 @@ func TestCreateObject(t *testing.T) {
487516
t.Run("is_idempotent", func(t *testing.T) {
488517
t.Parallel()
489518

490-
svc, s3Mock := testService(t, &infrav1.S3Bucket{})
519+
svc, s3Mock := testService(t, &testServiceInput{Bucket: &infrav1.S3Bucket{}})
491520

492521
machineScope := &scope.MachineScope{
493522
Machine: &clusterv1.Machine{},
@@ -516,7 +545,7 @@ func TestCreateObject(t *testing.T) {
516545
t.Run("object_creation_fails", func(t *testing.T) {
517546
t.Parallel()
518547

519-
svc, s3Mock := testService(t, &infrav1.S3Bucket{})
548+
svc, s3Mock := testService(t, &testServiceInput{Bucket: &infrav1.S3Bucket{}})
520549

521550
machineScope := &scope.MachineScope{
522551
Machine: &clusterv1.Machine{},
@@ -542,7 +571,7 @@ func TestCreateObject(t *testing.T) {
542571
t.Run("given_empty_machine_scope", func(t *testing.T) {
543572
t.Parallel()
544573

545-
svc, _ := testService(t, &infrav1.S3Bucket{})
574+
svc, _ := testService(t, &testServiceInput{Bucket: &infrav1.S3Bucket{}})
546575

547576
bootstrapDataURL, err := svc.Create(nil, []byte("foo"))
548577
if err == nil {
@@ -558,7 +587,7 @@ func TestCreateObject(t *testing.T) {
558587
t.Run("given_empty_bootstrap_data", func(t *testing.T) {
559588
t.Parallel()
560589

561-
svc, _ := testService(t, &infrav1.S3Bucket{})
590+
svc, _ := testService(t, &testServiceInput{Bucket: &infrav1.S3Bucket{}})
562591

563592
machineScope := &scope.MachineScope{
564593
Machine: &clusterv1.Machine{},
@@ -615,8 +644,10 @@ func TestDeleteObject(t *testing.T) {
615644

616645
expectedBucketName := "foo"
617646

618-
svc, s3Mock := testService(t, &infrav1.S3Bucket{
619-
Name: expectedBucketName,
647+
svc, s3Mock := testService(t, &testServiceInput{
648+
Bucket: &infrav1.S3Bucket{
649+
Name: expectedBucketName,
650+
},
620651
})
621652

622653
machineScope := &scope.MachineScope{
@@ -666,7 +697,7 @@ func TestDeleteObject(t *testing.T) {
666697
t.Run("succeeds_when_bucket_has_already_been_removed", func(t *testing.T) {
667698
t.Parallel()
668699

669-
svc, s3Mock := testService(t, &infrav1.S3Bucket{})
700+
svc, s3Mock := testService(t, &testServiceInput{Bucket: &infrav1.S3Bucket{}})
670701

671702
machineScope := &scope.MachineScope{
672703
Machine: &clusterv1.Machine{},
@@ -690,7 +721,7 @@ func TestDeleteObject(t *testing.T) {
690721
t.Run("object_deletion_fails", func(t *testing.T) {
691722
t.Parallel()
692723

693-
svc, s3Mock := testService(t, &infrav1.S3Bucket{})
724+
svc, s3Mock := testService(t, &testServiceInput{Bucket: &infrav1.S3Bucket{}})
694725

695726
machineScope := &scope.MachineScope{
696727
Machine: &clusterv1.Machine{},
@@ -712,7 +743,7 @@ func TestDeleteObject(t *testing.T) {
712743
t.Run("given_empty_machine_scope", func(t *testing.T) {
713744
t.Parallel()
714745

715-
svc, _ := testService(t, &infrav1.S3Bucket{})
746+
svc, _ := testService(t, nil)
716747

717748
if err := svc.Delete(nil); err == nil {
718749
t.Fatalf("Expected error")
@@ -742,7 +773,7 @@ func TestDeleteObject(t *testing.T) {
742773
t.Run("is_idempotent", func(t *testing.T) {
743774
t.Parallel()
744775

745-
svc, s3Mock := testService(t, &infrav1.S3Bucket{})
776+
svc, s3Mock := testService(t, &testServiceInput{Bucket: &infrav1.S3Bucket{}})
746777

747778
machineScope := &scope.MachineScope{
748779
Machine: &clusterv1.Machine{},
@@ -766,7 +797,14 @@ func TestDeleteObject(t *testing.T) {
766797
})
767798
}
768799

769-
func testService(t *testing.T, bucket *infrav1.S3Bucket) (*s3.Service, *mock_s3iface.MockS3API) {
800+
type testServiceInput struct {
801+
Bucket *infrav1.S3Bucket
802+
Region string
803+
}
804+
805+
const testAWSRegion string = "us-west-2"
806+
807+
func testService(t *testing.T, si *testServiceInput) (*s3.Service, *mock_s3iface.MockS3API) {
770808
t.Helper()
771809

772810
mockCtrl := gomock.NewController(t)
@@ -780,6 +818,13 @@ func testService(t *testing.T, bucket *infrav1.S3Bucket) (*s3.Service, *mock_s3i
780818
_ = infrav1.AddToScheme(scheme)
781819
client := fake.NewClientBuilder().WithScheme(scheme).Build()
782820

821+
if si == nil {
822+
si = &testServiceInput{}
823+
}
824+
if si.Region == "" {
825+
si.Region = testAWSRegion
826+
}
827+
783828
scope, err := scope.NewClusterScope(scope.ClusterScopeParams{
784829
Client: client,
785830
Cluster: &clusterv1.Cluster{
@@ -790,8 +835,8 @@ func testService(t *testing.T, bucket *infrav1.S3Bucket) (*s3.Service, *mock_s3i
790835
},
791836
AWSCluster: &infrav1.AWSCluster{
792837
Spec: infrav1.AWSClusterSpec{
793-
S3Bucket: bucket,
794-
Region: "us-west-2",
838+
S3Bucket: si.Bucket,
839+
Region: si.Region,
795840
AdditionalTags: infrav1.Tags{
796841
"additional": "from-aws-cluster",
797842
},

0 commit comments

Comments
 (0)