sns: support cross-region subscriptions

[dillon-odonovan]

When creating an SNS subscription in a region different from the SNS topic's region, there is a Region property in the CloudFormation resource that is not being set.

Reproduction Steps

sealed class Program
{
  public static void Main(string[] args)
  {
    var app = new App();

    var snsStack = new SnsStack(app, "Sns", new StackProps
    {
        Env = new Environment
        {
            Account = "XXXXXXXXXXXX",
            Region = "us-east-1",
        }
    });
    
    var lambdaStack = new LambdaStack(app, "Lambda", new StackProps
    {
        Env = new Environment
        {
            Account = "XXXXXXXXXXXX",
            Region = "us-west-2",
        }
    }, snsStack);
}

public class SnsStack : Stack
{
    public readonly ITopic Topic;

    public SnsStack(Construct scope, string id, StackProps props) : base(scope, id, props)
    {
        Topic = new Topic(this, "Topic", new TopicProps { TopicName = PhysicalName.GENERATE_IF_NEEDED, });
    }
}

public class LambdaStack : Stack
{
    public readonly IFunction Function;

    internal LambdaStack(Construct scope, string id, StackProps props, SnsStack snsStack) : base(scope, id, props)
    {
        Function = new PythonFunction(this, "Function", new PythonFunctionProps
        {
            Entry = "src/CdkSnsCrossRegion/Function",
        });

        snsStack.Topic.AddSubscription(new LambdaSubscription(Function));
    }
}

What did you expect to happen?

Expected Region property to be set in AWS::SNS::Subscription resource.

{
  "FunctionTopicD77BEC18": {
    "Type": "AWS::SNS::Subscription",
    "Properties": {
      "Protocol": "lambda",
      "TopicArn": {
        "Fn::Join": [
          "",
          [
            "arn:",
            {
              "Ref": "AWS::Partition"
            },
            ":sns:us-east-1:XXXXXXXXXXXX:snssnstopic2f96c0ab8390dbda1164"
          ]
        ]
      },
      "Endpoint": {
        "Fn::GetAtt": [
          "Function76856677",
          "Arn"
        ]
      },
      **"Region": "us-east-1"**
    },
  },
}

What actually happened?

Region property of AWS::SNS::Subscription is not set.

{
  "FunctionTopicD77BEC18": {
    "Type": "AWS::SNS::Subscription",
    "Properties": {
      "Protocol": "lambda",
      "TopicArn": {
        "Fn::Join": [
          "",
          [
            "arn:",
            {
              "Ref": "AWS::Partition"
            },
            ":sns:us-east-1:XXXXXXXXXXXX:snssnstopic2f96c0ab8390dbda1164"
          ]
        ]
      },
      "Endpoint": {
        "Fn::GetAtt": [
          "Function76856677",
          "Arn"
        ]
      }
    },
  },
}

Environment

• CDK CLI Version : 1.94.1 (build 60d8f91)
• Framework Version: 1.94.1
• Node.js Version: v12.16.3
• OS : macOS 10.13.6
• Language (Version): net5.0

Other

Possibly related:

#3842
#7044

I looked at the source and it seemed fine. Perhaps I'm missing something really obvious or the usage is incorrect?

---

This is 🐛 Bug Report

[dillon-odonovan]

Is this truly a feature request? This functionality is already supported, albeit only through Topic.FromTopicArn.

If I use the workaround of

public class LambdaStack : Stack
{
    public readonly IFunction Function;

    internal LambdaStack(Construct scope, string id, StackProps props, SnsStack snsStack) : base(scope, id, props)
    {
        Function = new PythonFunction(this, "Function", new PythonFunctionProps
        {
            Entry = "src/CdkSnsCrossRegion/Function",
        });

        Topic.FromTopicArn(snsStack.Topic).AddSubscription(new LambdaSubscription(Function));
    }
}

I get the expected output

{
  "Type": "AWS::SNS::Subscription",
  "Properties": {
    "Protocol": "lambda",
    "TopicArn": {
      "Fn::Join": [
        "",
        [
          "arn:",
          {
            "Ref": "AWS::Partition"
          },
          ":sns:us-east-1:XXXXXXXXXXXX:snssnstopic2f96c0ab8390dbda1164"
        ]
      ]
    },
    "Endpoint": {
      "Fn::GetAtt": [
        "Function76856677",
        "Arn"
      ]
    },
    "Region": {
      "Fn::Select": [
        3,
        {
          "Fn::Split": [
            ":",
            {
              "Fn::Join": [
                "",
                [
                  "arn:",
                  {
                    "Ref": "AWS::Partition"
                  },
                  ":sns:us-east-1:XXXXXXXXXXXX:snssnstopic2f96c0ab8390dbda1164"
                ]
              ]
            }
          ]
        }
      ]
    }
  }
}

Looking at the offending method:

aws-cdk/packages/@aws-cdk/aws-sns-subscriptions/lib/lambda.ts

Lines 50 to 56 in aa45ede

	private regionFromArn(topic: sns.ITopic): string | undefined {
	// no need to specify `region` for topics defined within the same stack.
	if (topic instanceof sns.Topic) {
	return undefined;
	}
	return Stack.of(topic).parseArn(topic.topicArn).region;
	}

checking for topic instanceof sns.Topic assumes that the topic was created in the same stack. This seems to be an incorrect assumption to me.

The topic must be of type ITopic, which is implemented by abstract type TopicBase which is extended by concrete types Topic and Import. The method above seems to assume that if you want to create a cross-region subscription, you will always have a topic of concrete type Import via Topic.FromTopicArn().

I'm fairly new to the CDK codebase, but an implementation such as below, seems to be more functionally correct.

private regionFromArn(topic: sns.ITopic): string | undefined {
  if (topic instanceof sns.Topic) {
    // we know topic is not imported

    // get the region of the stack that the topic was created in
    const topicRegion = topic.stack.region;
    // get the region of the stack that the function is being created in
    const functionRegion = this.fn.stack.region;

    // if the regions are not the same, we must provide the region of the topic
    if (topicRegion !== functionRegion) {
      return topicRegion;
    }

    // no need to specify region in subscription if topic is defined within the same region as the function.
    return undefined;
  }
  // if topic is not an instanceof sns.Topic, assume it's an instance of sns.Import
  // and get the region that it was defined/deployed in
  return Stack.of(topic).parseArn(topic.topicArn).region;
}

This way, even if we have a concrete Topic, we check its region to see if it is different from the region of the function. This supports multiple use-case patterns such as importing both the function and the topic, importing either, or neither. It will then appropriately return either the region of the topic, or undefined if the regions are the same.

I don't fully understand how tokens in the CDK work yet, so there may be a preferred alternative implementation, but I did write some unit tests to validate that this change did not break other functionality and does include the Region property for my use case.

I haven't created a PR because there is already one open for this issue, but am willing to do so.

I believe this change would also apply to the other subscription types such as SQS, as I believe this implementation originally came from a feature request for cross-region SNS subscriptions with SQS endpoints as a result of #4917

[MrArnoldPalmer]

@robertd how do you believe this compares to your PR?

The issue that I can see here is the use of the construct in environment-agnostic stacks, where the region will not be known at synth time and will be a tokenized value that can't be compared via '==='. We may be able to use intrinsics to do a bunch of template conditional logic but I'm not sure if that is doable for this case and would require some experimentation.

@dillon-odonovan do you any details about use cases that may not be supported by the fix in the existing PR?

[dillon-odonovan]

Unless I'm missing something, this appears to be the only fix so far in the current PR (where 114 is the only line that changed):

aws-cdk/packages/@aws-cdk/aws-sns/lib/subscription.ts

Lines 108 to 116 in c634295

	new CfnSubscription(this, 'Resource', {
	endpoint: props.endpoint,
	protocol: props.protocol,
	topicArn: props.topic.topicArn,
	rawMessageDelivery: props.rawMessageDelivery,
	filterPolicy: this.filterPolicy,
	region: props.region ?? Stack.of(this).region,
	redrivePolicy: this.buildDeadLetterConfig(this.deadLetterQueue),
	});

To me, this fix doesn't appear to change the behavior of the CfnSubscription. If no Region property is provided, CloudFormation will use the current region that the stack is being deployed in as the default. This appears to just make that CloudFormation default behavior explicit. It doesn't seem to address the use-case I was going after.

@MrArnoldPalmer
I imagine that if the region of the stack is not known at the time of synth, it's not reasonable to expect the subscription to point to the correct region if creating the subscription by passing the object reference of the Topic. In that case, I think it would be expected to use Topic.fromTopicArn and import the topic.

So, with that being said, perhaps the guidance is that whenever a cross-region subscription is desired, the expectation is to import the resource via Topic.fromTopicArn. I think that outcome is fine, it would just be a matter of how to communicate that to the user if they try to create a subscription like in the use-case above.

I looked into what the current behavior would be if given 2 environment-agnostic stacks a subscription was tried to be created in this manner:

Test Case:

test('subscription region property - two stacks where regions are not yet defined', () => {
  const snsStack = new Stack();
  const myTopic = new sns.Topic(snsStack, 'mytopic', {
    topicName: 'MyTopic',
  });

  const subscriptionStack = new Stack();
  const func = new lambda.Function(subscriptionStack, 'MyFunction', {
    code: lambda.Code.fromInline('exports.handler = function(e, c, cb) { return cb() }'),
    handler: 'index.handler',
    runtime: lambda.Runtime.NODEJS_10_X,
  });

  myTopic.addSubscription(new subs.LambdaSubscription(func));

  // note - I only had this be the expectation so that it would fail and I could see the generated CloudFormation
  expect(subscriptionStack).toMatchTemplate({});
});

Result:

● subscription region property - two stacks where regions are not yet defined

    Cannot reference across apps. Consuming and producing stacks must be defined within the same CDK app.

      47 |   // unsupported: stacks from different apps
      48 |   if (producer.node.root !== consumer.node.root) {
    > 49 |     throw new Error('Cannot reference across apps. Consuming and producing stacks must be defined within the same CDK app.');
         |           ^
      50 |   }
      51 |
      52 |   // unsupported: stacks are not in the same environment

      at resolveValue (../core/lib/private/refs.ts:49:11)
      at Object.resolveReferences (../core/lib/private/refs.ts:30:24)
      at Object.prepareApp (../core/lib/private/prepare-app.ts:31:3)
      at Object.synthesize (../core/lib/private/synthesis.ts:24:3)
      at App.synth (../core/lib/stage.ts:188:23)
      at synthesizeApp (../assert/lib/synth-utils.ts:76:15)
      at Function._synthesizeWithNested (../assert/lib/synth-utils.ts:53:22)
      at Object.expect (../assert/lib/expect.ts:9:45)
      at Object.toMatchTemplate (../assert/jest.ts:42:23)
      at __EXTERNAL_MATCHER_TRAP__ (../../../node_modules/expect/build/index.js:342:30)

So I think this matches with what I think expected behavior would be if the stacks are both environment-agnostic and they are used in a cross-reference manner.

To me, we could either raise an error similar to above if the subscription is tried to be created in a cross-reference manner (without importing via fromTopicArn), or we may be able to implement a similar fix to what I outlined above. If we are able to detect the former, then at that point it seems like we have all information to know how to correctly configure the subscription, and perhaps we should just do that.

[MrArnoldPalmer]

So this error is because the two stacks in the test have different App parents. The Stack construct creates its own App if you don't pass one explicitly in unit tests so you can get around that by building and passing an App yourself.

I think I understand why the current change isn't what you were looking for but tell me this is correct.

The current fix is defaulting to the region of the stack that the subscription is in when it should be the region of the topic being subscribed to.

[dillon-odonovan]

Yes, that's correct. The region needs to be relative to the topic not the subscription.

[MrArnoldPalmer]

So I wonder if we can just parse the region out of the topic arn, which we have access to here and always pass it. Or instead of Stack.of(this).region we just Stack.of(props.topic).region. We will have to test with various combinations of environment-agnostic stacks (both, either, none etc) and identify which break, because I have a feeling they will whether silently or with some unrelated errors, and then see if we can catch those early and provide users with appropriate messaging.

[github-actions]

This issue has not received any attention in 1 year. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.