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

[watany-dev]

fixes #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?

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[bot]]

[watany-dev]

@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!

[watany-dev]

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]

@corymhall

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

[watany-dev]

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.

[watany-dev]

I am glad to have gained another knowledge.I was able to put the test independent of region,account. Thanks for sharing your tips.

[watany-dev]

@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]

@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]

@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

1. 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.
2. 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.
3. Or if the solution in this PR is correct, it may be helpful https://github.com/aws/aws-cdk/pull/22444

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

[mergify[bot]]

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-cdk-automation]

AWS CodeBuild CI Report

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

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

[watany-dev]

Removed the conflict. please confirm.

[Naumel]

Is this conversation

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

concluded?

[watany-dev]

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

[mergify[bot]]

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