fix(@aws-cdk/aws-events-targets): event-rules cannot have targets with the same construct id

[made2591]

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). 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). 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 the uniqueId - that is unique across the same app. This changes affects every binding in the supported event targets (source).

close #2377

Pull Request Checklist

• Testing
  • Unit test added (prefer not to modify an existing test, otherwise, it's probably a breaking change)
  • CLI change?: coordinate update of integration tests with team
  • cdk-init template change?: coordinated update of integration tests with team
• Docs
  • jsdocs: All public APIs documented
  • README: README and/or documentation topic updated
  • Design: For significant features, design document added to design folder
• Title and Description
  • Change type: title prefixed with fix, feat and module name in parens, which will appear in changelog
  • Title: use lower-case and doesn't end with a period
  • Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
  • Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
• Sensitive Modules (requires 2 PR approvers)
  • IAM Policy Document (in @aws-cdk/aws-iam)
  • EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
  • Grant APIs (only if not based on official documentation with a reference)

---

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

[made2591]

@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

[eladb]

I don’t see a reason why not

[made2591]

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 bind should be like this

this.topic.grantPublish(new iam.ServicePrincipal("events.amazonaws.com",
  {
    conditions: {
      ArnEquals: { "aws:SourceArn": _rule.ruleArn }
    }
  })
);

return {
  id: this.topic.node.uniqueId,
  arn: this.topic.topicArn,
  input: this.props.message
};

What do you think?

[jogold]

Note that the id is limited to 64 characters (https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-events-rule-target.html#cfn-events-rule-target-id) and uniqueId to 255.

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.

[rix0rrr]

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?

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

[made2591]

@jogold you're right, thank you for your feedback: I will do like that

[made2591]

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 🤔

@aws-cdk/cdk: [2019-06-06T20:38:37.622] [ERROR] jsii/compiler - Compilation errors prevented the JSII assembly from being created
@aws-cdk/cdk: lib/synthesis.d.ts:1:10 - error TS2305: Module '"../../../../../../../../Users/matteo/Github/aws-cdk/packages/@aws-cdk/cdk/node_modules/@aws-cdk/cx-api/lib"' has no exported member 'BuildOptions'.
@aws-cdk/cdk: 1 import { BuildOptions, CloudAssembly, CloudAssemblyBuilder } from '@aws-cdk/cx-api';
@aws-cdk/cdk:            ~~~~~~~~~~~~
@aws-cdk/cdk: Error: /Users/matteo/Github/aws-cdk/tools/cdk-build-tools/node_modules/jsii/bin/jsii --project-references exited with error code 1
@aws-cdk/cdk: Build failed. Total time (5.5s) | /Users/matteo/Github/aws-cdk/tools/cdk-build-tools/node_modules/jsii/bin/jsii (5.5s)
@aws-cdk/cdk: !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
@aws-cdk/cdk: npm ERR! code ELIFECYCLE
@aws-cdk/cdk: npm ERR! errno 1
@aws-cdk/cdk: npm ERR! @aws-cdk/cdk@0.33.0 build: `cdk-build`
@aws-cdk/cdk: npm ERR! Exit status 1
@aws-cdk/cdk: npm ERR!
@aws-cdk/cdk: npm ERR! Failed at the @aws-cdk/cdk@0.33.0 build script.
@aws-cdk/cdk: npm ERR! This is probably not a problem with npm. There is likely additional logging output above.
@aws-cdk/cdk: npm ERR! A complete log of this run can be found in:
@aws-cdk/cdk: npm ERR!     /Users/matteo/.npm/_logs/2019-06-06T18_38_37_981Z-debug.log
@aws-cdk/cdk: npm ERR! code ELIFECYCLE
@aws-cdk/cdk: npm ERR! errno 1
@aws-cdk/cdk: npm ERR! @aws-cdk/cdk@0.33.0 build+test: `npm run build && npm test`
@aws-cdk/cdk: npm ERR! Exit status 1
@aws-cdk/cdk: npm ERR!
@aws-cdk/cdk: npm ERR! Failed at the @aws-cdk/cdk@0.33.0 build+test script.
@aws-cdk/cdk: npm ERR! This is probably not a problem with npm. There is likely additional logging output above.
@aws-cdk/cdk: npm ERR! A complete log of this run can be found in:
@aws-cdk/cdk: npm ERR!     /Users/matteo/.npm/_logs/2019-06-06T18_38_38_007Z-debug.log
lerna ERR! npm run build+test exited 1 in '@aws-cdk/cdk'
lerna WARN complete Waiting for 1 child process to exit. CTRL-C to exit immediately.

real	0m40.808s
user	2m47.675s
sys	0m20.022s
❌  Last command failed. Scroll up to see errors in log (search for '!!!!!!!!').

The test stack after successfully build+test over the module events-targets (the lambda throws error for syntax but is not relevent, they get called properly):

import cdk = require('@aws-cdk/cdk');
import events = require('@aws-cdk/aws-events');
import lambda = require('@aws-cdk/aws-lambda');
import targets = require('@aws-cdk/aws-events-targets');
import { Construct } from '@aws-cdk/cdk';
import { Runtime } from '@aws-cdk/aws-lambda';

export class CustomConstruct extends Construct {
  public readonly lambda: lambda.Function;

  constructor(scope: Construct, id: string) {
    super(scope, id);

    this.lambda = new lambda.Function(this, "Resource", {
      code: new lambda.InlineCode("def handler:\n\tprint(\"Hello\")"),
      handler: "index.handler",
      runtime: Runtime.Python36
    });
  }
}

export class AwsEventsNodeUniqueIdStack extends cdk.Stack {
  constructor(scope: cdk.Construct, id: string, props?: cdk.StackProps) {
    super(scope, id, props);

    const construct1 = new CustomConstruct(this, "Construct1");
    const construct2 = new CustomConstruct(this, "Construct2");

    var rule = new events.Rule(this, "ConflictingRule", {
      description: "This will fail because the targets both have the same Construct ID",
      scheduleExpression: "rate(1 minute)",
      enabled: true,
    });

    rule.addTarget(new targets.LambdaFunction(construct1.lambda));
    rule.addTarget(new targets.LambdaFunction(construct2.lambda));

  }
}

I don't have a lot of idea about the error (outside of the scope of modified code)... any idea? 😕

[made2591]

I think it's fine now

[made2591]

The change to ecs breaks the integ test across some packages: if the uniqueId reasoning above makes sense, I will fix them as well...