From a8350679cf33722289a779d3cecb5a58212baa6c Mon Sep 17 00:00:00 2001 From: Himani Anil Deshpande Date: Fri, 23 May 2025 15:38:48 -0400 Subject: [PATCH 1/3] Adding AccountID for s3 do-not-delete bucket --- .../policies/parallelcluster-policies.yaml | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/cloudformation/policies/parallelcluster-policies.yaml b/cloudformation/policies/parallelcluster-policies.yaml index 90f44fd335..5e028cb820 100644 --- a/cloudformation/policies/parallelcluster-policies.yaml +++ b/cloudformation/policies/parallelcluster-policies.yaml @@ -524,7 +524,6 @@ Resources: - Action: - s3:* Resource: - - !Sub arn:${AWS::Partition}:s3:::parallelcluster-* - !Sub arn:${AWS::Partition}:s3:::aws-parallelcluster-* Effect: Allow Condition: !If @@ -533,6 +532,19 @@ Resources: - StringEquals: aws:RequestedRegion: - !Ref Region + Sid: PclusterS3ResourcesBucket + - Action: + - s3:* + Resource: + - !Sub arn:${AWS::Partition}:s3:::parallelcluster-* + Effect: Allow + Condition: !If + - IsMultiRegion + - !Ref AWS::NoValue + - StringEquals: + aws:RequestedRegion: + - !Ref Region + aws:ResourceAccount: !Sub ${AWS::AccountId} Sid: S3ResourcesBucket - Action: - s3:Get* @@ -759,6 +771,9 @@ Resources: - s3:ListBucketVersions Resource: - !Sub 'arn:${AWS::Partition}:s3:::parallelcluster-*/*' + Condition: + StringEquals: + aws:ResourceAccount: !Sub ${AWS::AccountId} - Sid: CloudWatchDeleteImage Effect: Allow Action: From 865893556333119e39e7edd7049171b186e8152c Mon Sep 17 00:00:00 2001 From: Himani Anil Deshpande Date: Fri, 23 May 2025 16:04:33 -0400 Subject: [PATCH 2/3] Adding accountID for checking if default bucket is owned by the creator --- cli/src/pcluster/aws/s3.py | 7 +++++-- cli/src/pcluster/models/s3_bucket.py | 9 ++++++--- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/cli/src/pcluster/aws/s3.py b/cli/src/pcluster/aws/s3.py index 1bb84ca17d..817d0879db 100644 --- a/cli/src/pcluster/aws/s3.py +++ b/cli/src/pcluster/aws/s3.py @@ -39,10 +39,13 @@ def head_object(self, bucket_name, object_name, expected_bucket_owner=None): error_code=client_error.response["Error"]["Code"], ) - def head_bucket(self, bucket_name): + def head_bucket(self, bucket_name, expected_bucket_owner=None): """Retrieve metadata for a bucket without returning the object itself.""" try: - return self._client.head_bucket(Bucket=bucket_name) + if expected_bucket_owner: + return self._client.head_bucket(Bucket=bucket_name, ExpectedBucketOwner=expected_bucket_owner) + else: + return self._client.head_bucket(Bucket=bucket_name) except ClientError as client_error: raise AWSClientError( function_name="head_bucket", diff --git a/cli/src/pcluster/models/s3_bucket.py b/cli/src/pcluster/models/s3_bucket.py index fdd4cdeb5b..8232ae1ba5 100644 --- a/cli/src/pcluster/models/s3_bucket.py +++ b/cli/src/pcluster/models/s3_bucket.py @@ -133,9 +133,12 @@ def generate_s3_bucket_hash_suffix(account_id, region): """ return hashlib.sha256((account_id + region).encode()).hexdigest()[0:16] - def check_bucket_exists(self): + def check_bucket_exists(self, default_bucket): """Check bucket existence by bucket name.""" - AWSApi.instance().s3.head_bucket(bucket_name=self.name) + if default_bucket: + AWSApi.instance().s3.head_bucket(bucket_name=self.name, expected_bucket_owner=self.account_id) + else: + AWSApi.instance().s3.head_bucket(bucket_name=self.name) def create_bucket(self): """Create a new S3 bucket.""" @@ -457,7 +460,7 @@ def _check_custom_bucket(cls, service_name: str, custom_s3_bucket: str, artifact def _check_default_bucket(cls, service_name: str, artifact_directory: str, stack_name: str): bucket = S3Bucket(service_name=service_name, artifact_directory=artifact_directory, stack_name=stack_name) try: - bucket.check_bucket_exists() + bucket.check_bucket_exists(default_bucket=True) except AWSClientError as e: cls._create_bucket(bucket, e) From bd48616d276db29a9e7cbeab2701d6ccacf91905 Mon Sep 17 00:00:00 2001 From: Himani Anil Deshpande Date: Fri, 23 May 2025 17:12:44 -0400 Subject: [PATCH 3/3] Revert "Adding AccountID for s3 do-not-delete bucket" This reverts commit a8350679cf33722289a779d3cecb5a58212baa6c. --- .../policies/parallelcluster-policies.yaml | 17 +---------------- 1 file changed, 1 insertion(+), 16 deletions(-) diff --git a/cloudformation/policies/parallelcluster-policies.yaml b/cloudformation/policies/parallelcluster-policies.yaml index 5e028cb820..90f44fd335 100644 --- a/cloudformation/policies/parallelcluster-policies.yaml +++ b/cloudformation/policies/parallelcluster-policies.yaml @@ -521,22 +521,11 @@ Resources: - !Sub arn:${AWS::Partition}:lambda:${Region}:${AWS::AccountId}:function:pcluster-* Effect: Allow Sid: Lambda - - Action: - - s3:* - Resource: - - !Sub arn:${AWS::Partition}:s3:::aws-parallelcluster-* - Effect: Allow - Condition: !If - - IsMultiRegion - - !Ref AWS::NoValue - - StringEquals: - aws:RequestedRegion: - - !Ref Region - Sid: PclusterS3ResourcesBucket - Action: - s3:* Resource: - !Sub arn:${AWS::Partition}:s3:::parallelcluster-* + - !Sub arn:${AWS::Partition}:s3:::aws-parallelcluster-* Effect: Allow Condition: !If - IsMultiRegion @@ -544,7 +533,6 @@ Resources: - StringEquals: aws:RequestedRegion: - !Ref Region - aws:ResourceAccount: !Sub ${AWS::AccountId} Sid: S3ResourcesBucket - Action: - s3:Get* @@ -771,9 +759,6 @@ Resources: - s3:ListBucketVersions Resource: - !Sub 'arn:${AWS::Partition}:s3:::parallelcluster-*/*' - Condition: - StringEquals: - aws:ResourceAccount: !Sub ${AWS::AccountId} - Sid: CloudWatchDeleteImage Effect: Allow Action: