From f8d8795a610c3f49e31967001695caa648730d6d Mon Sep 17 00:00:00 2001 From: Sven Kirschbaum Date: Fri, 13 Aug 2021 11:59:48 +0200 Subject: [PATCH 1/4] fix(s3-deployment): BucketDeployment doesn't validate that distribution paths start with "/" (#15865) Cloudfront invalidation paths must start with a leading "/". This pull requests adds a corresponding validation to fail during build instead of during the Cloudformation deployment. closes #9317 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* --- .../lib/bucket-deployment.ts | 11 +++++++-- .../test/bucket-deployment.test.ts | 24 +++++++++++++++++++ 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/packages/@aws-cdk/aws-s3-deployment/lib/bucket-deployment.ts b/packages/@aws-cdk/aws-s3-deployment/lib/bucket-deployment.ts index 3a73b950792bf..990a1ec359d0b 100644 --- a/packages/@aws-cdk/aws-s3-deployment/lib/bucket-deployment.ts +++ b/packages/@aws-cdk/aws-s3-deployment/lib/bucket-deployment.ts @@ -196,8 +196,15 @@ export class BucketDeployment extends CoreConstruct { constructor(scope: Construct, id: string, props: BucketDeploymentProps) { super(scope, id); - if (props.distributionPaths && !props.distribution) { - throw new Error('Distribution must be specified if distribution paths are specified'); + if (props.distributionPaths) { + if (!props.distribution) { + throw new Error('Distribution must be specified if distribution paths are specified'); + } + if (!cdk.Token.isUnresolved(props.distributionPaths)) { + if (!props.distributionPaths.every(distributionPath => cdk.Token.isUnresolved(distributionPath) || distributionPath.startsWith('/'))) { + throw new Error('Distribution paths must start with "/"'); + } + } } const handler = new lambda.SingletonFunction(this, 'CustomResourceHandler', { diff --git a/packages/@aws-cdk/aws-s3-deployment/test/bucket-deployment.test.ts b/packages/@aws-cdk/aws-s3-deployment/test/bucket-deployment.test.ts index 2bbe5e34a41a6..893c5ae03b35f 100644 --- a/packages/@aws-cdk/aws-s3-deployment/test/bucket-deployment.test.ts +++ b/packages/@aws-cdk/aws-s3-deployment/test/bucket-deployment.test.ts @@ -491,6 +491,30 @@ test('fails if distribution paths provided but not distribution ID', () => { }); +test('fails if distribution paths don\'t start with "/"', () => { + // GIVEN + const stack = new cdk.Stack(); + const bucket = new s3.Bucket(stack, 'Dest'); + const distribution = new cloudfront.CloudFrontWebDistribution(stack, 'Distribution', { + originConfigs: [ + { + s3OriginSource: { + s3BucketSource: bucket, + }, + behaviors: [{ isDefaultBehavior: true }], + }, + ], + }); + + // THEN + expect(() => new s3deploy.BucketDeployment(stack, 'Deploy', { + sources: [s3deploy.Source.asset(path.join(__dirname, 'my-website.zip'))], + destinationBucket: bucket, + distribution, + distributionPaths: ['images/*'], + })).toThrow(/Distribution paths must start with "\/"/); +}); + testFutureBehavior('lambda execution role gets permissions to read from the source bucket and read/write in destination', s3GrantWriteCtx, cdk.App, (app) => { // GIVEN const stack = new cdk.Stack(app); From fdee43ccc3501cf628eea5204387b37977059539 Mon Sep 17 00:00:00 2001 From: Clarence Date: Fri, 13 Aug 2021 20:38:35 +0800 Subject: [PATCH 2/4] docs(ecs): Fix ecs readme markdown (#16018) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* --- packages/@aws-cdk/aws-ecs/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/@aws-cdk/aws-ecs/README.md b/packages/@aws-cdk/aws-ecs/README.md index 9e8170f86d826..8e3adc1e3d737 100644 --- a/packages/@aws-cdk/aws-ecs/README.md +++ b/packages/@aws-cdk/aws-ecs/README.md @@ -909,7 +909,7 @@ taskDefinition.addContainer('cont', { Please note, ECS Exec leverages AWS Systems Manager (SSM). So as a prerequisite for the exec command to work, you need to have the SSM plugin for the AWS CLI installed locally. For more information, see -[Install Session Manager plugin for AWS CLI] (https://docs.aws.amazon.com/systems-manager/latest/userguide/session-manager-working-with-install-plugin.html). +[Install Session Manager plugin for AWS CLI](https://docs.aws.amazon.com/systems-manager/latest/userguide/session-manager-working-with-install-plugin.html). To enable the ECS Exec feature for your containers, set the boolean flag `enableExecuteCommand` to `true` in your `Ec2Service` or `FargateService`. From a308cacf1fc48e24311caec246b768ffe6ae9153 Mon Sep 17 00:00:00 2001 From: Romain Marcadier Date: Fri, 13 Aug 2021 15:41:48 +0200 Subject: [PATCH 3/4] fix(ec2): opaque error when insufficient NAT EIPs are configured (#16040) When manually configuring NAT gateways for a VPC, a relatively opaque error is emitted when insufficient EIP allocation IDs are provided, compared to the subnet count. Added a proactive validation of the allocation IDs to improve the actionability of the error message. Fixes #16039 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* --- packages/@aws-cdk/aws-ec2/lib/nat.ts | 10 ++++- packages/@aws-cdk/aws-ec2/test/vpc.test.ts | 46 ++++++++++++++++------ 2 files changed, 44 insertions(+), 12 deletions(-) diff --git a/packages/@aws-cdk/aws-ec2/lib/nat.ts b/packages/@aws-cdk/aws-ec2/lib/nat.ts index e991e732f38b0..4743799762b51 100644 --- a/packages/@aws-cdk/aws-ec2/lib/nat.ts +++ b/packages/@aws-cdk/aws-ec2/lib/nat.ts @@ -222,6 +222,14 @@ class NatGatewayProvider extends NatProvider { } public configureNat(options: ConfigureNatOptions) { + if ( + this.props.eipAllocationIds != null + && !Token.isUnresolved(this.props.eipAllocationIds) + && this.props.eipAllocationIds.length < options.natSubnets.length + ) { + throw new Error(`Not enough NAT gateway EIP allocation IDs (${this.props.eipAllocationIds.length} provided) for the requested subnet count (${options.natSubnets.length} needed).`); + } + // Create the NAT gateways let i = 0; for (const sub of options.natSubnets) { @@ -413,4 +421,4 @@ function pickN(i: number, xs: string[]) { } return xs[i]; -} \ No newline at end of file +} diff --git a/packages/@aws-cdk/aws-ec2/test/vpc.test.ts b/packages/@aws-cdk/aws-ec2/test/vpc.test.ts index dd22090584a25..d65bc016271be 100644 --- a/packages/@aws-cdk/aws-ec2/test/vpc.test.ts +++ b/packages/@aws-cdk/aws-ec2/test/vpc.test.ts @@ -35,7 +35,7 @@ nodeunitShim({ 'vpc.vpcId returns a token to the VPC ID'(test: Test) { const stack = getTestStack(); const vpc = new Vpc(stack, 'TheVPC'); - test.deepEqual(stack.resolve(vpc.vpcId), { Ref: 'TheVPC92636AB0' } ); + test.deepEqual(stack.resolve(vpc.vpcId), { Ref: 'TheVPC92636AB0' }); test.done(); }, @@ -55,11 +55,11 @@ nodeunitShim({ new Vpc(stack, 'TheVPC'); cdkExpect(stack).to( haveResource('AWS::EC2::VPC', - hasTags( [{ Key: 'Name', Value: 'TestStack/TheVPC' }])), + hasTags([{ Key: 'Name', Value: 'TestStack/TheVPC' }])), ); cdkExpect(stack).to( haveResource('AWS::EC2::InternetGateway', - hasTags( [{ Key: 'Name', Value: 'TestStack/TheVPC' }])), + hasTags([{ Key: 'Name', Value: 'TestStack/TheVPC' }])), ); test.done(); }, @@ -86,7 +86,7 @@ nodeunitShim({ 'dns getters correspond to CFN properties': (() => { - const tests: any = { }; + const tests: any = {}; const inputs = [ { dnsSupport: false, dnsHostnames: false }, @@ -173,8 +173,7 @@ nodeunitShim({ }, ], }); - cdkExpect(stack).to(countResources('AWS::EC2::InternetGateway', 1)) - ; + cdkExpect(stack).to(countResources('AWS::EC2::InternetGateway', 1)); cdkExpect(stack).notTo(haveResource('AWS::EC2::NatGateway')); test.done(); }, @@ -223,7 +222,7 @@ nodeunitShim({ 'with no subnets defined, the VPC should have an IGW, and a NAT Gateway per AZ'(test: Test) { const stack = getTestStack(); const zones = stack.availabilityZones.length; - new Vpc(stack, 'TheVPC', { }); + new Vpc(stack, 'TheVPC', {}); cdkExpect(stack).to(countResources('AWS::EC2::InternetGateway', 1)); cdkExpect(stack).to(countResources('AWS::EC2::NatGateway', zones)); test.done(); @@ -251,7 +250,7 @@ nodeunitShim({ cdkExpect(stack).to(haveResource('AWS::EC2::InternetGateway')); cdkExpect(stack).to(haveResourceLike('AWS::EC2::Route', { DestinationCidrBlock: '8.8.8.8/32', - GatewayId: { }, + GatewayId: {}, })); test.done(); }, @@ -457,7 +456,7 @@ nodeunitShim({ } cdkExpect(stack).to(haveResourceLike('AWS::EC2::Route', { DestinationCidrBlock: '0.0.0.0/0', - NatGatewayId: { }, + NatGatewayId: {}, })); test.done(); @@ -475,7 +474,7 @@ nodeunitShim({ } cdkExpect(stack).to(haveResourceLike('AWS::EC2::Route', { DestinationCidrBlock: '0.0.0.0/0', - NatGatewayId: { }, + NatGatewayId: {}, })); test.done(); }, @@ -489,7 +488,7 @@ nodeunitShim({ cdkExpect(stack).to(countResources('AWS::EC2::NatGateway', 1)); cdkExpect(stack).to(haveResourceLike('AWS::EC2::Route', { DestinationCidrBlock: '0.0.0.0/0', - NatGatewayId: { }, + NatGatewayId: {}, })); test.done(); }, @@ -873,6 +872,31 @@ nodeunitShim({ test.done(); }, + 'NAT gateway provider with insufficient EIP allocations'(test: Test) { + const stack = new Stack(); + const natGatewayProvider = NatProvider.gateway({ eipAllocationIds: ['a'] }); + expect(() => new Vpc(stack, 'VpcNetwork', { natGatewayProvider })) + .toThrow(/Not enough NAT gateway EIP allocation IDs \(1 provided\) for the requested subnet count \(\d+ needed\)/); + + test.done(); + }, + + 'NAT gateway provider with token EIP allocations'(test: Test) { + const stack = new Stack(); + const eipAllocationIds = Fn.split(',', Fn.importValue('myVpcId')); + const natGatewayProvider = NatProvider.gateway({ eipAllocationIds }); + new Vpc(stack, 'VpcNetwork', { natGatewayProvider }); + + cdkExpect(stack).to(haveResource('AWS::EC2::NatGateway', { + AllocationId: stack.resolve(Fn.select(0, eipAllocationIds)), + })); + cdkExpect(stack).to(haveResource('AWS::EC2::NatGateway', { + AllocationId: stack.resolve(Fn.select(1, eipAllocationIds)), + })); + + test.done(); + }, + 'Can add an IPv6 route'(test: Test) { // GIVEN const stack = getTestStack(); From f570c94a7bc99cd5bebc96ee388d152220f9f613 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Fri, 13 Aug 2021 16:45:30 +0200 Subject: [PATCH 4/4] fix(events): cross-account event targets that have a Role are broken (#15717) When we started supporting cross-region event targets, we needed to add a role to the event-bus target. At that point, we also opted to fall back to the role that the event target requested for itself. However, that was wrong: the role used in that place is *only* for passing events between event buses, and *never* for triggering the actual target. Solution: don't fall back, only use a special role for event passing. In fact, don't even do it if the target isn't in a different region, because apparently it's not necessary for cross-account event passing at all. Refactor the `events` code a little to make clear what is happening and why, because it was starting to get messy. Fixes #15639. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* --- .../codecommit-source-action.test.ts | 87 +++++- packages/@aws-cdk/aws-events/lib/rule.ts | 276 ++++++++++-------- .../@aws-cdk/aws-events/test/test.rule.ts | 54 +++- 3 files changed, 291 insertions(+), 126 deletions(-) diff --git a/packages/@aws-cdk/aws-codepipeline-actions/test/codecommit/codecommit-source-action.test.ts b/packages/@aws-cdk/aws-codepipeline-actions/test/codecommit/codecommit-source-action.test.ts index 8c668be7f47d8..da226dd37a74d 100644 --- a/packages/@aws-cdk/aws-codepipeline-actions/test/codecommit/codecommit-source-action.test.ts +++ b/packages/@aws-cdk/aws-codepipeline-actions/test/codecommit/codecommit-source-action.test.ts @@ -1,4 +1,4 @@ -import { arrayWith, countResources, expect, haveResourceLike, not, objectLike } from '@aws-cdk/assert-internal'; +import { ABSENT, arrayWith, countResources, expect, haveResourceLike, not, objectLike } from '@aws-cdk/assert-internal'; import * as codebuild from '@aws-cdk/aws-codebuild'; import * as codecommit from '@aws-cdk/aws-codecommit'; import * as codepipeline from '@aws-cdk/aws-codepipeline'; @@ -38,6 +38,91 @@ nodeunitShim({ test.done(); }, + 'cross-account CodeCommit Repository Source does not use target role in source stack'(test: Test) { + // Test for https://github.com/aws/aws-cdk/issues/15639 + const app = new App(); + const sourceStack = new Stack(app, 'SourceStack', { env: { account: '1234', region: 'north-pole' } }); + const targetStack = new Stack(app, 'TargetStack', { env: { account: '5678', region: 'north-pole' } }); + + const repo = new codecommit.Repository(sourceStack, 'MyRepo', { + repositoryName: 'my-repo', + }); + + const sourceOutput = new codepipeline.Artifact(); + new codepipeline.Pipeline(targetStack, 'MyPipeline', { + stages: [ + { + stageName: 'Source', + actions: [ + new cpactions.CodeCommitSourceAction({ actionName: 'Source', repository: repo, output: sourceOutput }), + ], + }, + { + stageName: 'Build', + actions: [ + new cpactions.CodeBuildAction({ actionName: 'Build', project: new codebuild.PipelineProject(targetStack, 'MyProject'), input: sourceOutput }), + ], + }, + ], + }); + + // THEN - creates a Rule in the source stack targeting the pipeline stack's event bus using a generated role + expect(sourceStack).to(haveResourceLike('AWS::Events::Rule', { + EventPattern: { + source: ['aws.codecommit'], + resources: [ + { 'Fn::GetAtt': ['MyRepoF4F48043', 'Arn'] }, + ], + }, + Targets: [{ + RoleARN: ABSENT, + Arn: { + 'Fn::Join': ['', [ + 'arn:', + { 'Ref': 'AWS::Partition' }, + ':events:north-pole:5678:event-bus/default', + ]], + }, + }], + })); + + // THEN - creates a Rule in the pipeline stack using the role to start the pipeline + expect(targetStack).to(haveResourceLike('AWS::Events::Rule', { + 'EventPattern': { + 'source': [ + 'aws.codecommit', + ], + 'resources': [ + { + 'Fn::Join': [ + '', + [ + 'arn:', + { 'Ref': 'AWS::Partition' }, + ':codecommit:north-pole:1234:my-repo', + ], + ], + }, + ], + }, + 'Targets': [ + { + 'Arn': { + 'Fn::Join': ['', [ + 'arn:', + { 'Ref': 'AWS::Partition' }, + ':codepipeline:north-pole:5678:', + { 'Ref': 'MyPipelineAED38ECF' }, + ]], + }, + 'RoleArn': { 'Fn::GetAtt': ['MyPipelineEventsRoleFAB99F32', 'Arn'] }, + }, + ], + })); + + test.done(); + }, + 'does not poll for source changes and uses Events for CodeCommitTrigger.EVENTS'(test: Test) { const stack = new Stack(); diff --git a/packages/@aws-cdk/aws-events/lib/rule.ts b/packages/@aws-cdk/aws-events/lib/rule.ts index 08ecb18bbc1d7..47f5d05201a53 100644 --- a/packages/@aws-cdk/aws-events/lib/rule.ts +++ b/packages/@aws-cdk/aws-events/lib/rule.ts @@ -1,5 +1,5 @@ import { IRole, PolicyStatement, Role, ServicePrincipal } from '@aws-cdk/aws-iam'; -import { App, IConstruct, IResource, Lazy, Names, Resource, Stack, Token, PhysicalName } from '@aws-cdk/core'; +import { App, IResource, Lazy, Names, Resource, Stack, Token, PhysicalName } from '@aws-cdk/core'; import { Node, Construct } from 'constructs'; import { IEventBus } from './event-bus'; import { EventPattern } from './event-pattern'; @@ -9,10 +9,6 @@ import { Schedule } from './schedule'; import { IRuleTarget } from './target'; import { mergeEventPattern, renderEventPattern, sameEnvDimension } from './util'; -// keep this import separate from other imports to reduce chance for merge conflicts with v2-main -// eslint-disable-next-line no-duplicate-imports, import/order -import { Construct as CoreConstruct } from '@aws-cdk/core'; - /** * Properties for defining an EventBridge Rule */ @@ -118,7 +114,9 @@ export class Rule extends Resource implements IRule { private readonly eventPattern: EventPattern = { }; private readonly scheduleExpression?: string; private readonly description?: string; - private readonly targetAccounts: {[key: string]: Set} = {}; + + /** Set to keep track of what target accounts and regions we've already created event buses for */ + private readonly _xEnvTargetsAdded = new Set(); constructor(scope: Construct, id: string, props: RuleProps = { }) { super(scope, id, { @@ -194,63 +192,19 @@ export class Rule extends Resource implements IRule { // based on: // https://docs.aws.amazon.com/eventbridge/latest/userguide/eb-cross-account.html - // for cross-account or cross-region events, we cannot create new components for an imported resource - // because we don't have the target stack - const isImportedResource = !sameEnvDimension(targetStack.account, targetAccount) || !sameEnvDimension(targetStack.region, targetRegion); //(targetAccount !== targetStack.account) || (targetRegion !== targetStack.region); - if (isImportedResource) { - throw new Error('Cannot create a cross-account or cross-region rule with an imported resource'); - } - - // for cross-account or cross-region events, we require concrete accounts + // for cross-account or cross-region events, we require a concrete target account and region if (!targetAccount || Token.isUnresolved(targetAccount)) { throw new Error('You need to provide a concrete account for the target stack when using cross-account or cross-region events'); } - if (Token.isUnresolved(sourceAccount)) { - throw new Error('You need to provide a concrete account for the source stack when using cross-account or cross-region events'); - } - // and the target region has to be concrete as well if (!targetRegion || Token.isUnresolved(targetRegion)) { throw new Error('You need to provide a concrete region for the target stack when using cross-account or cross-region events'); } - - // the _actual_ target is just the event bus of the target's account - // make sure we only add it once per account per region - let targetAccountExists = false; - const accountKey = Object.keys(this.targetAccounts).find(account => account === targetAccount); - if (accountKey) { - targetAccountExists = this.targetAccounts[accountKey].has(targetRegion); - } - - if (!targetAccountExists) { - // add the current account-region pair to tracking structure - const regionsSet = this.targetAccounts[targetAccount]; - if (!regionsSet) { - this.targetAccounts[targetAccount] = new Set(); - } - this.targetAccounts[targetAccount].add(targetRegion); - - const eventBusArn = targetStack.formatArn({ - service: 'events', - resource: 'event-bus', - resourceName: 'default', - region: targetRegion, - account: targetAccount, - }); - - this.targets.push({ - id, - arn: eventBusArn, - // for cross-region we now require a role with PutEvents permissions - roleArn: roleArn ?? this.singletonEventRole(this, [this.putEventStatement(eventBusArn)]).roleArn, - }); + if (Token.isUnresolved(sourceAccount)) { + throw new Error('You need to provide a concrete account for the source stack when using cross-account or cross-region events'); } - // Grant the source account in the source region permissions to publish events to the event bus of the target account in the target region. - // Do it in a separate stack instead of the target stack (which seems like the obvious place to put it), - // because it needs to be deployed before the rule containing the above event-bus target in the source stack - // (EventBridge verifies whether you have permissions to the targets on rule creation), - // but it's common for the target stack to depend on the source stack - // (that's the case with CodePipeline, for example) + // Don't exactly understand why this code was here (seems unlikely this rule would be violated), but + // let's leave it in nonetheless. const sourceApp = this.node.root; if (!sourceApp || !App.isApp(sourceApp)) { throw new Error('Event stack which uses cross-account or cross-region targets must be part of a CDK app'); @@ -263,60 +217,28 @@ export class Rule extends Resource implements IRule { throw new Error('Event stack and target stack must belong to the same CDK app'); } - // if different accounts, we need to add the permissions to the target eventbus - if (!sameEnvDimension(sourceAccount, targetAccount)) { - const stackId = `EventBusPolicy-${sourceAccount}-${targetRegion}-${targetAccount}`; - let eventBusPolicyStack: Stack = sourceApp.node.tryFindChild(stackId) as Stack; - if (!eventBusPolicyStack) { - eventBusPolicyStack = new Stack(sourceApp, stackId, { - env: { - account: targetAccount, - region: targetRegion, - }, - stackName: `${targetStack.stackName}-EventBusPolicy-support-${targetRegion}-${sourceAccount}`, - }); - new CfnEventBusPolicy(eventBusPolicyStack, 'GivePermToOtherAccount', { - action: 'events:PutEvents', - statementId: `Allow-account-${sourceAccount}`, - principal: sourceAccount, - }); - } - // deploy the event bus permissions before the source stack - sourceStack.addDependency(eventBusPolicyStack); - } - - // The actual rule lives in the target stack. - // Other than the account, it's identical to this one - - // eventPattern is mutable through addEventPattern(), so we need to lazy evaluate it - // but only Tokens can be lazy in the framework, so make a subclass instead - const self = this; - - class CopyRule extends Rule { - public _renderEventPattern(): any { - return self._renderEventPattern(); - } - - // we need to override validate(), as it uses the - // value of the eventPattern field, - // which might be empty in the case of the copied rule - // (as the patterns in the original might be added through addEventPattern(), - // not passed through the constructor). - // Anyway, even if the original rule is invalid, - // we would get duplicate errors if we didn't override this, - // which is probably a bad idea in and of itself - protected validate(): string[] { - return []; - } - - } - - new CopyRule(targetStack, `${Names.uniqueId(this)}-${id}`, { + // The target of this Rule will be the default event bus of the target environment + this.ensureXEnvTargetEventBus(targetStack, targetAccount, targetRegion, id); + + // The actual rule lives in the target stack. Other than the account, it's identical to this one, + // but only evaluated at render time (via a special subclass). + // + // FIXME: the MirrorRule is a bit silly, forwarding the exact same event to another event bus + // and trigger on it there (there will be issues with construct references, for example). Especially + // in the case of scheduled events, we will just trigger both rules in parallel in both environments. + // + // A better solution would be to have the source rule add a unique token to the the event, + // and have the mirror rule trigger on that token only (thereby properly separating triggering, which + // happens in the source env; and activating, which happens in the target env). + // + // Don't have time to do that right now. + const mirrorRuleScope = this.obtainMirrorRuleScope(targetStack, targetAccount, targetRegion); + new MirrorRule(mirrorRuleScope, `${Names.uniqueId(this)}-${id}`, { targets: [target], eventPattern: this.eventPattern, schedule: this.scheduleExpression ? Schedule.expression(this.scheduleExpression) : undefined, description: this.description, - }); + }, this); return; } @@ -413,32 +335,140 @@ export class Rule extends Resource implements IRule { } /** - * Obtain the Role for the EventBridge event - * - * If a role already exists, it will be returned. This ensures that if multiple - * events have the same target, they will share a role. - * @internal - */ - private singletonEventRole(scope: IConstruct, policyStatements: PolicyStatement[]): IRole { - const id = 'EventsRole'; - const existing = scope.node.tryFindChild(id) as IRole; - if (existing) { return existing; } + * Make sure we add the target environments event bus as a target, and the target has permissions set up to receive our events + * + * For cross-account rules, uses a support stack to set up a policy on the target event bus. + */ + private ensureXEnvTargetEventBus(targetStack: Stack, targetAccount: string, targetRegion: string, id: string) { + // the _actual_ target is just the event bus of the target's account + // make sure we only add it once per account per region + const key = `${targetAccount}:${targetRegion}`; + if (this._xEnvTargetsAdded.has(key)) { return; } + this._xEnvTargetsAdded.add(key); + + const eventBusArn = targetStack.formatArn({ + service: 'events', + resource: 'event-bus', + resourceName: 'default', + region: targetRegion, + account: targetAccount, + }); - const role = new Role(scope as CoreConstruct, id, { - roleName: PhysicalName.GENERATE_IF_NEEDED, - assumedBy: new ServicePrincipal('events.amazonaws.com'), + // For some reason, cross-region requires a Role (with `PutEvents` on the + // target event bus) while cross-account doesn't + const roleArn = !sameEnvDimension(targetRegion, Stack.of(this).region) + ? this.crossRegionPutEventsRole(eventBusArn).roleArn + : undefined; + + this.targets.push({ + id, + arn: eventBusArn, + roleArn, }); - policyStatements.forEach(role.addToPolicy.bind(role)); + // Add a policy to the target Event Bus to allow the source account/region to publish into it. + // + // Since this Event Bus permission needs to be deployed before the stack containing the Rule is deployed + // (as EventBridge verifies whether you have permissions to the targets on rule creation), this needs + // to be in a support stack. + + const sourceApp = this.node.root as App; + const sourceAccount = Stack.of(this).account; + + // If different accounts, we need to add the permissions to the target eventbus + // + // For different region, no need for a policy on the target event bus (but a need + // for a role). + if (!sameEnvDimension(sourceAccount, targetAccount)) { + const stackId = `EventBusPolicy-${sourceAccount}-${targetRegion}-${targetAccount}`; + let eventBusPolicyStack: Stack = sourceApp.node.tryFindChild(stackId) as Stack; + if (!eventBusPolicyStack) { + eventBusPolicyStack = new Stack(sourceApp, stackId, { + env: { + account: targetAccount, + region: targetRegion, + }, + // The region in the stack name is rather redundant (it will always be the target region) + // Leaving it in for backwards compatibility. + stackName: `${targetStack.stackName}-EventBusPolicy-support-${targetRegion}-${sourceAccount}`, + }); + new CfnEventBusPolicy(eventBusPolicyStack, 'GivePermToOtherAccount', { + action: 'events:PutEvents', + statementId: `Allow-account-${sourceAccount}`, + principal: sourceAccount, + }); + } + // deploy the event bus permissions before the source stack + Stack.of(this).addDependency(eventBusPolicyStack); + } + } - return role; + /** + * Return the scope where the mirror rule should be created for x-env event targets + * + * This is the target resource's containing stack if it shares the same region (owned + * resources), or should be a fresh support stack for imported resources. + * + * We don't implement the second yet, as I have to think long and hard on whether we + * can reuse the existing support stack or not, and I don't have time for that right now. + */ + private obtainMirrorRuleScope(targetStack: Stack, targetAccount: string, targetRegion: string): Construct { + // for cross-account or cross-region events, we cannot create new components for an imported resource + // because we don't have the target stack + if (sameEnvDimension(targetStack.account, targetAccount) && sameEnvDimension(targetStack.region, targetRegion)) { + return targetStack; + } + + // For now, we don't do the work for the support stack yet + throw new Error('Cannot create a cross-account or cross-region rule for an imported resource (create a stack with the right environment for the imported resource)'); } - private putEventStatement(eventBusArn: string) { - return new PolicyStatement({ + /** + * Obtain the Role for the EventBridge event + * + * If a role already exists, it will be returned. This ensures that if multiple + * events have the same target, they will share a role. + * @internal + */ + private crossRegionPutEventsRole(eventBusArn: string): IRole { + const id = 'EventsRole'; + let role = this.node.tryFindChild(id) as IRole; + if (!role) { + role = new Role(this, id, { + roleName: PhysicalName.GENERATE_IF_NEEDED, + assumedBy: new ServicePrincipal('events.amazonaws.com'), + }); + } + + role.addToPrincipalPolicy(new PolicyStatement({ actions: ['events:PutEvents'], resources: [eventBusArn], - }); + })); + + return role; } } +/** + * A rule that mirrors another rule + */ +class MirrorRule extends Rule { + constructor(scope: Construct, id: string, props: RuleProps, private readonly source: Rule) { + super(scope, id, props); + } + + public _renderEventPattern(): any { + return this.source._renderEventPattern(); + } + + /** + * Override validate to be a no-op + * + * The rules are never stored on this object so there's nothing to validate. + * + * Instead, we mirror the other rule at render time. + */ + protected validate(): string[] { + return []; + } +} diff --git a/packages/@aws-cdk/aws-events/test/test.rule.ts b/packages/@aws-cdk/aws-events/test/test.rule.ts index 352ee30132733..b6ff7b62d86d4 100644 --- a/packages/@aws-cdk/aws-events/test/test.rule.ts +++ b/packages/@aws-cdk/aws-events/test/test.rule.ts @@ -1,3 +1,4 @@ +/* eslint-disable object-curly-newline */ import { expect, haveResource, haveResourceLike } from '@aws-cdk/assert-internal'; import * as iam from '@aws-cdk/aws-iam'; import * as cdk from '@aws-cdk/core'; @@ -417,6 +418,55 @@ export = { test.done(); }, + 'in cross-account scenario, target role is only used in target account'(test: Test) { + // GIVEN + const app = new cdk.App(); + const ruleStack = new cdk.Stack(app, 'RuleStack', { env: { account: '1234', region: 'us-east-1' } }); + const targetStack = new cdk.Stack(app, 'TargeTStack', { env: { account: '5678', region: 'us-east-1' } }); + + const rule = new Rule(ruleStack, 'EventRule', { + schedule: Schedule.rate(cdk.Duration.minutes(1)), + }); + + const role = new iam.Role(targetStack, 'SomeRole', { + assumedBy: new iam.ServicePrincipal('nobody'), + }); + + // a plain string should just be stringified (i.e. double quotes added and escaped) + rule.addTarget({ + bind: () => ({ + id: '', + arn: 'ARN2', + role, + targetResource: role, // Not really but good enough + }), + }); + + // THEN + expect(ruleStack).to(haveResourceLike('AWS::Events::Rule', { + Targets: [ + { + Arn: { 'Fn::Join': ['', [ + 'arn:', + { 'Ref': 'AWS::Partition' }, + ':events:us-east-1:5678:event-bus/default', + ]] }, + }, + ], + })); + expect(targetStack).to(haveResourceLike('AWS::Events::Rule', { + 'Targets': [ + { + 'Arn': 'ARN2', + 'Id': 'Target0', + 'RoleArn': { 'Fn::GetAtt': ['SomeRole6DDC54DD', 'Arn'] }, + }, + ], + })); + + test.done(); + }, + 'asEventRuleTarget can use the ruleArn and a uniqueId of the rule'(test: Test) { const stack = new cdk.Stack(); @@ -618,7 +668,7 @@ export = { test.throws(() => { rule.addTarget(new SomeTarget('T', resource)); - }, /You need to provide a concrete account for the source stack when using cross-account or cross-region events/); + }, /You need to provide a concrete region/); test.done(); }, @@ -843,7 +893,7 @@ export = { const resource = EventBus.fromEventBusArn(sourceStack, 'TargetEventBus', `arn:aws:events:${targetRegion}:${targetAccount}:event-bus/default`); test.throws(() => { rule.addTarget(new SomeTarget('T', resource)); - }, /Cannot create a cross-account or cross-region rule with an imported resource/); + }, /Cannot create a cross-account or cross-region rule for an imported resource/); test.done(); },