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.

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

sns: topic.addSubscription should create the subscription on the consumer's stack to avoid cycles #3064

Closed
1 task done
eladb opened this issue Jun 25, 2019 · 2 comments · Fixed by #3065 or MechanicalRock/tech-radar#14 · May be fixed by MechanicalRock/cdk-constructs#5, MechanicalRock/cdk-constructs#6 or MechanicalRock/cdk-constructs#7
Labels
@aws-cdk/aws-sns Related to Amazon Simple Notification Service bug This issue is a bug. p0

Comments

@eladb
Copy link
Contributor

eladb commented Jun 25, 2019

Note: for support questions, please first reference our documentation, then use Stackoverflow. This repository's issues are intended for feature requests and bug reports.

  • I'm submitting a ...

    • 🪲 bug report
  • What is the current behavior?
    If the current behavior is a 🪲bug🪲: Please provide the steps to reproduce

topic.addSubscription currently creates the SNS::Subscription resource in the scope of the SNS topic (this), but since in most cases the consumer will need to reference the topic in their e.g. resource policy, if the topic and the subscriber are from different stacks, that will surely create a cyclic reference.

  • What is the expected behavior (or behavior of feature suggested)?

TopicSubscriptionConfig should return the scope in which the subscription should be created and addSubscription should use that instead of this when creating a Subscription object.

  • What is the motivation / use case for changing the behavior or adding this feature?

Avoid cyclic references.

  • Please tell us about your environment:

    • CDK CLI Version: 0.36.0
    • Module Version: 0.36.0
    • OS: all
    • Language: all
eladb pushed a commit to eladb/grapl that referenced this issue Jun 25, 2019
To avoid cyclic references [1], we need to create
SNS subscriptions on the conuming stack instead of the
topic's stack.

This change implements a workaround to the issue.

aws/aws-cdk#3064
@eladb
Copy link
Contributor Author

eladb commented Jun 25, 2019

Workaround is to create an sns.Subscription manually (not through topic.addSubscription):

function addSubscription(scope, topic, subscription) {
    const config = subscription.bind(topic);

    new sns.Subscription(scope, 'Subscription', {
        topic: topic,
        endpoint: config.endpoint,
        filterPolicy: config.filterPolicy,
        protocol: config.protocol,
        rawMessageDelivery: config.rawMessageDelivery
    });
}

@eladb
Copy link
Contributor Author

eladb commented Jun 25, 2019

See grapl-security/grapl#25

eladb pushed a commit that referenced this issue Jun 25, 2019
Since in most cases the consumer needs to reference the topic to
permit it to send them messages (e.g. invoke a lambda function or send
messages to the queue), it makes much more send to create the SNS subscription
resource on the consumer's scope/stack instead of the topic's.

This change adds an optional `scope` field to `TopicSubscriptionConfig` which
is respected by `topic.addSubscription`. If `scope` is not defined, the topic's
scope will be used. We also changed `subscriberId` to be optional, since in the
case where `scope` is specified, the natural ID for the subscription construct
would be the topic's unique ID, which is now the default. A runtime error will 
be emitted if both `scope` and `subscriberId` are not provided.

Fixes #3064
@NGL321 NGL321 added bug This issue is a bug. @aws-cdk/aws-sns Related to Amazon Simple Notification Service labels Jun 25, 2019
@fulghum fulghum added the p0 label Jun 26, 2019
eladb pushed a commit that referenced this issue Jun 27, 2019
Since in most cases the consumer needs to reference the topic to
permit it to send them messages (e.g. invoke a lambda function or send
messages to the queue), it makes much more send to create the SNS subscription
resource on the consumer's scope/stack instead of the topic's.

This change adds an optional scope field to TopicSubscriptionConfig which
is respected by topic.addSubscription. If scope is not defined, the topic's
scope will be used. We also changed subscriberId to be optional, since in the
case where scope is specified, the natural ID for the subscription construct
would be the topic's unique ID, which is now the default. A runtime error will
be emitted if both scope and subscriberId are not provided.

Fixes #3064
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment