fix(iam): importedRoleStackSafeDefaultPolicyName feature flag results in excessively long IAM policy names #27548

yamoyamoto · 2023-10-15T08:20:31Z

When the importedRoleStackSafeDefaultPolicyName feature flag is enabled, the method to calculate the IAM Policy Name within aws_iam.ImportedRole.addToPrincipalPolicy() changes. Specifically, if the generated IAM Policy Name exceeds the maximum allowed length of 128 characters, it will be truncated using Names.uniqueResourceName().

Previously, the Names.UniqueId() method was used to generate the Policy Name. This method does not allow you to set a maximum length, so if the name exceeded the limit, it would be overwritten using Names.uniqueResourceName()—a function that allows for length specification.

I considered replacing Names.UniqueId() entirely with Names.uniqueResourceName(). However, this is on hold due to concerns that existing Policy Names could be affected. If a complete replacement poses no issues, your guidance is appreciated, as I'm not fully versed in the logic behind these methods.

Closes #27409 , #24441

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

…cy name length

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

kaizencc

Looks good @yamoyamoto. Sorry it took us so long to get to this PR

kaizencc · 2023-12-18T16:53:28Z

packages/aws-cdk-lib/aws-iam/lib/private/imported-role.ts

+        ? `${prefix}${Names.uniqueId(this)}`
+        : prefix;
+      if (defaultDefaultPolicyName.length > MAX_POLICY_NAME_LEN) {
+        defaultDefaultPolicyName = `Policy${Names.uniqueResourceName(this, { maxLength: MAX_POLICY_NAME_LEN - prefix.length })}`;


I think making "Policy" its on var was kind of overkill, but if we're doing that then we should use it on line 54 also.

Indeed, it does become a bit difficult to read. However, we need prefix.length to subtract the length of 'Policy' from the policy name limit when using Names.uniqueResourceName(), so it seems reasonable to keep it as a variable.

but if we're doing that then we should use it on line 54 also.

My apologies, it was a simple oversight. I have now used the prefix variable.

c009d92

kaizencc · 2023-12-18T17:55:44Z

packages/aws-cdk-lib/aws-iam/test/role.from-role-arn.test.ts

@@ -78,6 +78,21 @@ describe('IAM Role.fromRoleArn', () => {
          expect(stack2PolicyNameCapture.asString()).toMatch(/PolicyRoleStack2ImportedRole.*/);
        });

+        test('Policy name is truncated to a maximum length of 128 characters when the original name exceeds this limit', () =>{


Suggested change

test('Policy name is truncated to a maximum length of 128 characters when the original name exceeds this limit', () =>{

test('Policy name is truncated to a maximum length of 128 characters when the original name exceeds this limit', () => {

Sorry, I updated this.

0713874

kaizencc · 2023-12-18T18:12:22Z

packages/aws-cdk-lib/aws-iam/lib/private/imported-role.ts

-      const defaultDefaultPolicyName = useUniqueName
-        ? `Policy${Names.uniqueId(this)}`
-        : 'Policy';
+      const prefix = 'Policy';


can we add a comment here on why we are doing it this way (to hopefully preserve existing policy names)

I added comment. I would appreciate it if you could confirm.

4850d82

Pull request has been modified.

kaizencc

Thanks @yamoyamoto!

mergify · 2023-12-20T17:10:15Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

mergify · 2023-12-20T17:14:05Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

Pull request has been modified.

mergify · 2023-12-21T03:33:58Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2023-12-21T04:02:25Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 46967dc
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2023-12-21T04:02:47Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

… in excessively long IAM policy names (aws#27548) When the importedRoleStackSafeDefaultPolicyName feature flag is enabled, the method to calculate the IAM Policy Name within `aws_iam.ImportedRole.addToPrincipalPolicy()` changes. Specifically, if the generated IAM Policy Name exceeds the maximum allowed length of 128 characters, it will be truncated using `Names.uniqueResourceName()`. Previously, the `Names.UniqueId()` method was used to generate the Policy Name. This method does not allow you to set a maximum length, so if the name exceeded the limit, it would be overwritten using `Names.uniqueResourceName()`—a function that allows for length specification. I considered replacing `Names.UniqueId()` entirely with `Names.uniqueResourceName()`. However, this is on hold due to concerns that existing Policy Names could be affected. If a complete replacement poses no issues, your guidance is appreciated, as I'm not fully versed in the logic behind these methods. Closes aws#27409 , aws#24441 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

yamoyamoto added 2 commits October 13, 2023 22:14

add: test case when generated policy name too long

93de00f

fix: truncate Policy name attached imported Role when it's too long

ec60dfb

github-actions bot added the beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK label Oct 15, 2023

aws-cdk-automation requested a review from a team October 15, 2023 08:20

github-actions bot added bug This issue is a bug. effort/small Small work item – less than a day of effort p1 labels Oct 15, 2023

aws-cdk-automation previously requested changes Oct 15, 2023

View reviewed changes

yamoyamoto changed the title ~~fix(aws-iam): importedRoleStackSafeDefaultPolicyName feature flag results in excessively long IAM policy names~~ fix(iam): importedRoleStackSafeDefaultPolicyName feature flag results in excessively long IAM policy names Oct 15, 2023

test(iam): add ImportedRole that has long id to assert generated poli…

fdb6fbb

…cy name length

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Oct 16, 2023

github-actions bot mentioned this pull request Nov 1, 2023

Monthly PR metrics report - October 2023 #27786

Closed

paulhcsun added 2 commits December 5, 2023 09:17

Merge branch 'main' into truncate-policy-name-attached-imported-role

cd7d4f3

Merge branch 'main' into truncate-policy-name-attached-imported-role

f284e7c

kaizencc previously requested changes Dec 18, 2023

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Dec 18, 2023

yamoyamoto added 3 commits December 19, 2023 09:06

add comment explaining why Names.uniqueId() is retained

4850d82

fix: added spaces because it does not conform to the code style

0713874

fix: use prefix variable instead of raw string

c009d92

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Dec 19, 2023

kaizencc previously approved these changes Dec 20, 2023

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Dec 20, 2023

Merge branch 'main' into truncate-policy-name-attached-imported-role

67e5495

kaizencc approved these changes Dec 21, 2023

View reviewed changes

Merge branch 'main' into truncate-policy-name-attached-imported-role

46967dc

mergify bot merged commit 4f88db6 into aws:main Dec 21, 2023
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(iam): importedRoleStackSafeDefaultPolicyName feature flag results in excessively long IAM policy names #27548

fix(iam): importedRoleStackSafeDefaultPolicyName feature flag results in excessively long IAM policy names #27548

yamoyamoto commented Oct 15, 2023 •

edited

Loading

aws-cdk-automation left a comment

kaizencc left a comment

kaizencc Dec 18, 2023

yamoyamoto Dec 19, 2023

kaizencc Dec 18, 2023

yamoyamoto Dec 19, 2023

kaizencc Dec 18, 2023

yamoyamoto Dec 19, 2023

kaizencc left a comment

mergify bot commented Dec 20, 2023

mergify bot commented Dec 20, 2023

mergify bot commented Dec 21, 2023

aws-cdk-automation commented Dec 21, 2023

mergify bot commented Dec 21, 2023

	test('Policy name is truncated to a maximum length of 128 characters when the original name exceeds this limit', () =>{
	test('Policy name is truncated to a maximum length of 128 characters when the original name exceeds this limit', () => {

fix(iam): importedRoleStackSafeDefaultPolicyName feature flag results in excessively long IAM policy names #27548

fix(iam): importedRoleStackSafeDefaultPolicyName feature flag results in excessively long IAM policy names #27548

Conversation

yamoyamoto commented Oct 15, 2023 • edited Loading

aws-cdk-automation left a comment

Choose a reason for hiding this comment

kaizencc left a comment

Choose a reason for hiding this comment

kaizencc Dec 18, 2023

Choose a reason for hiding this comment

yamoyamoto Dec 19, 2023

Choose a reason for hiding this comment

kaizencc Dec 18, 2023

Choose a reason for hiding this comment

yamoyamoto Dec 19, 2023

Choose a reason for hiding this comment

kaizencc Dec 18, 2023

Choose a reason for hiding this comment

yamoyamoto Dec 19, 2023

Choose a reason for hiding this comment

kaizencc left a comment

Choose a reason for hiding this comment

mergify bot commented Dec 20, 2023

mergify bot commented Dec 20, 2023

mergify bot commented Dec 21, 2023

aws-cdk-automation commented Dec 21, 2023

AWS CodeBuild CI Report

mergify bot commented Dec 21, 2023

yamoyamoto commented Oct 15, 2023 •

edited

Loading