fix(iam): `Role.addManagedPolicy()` does not work for imported roles `IRole` #8307 #31212

stm29 · 2024-08-24T19:01:23Z

Issue #8307

Reason for this change

This addManagedPolicy() just does nothing, neither throws warning nor fails.

Description of changes

This implements addManagedPolicy() for ImportedRole
This throws Error, when IRole is used with IManagedPolicy, due to constrains

Approaches decided

we can't change addManagedPolicy(policy: IManagedPolicy) to addManagedPolicy(policy: ManagedPolicy) (i.e., to ManagedPolicy) , like how attachInlinePolicy(policy: Policy), it will break for almost many customers.

aws-cdk/packages/aws-cdk-lib/aws-iam/lib/identity-base.ts

Lines 17 to 21 in 823ff6e

    
             /** 
        
              * Attaches a managed policy to this principal. 
        
              * @param policy The managed policy 
        
              */ 
        
             addManagedPolicy(policy: IManagedPolicy): void;

So, we need to handle this gracefully when accepting only ManagedPolicy.

We can't use IRole & IManagedPolicy to do the work at the same time, so we need to allow ManagedPolicy in params, and throw Error when IManagedPolicy is being sent, because of how attachToRole() will be used by addManagedPolicy()
This PR Implements follows,
- enables addManagedPolicy()
- Accepts ManagedPolicy
- Throws understandable Error, when IRole is used with IManagedPolicy.

Description of how you validated changes

unit & integration tests

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

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

…ill result in IRole used with IManagedPolicy

epoctic

Changes seem reasonable to me.

stm29 · 2024-10-15T08:42:28Z

Can I get one more community review?

stm29 · 2024-10-20T08:44:22Z

Communicated in #contributing channel. waiting for one more community review.

tmokmss

Thank you for the fix! I left some comments :)

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

packages/aws-cdk-lib/aws-s3/lib/notifications-resource/notifications-resource-handler.ts

tmokmss · 2024-10-20T14:48:55Z

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

+      policy.attachToRole(this);
+    } catch (e) {
+      if (e instanceof Error && e.message === 'policy.attachToRole is not a function') {
+        throw new Error('Can\'t Combine IRole with IManagedPolicy. use ManagedPolicy directly.');


Throwing an error from this function is also a breaking change. Can we just add a warning as the previous behavior?

packages/aws-cdk-lib/aws-s3/lib/notifications-resource/notifications-resource-handler.ts

tmokmss · 2024-10-22T01:42:02Z

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

+    } else {
+      Annotations.of(this).addWarningV2(
+        '@aws-cdk/aws-iam:IRoleCantBeUsedWithIManagedPolicy',
+        'Can\'t combine IRole with IManagedPolicy. Use ManagedPolicy directly.',


We can add more information like ${policy.managedPolicyArn} to imported role: ${this.roleName} as in the original code to make it clearer why and where the warning is emitted. So that we do not have to add another warning from a caller.

Corrected with One Common message.

tmokmss

LGTM, thanks!

packages/aws-cdk-lib/aws-s3/lib/notifications-resource/notifications-resource-handler.ts

Leo10Gama · 2024-11-01T18:58:02Z

Hi @stm29 thanks for your contribution! Reading through the changes, I'm trying to understand the nature of the changes being made. The PR description mentions you're adding functionality to IRole, but it looks like you're adding it to ImportedRole. Could you please update the description to better reflect this?

I also did some digging through the rest of the codebase, and it seems like some of the other extensions of IRole have other implementations of addManagedPolicy(), like here:

  public addManagedPolicy(policy: IManagedPolicy): void {
    this.managedPolicies.push(policy);
  }

Would a change like this work instead of the more complex logic?

It also looks like the changes in the S3 notifications resource are minor, so I would agree that moving that change to another PR and opening a separate issue would be best, as long as the tests that currently exist for it still pass with this change. Though I'm not quite sure what the issue is in this case. It seems like a few other libraries already call on the addManagedPolicy() method, so is there something specific to S3 in this regard?

codecov · 2024-11-27T01:29:59Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.67%. Comparing base (f4c19c7) to head (36deb38).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #31212   +/-   ##
=======================================
  Coverage   78.67%   78.67%           
=======================================
  Files         107      107           
  Lines        7237     7237           
  Branches     1329     1329           
=======================================
  Hits         5694     5694           
  Misses       1357     1357           
  Partials      186      186

Flag	Coverage Δ
suite.unit	`78.67% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
packages/aws-cdk	`78.67% <ø> (ø)`

stm29 · 2024-11-27T07:54:26Z

@Leo10Gama , Thanks for the review.

I'm trying to understand the nature of the changes being made. The PR description mentions you're adding functionality to IRole, but it looks like you're adding it to ImportedRole. Could you please update the description to better reflect this?

Updated this with correct ImportedRole

Would a change like this work instead of the more complex logic?

Indeed Logic is simple, following is the working Logic which adds Policy to the Role. and this is same as how other Implementations are already in place.

policy.attachToRole(this);

Other if ... else and type predicate is to make sure Implementation is working as expected.

It seems like a few other libraries already call on the addManagedPolicy() method, so is there something specific to S3 in this regard?

Sorry for the confusion, There is no change in notifications-resource-handler.ts file, I have reverted to main.

Quick Question: Should we need to add Unit Tests in all the possible occurrence of IManagedPolicy with IRole, or Just UnitTests in packages/aws-cdk-lib/aws-iam/test/imported-role.test.ts is sufficient?
If UnitTests in imported-role.test.ts is sufficient, then notification.test.ts can also be reverted to main

mergify · 2024-12-04T20:12:57Z

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 · 2024-12-04T20:17:51Z

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 · 2024-12-05T19:08:30Z

This pull request has been removed from the queue for the following reason: pull request branch update failed.

The pull request can't be updated.

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.

If you want to requeue this pull request, you need to post a comment with the text: @mergifyio requeue

Pull request has been modified.

aws-cdk-automation · 2024-12-05T23:09:45Z

AWS CodeBuild CI Report

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

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

mergify · 2024-12-05T23:10:12Z

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

github-actions · 2024-12-06T00:09:26Z

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

fix(iam): does not work for imported roles aws#8307

4efe2f3

aws-cdk-automation requested a review from a team August 24, 2024 19:01

github-actions bot added bug This issue is a bug. effort/small Small work item – less than a day of effort p2 repeat-contributor [Pilot] contributed between 3-5 PRs to the CDK labels Aug 24, 2024

If Role is imported, we should allow Edit the Role anymore, as this w…

dbe344b

…ill result in IRole used with IManagedPolicy

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Aug 25, 2024

This was referenced Aug 26, 2024

Weekly PR metrics report #31217

Closed

Monthly PR metrics report #31279

Closed

epoctic approved these changes Oct 2, 2024

View reviewed changes

tmokmss suggested changes Oct 20, 2024

View reviewed changes

stm29 added 3 commits October 20, 2024 21:20

Merge branch 'main' into issue_8307

699479d

Fixed Comments

f07668a

Fixed Integration tests

a832873

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Oct 20, 2024

tmokmss reviewed Oct 21, 2024

View reviewed changes

packages/aws-cdk-lib/aws-s3/lib/notifications-resource/notifications-resource-handler.ts Outdated Show resolved Hide resolved

packages/aws-cdk-lib/aws-s3/lib/notifications-resource/notifications-resource-handler.ts Outdated Show resolved Hide resolved

Fixed comments and altered aws-s3-notifications

38f93e3

stm29 changed the title ~~fix(iam): Role.addManagedPolicy() does not work for imported roles #8307~~ fix(iam): Role.addManagedPolicy() does not work for imported roles IRole #8307 Oct 21, 2024

tmokmss reviewed Oct 22, 2024

View reviewed changes

stm29 added 2 commits October 22, 2024 18:36

Added only One Common warning

21fdf4a

Fixing Unit Tests

5c116fd

tmokmss approved these changes Oct 22, 2024

View reviewed changes

packages/aws-cdk-lib/aws-s3/lib/notifications-resource/notifications-resource-handler.ts Outdated Show resolved Hide resolved

tmokmss mentioned this pull request Oct 24, 2024

community review workflow stuck with Action Required state #31870

Closed

1 task

Leo10Gama self-assigned this Oct 25, 2024

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

Leo10Gama removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Nov 14, 2024

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Nov 14, 2024

Merge branch 'main' into issue_8307

7513f82

Rerverting to main - notifications-resource-handler.ts

f5066af

GavinZZ previously approved these changes Dec 4, 2024

View reviewed changes

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

Merge branch 'main' into issue_8307

36deb38

GavinZZ approved these changes Dec 5, 2024

View reviewed changes

GavinZZ merged commit c78ef1b into aws:main Dec 6, 2024
16 of 18 checks passed

github-actions bot locked as resolved and limited conversation to collaborators Dec 6, 2024

stm29 deleted the issue_8307 branch December 6, 2024 03:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(iam): `Role.addManagedPolicy()` does not work for imported roles `IRole` #8307 #31212

fix(iam): `Role.addManagedPolicy()` does not work for imported roles `IRole` #8307 #31212

stm29 commented Aug 24, 2024 •

edited

Loading

epoctic left a comment

stm29 commented Oct 15, 2024

stm29 commented Oct 20, 2024

tmokmss left a comment

tmokmss Oct 20, 2024

stm29 Oct 20, 2024

tmokmss Oct 22, 2024 •

edited

Loading

stm29 Oct 22, 2024

tmokmss left a comment

Leo10Gama commented Nov 1, 2024

codecov bot commented Nov 27, 2024 •

edited

Loading

stm29 commented Nov 27, 2024

mergify bot commented Dec 4, 2024

mergify bot commented Dec 4, 2024

mergify bot commented Dec 5, 2024

aws-cdk-automation commented Dec 5, 2024

mergify bot commented Dec 5, 2024

github-actions bot commented Dec 6, 2024

	/**
	* Attaches a managed policy to this principal.
	* @param policy The managed policy
	*/
	addManagedPolicy(policy: IManagedPolicy): void;

fix(iam): Role.addManagedPolicy() does not work for imported roles IRole #8307 #31212

fix(iam): Role.addManagedPolicy() does not work for imported roles IRole #8307 #31212

Conversation

stm29 commented Aug 24, 2024 • edited Loading

Issue #8307

Reason for this change

Description of changes

Approaches decided

This PR Implements follows,

Description of how you validated changes

Checklist

epoctic left a comment

Choose a reason for hiding this comment

stm29 commented Oct 15, 2024

stm29 commented Oct 20, 2024

tmokmss left a comment

Choose a reason for hiding this comment

tmokmss Oct 20, 2024

Choose a reason for hiding this comment

stm29 Oct 20, 2024

Choose a reason for hiding this comment

tmokmss Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

stm29 Oct 22, 2024

Choose a reason for hiding this comment

tmokmss left a comment

Choose a reason for hiding this comment

Leo10Gama commented Nov 1, 2024

codecov bot commented Nov 27, 2024 • edited Loading

Codecov Report

stm29 commented Nov 27, 2024

mergify bot commented Dec 4, 2024

mergify bot commented Dec 4, 2024

mergify bot commented Dec 5, 2024

aws-cdk-automation commented Dec 5, 2024

AWS CodeBuild CI Report

mergify bot commented Dec 5, 2024

github-actions bot commented Dec 6, 2024

fix(iam): `Role.addManagedPolicy()` does not work for imported roles `IRole` #8307 #31212

fix(iam): `Role.addManagedPolicy()` does not work for imported roles `IRole` #8307 #31212

stm29 commented Aug 24, 2024 •

edited

Loading

tmokmss Oct 22, 2024 •

edited

Loading

codecov bot commented Nov 27, 2024 •

edited

Loading