Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
fix(lambda): updating addToRolePolicy to avoid circular dependency po…
…tential (under feature flag) (#33291) ## Issue Closes #7016 I did NOT mean to drop the branch which closed #33268 🤦 My bad ## Reason for this change When using the function `Function.addToRolePolicy()` from lambda, behind the scenes it's calling `Role.addToPrincipalPolicy()`. What this does is it adds to the existing Lambda Role that is defined for the function, whether that is a predefined Role, or the default Role created from including no Role in the props. The issue with using specifically this method however, is if the policy statement that you are adding is related to a resource that this same Lambda function is added to as a prop, it causes a circular dependency. The simplest example of this is from the original issue, but it's not limited to this use case. (I tested with API Gateway also and got the same results) In the original issues, a user is creating a cognito UserPool, and adding a lambda trigger, which would run the lambda function after a new user has authorized. So in order to allow this lambda to perform operations related to the cognito user pool itself, it needs to add to it's execution role's policy, some kind of cognito permissions to that UserPool. So; `Function -> Cognito Trigger -> IAM Policy Statement with UserPool reference -> Function.addToRolePolicy()` This snippet from the integ I added shows what this would look like. See also the original issue. ``` const fn = new lambda.Function(stack, 'MyLambda', { code: new lambda.InlineCode('foo'), handler: 'index.handler', runtime: STANDARD_NODEJS_RUNTIME, }); const userPool = new UserPool(stack, 'myUserPoolTest', { lambdaTriggers: { fn, }, }); const cognitoPolicy = new iam.PolicyStatement({ actions: ['cognito:*'], resources: [userPool.userPoolArn], }); fn.addToRolePolicy(cognitoPolicy); ``` The reason why this causes an issue is because when using `Role.addToPrincipalPolicy()`, it adds a dependency check to ensure that the "PrincipalPolicy" actually exists first. This causes a circular reference. Now; * Lambda depends on the Policy * The policy has a reference call to GetAtt something from the UserPool * The UserPool has the lambda in the trigger props thus a dependency on lambda * repeat This logically makes no sense, because why would lambda depend on the policy? It really should just depend on the IAM Role and the policy should also depend on the Role. In fact if you build a template using this error, you can just delete the policy dependency in the Function, and upload by hand to CFN and it works just fine. So the question is, how can we avoid creating this dependency without some insane fundamental change to aws-iam? ## Description of changes Use `Role.attachInlinePolicy()` instead behind the scenes of `Function.addToRolePolicy()`. `Role.attachInlinePolicy()` will define a 2nd new policy. This means that we will no longer depend on the original policy existing in the first place. Instead we can just use these outside references in their own inline policy. Although this seems like it's changing a lot about this feature, functionality wise the permissions granted to the lambda function will not change because of this. ``` /** * The number of permissions added to this function * @internal */ private _policyCounter: number = 0; ``` A counter was added to help dedup the policies that are added, since you should be able to call this more than once without it exploding. ``` public addToRolePolicy(statement: iam.PolicyStatement) { if (!this.role) { return; } const policyToAdd = new iam.Policy(this, `inlinePolicyAddedToExecutionRole-${this._policyCounter++}`, { statements: [statement], }); this.role.attachInlinePolicy(policyToAdd); } ``` Of course the input from the user should remain the same, so a policy statement is passed in, and since `Role.attachInlinePolicy()` requires a Policy (not just a statement), we can rebuild from the statement input to allow for `Role.attachInlinePolicy()` to function properly. ## Description of how you validated changes Unit tests needed a few edits to match this, mainly just removing the policy dependency from the lambda function, then changing the reference name of the policy. For Integ similar changes were made to some snapshots. **All integ updates related to this, are "destructive" updates.** This is by design and should be reviewed but not changed. The reason is that none of the policies are actually being destroyed, rather, their logical IDs are just being renamed / new policies are being added. So if the primary policy had 3 statements before, now it has 3 policies with one statement each. I also added `integ.lambda-circular-test.ts` to specifically check for this circular dependency. I left a comment that this test's snapshot cannot be updated by hand since only CFN throws the error during validation for the circular dependency, so locally building you won't be able to tell if it works or not without using `--update-on-failure` to update it in the future. Edit: It's hard to determine which integs are okay to fail locally and not. During my first push the build failed on the PR, and it was showing an integ for an alpha construct. Since just using `yarn integ` on it's own makes it unrealistic to find the tests I need. I'm just solving each integ fail one at a time using the PR builder instead. Edit 2: After updating a dump truck of integs, I'm thinking that we might need a few people to review this first. Edit 3: Turns out everything is made from lambda Edit 4: I did some tests to ensure that if you update a template with this new structure of policy that it wouldn't break, and they worked just fine. However I only did this on areas of aws I was familiar with. Due to how many integs I had to update, there are clearly a lot more things than I was aware of that use Lambda, and replacement tests like the one I did are probably needed from anyone who is willing. ## Checklist - [X] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
- Loading branch information