Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(iam): Modify addManagedPolicy to compare ARN instead of instance reference #25529

Merged
merged 4 commits into from
Jun 8, 2023

Conversation

d0819
Copy link
Contributor

@d0819 d0819 commented May 11, 2023

Issue

When creating a role, the following warning message appeared:

Policy large: 11 exceeds 10 managed policies attached to a Role, this requires a quota increase

This was caused by the same managed policy being added multiple times.

Although there was only one managed policy in the created template, it appears that the managedPolicies field of the Role class has multiple instances of the same managed policy added to it.


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

@aws-cdk-automation aws-cdk-automation requested a review from a team May 11, 2023 05:35
@gitpod-io
Copy link

gitpod-io bot commented May 11, 2023

@github-actions github-actions bot added p2 beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK labels May 11, 2023
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

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.

@d0819 d0819 changed the title chore(aws-iam): Modify addManagedPolicy to compare ARN instead of instance reference chore(iam): Modify addManagedPolicy to compare ARN instead of instance reference May 11, 2023
@aws-cdk-automation aws-cdk-automation dismissed their stale review May 11, 2023 05:39

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

@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label May 11, 2023
@corymhall corymhall changed the title chore(iam): Modify addManagedPolicy to compare ARN instead of instance reference fix(iam): Modify addManagedPolicy to compare ARN instead of instance reference May 24, 2023
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

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.

@d0819
Copy link
Contributor Author

d0819 commented May 25, 2023

Exemption Request

This change only affects the managedPolicies field that is held within the Role class, and does not have any impact on the generated template. Therefore, I believe that no changes are necessary for testing.

@aws-cdk-automation aws-cdk-automation added the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label May 25, 2023
@@ -551,7 +551,7 @@ export class Role extends Resource implements IRole {
if (this._precreatedRole) {
return this._precreatedRole.addManagedPolicy(policy);
} else {
if (this.managedPolicies.find(mp => mp === policy)) { return; }
if (this.managedPolicies.some(mp => mp.managedPolicyArn === policy.managedPolicyArn)) { return; }
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need an integration test, but we do need a unit test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried writing a unit test. Could you please take a look at it?

I added addToPrincipalPolicy() to this test because this warning only appears when defaultPolicy exists.

@corymhall corymhall added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label May 25, 2023
@aws-cdk-automation aws-cdk-automation dismissed their stale review May 25, 2023 17:17

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

@aws-cdk-automation aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label May 25, 2023
@d0819 d0819 requested a review from corymhall May 29, 2023 03:34
@mergify mergify bot dismissed corymhall’s stale review June 5, 2023 13:15

Pull request has been modified.

@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jun 5, 2023
@mergify
Copy link
Contributor

mergify bot commented Jun 8, 2023

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
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 0d6c30a
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify mergify bot merged commit 5cc2b0b into aws:main Jun 8, 2023
@mergify
Copy link
Contributor

mergify bot commented Jun 8, 2023

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK p2 pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. pr-linter/exempt-integ-test The PR linter will not require integ test changes pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants