Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ CHANGELOG

**BUG FIXES**
- Reduce EFA installation time for Ubuntu by ~20 minutes by only holding kernel packages for the installed kernel.
- Add GetFunction and GetPolicy permissions to PClusterBuildImageCleanupRole to prevent AccessDenied errors during build image stack deletion.

3.14.1
------
Expand Down
2 changes: 1 addition & 1 deletion cli/src/pcluster/constants.py
Original file line number Diff line number Diff line change
Expand Up @@ -343,7 +343,7 @@ class Operation(Enum):

PCLUSTER_BUILD_IMAGE_CLEANUP_ROLE_PREFIX = "PClusterBuildImageCleanupRole"
# Tag key & expected revision (increment when policy widens)
PCLUSTER_BUILD_IMAGE_CLEANUP_ROLE_REVISION = 1
PCLUSTER_BUILD_IMAGE_CLEANUP_ROLE_REVISION = 2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC we need to increment only when we introduce breaking changes to the policy. This is an additive fix, not a breaking change. So I think we can keep the version to 1.

Can you please double check?
If this is the case, then please also fix the comment, as it could be misleading.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, I was wrong. We need to increase the version at every change, because we do not modify the exisitng role
See code

if already_bootstrapped:
return role_arn

PCLUSTER_BUILD_IMAGE_CLEANUP_ROLE_BOOTSTRAP_TAG_KEY = "parallelcluster:build-image-cleanup-role-bootstrapped"

P6E_GB200 = "p6e-gb200"
Expand Down
7 changes: 6 additions & 1 deletion cli/src/pcluster/imagebuilder_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,12 @@ def _expected_inline_policy(account_id: str, partition: str):
{"Action": "ec2:CreateTags", "Resource": f"arn:{partition}:ec2:*::image/*", "Effect": "Allow"},
{"Action": "tag:TagResources", "Resource": "*", "Effect": "Allow"},
{
"Action": ["lambda:DeleteFunction", "lambda:RemovePermission"],
"Action": [
"lambda:DeleteFunction",
"lambda:RemovePermission",
"lambda:GetFunction",
"lambda:GetPolicy",
],
"Resource": f"arn:{partition}:lambda:*:{account_id}:function:ParallelClusterImage-*",
"Effect": "Allow",
},
Expand Down
24 changes: 20 additions & 4 deletions cli/tests/pcluster/cli/test_build_image.py
Original file line number Diff line number Diff line change
Expand Up @@ -283,26 +283,42 @@ def test_ensure_default_build_image_stack_cleanup_role_permission_denied(self, a
aws_api_mock.iam.tag_role.assert_not_called()

@pytest.mark.parametrize(
"account_id, partition",
"account_id, partition, actions",
[
("123456789012", "aws"),
("000000000000", "aws-us-gov"),
(
"123456789012",
"aws",
["lambda:DeleteFunction", "lambda:RemovePermission", "lambda:GetFunction", "lambda:GetPolicy"],
),
(
"000000000000",
"aws-us-gov",
["lambda:DeleteFunction", "lambda:RemovePermission", "lambda:GetFunction", "lambda:GetPolicy"],
),
],
)
def test_expected_inline_policy_dynamic_fields(self, account_id, partition):
def test_expected_inline_policy_dynamic_fields(self, account_id, partition, actions):
raw = _expected_inline_policy(account_id, partition)
policy = json.loads(raw)
assert policy["Version"] == "2012-10-17"
assert len(policy["Statement"]) == 13
for statement in policy["Statement"]:
resources = statement["Resource"]
action = statement["Action"]
action = action if isinstance(action, list) else [action]
for act in action:
if act in actions:
actions.remove(act)

resources = resources if isinstance(resources, list) else [resources]
for res in resources:
if res == "*":
continue
assert f"arn:{partition}" in res
if not res == f"arn:{partition}:ec2:*::image/*":
assert f":{account_id}:" in res
if len(actions) != 0:
raise AssertionError(f"Actions {actions} are not in the policy")

def _build_args(self, args):
args = [[k, v] if v is not None else [k] for k, v in args.items()]
Expand Down
Loading