-
Notifications
You must be signed in to change notification settings - Fork 4k
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(@aws-cdk/aws-events-targets): event-rules cannot have targets with the same construct id #2744
Conversation
…h the same construct id.
@rix0rrr do you also think we have to change this in the other targets as well? If so, I can open the respective issues and work on them |
I don’t see a reason why not |
I started working on SNS (source) and I noticed that it also violates the least privilege principle we discussed in #2683. Should I have to change it together with this fix or is it something to address in another PR? If it can be solved here, I would introduce also this change... the code of
What do you think? |
Note that the Something similar to https://github.com/awslabs/aws-cdk/blob/0e101d534ec2b39abcf19f93fde1e67d651e4089/packages/%40aws-cdk/aws-servicediscovery/lib/instance.ts#L51-L54 should be done here. |
Another PR please, if it's not too much effort for you. Let's try and keep unrelated fixes in separate PRs (as long as they don't interfere with each other too much, which this seems like they don't). |
@jogold you're right, thank you for your feedback: I will do like that |
I fixed the error, tested on the following stack... it seems fine but I'm getting a problem in the build regarding. The error I get during the build 🤔
The test stack after successfully
I don't have a lot of idea about the error (outside of the scope of modified code)... any idea? 😕 |
I think it's fine now |
packages/@aws-cdk/aws-events-targets/test/ecs/integ.event-fargate-task.expected.json
Outdated
Show resolved
Hide resolved
The change to ecs breaks the integ test across some packages: if the uniqueId reasoning above makes sense, I will fix them as well... |
…bda-deployed-through-codepipeline.lit.expected.json
Extend the Target for Event Rule Lambda cannot have same construct ID
Two supported Event Targets resources with the same Construct ID but in different scopes cannot both be used as targets to the same EventRule:
Assuming a Lambda Function as a RuleTarget, the issue is that Function's asEventRuleTarget uses
node.id
as the id field for the EventRuleTargetProps (source). This isn't unique, as it's just the ID that was passed when creating the Function construct - instead, it should usenode.uniqueId
(source). This needs to be globally unique as when calling EventRule.addTarget this value is used as a unique identifier (source).I fix the bug as suggested by @robwettach in the package
@aws-cdk/aws-events
by using 64-maximum chars taken from theuniqueId
- that is unique across the same app. This changes affects every binding in the supported event targets (source).close #2377
Pull Request Checklist
design
folderBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.