- 
                Notifications
    You must be signed in to change notification settings 
- Fork 4.3k
feat(iam): custom role support for OpenIdConnectProvider #35280
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
Conversation
…user, user has to take care of adding policies.
This reverts commit 39743a9.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(This review is outdated)
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice test coverage, left a couple of comments on typing. Thanks for working on this!
| * | ||
| * @default - No role, will be created automatically with the required permissions. | ||
| */ | ||
| readonly role?: any; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we type this as IRole here?
| @@ -1,3 +1,4 @@ | |||
| // import { IRole } from '../../../aws-iam'; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented out code, why don't we use this for the optionally-provided role?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your comments! I missed removing this. Added your suggestions.
Do you have any opinions if the permissions for the custom role should be added automatically in CustomResourceProviderBaseor should it be the responsibility of the user? If they should be added automatically, how can I fix TypeError: Class extends value undefined is not a constructor or null when importing
import { PolicyStatement } from '../../../aws-iam'; in aws-cdk/packages/aws-cdk-lib/core/lib/custom-resource-provider/custom-resource-provider-base.ts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a custom role is being passed in, we should make it clear what the role needs to look like—I see you do this in the documentation. We can do one of the following:
- Have checks to ensure the role has the right permissions, and fail synth if not
- We add the permissions in CDK
- Synth without checks, and fail at deploy time if permissions are wrong
1 and 2 require us to maintain parity with the service's permission requirements, whereas the service should already have these checks. For 2, we would also need to handle failures in adding permissions.
3 will allow synthing of a potentially errant template, but only from the service perspective. Generally we follow this approach, to reduce duplication of validation that the service should do anyway, and pass through the error from the service.
Curious on @mrgrain's thoughts on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Synth without checks, and fail at deploy time if permissions are wrong
This wouldn't happen at deploy time, but at runtime (when the provider is used). It's the worst option.
- We add the permissions in CDK
This is what we usually do. There should be no difference to the role being user provided or created automatically by the CDK. It should really just be this:
this.role = props.role ?? new iam.Role(...);| if (this.customRole) { | ||
| throw new ValidationError('Cannot call addToRolePolicy() when using a custom role. Add the required policies to your role directly.', this); | ||
| } | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not something we would usually do.
| Hey, looks like OpenIdConnectProvider is replaced by OidcProviderNative, but the former cannot be deprecated yet due to EKS still using it. However, we generally should not be updating OpenIdConnectProvider unless it's for critical security issues, or something that affects existing functionality. Apologies, it's not super clear in the docs, since OpenIdConnectProvider is not actually deprecated... See the discussion here: #28634 (comment) | 
| Comments on closed issues and PRs are hard for our team to see. | 
Issue #20460
Closes #20460
Reason for this change
OpenIdConnectProvidercreates a custom resource, which includes a lambda function. Currently, users cannot provide their own IAM role for this lambda function, it is auto generated. This limits users who need to use pre-existing roles due to organizational policies or specific permission requirements.Description of changes
Added an optional role property to OpenIdConnectProviderProps that allows users to provide their own IAM role for the custom resource's lambda function. When a custom role is provided:
I have tried to automatically add the required IAM permissions to the custom role, but converting the JSON policy statements using PolicyStatement.fromJson() was not possible because it created a circular dependency. I did not find a good way to implement this functionality. Please let me know if you know how to do this.
Describe any new or updated permissions being added
No new permissions are added by this change. When users provide a custom role, they must manually add the required IAM permissions:
Description of how you validated changes
I added unit tests for the OpenIdConnectProvider and CustomResourceProvider to verify:
I also added an integration test where the OpenIDConnectProvider is created with a custom role.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license