feat(events): cross-region event rules #14731

stephenhibbert · 2021-05-17T13:56:00Z

This pull request aims to extend the current support for cross-account event targets to also support limited cross-region event targets. Currently, the initial list of supported destination regions is: US East (N. Virginia – us-east-1), US West (Oregon – us-west-2), and Europe (Ireland – eu-west-1). The event can originate in any AWS region.

The original feature request is described here: #14635 and the blog post describing this feature launch is here: https://aws.amazon.com/blogs/compute/introducing-cross-region-event-routing-with-amazon-eventbridge/

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

gitpod-io · 2021-05-17T13:56:02Z

packages/@aws-cdk/aws-events/lib/rule.ts

rix0rrr · 2021-05-17T14:29:05Z

packages/@aws-cdk/aws-events/lib/rule.ts

        if (Token.isUnresolved(targetAccount)) {
-          throw new Error('You need to provide a concrete account for the target stack when using cross-account events');
+          throw new Error('You need to provide a concrete account for the source stack when using cross-account or cross-region events');


Well the conditional really says:

if (Token.isUnresolved(targetAccount)) {

rix0rrr · 2021-05-17T14:31:28Z

packages/@aws-cdk/aws-events/lib/rule.ts

-        // make sure we only add it once per account
-        const exists = this.accountEventBusTargets[targetAccount];
-        if (!exists) {
+        // make sure we only add it once per account per region


Shouldn't we check for a pair of account,region then, instead of different sets for account and regions?

That would be a neater data structure but thinking ahead to requiring that the target region is one of us-east-1, us-west-2, eu-west-1

So do we want to couple the region and account into one data structure?

Hi, I really need this field, any plan to merge?
I see this is idle since many days, If needed I'm available to work on this

@gianlucb I would really appreciate some help on this if you're able to? This was my first pull request and struggling a bit.

@gianlucb I would really appreciate some help on this if you're able to? This was my first pull request and struggling a bit.

I have addressed the comments from rix0rrr. Can you give me write permissions to your fork? I will push my changes, so we can merge this PR

@gianlucb 👍

rix0rrr · 2021-05-17T14:32:32Z

Can you try to explain what the PR is trying to achieve in the PR body?

gianlucb · 2021-06-13T21:14:06Z

AWS CodeBuild CI Report

* CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o

* Commit ID: [ad497fb](https://github.com/aws/aws-cdk/commit/ad497fba14e7c2890149c899ee5acd60b79f0d92)

* Result: FAILED

* [Build Logs](https://xuogv3vm9j.execute-api.us-east-1.amazonaws.com/Prod/buildlogs?key=7053fa37-5408-4ade-b309-b14e4a51d79d%2Fbuild.log) (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

Looks like is failing in a S3 unit test, not related with our changes

gianlucb · 2021-06-13T22:04:30Z

I have implemented the changes to support cross-region targets. It was more complex than initially thought.
Some notes:

the current implementation was theoretically wrong, even if was working fine, for imported targets. Because Stack.of(targetProps.targetResource) for an imported resource basically returns the caller (i.e: source) stack, so account/region are not correct.
just removing the cross-region check wasn't enough. In a cross-region scenario we also have to have a Role to allow PutEvents
also in my opinion the current implementation can be misleading, as in case of the target is an object in the same cdk App, it actually does interesting things (forwarding rule + event bus permissions + target rule in the target stack). But if the target is imported (even if defined in the same app) it doesn't fail and creates an invalid rule:

    // VALID
    // props.topic is defined in another stack (can target a different account)
    rule2.addTarget(new eventstargets.SnsTopic(props.topic))

    // INVALID
    // if we import the same topic (typical usage when defined elsewhere)
    // the current implementation creates an invalid rule with the target ARN
    // the deployment will fail with "cross-account target is not permitted"
    const importedTopic = sns.Topic.fromTopicArn(
      this,
      "sns-topic",
      "arn:aws:sns:eu-west-1:targetAccount:test"
    );
    rule2.addTarget(new eventstargets.SnsTopic(importedTopic))

I think the correct behavior should be to fail otherwise creates an invalid rule, and also is not clear why this doesn't work. It actually can't work because is not possible to create the necessary components for an imported resource (we don't have the target stack - and eventually the credentials for a "remote" resource).

I tried to address this in this PR, throwing an exception in case the resource is imported, but :-( even if unit testing is working this is unfortunately not consistent:

EventBus.fromEventBusArn --> set env dimensions based on target ARN properties, not the scope
Topic.fromTopicArn --> set env dimensions from the scope/current stack (do not reflect real environment)
Function.fromFunctionArn --> set env dimensions from the scope/current stack (do not reflect real environment)

therefore not sure if this "imported" sanity check worth to be included. And not sure which is the correct behavior either.

What do you think?

rix0rrr · 2021-06-14T13:59:05Z

therefore not sure if this "imported" sanity check worth to be included. And not sure which is the correct behavior either.

The correct behavior should be to always query the "real" target environment, which we get by accessing resource.env.region (NOT: Stack.of(resource).region, which you already noted is incorrect).

The next hurdle is that not all resources (yet) correctly set this.env, and if not explicitly configured it defaults to the same (incorrect) default Stack.of(this).region. However, that's a different problem, and one that can and should be fixed by passing
environmentFromArn for imported resources everywhere:

aws-cdk/packages/@aws-cdk/core/lib/resource.ts

Lines 94 to 104 in 92cadef

    
             /** 
        
              * ARN to deduce region and account from 
        
              * 
        
              * The ARN is parsed and the account and region are taken from the ARN. 
        
              * This should be used for imported resources. 
        
              * 
        
              * Cannot be supplied together with either `account` or `region`. 
        
              * 
        
              * @default - take environment from `account`, `region` parameters, or use Stack environment. 
        
              */ 
        
             readonly environmentFromArn?: string;

I think you might be right that it'll be impossible to detect whether the targetResource class does or does not have the bug... but we can squash those bugs on a case-by-case basis when we encounter them.

Sound good?

gianlucb · 2021-06-14T14:23:25Z

therefore not sure if this "imported" sanity check worth to be included. And not sure which is the correct behavior either.

The correct behavior should be to always query the "real" target environment, which we get by accessing resource.env.region (NOT: Stack.of(resource).region, which you already noted is incorrect).

The next hurdle is that not all resources (yet) correctly set this.env, and if not explicitly configured it defaults to the same (incorrect) default Stack.of(this).region. However, that's a different problem, and one that can and should be fixed by passing
environmentFromArn for imported resources everywhere:

aws-cdk/packages/@aws-cdk/core/lib/resource.ts

Lines 94 to 104 in 92cadef

/**

* ARN to deduce region and account from

*

* The ARN is parsed and the account and region are taken from the ARN.

* This should be used for imported resources.

*

* Cannot be supplied together with either `account` or `region`.

*

* @default - take environment from `account`, `region` parameters, or use Stack environment.

*/

readonly environmentFromArn?: string;

I think you might be right that it'll be impossible to detect whether the targetResource class does or does not have the bug... but we can squash those bugs on a case-by-case basis when we encounter them.

Sound good?

Perfect!

rix0rrr · 2021-07-02T11:57:06Z

packages/@aws-cdk/aws-events/lib/rule.ts

+        // 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');


I guess I'm confused by this check.

The motivation given is "we don't have the target stack", but why do we actually need the target stack?

For cross-account access, we could create a support stack, which in fact is done below, so why do we need the target stack? (We could do this opportunistically, create a support stack if we have an imported resource or add to the target stack if we have an owned resource -- and add a flag to disable it maybe?)

For cross-region access, it seems nothing special needs to be done? Or am I missing something?

rix0rrr

Discussed with @gianlucb that we should assume that thing.env.account and thing.env.region are correct -- if they are not correct then that's a bug in the implementation of Thing.fromThingArn() and should be addressed there, not here.

Modulo that, ready to accept this.

Pull request has been modified.

aws-cdk-automation · 2021-07-12T12:11:58Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
Commit ID: ee60dae
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2021-07-12T12:12:34Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

This pull request aims to extend the current support for cross-account event targets to also support limited cross-region event targets. Currently, the initial list of supported destination regions is: US East (N. Virginia – us-east-1), US West (Oregon – us-west-2), and Europe (Ireland – eu-west-1). The event can originate in any AWS region. The original feature request is described here: aws#14635 and the blog post describing this feature launch is here: https://aws.amazon.com/blogs/compute/introducing-cross-region-event-routing-with-amazon-eventbridge/ ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

feat(aws-events): Multi-region support for event rules aws#14635

6921374

github-actions bot assigned rix0rrr May 17, 2021

github-actions bot added the @aws-cdk/aws-events Related to CloudWatch Events label May 17, 2021

rix0rrr reviewed May 17, 2021

View reviewed changes

gianlucb and others added 5 commits June 13, 2021 20:18

feat(aws-events): cross-region support for event rules

1a90b58

feat(aws-events): cross-region support for event rules

227c8cb

Merge branch 'master' into feat-crossregioneventtargets

ad497fb

feat(aws-events): cross-region support for event rules

1211404

feat(aws-events): cross-region support for event rules

8ec002e

Merge branch 'master' into feat-crossregioneventtargets

588aa04

gianlucb and others added 5 commits June 18, 2021 17:10

Merge branch 'master' into feat-crossregioneventtargets

938c730

Merge branch 'master' into feat-crossregioneventtargets

9e8e504

Merge branch 'master' into feat-crossregioneventtargets

0990365

Merge branch 'aws:master' into feat-crossregioneventtargets

1946be0

feat(aws-events): cross-region support for event rules

f3eb9e4

rix0rrr changed the title ~~feat(aws-events): Multi-region support for event rules #14635~~ feat(events): cross-region event rules Jul 2, 2021

rix0rrr reviewed Jul 2, 2021

View reviewed changes

rix0rrr marked this pull request as ready for review July 5, 2021 12:36

rix0rrr previously requested changes Jul 5, 2021

View reviewed changes

Merge branch 'aws:master' into feat-crossregioneventtargets

cf50188

Merge branch 'master' into feat-crossregioneventtargets

ee60dae

rix0rrr approved these changes Jul 12, 2021

View reviewed changes

mergify bot merged commit c62afe9 into aws:master Jul 12, 2021

gianlucb deleted the feat-crossregioneventtargets branch July 12, 2021 12:15

jdvornek mentioned this pull request Jul 19, 2021

(aws-events): cross-region event rules breaks cross-account codepipelines #15639

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(events): cross-region event rules #14731

feat(events): cross-region event rules #14731

stephenhibbert commented May 17, 2021 •

edited

Loading

gitpod-io bot commented May 17, 2021

rix0rrr May 17, 2021

rix0rrr May 17, 2021

stephenhibbert May 18, 2021

gianlucb Jun 9, 2021

stephenhibbert Jun 10, 2021

gianlucb Jun 10, 2021

titanjer Jun 13, 2021

rix0rrr commented May 17, 2021

gianlucb commented Jun 13, 2021

AWS CodeBuild CI Report

gianlucb commented Jun 13, 2021

rix0rrr commented Jun 14, 2021

gianlucb commented Jun 14, 2021

rix0rrr Jul 2, 2021

rix0rrr left a comment

aws-cdk-automation commented Jul 12, 2021

mergify bot commented Jul 12, 2021

feat(events): cross-region event rules #14731

feat(events): cross-region event rules #14731

Conversation

stephenhibbert commented May 17, 2021 • edited Loading

gitpod-io bot commented May 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rix0rrr commented May 17, 2021

gianlucb commented Jun 13, 2021

AWS CodeBuild CI Report

gianlucb commented Jun 13, 2021

rix0rrr commented Jun 14, 2021

gianlucb commented Jun 14, 2021

Choose a reason for hiding this comment

rix0rrr left a comment

Choose a reason for hiding this comment

aws-cdk-automation commented Jul 12, 2021

AWS CodeBuild CI Report

mergify bot commented Jul 12, 2021

stephenhibbert commented May 17, 2021 •

edited

Loading