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

feat(ecs-service-extensions): Subscribe Extension #16049

Merged
merged 9 commits into from
Sep 1, 2021

Conversation

upparekh
Copy link
Contributor

@upparekh upparekh commented Aug 13, 2021


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

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@gitpod-io
Copy link

gitpod-io bot commented Aug 13, 2021

@upparekh upparekh added the @aws-cdk-containers/ecs-service-extensions Related to ecs-service-extensions package label Aug 13, 2021
@upparekh upparekh self-assigned this Aug 13, 2021
Copy link
Member

@nathanpeck nathanpeck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

* The created subscriptions can be accessed using `subscriptions` field and the
* default queue for this service can be accessed using the getter `<extension>.eventsQueue`.
*/
export class SubscribeExtension extends ServiceExtension {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

}
}

private createQueues(name: string, queueProps?: sqs.QueueProps, deadLetterQueueProps?: DeadLetterQueueProps) : sqs.IQueue {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return {
...props,

environment: { ...(props.environment || {}), ...this.environment },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a cool one liner!

@@ -327,6 +327,7 @@ We encourage the development of Community Service Extensions that support
advanced features. Here are some useful extensions that we have reviewed:

- [ListenerRulesExtension](https://www.npmjs.com/package/@wheatstalk/ecs-service-extension-listener-rules) for more precise control over Application Load Balancer rules
- SubscribeExtension to allow the service to create SQS Queues to subscribe and consume messages published to SNS Topics
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

*
* @default none
*/
readonly eventsDeadLetterQueueProps?: DeadLetterQueueProps;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@upparekh upparekh Aug 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sgtm

*
* @default none
*/
readonly eventsQueueProps?: sqs.QueueProps;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

} else {
this._eventsQueue = this.createQueues('Events', this.props?.eventsQueueProps, this.props?.eventsDeadLetterQueueProps);
}
this.environment[`${this.parentService.id.toUpperCase()}_EVENTS_QUEUE_URL`] = this._eventsQueue.queueUrl;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}?

Copy link
Contributor Author

@upparekh upparekh Aug 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@mergify mergify bot dismissed nathanpeck’s stale review August 18, 2021 20:33

Pull request has been modified.

@upparekh
Copy link
Contributor Author

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

Yes, working on it next!

Copy link
Contributor

@efekarakus efekarakus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines 330 to 339
const topicSubscription = {
topic: new sns.Topic(stack, 'my-topic'),
};

const myServiceDescription = nameDescription.add(new EventsQueue({
// Provide a custom queue for the service. If a queue is not provided, a default queue is created for the service
eventsQueue: myCustomQueue,
// Provide list of topics the you want the `eventsQueue` to subscribe to
topicSubscriptions: [topicSubscription],
}));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'),
  }],
}));

*
* @default none
*/
readonly topicSubscriptions?: TopicSubscriptionProps[];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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[];

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@efekarakus efekarakus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! one more refactor request and then we can :shipit:

Comment on lines 81 to 88
public subscribe(queue: sqs.IQueue) : sqs.IQueue | undefined {
if (this.queue) {
this.topic.addSubscription(new subscription.SqsSubscription(this.queue));
} else {
this.topic.addSubscription(new subscription.SqsSubscription(queue));
}
return this.queue;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines 175 to 178
const subsQueue = subs.subscribe(this._eventsQueue);
if (subsQueue) {
this.subscriptionQueues.push(subsQueue);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

mergify bot commented Sep 1, 2021

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
Copy link
Collaborator

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 mergify bot merged commit 66baca5 into aws:master Sep 1, 2021
@mergify
Copy link
Contributor

mergify bot commented Sep 1, 2021

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).

@upparekh upparekh deleted the upparekh/pub-sub-extensions branch September 1, 2021 22:08
madeline-k pushed a commit to madeline-k/aws-cdk that referenced this pull request Sep 2, 2021
----

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*
TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Sep 6, 2021
----

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*
david-doyle-as24 pushed a commit to david-doyle-as24/aws-cdk that referenced this pull request Sep 7, 2021
----

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*
mergify bot pushed a commit that referenced this pull request Sep 17, 2021
----
This PR adds a new service extension, `PublisherExtension`. This extension can be added to a service to allow it to publish events to SNS Topics. (This PR when paired with #16049 can be used to set up the pub/ sub architecture pattern)

It sets up publish permissions for the service to be able to publish events to the topics provided. The user can also provide a list of accounts that will be given permissions to subscribe to the given topics.

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk-containers/ecs-service-extensions Related to ecs-service-extensions package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants