S3 bucket region creation, ensure transport policy w/ presigned URLs

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. Signed-off-by: Vince Prignano <vincepri@redhat.com>
kubernetes-sigs · Jan 11, 2024 · c0e941b · c0e941b
1 parent f8b9cfd
commit c0e941b
Show file tree

Hide file tree

Showing 2 changed files with 31 additions and 24 deletions.
diff --git 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{

diff --git 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",
 				},