fix(config): Creating multiple rules from the same lambda #21594

watany-dev · 2022-08-14T15:11:04Z

because the id of ".addPermission" is set to a fixed value of ″permission″, which means that only one can be set in the stack.

and add a unique suffix to the id. This will allow multiple custom rules to be handled in one stack.
Do the id check before addPermission. This will allow only one permission to be granted to a custom rule from the config service.

Addendum:.
I have created a hash from FunctionName, AccountID, and Region to make the suffix unique.
Therefore, the omitted parts in the test code have been modified to fix the result.

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

This PR adds new unconventional dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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

gitpod-io · 2022-08-14T15:11:10Z

watany-dev · 2022-08-24T12:29:36Z

@corymhall

Hi.
I understand from the PR that the CDK team is busy and it pains me to say this, but may I ask when you expect to be able to review this?
*If you look at it a little and see something obviously missing, a rough comment would be fine!

corymhall · 2022-08-24T13:15:15Z

packages/@aws-cdk/aws-config/lib/rule.ts

+    if (!props.lambdaFunction.permissionsNode.tryFindChild(customRulePermissionId)) {
+      props.lambdaFunction.addPermission(customRulePermissionId, {
+        principal: new iam.ServicePrincipal('config.amazonaws.com'),
+        sourceAccount: this.env.account,


I think this wouldn't work if we had multiple config rules in different accounts. This would only add the permission for the first account. I think we somehow need to add the account into the id.

And actually I wonder if it would be better to handle this in the addPermission method since this problem is probably not unique to config. We could probably do something similar to what we do for grantInvoke

https://github.com/aws/aws-cdk/blob/8a733bad022fb2ac0551e32fd26a4847c0b36ff2/packages/%40aws-cdk/aws-lambda/lib/function-base.ts#L433-L414

Add.
I have created a hash from FunctionName, AccountID, and Region to make the suffix unique.
Therefore, the omitted parts in the test code have been modified to fix the result.

@watany-dev what do you think about my suggestion for doing this in the addPermission method instead? That would be a more global fix for this issue.

I guess I misunderstood your comment. I should have fixed the Lambda addPermission.I will implement it.

@corymhall
I have a concern during the design process and am seeking advice
...Example grantInvoke is

generate a unique ID from grantee information

If the ID has not been used before, grant it.
Also, addpermissions will set the id as it comes in the argument.

If addPermission is modified with this requirement, it would look like the following.
"Generate a unique hash from the argument permission of addPermisson and set the incoming id and permission if it was not used before."

This appears to be a vexing problem specific to addPermission taking the id as an argument rather than generating it dynamically; how should this work if the permission is different and the id is duplicated? I fear that some destructive change would be necessary.

yeah I see your point. Ok i'm good with your solution, just a couple of comments on the integ tests.

Pull request has been modified.

watany-dev · 2022-08-26T00:41:09Z

Addendum:.
I have created a hash from FunctionName, AccountID, and Region to make the suffix unique.
Therefore, the omitted parts in the test code have been modified to fix the result.

watany-dev · 2022-09-10T13:18:20Z

@corymhall

Excuse me for being busy, I tried to fix it with reference to the lambda implementation, but what do you think?

corymhall

Putting back in request changes.

Pull request has been modified.

corymhall

back in request changes.

corymhall · 2022-09-21T16:50:10Z

packages/@aws-cdk/aws-config/lib/rule.ts

+    if (!props.lambdaFunction.permissionsNode.tryFindChild(customRulePermissionId)) {
+      props.lambdaFunction.addPermission(customRulePermissionId, {
+        principal: new iam.ServicePrincipal('config.amazonaws.com'),
+        sourceAccount: this.env.account,


yeah I see your point. Ok i'm good with your solution, just a couple of comments on the integ tests.

packages/@aws-cdk/aws-config/test/integ.rule.lit.ts

corymhall · 2022-09-21T16:50:54Z

packages/@aws-cdk/aws-config/test/integ.rule.lit.ts

@@ -38,5 +39,7 @@ class ConfigStack extends cdk.Stack {
  }
 }

-new ConfigStack(app, 'aws-cdk-config-rule-integ');
+new ConfigStack(app, 'aws-cdk-config-rule-integ', {
+  env: { account: '123456789012', region: 'us-east-1' },


Suggested change

env: { account: '123456789012', region: 'us-east-1' },

The integ tests stacks should be environment agnostic.

Let me leave this comment as it is. It is certainly not a process that asks for region and account ID. However, if it is randomized, the hash seed of the snapshot test will change every time, and the test will not converge.

I think it's probably because when the env is not defined then the account and region are tokens.

I left a review comment on what you need to updated.

corymhall · 2022-09-21T16:51:05Z

packages/@aws-cdk/aws-config/test/integ.scoped-rule.ts

@@ -4,12 +4,15 @@ import * as config from '../lib';

 const app = new cdk.App();

-const stack = new cdk.Stack(app, 'aws-cdk-config-rule-scoped-integ');
+const stack = new cdk.Stack(app, 'aws-cdk-config-rule-scoped-integ', {
+  env: { account: '123456789012', region: 'us-east-1' },


Suggested change

env: { account: '123456789012', region: 'us-east-1' },

Let me leave this comment as it is. It is certainly not a process that asks for region and account ID. However, if it is randomized, the hash seed of the snapshot test will change every time, and the test will not converge.

packages/@aws-cdk/aws-config/test/integ.scoped-rule.ts

packages/@aws-cdk/aws-config/lib/rule.ts

Co-authored-by: Cory Hall <43035978+corymhall@users.noreply.github.com>

Pull request has been modified.

Co-authored-by: Cory Hall <43035978+corymhall@users.noreply.github.com>

watany-dev · 2022-09-22T01:12:57Z

Thanks for confirming. I was satisfied with your comments and have incorporated all of them.
I have imported them with the suggestion feature of Github and will add the test now.

corymhall · 2022-09-22T13:45:01Z

packages/@aws-cdk/aws-config/lib/rule.ts

+    const hash = createHash('sha256')
+      .update(JSON.stringify({
+        fnName: props.lambdaFunction.functionName.toString,
+        accountId: this.env.account,


Both account and region need to be resolved. This should fix the issue you are having with the environment agnostic integ tests.

accountId: Stack.of(this).resolve(this.env.account),

…com/watany-dev/aws-cdk into fix-multiple-rules-from-same-lambda

watany-dev · 2022-10-07T14:35:22Z

@Naumel
How did you fix the comment (typo) you received? I am sending you a reviewer once, but if you need to change it, I would appreciate it if you could reply.

corymhall · 2022-10-10T12:18:53Z

@watany-dev sorry for the delay on this. We have been discussing this change internally and our hesitation is whether or not changing the logicalId of the permission resource is a breaking change or not. When the logicalId changes a new resource will be created and the existing resource will be deleted. I think that should be fine because the new permission will be created first, establishing the permission and then the old one will be deleted. What we don't know is whether there is any delay in this taking place. For this to work, the change needs to be instant and not cause any permission errors.

We need to test this scenario to be sure, but i'm not sure how.

watany-dev · 2022-10-10T13:58:13Z

@corymhall
Thank you for your answer.
To be honest, it's hard for me to come up with test ideas right away.
If you hesitate to implement a variable fixed ID for the purpose of avoiding duplication like this time, it is a concern that the CDK may become a complicated implementation that creates a stack for each resource.

Here's a suggested implementation

The resource for the first time sets the id to a fixed value. Specifically, it searches for id="permission" and sets "permission" to id if it does not exist. If it exists, use this implementation to make the id variable. The latter pattern is a bug in this issue and should not exist now, so it seems that there is no existing impact.
Add an option manualPermissionId to set the id dynamically. This breaking change will be done as planned, and if you really want to avoid replacing existing resources, specify "permission" manually. If this conflicts, it is the user's responsibility to change.
Or if the solution in this PR is correct, it may be helpful
fix(cognito): cannot use same lambda function as trigger in multiple user pools #22444

I will follow and respond to the above implementation suggestions, or better ideas.

mergify · 2022-10-18T17:37:27Z

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

TheRealAmazonKendra

We now need the conflicts resolved.

Pull request has been modified.

aws-cdk-automation · 2022-10-19T06:16:47Z

AWS CodeBuild CI Report

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

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

watany-dev · 2022-10-19T06:59:55Z

Removed the conflict. please confirm.

Naumel · 2022-10-19T08:36:28Z

Is this conversation

I will follow and respond to the above implementation suggestions, or better ideas.

concluded?

watany-dev · 2022-10-19T09:18:49Z

I haven't changed the implementation since corymhall tried to merge. This time I only fixed the conflicting test code

Naumel

♻️ conflicts were addressed and the PR has been previously approved.

mergify · 2022-10-19T11:28:03Z

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

fixes aws#17582 because the id of ".addPermission" is set to a fixed value of ″permission″, which means that only one can be set in the stack. 1. and add a unique suffix to the id. This will allow multiple custom rules to be handled in one stack. 2. Do the id check before addPermission. This will allow only one permission to be granted to a custom rule from the config service. Addendum:. I have created a hash from FunctionName, AccountID, and Region to make the suffix unique. Therefore, the omitted parts in the test code have been modified to fix the result. ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

github-actions bot added the p2 label Aug 14, 2022

aws-cdk-automation requested a review from a team August 14, 2022 15:11

github-actions bot added bug This issue is a bug. effort/medium Medium work item – several days of effort p1 and removed p2 labels Aug 14, 2022

watany-dev changed the title ~~fix:(config) Creating multiple rules from the same lambda~~ fix(config): Creating multiple rules from the same lambda Aug 14, 2022

watany-dev force-pushed the fix-multiple-rules-from-same-lambda branch from b4280f0 to b7fe9f8 Compare August 15, 2022 00:32

corymhall self-assigned this Aug 15, 2022

corymhall previously requested changes Aug 24, 2022

View reviewed changes

aws#17582 Creating-multiple-rules-from-the-same-lambda-fails

41b31dc

watany-dev force-pushed the fix-multiple-rules-from-same-lambda branch from b7fe9f8 to 41b31dc Compare August 26, 2022 00:37

watany-dev requested a review from corymhall August 27, 2022 02:39

corymhall previously requested changes Sep 12, 2022

View reviewed changes

Merge branch 'aws:main' into fix-multiple-rules-from-same-lambda

722c5f4

corymhall suggested changes Sep 21, 2022

View reviewed changes

corymhall previously requested changes Sep 21, 2022

View reviewed changes

Apply suggestions from code review

31cbbf5

Co-authored-by: Cory Hall <43035978+corymhall@users.noreply.github.com>

Update packages/@aws-cdk/aws-config/lib/rule.ts

ae9f735

Co-authored-by: Cory Hall <43035978+corymhall@users.noreply.github.com>

watany-dev force-pushed the fix-multiple-rules-from-same-lambda branch from 62918dd to 368ecd2 Compare September 22, 2022 08:29

watany-dev requested a review from corymhall September 22, 2022 09:52

corymhall previously requested changes Sep 22, 2022

View reviewed changes

watany-dev requested a review from Naumel September 24, 2022 00:16

watany-dev and others added 2 commits September 24, 2022 00:25

Modification of integ test based on review comments

8c8b935

Merge branch 'fix-multiple-rules-from-same-lambda' of https://github.…

d50e428

…com/watany-dev/aws-cdk into fix-multiple-rules-from-same-lambda

watany-dev force-pushed the fix-multiple-rules-from-same-lambda branch from 9d269e0 to d50e428 Compare September 24, 2022 00:27

watany-dev requested review from corymhall and Naumel and removed request for Naumel and corymhall September 28, 2022 14:14

corymhall previously approved these changes Oct 18, 2022

View reviewed changes

TheRealAmazonKendra previously requested changes Oct 19, 2022

View reviewed changes

Merge branch 'main' into fix-multiple-rules-from-same-lambda

776b01b

Merge branch 'aws:main' into fix-multiple-rules-from-same-lambda

4bfa8b9

watany-dev force-pushed the fix-multiple-rules-from-same-lambda branch from 387470a to 4bfa8b9 Compare October 19, 2022 05:39

watany-dev requested review from TheRealAmazonKendra and removed request for Naumel October 19, 2022 07:00

Naumel approved these changes Oct 19, 2022

View reviewed changes

mergify bot merged commit 0d2b529 into aws:main Oct 19, 2022

watany-dev deleted the fix-multiple-rules-from-same-lambda branch October 19, 2022 12:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(config): Creating multiple rules from the same lambda #21594

fix(config): Creating multiple rules from the same lambda #21594

watany-dev commented Aug 14, 2022 •

edited

Loading

gitpod-io bot commented Aug 14, 2022

watany-dev commented Aug 24, 2022

corymhall Aug 24, 2022

corymhall Aug 24, 2022

watany-dev Aug 30, 2022 •

edited

Loading

corymhall Sep 12, 2022

watany-dev Sep 12, 2022

watany-dev Sep 13, 2022

corymhall Sep 21, 2022

watany-dev commented Aug 26, 2022

watany-dev commented Sep 10, 2022

corymhall left a comment

corymhall left a comment

corymhall Sep 21, 2022

corymhall Sep 21, 2022

watany-dev Sep 22, 2022

corymhall Sep 22, 2022

corymhall Sep 21, 2022

watany-dev Sep 22, 2022

watany-dev commented Sep 22, 2022

corymhall Sep 22, 2022

watany-dev commented Oct 7, 2022

corymhall commented Oct 10, 2022

watany-dev commented Oct 10, 2022 •

edited

Loading

mergify bot commented Oct 18, 2022

TheRealAmazonKendra left a comment

aws-cdk-automation commented Oct 19, 2022

watany-dev commented Oct 19, 2022

Naumel commented Oct 19, 2022

watany-dev commented Oct 19, 2022

Naumel left a comment

mergify bot commented Oct 19, 2022

fix(config): Creating multiple rules from the same lambda #21594

fix(config): Creating multiple rules from the same lambda #21594

Conversation

watany-dev commented Aug 14, 2022 • edited Loading

All Submissions:

Adding new Unconventional Dependencies:

New Features

gitpod-io bot commented Aug 14, 2022

watany-dev commented Aug 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

watany-dev Aug 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

watany-dev commented Aug 26, 2022

watany-dev commented Sep 10, 2022

corymhall left a comment

Choose a reason for hiding this comment

corymhall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

watany-dev commented Sep 22, 2022

Choose a reason for hiding this comment

watany-dev commented Oct 7, 2022

corymhall commented Oct 10, 2022

watany-dev commented Oct 10, 2022 • edited Loading

mergify bot commented Oct 18, 2022

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

aws-cdk-automation commented Oct 19, 2022

AWS CodeBuild CI Report

watany-dev commented Oct 19, 2022

Naumel commented Oct 19, 2022

watany-dev commented Oct 19, 2022

Naumel left a comment

Choose a reason for hiding this comment

mergify bot commented Oct 19, 2022

watany-dev commented Aug 14, 2022 •

edited

Loading

watany-dev Aug 30, 2022 •

edited

Loading

watany-dev commented Oct 10, 2022 •

edited

Loading