feat(ecs-service-extensions): Publish Extension #16326

upparekh · 2021-09-01T17:09:59Z

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

gitpod-io · 2021-09-01T17:10:01Z

packages/@aws-cdk-containers/ecs-service-extensions/README.md

bvtujo · 2021-09-03T15:34:21Z

Looks really good! Just had some docs nits that you can address or not as you want. Feel free to remove the label and get it merged.

Pull request has been modified.

packages/@aws-cdk-containers/ecs-service-extensions/README.md

iamhopaul123 · 2021-09-03T20:26:09Z

packages/@aws-cdk-containers/ecs-service-extensions/lib/extensions/publisher.ts

+export interface IPublisher {
+  publish(taskDefinition: ecs.TaskDefinition): void;
+
+  envVarKey(): string;
+
+  envVarValue(): string;
+}


This interface doesn't look quite right to me...It's difficult to related envVarKey and envVarValue with IPublisher

Yes I agree, these aren't related to the actual 'publish' functionality that we want to provide the users. But if these aren't added to the interface, we will end up doing instanceof type checks later in the code and we wanted to avoid such logic and find the most common interface. That's why envVarKey and envVarValue were added to the interface itself.

iamhopaul123 · 2021-09-03T20:34:40Z

packages/@aws-cdk-containers/ecs-service-extensions/lib/extensions/publisher.ts

+  public publish(taskDefinition: ecs.TaskDefinition) {
+    this.topic.grantPublish(taskDefinition.taskRole);
+  }
+
+  public envVarKey(): string {
+    return this.topic.node.id;
+  }
+
+  public envVarValue(): string {
+    return this.topic.topicArn;
+  }
+}


hmm these function only contain one line logic. I wonder if we want this extension since it doesn't seem to bring a lot of convenience for users. Could you remind me how users are going to publish to a topic without this extension, so that we can better evaluate the value of bring this extra public extension 🙏

This extension helps set up the publish permissions for SNS Topics and optionally restrict subscribe permissions to particular accounts. The simplest use case for setting up the publish permissions can be done by calling topic.grantPublish(taskDefinition.taskRole) .

This extension might seem to be more useful after we add more IPublisher resources perhaps? (like publishing to an EventBus)

efekarakus · 2021-09-14T20:59:27Z

packages/@aws-cdk-containers/ecs-service-extensions/lib/extensions/publisher.ts

+/**
+ * An interface that will be implemented by all the resources that can be published events to.
+ */
+export interface IPublisher {
+  publish(taskDefinition: ecs.TaskDefinition): void;
+
+  environmentVar(): { [key: string]: string };
+}


You know I think this is a neat interface, but it doesn't have to do much with being a publisher. For example, adding a DynamoDB table as an extension could be done by implementing this interface as well.

I see @iamhopaul123 's point (#16326 (comment)), what do you think of renaming this extension and interface like this?

export interface Injectable { environmentVariables(): { [key: string]: string }; } export interface GrantInjectable extends Injectable { grant(taskDefinition: ecs.TaskDefinition): void; }

SNS topics can implement this interface, S3 buckets, or DynamoDB tables for example.
In the future, to add RDS for example as an injected we can create another interface ConnectInjectable.

The name of of the extension will be: InjecterExtension.

and it will take as input injectables: []Injectable
Then we can loop through each injectable and do this:

for (const injectable of injectables) { if (injectable as GrantInjectable).grant !== undefined { injectable.grant(taskDef) } merge(this.environment, injectable.environmentVariables()) }

Let me know what y'all think!

That makes a lot of sense to me! I think overall the concern here is: when we design a new interface, we need to think it as an interface instead of a concrete class.

SNS topics can implement this interface, S3 buckets, or DynamoDB tables for example.
In the future, to add RDS for example as an injected we can create another interface ConnectInjectable.

This sounds super cool ❤️

efekarakus

Looks great! Just some doc recommendations and a small request for the env variables 🚀

efekarakus · 2021-09-16T16:26:28Z

packages/@aws-cdk-containers/ecs-service-extensions/README.md

@@ -19,7 +19,8 @@ The `Service` construct provided by this module can be extended with optional `S
 - [AWS AppMesh](https://aws.amazon.com/app-mesh/) for adding your application to a service mesh
 - [Application Load Balancer](https://docs.aws.amazon.com/elasticloadbalancing/latest/application/introduction.html), for exposing your service to the public
 - [AWS FireLens](https://docs.aws.amazon.com/AmazonECS/latest/developerguide/using_firelens.html), for filtering and routing application logs
- Queue to allow your service to consume messages from an SQS Queue which is populated by one or more SNS Topics that it is subscribed to
+- [Injecter Extension](#injecter-extension), for allowing your service to publish events to resources like SNS Topics


nit:

Suggested change

- [Injecter Extension](#injecter-extension), for allowing your service to publish events to resources like SNS Topics

- [Injecter Extension](#injecter-extension), for allowing your service to connect to other AWS services by granting permission and injecting environment variables.

efekarakus · 2021-09-16T16:33:56Z

packages/@aws-cdk-containers/ecs-service-extensions/README.md

@@ -322,9 +323,26 @@ const environment = Environment.fromEnvironmentAttributes(stack, 'Environment',

 ```

+## Injecter Extension
+
+This service extension accepts a list of `Injectable` resources that the service can publish events or write data to and sets up the corresponding permissions for the service.


I wanna avoid saying publish events specifically in the first sentence. The Injectable interface can be implemented by various resource. But we can give an example with InjectableTopic.

Suggested change

This service extension accepts a list of `Injectable` resources that the service can publish events or write data to and sets up the corresponding permissions for the service.

This service extension accepts a list of `Injectable` resources that grants access to the resources and adds the necessary environment variables to the tasks part of the service.

For example, an `InjectableTopic` is an SNS topic that grants permission to the task role and adds the topic name as an environment variable to the task definition.

feel free to change the wording!

efekarakus · 2021-09-16T16:35:33Z

packages/@aws-cdk-containers/ecs-service-extensions/README.md

+
+The following example adds the `InjecterExtension` to a `Publisher` Service which can publish events to an SNS Topic and adds the `QueueExtension` to a `Worker` Service which can poll its `eventsQueue` to consume messages populated by the topic.
+
+```ts


Awesome example! 🤩

efekarakus · 2021-09-16T16:41:49Z

packages/@aws-cdk-containers/ecs-service-extensions/lib/extensions/injecter.ts

+
+    for (const injectable of this.props.injectables) {
+      for (const [key, val] of Object.entries(injectable.environmentVariables())) {
+        this.environment[`${service.id.toUpperCase()}_${key}`] = val;


so will this create an environment variable like:

PUBLISHER_MYTOPIC_TOPIC_ARN

Was there a reason why you prefered to prefix with the service.id? I think customers that implement the Injectable interface might expect their environment variable to appear as is 💭

Yes, it creates PUBLISHER_MYTOPIC_TOPIC_ARN .

The service.id was added as a prefix to distinguish between topics in cases of collision, when two topics with same name are created in different services.

gotcha, I think since this works a bit different than copi where users create their own topics, I think we should pass the env var as is!

Makes sense!

mergify · 2021-09-16T18:46:46Z

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

Pull request has been modified.

mergify · 2021-09-17T16:22:13Z

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 · 2021-09-17T17:11:35Z

AWS CodeBuild CI Report

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

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

mergify · 2021-09-17T17:12:05Z

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 self-assigned this Sep 1, 2021

upparekh added the @aws-cdk-containers/ecs-service-extensions Related to ecs-service-extensions package label Sep 1, 2021

upparekh requested review from nathanpeck, bvtujo, efekarakus and iamhopaul123 September 1, 2021 18:11

upparekh force-pushed the upparekh/publish-service-extension branch 2 times, most recently from 25a6359 to df0d027 Compare September 1, 2021 21:54

bvtujo previously approved these changes Sep 3, 2021

View reviewed changes

bvtujo added the pr/do-not-merge This PR should not be merged at this time. label Sep 3, 2021

upparekh removed the pr/do-not-merge This PR should not be merged at this time. label Sep 3, 2021

iamhopaul123 reviewed Sep 3, 2021

View reviewed changes

efekarakus reviewed Sep 14, 2021

View reviewed changes

efekarakus reviewed Sep 16, 2021

View reviewed changes

efekarakus previously approved these changes Sep 16, 2021

View reviewed changes

upparekh added 11 commits September 16, 2021 14:19

Added publish extension

0bc9a8e

IPublisher methods for accessing env vars

c221a10

README update

3a7f64f

Updated env var key

05ed02f

Added integ test

ca7378e

Updated README

f7fc4f5

README updates after review

dc37e37

README update after review

f5f9994

Updated env var injection

03d3ea7

Removed account IDs

cb9db5e

Refactor for adding Injectable interfaces

e4d50d7

upparekh added 4 commits September 16, 2021 14:19

name update: InjectableTopic

b0c5523

Updated README

dba8b7f

Updated env var names

fa7d351

Test refactor for failing build

8e92b07

upparekh force-pushed the upparekh/publish-service-extension branch from 6dac16d to 8e92b07 Compare September 17, 2021 00:18

Deleted test file

a6eb709

efekarakus approved these changes Sep 17, 2021

View reviewed changes

Merge branch 'master' into upparekh/publish-service-extension

e76126c

mergify bot merged commit c6c5941 into aws:master Sep 17, 2021

upparekh deleted the upparekh/publish-service-extension branch September 17, 2021 17:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ecs-service-extensions): Publish Extension #16326

feat(ecs-service-extensions): Publish Extension #16326

upparekh commented Sep 1, 2021

gitpod-io bot commented Sep 1, 2021

bvtujo commented Sep 3, 2021

iamhopaul123 Sep 3, 2021

upparekh Sep 7, 2021

iamhopaul123 Sep 3, 2021

upparekh Sep 7, 2021

efekarakus Sep 14, 2021

efekarakus Sep 14, 2021

efekarakus Sep 14, 2021

iamhopaul123 Sep 14, 2021

upparekh Sep 15, 2021 •

edited

Loading

efekarakus left a comment

efekarakus Sep 16, 2021

efekarakus Sep 16, 2021

efekarakus Sep 16, 2021

efekarakus Sep 16, 2021

upparekh Sep 16, 2021

efekarakus Sep 16, 2021

upparekh Sep 16, 2021

mergify bot commented Sep 16, 2021

mergify bot commented Sep 17, 2021

aws-cdk-automation commented Sep 17, 2021

mergify bot commented Sep 17, 2021

	- [Injecter Extension](#injecter-extension), for allowing your service to publish events to resources like SNS Topics
	- [Injecter Extension](#injecter-extension), for allowing your service to connect to other AWS services by granting permission and injecting environment variables.

-This service extension accepts a list of `Injectable` resources that the service can publish events or write data to and sets up the corresponding permissions for the service.
+This service extension accepts a list of `Injectable` resources that grants access to the resources and adds the necessary environment variables to the tasks part of the service.
+For example, an `InjectableTopic` is an SNS topic that grants permission to the task role and adds the topic name as an environment variable to the task definition.


		The following example adds the `InjecterExtension` to a `Publisher` Service which can publish events to an SNS Topic and adds the `QueueExtension` to a `Worker` Service which can poll its `eventsQueue` to consume messages populated by the topic.

		```ts

feat(ecs-service-extensions): Publish Extension #16326

feat(ecs-service-extensions): Publish Extension #16326

Conversation

upparekh commented Sep 1, 2021

gitpod-io bot commented Sep 1, 2021

bvtujo commented Sep 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

upparekh Sep 15, 2021 • edited Loading

Choose a reason for hiding this comment

efekarakus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Sep 16, 2021

mergify bot commented Sep 17, 2021

aws-cdk-automation commented Sep 17, 2021

AWS CodeBuild CI Report

mergify bot commented Sep 17, 2021

upparekh Sep 15, 2021 •

edited

Loading