Skip to content

Conversation

@alvazjor
Copy link
Contributor

@alvazjor alvazjor commented Jul 4, 2025

Issue # (if applicable)

Closes #34894.

Reason for this change

When createNewPoliciesWithAddToRolePolicy flag is false, addToRolePolicy() fails with CloudFormation intrinsic functions due to token resolution issues. More details in the issue itself.

Description of changes

This fix detects complex tokens and forces separate inline policies to prevent the error while maintaining backward compatibility.

Describe any new or updated permissions being added

No new permissions being added

Description of how you validated changes

Tested the new changes in the broken cdk app that was able to reproduce the issue (used reproduction steps from the original issue). The app can be synth now, and deployed. Verifying the lambda role manually in the console also showed that a single role is preserved but the role policies are now split in 2: one from the string literal and one from the token. Both are correctly resolved.

Checklist


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

@github-actions github-actions bot added bug This issue is a bug. effort/medium Medium work item – several days of effort p0 labels Jul 4, 2025
@aws-cdk-automation aws-cdk-automation requested a review from a team July 4, 2025 10:05
@alvazjor alvazjor added pr/work-in-progress This PR is a draft and needs further work. and removed bug This issue is a bug. p0 effort/medium Medium work item – several days of effort labels Jul 4, 2025
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Jul 4, 2025
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.

(This review is outdated)

private statementHasComplexTokens(statement: iam.PolicyStatement): boolean {
// Check if resources contain tokens that represent CloudFormation functions
const resources = statement.resources;
if (!resources || resources.length === 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you check resources.length if this is a token? Does this not error?

* that would cause issues when merged with other statements in a single policy document.
* @internal
*/
private statementHasComplexTokens(statement: iam.PolicyStatement): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure what a "complex token" would be just from reading the description. Looking at the code this might more accurately as "nested token".

@alvazjor alvazjor force-pushed the fix/issue-34894-addtorolepolicy-token-resolution branch 6 times, most recently from 61f69d5 to 9a95a29 Compare July 7, 2025 14:17
@alvazjor alvazjor changed the title fix(aws-lambda): handle CloudFormation intrinsic functions in addToRolePolicy fix(lambda): handle oken resolution issues in addToRolePolicy Jul 7, 2025
@github-actions github-actions bot added bug This issue is a bug. effort/medium Medium work item – several days of effort p0 labels Jul 7, 2025
@alvazjor alvazjor removed the pr/work-in-progress This PR is a draft and needs further work. label Jul 7, 2025
@aemada-aws aemada-aws self-assigned this Jul 7, 2025
@alvazjor alvazjor changed the title fix(lambda): handle oken resolution issues in addToRolePolicy fix(lambda): handle token resolution issues in addToRolePolicy Jul 8, 2025
@alvazjor alvazjor force-pushed the fix/issue-34894-addtorolepolicy-token-resolution branch from 9f4a242 to c4591ae Compare July 9, 2025 19:35
@aws-cdk-automation aws-cdk-automation dismissed their stale review July 9, 2025 19:37

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

@mergify
Copy link
Contributor

mergify bot commented Jul 10, 2025

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 bot added a commit that referenced this pull request Jul 10, 2025
@mergify
Copy link
Contributor

mergify bot commented Jul 10, 2025

This pull request has been removed from the queue for the following reason: pull request dequeued.

Pull request #34904 has been dequeued. The pull request could not be merged. This could be related to an activated branch protection or ruleset rule that prevents us from merging. (details: 2 of 2 required status checks are expected.).

You can check the last failing draft PR here: #34952.

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 do update this pull request, it will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue instead, you can requeue the pull request, without updating it, by posting a @mergifyio requeue comment.

@alvazjor
Copy link
Contributor Author

@Mergifyio requeue

@mergify
Copy link
Contributor

mergify bot commented Jul 10, 2025

requeue

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

mergify bot added a commit that referenced this pull request Jul 10, 2025
@mergify
Copy link
Contributor

mergify bot commented Jul 10, 2025

This pull request has been removed from the queue for the following reason: pull request dequeued.

Pull request #34904 has been dequeued. The pull request could not be merged. This could be related to an activated branch protection or ruleset rule that prevents us from merging. (details: 2 of 2 required status checks are expected.).

You can check the last failing draft PR here: #34958.

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 do update this pull request, it will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue instead, you can requeue the pull request, without updating it, by posting a @mergifyio requeue comment.

@mergify
Copy link
Contributor

mergify bot commented Jul 11, 2025

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 bot added a commit that referenced this pull request Jul 11, 2025
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@mergify
Copy link
Contributor

mergify bot commented Jul 11, 2025

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 mergify bot merged commit 6cd1802 into main Jul 11, 2025
18 checks passed
@mergify mergify bot deleted the fix/issue-34894-addtorolepolicy-token-resolution branch July 11, 2025 10:50
@github-actions
Copy link
Contributor

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 11, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

bug This issue is a bug. contribution/core This is a PR that came from AWS. effort/medium Medium work item – several days of effort p0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

(aws-lambda): addRoleToPolicy() breaks when feature flag createNewPoliciesWithAddToRolePolicy set to true

4 participants