Skip to content

Commit

Permalink
fix(events): cross stack rules require concrete environment (#23549)
Browse files Browse the repository at this point in the history
When a rule and a rule target are in different environments (account and/or region) we have to perform some extra setup to get the cross environment stuff working. Previously if one of the environments was not defined then we assumed that we were working in a cross environment fashion and we threw an error message. It is probably a much more common scenario for both the stacks to be deployed to the same environment if one of the environments is not defined.

This PR updates the logic to make that assumption and to provide a warning to the user that if they _are_ working in a cross environment setup they need to provide the environment.

fix #18405


----

### All Submissions:

* [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Construct Runtime Dependencies:

* [ ] This PR adds new construct runtime dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-construct-runtime-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] 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*
  • Loading branch information
corymhall committed Jan 4, 2023
1 parent 44a4812 commit 22d3341
Show file tree
Hide file tree
Showing 3 changed files with 58 additions and 29 deletions.
33 changes: 27 additions & 6 deletions packages/@aws-cdk/aws-events/lib/rule.ts
@@ -1,5 +1,5 @@
import { IRole, PolicyStatement, Role, ServicePrincipal } from '@aws-cdk/aws-iam';
import { App, IResource, Lazy, Names, Resource, Stack, Token, TokenComparison, PhysicalName, ArnFormat } from '@aws-cdk/core';
import { App, IResource, Lazy, Names, Resource, Stack, Token, TokenComparison, PhysicalName, ArnFormat, Annotations } from '@aws-cdk/core';
import { Node, Construct } from 'constructs';
import { IEventBus } from './event-bus';
import { EventPattern } from './event-pattern';
Expand All @@ -8,7 +8,7 @@ import { EventCommonOptions } from './on-event-options';
import { IRule } from './rule-ref';
import { Schedule } from './schedule';
import { IRuleTarget } from './target';
import { mergeEventPattern, renderEventPattern, sameEnvDimension } from './util';
import { mergeEventPattern, renderEventPattern } from './util';

/**
* Properties for defining an EventBridge Rule
Expand Down Expand Up @@ -163,7 +163,7 @@ export class Rule extends Resource implements IRule {
// - forwarding rule in the source stack (target: default event bus of the receiver region)
// - eventbus permissions policy (creating an extra stack)
// - receiver rule in the target stack (target: the actual target)
if (!sameEnvDimension(sourceAccount, targetAccount) || !sameEnvDimension(sourceRegion, targetRegion)) {
if (!this.sameEnvDimension(sourceAccount, targetAccount) || !this.sameEnvDimension(sourceRegion, targetRegion)) {
// cross-account and/or cross-region event - strap in, this works differently than regular events!
// based on:
// https://docs.aws.amazon.com/eventbridge/latest/userguide/eb-cross-account.html
Expand Down Expand Up @@ -332,7 +332,7 @@ export class Rule extends Resource implements IRule {

// 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)
const roleArn = !this.sameEnvDimension(targetRegion, Stack.of(this).region)
? this.crossRegionPutEventsRole(eventBusArn).roleArn
: undefined;

Expand All @@ -355,7 +355,7 @@ export class Rule extends Resource implements IRule {
//
// For different region, no need for a policy on the target event bus (but a need
// for a role).
if (!sameEnvDimension(sourceAccount, targetAccount)) {
if (!this.sameEnvDimension(sourceAccount, targetAccount)) {
const stackId = `EventBusPolicy-${sourceAccount}-${targetRegion}-${targetAccount}`;
let eventBusPolicyStack: Stack = sourceApp.node.tryFindChild(stackId) as Stack;
if (!eventBusPolicyStack) {
Expand Down Expand Up @@ -394,7 +394,7 @@ export class Rule extends Resource implements IRule {
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)) {
if (this.sameEnvDimension(targetStack.account, targetAccount) && this.sameEnvDimension(targetStack.region, targetRegion)) {
return targetStack;
}

Expand Down Expand Up @@ -426,6 +426,27 @@ export class Rule extends Resource implements IRule {

return role;
}


/**
* Whether two string probably contain the same environment dimension (region or account)
*
* Used to compare either accounts or regions, and also returns true if one or both
* are unresolved (in which case both are expected to be "current region" or "current account").
*/
private sameEnvDimension(dim1: string, dim2: string) {
switch (Token.compareStrings(dim1, dim2)) {
case TokenComparison.ONE_UNRESOLVED:
Annotations.of(this).addWarning('Either the Event Rule or target has an unresolved environment. \n \
If they are being used in a cross-environment setup you need to specify the environment for both.');
return true;
case TokenComparison.BOTH_UNRESOLVED:
case TokenComparison.SAME:
return true;
default:
return false;
}
}
}

function determineRuleScope(scope: Construct, props: RuleProps): Construct {
Expand Down
14 changes: 1 addition & 13 deletions packages/@aws-cdk/aws-events/lib/util.ts
@@ -1,4 +1,3 @@
import { Token, TokenComparison } from '@aws-cdk/core';
import { EventPattern } from './event-pattern';

/**
Expand Down Expand Up @@ -55,17 +54,6 @@ export function mergeEventPattern(dest: any, src: any) {
}
}

/**
* Whether two string probably contain the same environment dimension (region or account)
*
* Used to compare either accounts or regions, and also returns true if both
* are unresolved (in which case both are expted to be "current region" or "current account").
* @internal
*/
export function sameEnvDimension(dim1: string, dim2: string) {
return [TokenComparison.SAME, TokenComparison.BOTH_UNRESOLVED].includes(Token.compareStrings(dim1, dim2));
}

/**
* Transform an eventPattern object into a valid Event Rule Pattern
* by changing detailType into detail-type when present.
Expand All @@ -86,4 +74,4 @@ export function renderEventPattern(eventPattern: EventPattern): any {
}

return out;
}
}
40 changes: 30 additions & 10 deletions packages/@aws-cdk/aws-events/test/rule.test.ts
Expand Up @@ -689,30 +689,50 @@ describe('rule', () => {
const app = new cdk.App();

const sourceStack = new cdk.Stack(app, 'SourceStack');
const rule = new Rule(sourceStack, 'Rule');
const rule = new Rule(sourceStack, 'Rule', {
eventPattern: {
source: ['some-event'],
},
});

const targetAccount = '234567890123';
const targetStack = new cdk.Stack(app, 'TargetStack', { env: { account: targetAccount } });
const resource = new Construct(targetStack, 'Resource');

expect(() => {
rule.addTarget(new SomeTarget('T', resource));
}).toThrow(/You need to provide a concrete region/);
rule.addTarget(new SomeTarget('T', resource));
Template.fromStack(sourceStack).hasResourceProperties('AWS::Events::Rule', {
'Targets': [
{
'Id': 'T',
'Arn': 'ARN1',
},
],
});
Annotations.fromStack(sourceStack).hasWarning('/'+rule.node.path, Match.stringLikeRegexp('Either the Event Rule or target has an unresolved environment'));
});

test('requires that the target stack specify a concrete account', () => {
const app = new cdk.App();

const sourceAccount = '123456789012';
const sourceStack = new cdk.Stack(app, 'SourceStack', { env: { account: sourceAccount } });
const rule = new Rule(sourceStack, 'Rule');
const rule = new Rule(sourceStack, 'Rule', {
eventPattern: {
source: ['some-event'],
},
});

const targetStack = new cdk.Stack(app, 'TargetStack');
const resource = new Construct(targetStack, 'Resource');

expect(() => {
rule.addTarget(new SomeTarget('T', resource));
}).toThrow(/You need to provide a concrete account for the target stack when using cross-account or cross-region events/);
rule.addTarget(new SomeTarget('T', resource));
Template.fromStack(sourceStack).hasResourceProperties('AWS::Events::Rule', {
'Targets': [
{
'Id': 'T',
'Arn': 'ARN1',
},
],
});
Annotations.fromStack(sourceStack).hasWarning('/'+rule.node.path, Match.stringLikeRegexp('Either the Event Rule or target has an unresolved environment'));
});

test('requires that the target stack specify a concrete region', () => {
Expand Down

0 comments on commit 22d3341

Please sign in to comment.