-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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(core): fix policy synthesizer logic for precreated roles #31710
Conversation
60ae7c1
to
1ce33ef
Compare
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.
Overall changes look good! Just a few minor comments from me.
packages/@aws-cdk-testing/framework-integ/test/aws-iam/test/integ.customize-role.ts
Outdated
Show resolved
Hide resolved
...eg/test/aws-dynamodb/test/integ.table-with-customized-role.js.snapshot/iam-policy-report.txt
Outdated
Show resolved
Hide resolved
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.
Changes LGTM! Will leave the do-not-merge
label on for a second review from core to confirm changes are fine
Discussed with Kendra offline and she's fine to merge this. Removing |
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 CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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). |
Comments on closed issues and PRs are hard for our team to see. |
Issue # (if applicable)
Closes #31653
Reason for this change
With Role.customizeRoles enabled, dynamodb.Table.addGlobalSecondaryIndex causes an error. This is a critical blocker for customers who require the use of customizeRoles.
Description of changes
Intended behaviour
When
customizeRoles
is used, theiam-policy-report.txt
report will contain a listof IAM roles and associated permissions that would have been created. This report is
generated so that it attempts to resolve any references and replace with a more user
friendly value.
The following are some examples of the value that will appear in the report:
The policy report will instead get:
"(Path/To/SomeResource.Arn)"
Current issues
There are two main issues here:
App
scope. This caused the failure in the original issueResolution error: PolicySynthesizer at 'PolicySynthesizer' should be created in the scope of a Stack, but no Stack found.
because token resolution requires a Stack scope not an App scope.DefaultTokenResolver
. The default token resolution class does not generate the same format of output values for the policy report. i.e. A concatenated token value, i.e.${Token[Token.X]}/index/*
would be converted to(PhysicalId).Arn
instead of"(Path/To/SomeResource.Arn)"
.AWS::NoValue
would be rendered asTokens
in the policy report which is not idea. Update it to make it outputNOVALUE
.This PR addresses the above two issues.
Description of how you validated changes
New and existing tests pass.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license