Repository: kubernetes-sigs/cluster-api-provider-aws
Pull request #4732 (2024-01-11 18:39:28)

:bug: S3 Bucket should be created in the same region, always add transport encryption policy
<!-- Thanks for this PR! If this is your first PR please read the [contributing guide](../CONTRIBUTING.md) -->
<!-- If this PR is still work-in-progress and is being open for visibility please prefix the title with `WIP:` -->

**What type of PR is this?**

<!--
Add one of the following kinds:
/kind feature
/kind bug
/kind api-change
/kind cleanup
/kind deprecation
/kind design
/kind documentation
/kind failing-test
/kind flake
/kind regression
/kind support
-->
/kind bug

**What this PR does / why we need it**:

Currently the S3 bucket is always created in us-east-1, regardless of where the cluster is located. In addition, this PR always ensures that we add a policy to ensure transport encryption is enabled, even when using presigned URLs.

<!-- Enter a description of the change and why this change is needed -->

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
Fixes #

**Special notes for your reviewer**:

**Checklist**:
<!-- Put an "X" character inside the brackets of each completed task. Some may be optional depending on the PR in which case these can be deleted -->

- [x] squashed commits
- [ ] includes documentation
- [ ] includes [emojis](https://github.com/kubernetes-sigs/kubebuilder-release-tools?tab=readme-ov-file#kubebuilder-project-versioning)
- [ ] adds unit tests
- [ ] adds or updates e2e tests

**Release note**:
<!--  Write your release note:
1. Enter your extended release note in the below block. If the PR requires additional action from users switching to the new release, include the string "action required".
2. If no release note is required, just write "NONE".
-->
```release-note
When using Ignition, the S3 bucket was previously created in us-east-1 regardless of where the cluster was located, new S3 buckets will be created within the same region as the cluster.
```


k8s-ci-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by:
**Once this PR has been reviewed and has the lgtm label**, please ask for approval from [vincepri](https://github.com/vincepri). For more information see [the Kubernetes Code Review Process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process).

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubernetes-sigs%2Fcluster-api-provider-aws).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/kubernetes-sigs/cluster-api-provider-aws/blob/main/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["vincepri"]} -->

Patch:
diff --git a/pkg/cloud/services/s3/s3.go b/pkg/cloud/services/s3/s3.go
index 4bccb6307..a6bbf26b8 100644
--- a/pkg/cloud/services/s3/s3.go
+++ b/pkg/cloud/services/s3/s3.go
@@ -225,6 +225,9 @@ 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{
+			LocationConstraint: aws.String(s.scope.Region()),
+		},
 	}
 
 	_, err := s.S3Client.CreateBucket(input)
@@ -251,11 +254,6 @@ func (s *Service) createBucketIfNotExist(bucketName string) error {
 }
 
 func (s *Service) ensureBucketPolicy(bucketName string) error {
-	if s.scope.Bucket().PresignedURLDuration != nil {
-		// If presigned URL is enabled, we don't need to set bucket policy.
-		return nil
-	}
-
 	bucketPolicy, err := s.bucketPolicy(bucketName)
 	if err != nil {
 		return errors.Wrap(err, "generating Bucket policy")
@@ -322,15 +320,6 @@ func (s *Service) bucketPolicy(bucketName string) (string, error) {
 	partition := system.GetPartitionFromRegion(s.scope.Region())
 
 	statements := []iam.StatementEntry{
-		{
-			Sid:    "control-plane",
-			Effect: iam.EffectAllow,
-			Principal: map[iam.PrincipalType]iam.PrincipalID{
-				iam.PrincipalAWS: []string{fmt.Sprintf("arn:%s:iam::%s:role/%s", partition, *accountID.Account, bucket.ControlPlaneIAMInstanceProfile)},
-			},
-			Action:   []string{"s3:GetObject"},
-			Resource: []string{fmt.Sprintf("arn:%s:s3:::%s/control-plane/*", partition, bucketName)},
-		},
 		{
 			Sid:    "ForceSSLOnlyAccess",
 			Effect: iam.EffectDeny,
@@ -347,16 +336,30 @@ func (s *Service) bucketPolicy(bucketName string) (string, error) {
 		},
 	}
 
-	for _, iamInstanceProfile := range bucket.NodesIAMInstanceProfiles {
-		statements = append(statements, iam.StatementEntry{
-			Sid:    iamInstanceProfile,
-			Effect: iam.EffectAllow,
-			Principal: map[iam.PrincipalType]iam.PrincipalID{
-				iam.PrincipalAWS: []string{fmt.Sprintf("arn:%s:iam::%s:role/%s", partition, *accountID.Account, iamInstanceProfile)},
-			},
-			Action:   []string{"s3:GetObject"},
-			Resource: []string{fmt.Sprintf("arn:%s:s3:::%s/node/*", partition, bucketName)},
-		})
+	if bucket.PresignedURLDuration == nil {
+		if bucket.ControlPlaneIAMInstanceProfile != "" {
+			statements = append(statements, iam.StatementEntry{
+				Sid:    "control-plane",
+				Effect: iam.EffectAllow,
+				Principal: map[iam.PrincipalType]iam.PrincipalID{
+					iam.PrincipalAWS: []string{fmt.Sprintf("arn:%s:iam::%s:role/%s", partition, *accountID.Account, bucket.ControlPlaneIAMInstanceProfile)},
+				},
+				Action:   []string{"s3:GetObject"},
+				Resource: []string{fmt.Sprintf("arn:%s:s3:::%s/control-plane/*", partition, bucketName)},
+			})
+		}
+
+		for _, iamInstanceProfile := range bucket.NodesIAMInstanceProfiles {
+			statements = append(statements, iam.StatementEntry{
+				Sid:    iamInstanceProfile,
+				Effect: iam.EffectAllow,
+				Principal: map[iam.PrincipalType]iam.PrincipalID{
+					iam.PrincipalAWS: []string{fmt.Sprintf("arn:%s:iam::%s:role/%s", partition, *accountID.Account, iamInstanceProfile)},
+				},
+				Action:   []string{"s3:GetObject"},
+				Resource: []string{fmt.Sprintf("arn:%s:s3:::%s/node/*", partition, bucketName)},
+			})
+		}
 	}
 
 	policy := iam.PolicyDocument{


Test patch:
diff --git a/pkg/cloud/services/s3/s3_test.go b/pkg/cloud/services/s3/s3_test.go
index 0f4483522..7ce58d19d 100644
--- a/pkg/cloud/services/s3/s3_test.go
+++ b/pkg/cloud/services/s3/s3_test.go
@@ -72,6 +72,9 @@ func TestReconcileBucket(t *testing.T) {
 
 		input := &s3svc.CreateBucketInput{
 			Bucket: aws.String(expectedBucketName),
+			CreateBucketConfiguration: &s3svc.CreateBucketConfiguration{
+				LocationConstraint: aws.String("us-west-2"),
+			},
 		}
 
 		s3Mock.EXPECT().CreateBucket(gomock.Eq(input)).Return(nil, nil).Times(1)
@@ -788,6 +791,7 @@ func testService(t *testing.T, bucket *infrav1.S3Bucket) (*s3.Service, *mock_s3i
 		AWSCluster: &infrav1.AWSCluster{
 			Spec: infrav1.AWSClusterSpec{
 				S3Bucket: bucket,
+				Region:   "us-west-2",
 				AdditionalTags: infrav1.Tags{
 					"additional": "from-aws-cluster",
 				},