Skip to content
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.

Sign up for GitHub

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

Jump to bottom

aws-sns-subscriptions: Adding SqsSubscription on SNS topic does not create correct queue policy for SQS when SNS topic is present in opt-in AWS regions #32526

Open
1 task
rahuldeverani opened this issue Dec 14, 2024 · 4 comments · May be fixed by #32542
Open
1 task

aws-sns-subscriptions: Adding SqsSubscription on SNS topic does not create correct queue policy for SQS when SNS topic is present in opt-in AWS regions #32526

rahuldeverani opened this issue Dec 14, 2024 · 4 comments · May be fixed by #32542
Labels
@aws-cdk/aws-sns Related to Amazon Simple Notification Service bug This issue is a bug. effort/medium Medium work item – several days of effort p2

Comments

@rahuldeverani
Copy link

rahuldeverani commented Dec 14, 2024 •
edited
Loading

Describe the bug

When adding sqs subscription on SNS topic where the SNS topic is cross region and is present in opt-in AWS regions , then the queue policy attached to SQS queue is not correct.

The policy generated for sqs queue is like:

{
  "Version": "2012-10-17",
  "Statement": [
    {
      "Effect": "Allow",
      "Principal": {
        "Service": "sns.amazonaws.com"
      },
      "Action": "sqs:SendMessage",
      "Resource": "<Queue Arn>",
      "Condition": {
        "ArnEquals": {
          "aws:SourceArn": "<Topic Arn>"
        }
      }
    }
  ]
}

Here the Service in principal should be sns.opt-in-region.amazonaws.com instead of sns.amazonaws.com . Check this for more details: https://docs.aws.amazon.com/sns/latest/dg/sns-cross-region-delivery.html

Regression Issue

  • Select this option if this issue appears to be a regression.

Last Known Working CDK Version

No response

Expected Behavior

The queue policy should have the right principal as: sns.opt-in-region.amazonaws.com

Current Behavior

currently , when the sns topic is in opt-in AWS region, then also the sqs queue policy has principal as sns.amazonaws.com.

Reproduction Steps

   
const queue = new sqs.Queue(this ,'queue')

const topicArn= '<Topic-Arn>'  # topic must be present in AWS opt-in regions                   
# https://docs.aws.amazon.com/sns/latest/dg/sns-cross-region-delivery.html
   
 const topic = sns.Topic.fromTopicArn(this, 'Topic', topicArn);

    topic.addSubscription(
      new snssub.SqsSubscription(queue, {
        rawMessageDelivery: true,
      }),
    );
  }}

Possible Solution

In case if the SNS topic is present in opt-in regions, queue policy can be changed to add the opt-in region in the principal of sqs queue policy.

We have a workaround where we can explicitly add queue policy, but it would be better if CDK could handle this case as well.

Additional Information/Context

Seems like this is coming from here: https://github.com/aws/aws-cdk/blob/bf026bdd8557305d427510af49f1bc538d439cb6/packages/aws-cdk-lib/aws-sns-subscriptions/lib/sqs.ts#L39C11-L39C30

CDK CLI Version

2.173.0

Framework Version

No response

Node.js Version

v23.1.0

OS

MacOs

Language

TypeScript

Language Version

No response

Other information

No response
@rahuldeverani rahuldeverani added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Dec 14, 2024
@github-actions github-actions bot added the @aws-cdk/aws-sns Related to Amazon Simple Notification Service label Dec 14, 2024
@nmussy
Copy link
Contributor

nmussy commented Dec 16, 2024

This should be fairly easy to handle using FactName.IS_OPT_IN_REGION, I'll have a look 👍

@nmussy nmussy linked a pull request Dec 16, 2024 that will close this issue
Open
1 task
@nmussy
Copy link
Contributor

nmussy commented Dec 16, 2024

I started with the integration test for the PR to confirm the issue, and I do the following error when attempting to post a message across a default and an opt-in version: Credential should be scoped to a valid region. See integ test

@ashishdhingra
Copy link
Contributor

Reproducible using code below:

import * as cdk from 'aws-cdk-lib';
import * as sns from 'aws-cdk-lib/aws-sns';
import * as sqs from 'aws-cdk-lib/aws-sqs';
import * as snssubscriptions from 'aws-cdk-lib/aws-sns-subscriptions';

export class CdktestStackNew extends cdk.Stack {
  constructor(scope: cdk.App, id: string, props?: cdk.StackProps) {
    super(scope, id, props);

    const queue = new sqs.Queue(this ,'queue');
    const topicArn = 'arn:aws:sns:ap-southeast-4:<<ACCOUNT-ID>>:TestOptInTopic'; // Topic must be present in AWS opt-in regions, https://docs.aws.amazon.com/sns/latest/dg/sns-cross-region-delivery.html

    const topic = sns.Topic.fromTopicArn(this, 'Topic', topicArn);
    topic.addSubscription(
      new snssubscriptions.SqsSubscription(queue, {
        rawMessageDelivery: true,
      }),
    );
  }
}

Topic had been manually created in Opt-in ap-southeast-4 region per Sending Amazon SNS messages to an Amazon SQS queue or AWS Lambda function in a different Region.

Running cdk synth produces the below CloudFormation template:

Resources:
  queue276F7297:
    Type: AWS::SQS::Queue
    UpdateReplacePolicy: Delete
    DeletionPolicy: Delete
    Metadata:
      aws:cdk:path: CdktestStackNew/queue/Resource
  queuePolicy89DB7105:
    Type: AWS::SQS::QueuePolicy
    Properties:
      PolicyDocument:
        Statement:
          - Action: sqs:SendMessage
            Condition:
              ArnEquals:
                aws:SourceArn: arn:aws:sns:ap-southeast-4:<<ACCOUNT-ID>>:TestOptInTopic
            Effect: Allow
            Principal:
              Service: sns.amazonaws.com
            Resource:
              Fn::GetAtt:
                - queue276F7297
                - Arn
        Version: "2012-10-17"
      Queues:
        - Ref: queue276F7297
    Metadata:
      aws:cdk:path: CdktestStackNew/queue/Policy/Resource
  queueCdktestStackNewTopic1B7F5337F1CEB735:
    Type: AWS::SNS::Subscription
    Properties:
      Endpoint:
        Fn::GetAtt:
          - queue276F7297
          - Arn
      Protocol: sqs
      RawMessageDelivery: true
      Region: ap-southeast-4
      TopicArn: arn:aws:sns:ap-southeast-4:<<ACCOUNT-ID>>:TestOptInTopic
    DependsOn:
      - queuePolicy89DB7105
    Metadata:
      aws:cdk:path: CdktestStackNew/queue/CdktestStackNewTopic1B7F5337/Resource
  CDKMetadata:
    Type: AWS::CDK::Metadata
    Properties:
      Analytics: v2:deflate64:H4sIAAAAAAAA/1WKwQ6CMBBEv4V7WaEx0bP+gIJ3U8qaLGAL3VZimv67wSYmXmbmzYyE+iChKtTKpe7HcqIOYuuVHoVa+R55YYjXgAHF+WFy+OrFTqTfvzJjEmwYYhs61o5mT9Zsjz++2Zn0STGmtG0Nsg1OYxLG9ggD7171EWQF+2JgotIF4+mJ0GT/AO+S86CwAAAA
    Metadata:
      aws:cdk:path: CdktestStackNew/CDKMetadata/Default
Parameters:
  BootstrapVersion:
    Type: AWS::SSM::Parameter::Value<String>
    Default: /cdk-bootstrap/hnb659fds/version
    Description: Version of the CDK Bootstrap resources in this environment, automatically retrieved from SSM Parameter Store. [cdk:skip]

The service principal of the generated policy document is sns.amazonaws.com instead of sns.ap-southeast-4.amazonaws.com.

@ashishdhingra ashishdhingra self-assigned this Dec 16, 2024
@ashishdhingra ashishdhingra added p2 investigating This issue is being investigated and/or work is in progress to resolve the issue. effort/medium Medium work item – several days of effort and removed needs-triage This issue or PR still needs to be triaged. investigating This issue is being investigated and/or work is in progress to resolve the issue. labels Dec 16, 2024
@ashishdhingra ashishdhingra removed their assignment Dec 16, 2024
@nmussy
Copy link
Contributor

nmussy commented Dec 16, 2024

#32542 addresses this issue for both SqsSubscription and LambdaSubscription, I'm calling it for the night but it's pretty much done, just need to clean up a bit and finish the PR message

This was referenced Jan 1, 2025
Open
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
@aws-cdk/aws-sns Related to Amazon Simple Notification Service bug This issue is a bug. effort/medium Medium work item – several days of effort p2
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(sns-subscriptions): cross opt-in region support nmussy/aws-cdk
3 participants
@nmussy @rahuldeverani @ashishdhingra