-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(sns-subscriptions): cross opt-in region support #32542
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #32542 +/- ##
==========================================
+ Coverage 78.77% 78.85% +0.07%
==========================================
Files 108 108
Lines 7163 7165 +2
Branches 1320 1319 -1
==========================================
+ Hits 5643 5650 +7
+ Misses 1335 1330 -5
Partials 185 185
Flags with carried forward coverage won't be shown. Click here to find out more.
|
* Generic subscription target | ||
*/ | ||
export abstract class Subscription implements sns.ITopicSubscription { | ||
constructor(private readonly subscriber: IResource) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Subscription.subscriber
ends up having the same value as the existing SqsSubscription.queue
and LambdaSubscription.fn
members, but with a broader typing.
I would have preferred to have a common protected
property between all 3, and have the class implementations overload the abstract class's IResource
, but jsii wouldn't allow it. The alternatives would be to convert generateGrantPrincipal
into a static method, and pass both the topic and the subscriber as parameters. Neither feel particularly great, but I slightly prefer my current implementation
const [isSubscriberRegionOptIn, isTopicRegionOptIn] = [subscriberRegion, topicRegion].map((region) => regionInfo.Fact.find( | ||
region, | ||
regionInfo.FactName.IS_OPT_IN_REGION, | ||
)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although all the SNS supported opt-in Regions are IS_OPT_IN_REGION
, not all IS_OPT_IN_REGION
are supported:
aws-cdk/packages/aws-cdk-lib/region-info/lib/aws-entities.ts
Lines 52 to 69 in bf026bd
RULE_CLASSIC_PARTITION_BECOMES_OPT_IN, | |
'ap-east-1', // Asia Pacific (Hong Kong) | |
'me-south-1', // Middle East (Bahrain) | |
'af-south-1', // Africa (Cape Town) | |
'eu-south-1', // Europe (Milan) | |
'us-iso-west-1', // US ISO West | |
'ap-southeast-3', // Asia Pacific (Jakarta) | |
'me-central-1', // Middle East (UAE) | |
'eu-central-2', // Europe (Zurich) | |
'eu-south-2', // Europe (Spain) | |
'ap-south-2', // Asia Pacific (Hyderabad) | |
'ap-southeast-4', // Asia Pacific (Melbourne) | |
'il-central-1', // Israel (Tel Aviv) | |
'ca-west-1', // Canada West (Calgary) | |
'ap-southeast-5', // Asia Pacific (Malaysia) | |
'ap-southeast-7', // Asia Pacific (Thailand) | |
'mx-central-1', // Mexico (Central) | |
'eu-isoe-west-1', // EU ISO-E West |
I'm not sure if this is something the CDK should maintain or let users "just figure it out" when their SNS messages don't show up. A happy medium could be to store the list of regions known to be supported, and only warn the user that the opt-in region they are using might be unsupported, and to open an issue if that's incorrect
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns out the region list in SNS supported opt-in Regions is inaccurate, or at least not up to date. I was able to successfully run the integ tests in ca-west-1
, which is an opt-in region not listed in the documentation table.
I'm going to assume that all opt-in regions are supported and this is just a case of stale documentation, but confirmation from the SNS team would be appreciated
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Issue # (if applicable)
Closes #32526.
Reason for this change
Although SNS supports cross-region message delivery to SQS Queues and Lambda Functions, it requires replacing the service principal for opt-in regions, see Sending Amazon SNS messages to an Amazon SQS queue or AWS Lambda function in a different Region. Additionally, some cross-region configurations are also not supported, such as opt-in to opt-in cross region delivery.
Description of changes
generateGrantPrincipal
method to replace the previously staticsns.amazonaws.com
principal with a potentially opt-in region-dependentsns.<region>.amazonaws.com
. Invalid cross region setups result in an exception.Subscription
construct class to provide the method to bothLambdaSubscription
andSqsSubscription
lambda.Function.isFunction
to determine whether or not the given resource is a generated or imported Functionsqs.Queue.isQueue
to determine whether or not the given resource is a generated or imported QueueDescription of how you validated changes
Added unit and integ tests. The integ tests assertions had to be ran manually to allow for running AWS API calls across multiple regions.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license