fix(sns-subscriptions): use regionalized service principal for opt-in region SQS subscriptions - #38339
Conversation
…region subscriptions involving opt-in regions When subscribing an SQS queue or Lambda function to an SNS topic where one or both regions are opt-in regions, the generated policies now use the correct regionalized service principal (e.g., sns.<region>.amazonaws.com) instead of always using the global sns.amazonaws.com principal. Per the SNS cross-region delivery documentation, the regionalized principal is required when opt-in regions are involved: - Default region to opt-in region: sns.<subscriber-region>.amazonaws.com - Opt-in region to default region: sns.<topic-region>.amazonaws.com - Opt-in region to opt-in region: sns.<subscriber-region>.amazonaws.com Without this fix, cross-region SNS-to-SQS/Lambda delivery involving opt-in regions silently fails with messages being dropped. When regions cannot be determined at synth time (tokenized ARNs or env-agnostic stacks), the behavior falls back to the global principal to maintain backwards compatibility. Closes aws#32526.
… subscription Adds an integration test that verifies the SQS queue policy uses a regionalized service principal (sns.ap-southeast-4.amazonaws.com) when the SNS topic is in an opt-in region and the queue is in a default region.
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
…oth opt-in cases Use the same explicit principal construction for both subscriber-opt-in and topic-opt-in scenarios rather than mixing approaches. This avoids relying on ServicePrincipalToken's resolve-time region comparison logic and makes the behavior deterministic at construct time.
…le instead Use the imported function name directly and rename the local variable from snsServicePrincipal to principal to avoid the naming conflict.
alvazjor
left a comment
There was a problem hiding this comment.
The principal selection matches the SNS cross-region delivery docs in every case, nice work.
This changes the synthesized template for existing stacks, which normally needs a feature flag. I'm fine shipping it without one here: the only templates that change belong to cross-region opt-in subscriptions, and those were already dropping messages with the global principal. We are not changing any working setup, so a flag would just keep broken stacks broken until users opt in. Non-opt-in setups synthesize exactly as before.
| function resolveTopicRegion(topic: sns.ITopic, subscriber: IResource): string | undefined { | ||
| if (topic instanceof sns.Topic) { | ||
| if (!Token.isUnresolved(topic.env.region)) { | ||
| return topic.env.region; | ||
| } | ||
| return undefined; | ||
| } | ||
| return Stack.of(subscriber).splitArn(topic.topicArn, ArnFormat.SLASH_RESOURCE_NAME).region; |
There was a problem hiding this comment.
Two things on region resolution here:
- Guard the imported-topic ARN before splitting it.
splitArnruns ontopic.topicArnwithout aToken.isUnresolved(topic.topicArn)check first. Add the check so a token ARN resolves toundefined(global) on purpose rather than by accident, and validate the extracted region:
function resolveTopicRegion(topic: sns.ITopic, subscriber: IResource): string | undefined {
if (topic instanceof sns.Topic) {
return Token.isUnresolved(topic.env.region) ? undefined : topic.env.region;
}
if (Token.isUnresolved(topic.topicArn)) {
return undefined;
}
const region = Stack.of(subscriber).splitArn(topic.topicArn, ArnFormat.SLASH_RESOURCE_NAME).region;
return region && !Token.isUnresolved(region) ? region : undefined;
}- Make the fallback limitation explicit in the docs. When either region is unresolved (env-agnostic stacks, or an imported token ARN), the principal stays global and the opt-in fix silently does not apply, so messages can still be dropped. Please spell this out in the
snsServicePrincipalJSDoc: the stack needs an explicitenvfor the fix to take effect, and if that is not possible the user will need to add the regionalizedsns.<region>.amazonaws.comprincipal to the resource policy themselves.
There was a problem hiding this comment.
Makes sense, I implemented the feedback.
| this.fn.addPermission(`AllowInvoke:${Names.nodeUniqueId(topic.node)}`, { | ||
| sourceArn: topic.topicArn, | ||
| principal: new iam.ServicePrincipal('sns.amazonaws.com'), | ||
| principal: snsServicePrincipal(topic, this.fn), |
There was a problem hiding this comment.
Question
There's a conflict between two AWS docs on which Lambda cross-region opt-in combinations are actually supported. The SNS cross-region delivery page lists all three opt-in combinations as supported for Lambda, but the Lambda-specific prerequisites page says the opposite:
"AWS doesn't support cross-region delivery to Lambda from a region that is enabled by default to an opt-in region. Also, cross-region forwarding of Amazon SNS messages from opt-in regions to other opt-in regions is not supported."
If the prereq page is current, then for a Lambda subscriber this helper emits a regionalized principal for combinations AWS does not support for delivery anyway (default to opt-in, and opt-in to opt-in). The principal itself is not wrong, so this is not harmful. Have you tested this end to end in an opt-in region to confirm whether the Lambda prereq page is right or wrong? If you have verified that default to opt-in or opt-in to opt-in delivery actually works for Lambda, that settles it. If it does not work, it may be worth scoping the Lambda path or noting the limitation, rather than implying full support.
There was a problem hiding this comment.
Sounds good, yeah I descoped the Lambda work from this PR. Do you know who we can contact to request clarification on these docs?
I'm not impacted by those changes, but it's still best if we can resolve the discrepancy.
There was a problem hiding this comment.
I will follow up with Lambda team, that is not a blocker for getting this merged. Thanks
Revert the LambdaSubscription change and remove its unit test. AWS documentation is inconsistent on whether SNS-to-Lambda cross-region delivery is supported for all opt-in region combinations (the SNS cross-region delivery page lists them as supported, while the Lambda prerequisites page states default-to-opt-in and opt-in-to-opt-in are not), so the Lambda path is descoped pending confirmation. The original issue (aws#32526) concerns SQS.
Address review feedback on resolveTopicRegion: check Token.isUnresolved on the imported topic's ARN before calling splitArn, and validate the extracted region is a concrete non-empty string. Also document in the snsServicePrincipal JSDoc that the opt-in regionalization only applies when both regions are known at synth time, and how users can work around env-agnostic stacks or tokenized ARNs.
alvazjor
left a comment
There was a problem hiding this comment.
Thanks for the contribution
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Merge Queue Status
This pull request spent 5 minutes 6 seconds in the queue, including 6 seconds running CI. Required conditions to merge
|
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
Comments on closed issues and PRs are hard for our team to see. |
Issue # (if applicable)
Closes #32526.
Reason for this change
When subscribing an SQS queue or Lambda function to an SNS topic where one or both regions are opt-in regions,
SqsSubscriptionandLambdaSubscriptiongenerate policies with the globalsns.amazonaws.comservice principal. Per the SNS cross-region delivery documentation, a regionalized service principal is required when opt-in regions are involved, otherwise messages are silently dropped.Description of changes
Added a
snsServicePrincipal()helper inaws-sns-subscriptions/lib/private/util.tsthat determines the correct SNS service principal based on the topic and subscriber regions:sns.<subscriber-region>.amazonaws.comsns.<topic-region>.amazonaws.comsns.<subscriber-region>.amazonaws.comsns.amazonaws.com(unchanged)sns.amazonaws.com(safe default)The helper uses
RegionInfo.get(region).isOptInRegionto determine whether regionalization is needed, andServicePrincipal.fromStaticServicePrincipleName()to construct the regionalized principal directly. We usefromStaticServicePrincipleNamerather thannew ServicePrincipal('sns.amazonaws.com', { region })because the standardServicePrincipalregion option relies on token resolution that compares the passed region against the stack's region at synth time — this fails when the policy lives in the same stack as the opt-in region (e.g., the subscriber is in the opt-in region), sincestack.region === opts.regioncauses it to skip regionalization. Using the static method bypasses this entirely and makes the behavior deterministic.Updated both
SqsSubscription.bind()andLambdaSubscription.bind()to use this helper.Description of how you validated changes
Added unit tests covering all scenarios:
Added integration test (
integ.sns-sqs-cross-opt-in-region.ts) with snapshot verifying that a topic inap-southeast-4(opt-in) subscribing to a queue inus-east-1produces the regionalized principal in the queue policy.Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license