feat(ecs-service-extensions): Subscribe Extension

[upparekh]

---

This PR adds a new service extension, SubscribeExtension. This extension can be added to a service to create SQS Queues which can subscribe to the SNS Topics provided by the user.

It creates a default SQS Queue called eventsQueue . It also supports creation of topic-specific queues and sets up the SNS subscriptions accordingly. The created topic-queue subscriptions can be accessed using subscriptions field of the extension and the default queue for this service can be accessed using the eventsQueue getter method. (This PR does not include autoscaling, will be adding it in a separate PR)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

[gitpod-io]

[nathanpeck]

Looks good overall! I requested a few minor changes. I assume there is another paired Topic extension that needs to go with this, in another PR?

[nathanpeck]

The name of the extension doesn't feel quite right. The naming methodology of CDK favors having nouns for the classes, and then verb methods on those classes. And I think in general we've tried to keep the same methodology for the extensions too. So it should be something like "service.add(noun)"

I'd suggest something like "Service.add(new IncomingQueue())" or something like that. You may have a better name idea as well, but think of nouns and adjectives for the name, rather than verbs.

[upparekh]

Updated the extension name to EventsQueue(), I felt we are already creating a default queue with name eventsQueue for the service so it would be more intuitive to name the extension the same. Let me know if that sounds good!

[nathanpeck]

It might be possible that some folks will not want a dead letter queue. For example if a queue has highly valuable messages going to it, and a message is failing to be processed, they may want to leave it on the queue and fix the code of their queue consuming service, rather than rejecting the message to a dead letter queue. We should make sure dead letter queue is optional

[nathanpeck]

That's a cool one liner!

[iamhopaul123]

Do we need to add more docs so that users can at least have some example showing them how to use this feature?

[upparekh]

Updated the docs with more guidance on using the extension. Will add a full example once the publish part is over.

[iamhopaul123]

I don't get why we need this field. Can't they just specify dead letter queue within eventsQueueProps?

[upparekh]

We decided to drop the queue and dead letter queue props completely as they were only being used to instantiate the SQS Queue and served no other purpose. So now if the user doesn't want a queue to be created with the set defaults they need to provide a custom queue as an input prop. This seems to largely simplify the experience as well.

Do let me know if that makes sense or if you have any suggestions!

[iamhopaul123]

sgtm

[bvtujo]

Is it possible to default to eventsQueueProps and make assumptions for the customer if they don't want to customize?

[upparekh]

How does this sound? The defaults will be the one's set by CF.

[bvtujo]

Can we standardize these variable names with Copilot?

For example, we'll use

COPILOT_QUEUE_URI for the primary queue
COPILOT_TOPIC_QUEUE_URIS for a JSON map: {"service-topic": "https://queue-uri.amazonaws.com"}

COPILOT_SNS_TOPIC_ARNS

Could we do something similar and swap COPILOT for ${PARENTSERVICE}?

[upparekh]

Actually, the environment in CDK is typed in to be a set of key-value pairs of strings. That sort of limits our ability to add the variables exactly like that.

[upparekh]

Update: As the topic-specific queues will be user input, we will be removing the addition of these queueURLs as environment variables. The only queue possibly created by us is the default eventsQueue, and that URL will still be added to the environment.

Pull request has been modified.

[upparekh]

I assume there is another paired Topic extension that needs to go with this, in another PR?

Yes, working on it next!

[efekarakus]

Looks great! Mostly a recommendation around adding a new interface so that we can augment the extension with other types of subscriptions.

[efekarakus]

nit: to illustrate how simple it is to get started, what do you think of updating the first example to:

const myServiceDescription = nameDescription.add(new EventsQueue({
  // Provide list of topics the you want the `eventsQueue` to subscribe to
  topicSubscriptions: [{
     topic: new sns.Topic(stack, 'my-topic'),
  }],
}));

[efekarakus]

My typescript foo is weak 😅 since we want to have at least one subscription would it make a different to have the following?

Suggested change

	readonly topicSubscriptions?: TopicSubscriptionProps[];
	readonly topicSubscriptions: TopicSubscriptionProps[];

[efekarakus]

I wonder if we can make this even more generic for the future so that it's not only topic subscriptions that we accept but also eventbus subscriptions.

We could create a Susbcribable interface that a TopicSubscription class implements, here is a bad skeleton 😛 : https://gist.github.com/efekarakus/78567b5f4aff33434963dc42080be541

[upparekh]

since we want to have at least one subscription would it make a different to have the following?

As this extension is an ‘extension’ 😛 of the current queue processing service (which allows the user to set up a queue and DLQ for their service), I feel we should also support the case where no subscriptions are provided. But if we don’t want to do that then this change would be necessary.

[efekarakus]

Looks good! one more refactor request and then we can

[efekarakus]

From our discussion, what do you think of having subscribe instead take as input a QueueExtension?

public subscribe(extension: QueueExtension) {
   let queue = extension.eventsQueue();
   if (this.queue) {
       queue = this.queue;
   }
   this.topic.addSubscription(new subscription.SqsSubscription(queue));
}

This way, I think the interface becomes a bit clearer. If a TopicSubscription has a queue then we will use it, otherwise it will be the eventsQueue.

[efekarakus]

and this would become:

Suggested change

	const subsQueue = subs.subscribe(this._eventsQueue);
	if (subsQueue) {
	this.subscriptionQueues.push(subsQueue);
	}
	sub.subscribe(this);
	if (sub.queue) {
	this.subscriptionQueues.push(sub.queue);
	}

[upparekh]

The variable subs is of the type ISubscribable so we first need to do a type check before we can access it’s queue, something like this:

if (subs instanceof TopicSubscription) {
    if (subs.queue) {
      this.subscriptionQueues.push(subs.queue);
    }
}

So it was either the type-check here or dealing with a return in the subscribe method. I feel your suggestion makes the interface look neater, I just wanted to confirm if the type-check was a concern.

[mergify]

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
• Commit ID: 833ea33
• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[mergify]

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).