Skip to content

Commit

Permalink
fix(events-targets): event targets can have the same construct id (#2744
Browse files Browse the repository at this point in the history
)

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:
```
Error: Duplicate event rule target with ID: Resource
    at EventRule.addTarget (.../node_modules/@aws-cdk/aws-events/lib/rule.js:57:19)
    at new EventRule (.../node_modules/@aws-cdk/aws-events/lib/rule.js:26:18)
```
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](https://github.com/awslabs/aws-cdk/blob/48d536b191fb205869ef6724b718540be1037135/packages/%40aws-cdk/aws-events-targets/lib/lambda.ts#L32)). This isn't unique, as it's just the ID that was passed when creating the Function construct - instead, it should use `node.uniqueId` ([source](https://github.com/awslabs/aws-cdk/blob/master/packages/%40aws-cdk/cdk/lib/construct.ts#L31-L36)). This needs to be globally unique as when calling EventRule.addTarget this value is used as a unique identifier ([source](https://github.com/awslabs/aws-cdk/blob/master/packages/@aws-cdk/aws-events/lib/rule.ts#L124-L126)).

I fix the bug as suggested by @robwettach in the package `@aws-cdk/aws-events` by using 64-maximum chars taken from the `uniqueId` - that is unique across the same app. This changes affects every binding in the supported event targets ([source](https://github.com/made2591/aws-cdk/blob/feature/aws-events-targets-sqs/packages/%40aws-cdk/aws-events-targets/lib/)).

Close #2377
  • Loading branch information
made2591 authored and rix0rrr committed Jun 17, 2019
1 parent 1a386d8 commit 210dd0f
Show file tree
Hide file tree
Showing 28 changed files with 227 additions and 202 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -679,7 +679,7 @@
]
]
},
"Id": "Pipeline",
"Id": "PipelineStackPipeline9DB740AF",
"RoleArn": {
"Fn::GetAtt": [
"PipelineEventsRole46BEEA7C",
Expand Down Expand Up @@ -751,7 +751,7 @@
]
]
},
"Id": "Pipeline",
"Id": "PipelineStackPipeline9DB740AF",
"RoleArn": {
"Fn::GetAtt": [
"PipelineEventsRole46BEEA7C",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -444,7 +444,7 @@
]
]
},
"Id": "Pipeline",
"Id": "awscdkcodepipelinelambdaPipeline87A4B3D3",
"RoleArn": {
"Fn::GetAtt": [
"PipelineEventsRole46BEEA7C",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@
]
]
},
"Id": "Pipeline",
"Id": "awscdkcodepipelinecodebuildmultipleinputsoutputsPipeline314D3A85",
"RoleArn": {
"Fn::GetAtt": [
"PipelineEventsRole46BEEA7C",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@
]
]
},
"Id": "Pipeline",
"Id": "awscdkcodepipelinecodecommitPipelineF780CA18",
"RoleArn": {
"Fn::GetAtt": [
"PipelineEventsRole46BEEA7C",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,7 @@
]
]
},
"Id": "MyPipeline",
"Id": "awscdkcodepipelineecrsourceMyPipeline63CF3194",
"RoleArn": {
"Fn::GetAtt": [
"MyPipelineEventsRoleFAB99F32",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -374,7 +374,7 @@
"Arn": {
"Ref": "MyTopic86869434"
},
"Id": "MyTopic"
"Id": "awscdkpipelineeventtargetMyTopic8D32776A"
}
]
}
Expand Down Expand Up @@ -426,7 +426,7 @@
"Arn": {
"Ref": "MyTopic86869434"
},
"Id": "MyTopic"
"Id": "awscdkpipelineeventtargetMyTopic8D32776A"
}
]
}
Expand Down Expand Up @@ -473,7 +473,7 @@
"Arn": {
"Ref": "MyTopic86869434"
},
"Id": "MyTopic",
"Id": "awscdkpipelineeventtargetMyTopic8D32776A",
"InputTransformer": {
"InputPathsMap": {
"detail-pipeline": "$.detail.pipeline",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@
"Arn": {
"Ref": "ComplianceTopic0229448B"
},
"Id": "ComplianceTopic"
"Id": "awscdkconfigruleintegComplianceTopic55CAF01A"
}
]
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -847,7 +847,7 @@
"Ref": "ScheduledEc2TaskScheduledTaskDef56328BA4"
}
},
"Id": "ScheduledTaskDef-on-EcsCluster",
"Id": "awsecsintegecsScheduledEc2TaskScheduledTaskDef18FB4348",
"Input": "{}",
"RoleArn": {
"Fn::GetAtt": [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ export = {
TaskCount: 1,
TaskDefinitionArn: { Ref: "ScheduledEc2TaskScheduledTaskDef56328BA4" }
},
Id: "ScheduledTaskDef-on-EcsCluster",
Id: "ScheduledEc2TaskScheduledTaskDef1EA607E3",
Input: "{}",
RoleArn: { "Fn::GetAtt": ["ScheduledEc2TaskScheduledTaskDefEventsRole64113C5F", "Arn"] }
}
Expand Down Expand Up @@ -97,7 +97,7 @@ export = {
TaskCount: 2,
TaskDefinitionArn: { Ref: "ScheduledEc2TaskScheduledTaskDef56328BA4" }
},
Id: "ScheduledTaskDef-on-EcsCluster",
Id: "ScheduledEc2TaskScheduledTaskDef1EA607E3",
Input: "{}",
RoleArn: { "Fn::GetAtt": ["ScheduledEc2TaskScheduledTaskDefEventsRole64113C5F", "Arn"] }
}
Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-events-targets/lib/codebuild.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ export class CodeBuildProject implements events.IRuleTarget {
*/
public bind(_rule: events.IRule): events.RuleTargetConfig {
return {
id: this.project.node.id,
id: this.project.node.uniqueId,
arn: this.project.projectArn,
role: singletonEventRole(this.project, [new iam.PolicyStatement()
.addAction('codebuild:StartBuild')
Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-events-targets/lib/codepipeline.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ export class CodePipeline implements events.IRuleTarget {

public bind(_rule: events.IRule): events.RuleTargetConfig {
return {
id: this.pipeline.node.id,
id: this.pipeline.node.uniqueId,
arn: this.pipeline.pipelineArn,
role: singletonEventRole(this.pipeline, [new iam.PolicyStatement()
.addResource(this.pipeline.pipelineArn)
Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-events-targets/lib/ecs-task.ts
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ export class EcsTask implements events.IRuleTarget {
.addResource(this.taskDefinition.taskRole.roleArn));
}

const id = this.taskDefinition.node.id + '-on-' + this.cluster.node.id;
const id = this.taskDefinition.node.uniqueId;
const arn = this.cluster.clusterArn;
const role = singletonEventRole(this.taskDefinition, policyStatements);
const containerOverrides = this.props.containerOverrides && this.props.containerOverrides
Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-events-targets/lib/lambda.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ export class LambdaFunction implements events.IRuleTarget {
}

return {
id: this.handler.node.id,
id: this.handler.node.uniqueId,
arn: this.handler.functionArn,
input: this.props.event,
};
Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-events-targets/lib/sns.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ export class SnsTopic implements events.IRuleTarget {
this.topic.grantPublish(new iam.ServicePrincipal('events.amazonaws.com'));

return {
id: this.topic.node.id,
id: this.topic.node.uniqueId,
arn: this.topic.topicArn,
input: this.props.message,
};
Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-events-targets/lib/sqs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ export class SqsQueue implements events.IRuleTarget {
);

const result = {
id: this.queue.node.id,
id: this.queue.node.uniqueId,
arn: this.queue.queueArn,
input: this.props.message,
};
Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-events-targets/lib/state-machine.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ export class SfnStateMachine implements events.IRuleTarget {
*/
public bind(_rule: events.IRule): events.RuleTargetConfig {
return {
id: this.machine.node.id,
id: this.machine.node.uniqueId,
arn: this.machine.stateMachineArn,
role: singletonEventRole(this.machine, [new iam.PolicyStatement()
.addAction('states:StartExecution')
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@
"Arn"
]
},
"Id": "MyProject",
"Id": "awscdkcodebuildeventsMyProjectEF919B0E",
"RoleArn": {
"Fn::GetAtt": [
"MyProjectEventsRole5B7D93F5",
Expand All @@ -56,7 +56,7 @@
"Arn": {
"Ref": "MyTopic86869434"
},
"Id": "MyTopic",
"Id": "awscdkcodebuildeventsMyTopic550011DC",
"InputTransformer": {
"InputPathsMap": {
"detail-repositoryName": "$.detail.repositoryName",
Expand All @@ -68,59 +68,6 @@
]
}
},
"MyProjectEventsRole5B7D93F5": {
"Type": "AWS::IAM::Role",
"Properties": {
"AssumeRolePolicyDocument": {
"Statement": [
{
"Action": "sts:AssumeRole",
"Effect": "Allow",
"Principal": {
"Service": {
"Fn::Join": [
"",
[
"events.",
{
"Ref": "AWS::URLSuffix"
}
]
]
}
}
}
],
"Version": "2012-10-17"
}
}
},
"MyProjectEventsRoleDefaultPolicy397DCBF8": {
"Type": "AWS::IAM::Policy",
"Properties": {
"PolicyDocument": {
"Statement": [
{
"Action": "codebuild:StartBuild",
"Effect": "Allow",
"Resource": {
"Fn::GetAtt": [
"MyProject39F7B0AE",
"Arn"
]
}
}
],
"Version": "2012-10-17"
},
"PolicyName": "MyProjectEventsRoleDefaultPolicy397DCBF8",
"Roles": [
{
"Ref": "MyProjectEventsRole5B7D93F5"
}
]
}
},
"MyProjectRole9BBE5233": {
"Type": "AWS::IAM::Role",
"Properties": {
Expand Down Expand Up @@ -267,24 +214,24 @@
"source": [
"aws.codebuild"
],
"detail-type": [
"CodeBuild Build State Change"
],
"detail": {
"project-name": [
{
"Ref": "MyProject39F7B0AE"
}
]
}
},
"detail-type": [
"CodeBuild Build State Change"
]
},
"State": "ENABLED",
"Targets": [
{
"Arn": {
"Ref": "MyTopic86869434"
},
"Id": "MyTopic"
"Id": "awscdkcodebuildeventsMyTopic550011DC"
}
]
}
Expand All @@ -296,24 +243,24 @@
"source": [
"aws.codebuild"
],
"detail-type": [
"CodeBuild Build Phase Change"
],
"detail": {
"project-name": [
{
"Ref": "MyProject39F7B0AE"
}
]
}
},
"detail-type": [
"CodeBuild Build Phase Change"
]
},
"State": "ENABLED",
"Targets": [
{
"Arn": {
"Ref": "MyTopic86869434"
},
"Id": "MyTopic",
"Id": "awscdkcodebuildeventsMyTopic550011DC",
"InputTransformer": {
"InputPathsMap": {
"detail-completed-phase": "$.detail.completed-phase"
Expand All @@ -324,6 +271,59 @@
]
}
},
"MyProjectEventsRole5B7D93F5": {
"Type": "AWS::IAM::Role",
"Properties": {
"AssumeRolePolicyDocument": {
"Statement": [
{
"Action": "sts:AssumeRole",
"Effect": "Allow",
"Principal": {
"Service": {
"Fn::Join": [
"",
[
"events.",
{
"Ref": "AWS::URLSuffix"
}
]
]
}
}
}
],
"Version": "2012-10-17"
}
}
},
"MyProjectEventsRoleDefaultPolicy397DCBF8": {
"Type": "AWS::IAM::Policy",
"Properties": {
"PolicyDocument": {
"Statement": [
{
"Action": "codebuild:StartBuild",
"Effect": "Allow",
"Resource": {
"Fn::GetAtt": [
"MyProject39F7B0AE",
"Arn"
]
}
}
],
"Version": "2012-10-17"
},
"PolicyName": "MyProjectEventsRoleDefaultPolicy397DCBF8",
"Roles": [
{
"Ref": "MyProjectEventsRole5B7D93F5"
}
]
}
},
"MyQueueE6CA6235": {
"Type": "AWS::SQS::Queue"
},
Expand Down Expand Up @@ -417,4 +417,4 @@
}
}
}
}
}
Loading

0 comments on commit 210dd0f

Please sign in to comment.