feat: construct library for ECS #1041

rix0rrr · 2018-10-30T15:54:48Z

Add an initial version of a construct library for AWS Elastic Container Service.

This construct library isn't fully feature-complete yet. This is the first iteration so
that people can use it for experimentation and put it to the test, and we can improve
on it later.

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

- Fix linting errors - VPC is now an argument and not created inside the cluster - Link the AutoScalingGroup up to the cluster via userData - Give the ASG instance role the required IAM permissions - Extend ECS-optimized AMI table with all regions - Add an example configuration option to deny containers access to EC2 instance metadata service.

Was getting error: lib/index.ts(7,1): error TS2308: Module './cluster' has already exported a member named 'ClusterName'. Consider explicitly re-exporting to resolve the ambiguity. lib/index.ts(7,1): error TS2308: Module './task-definition' has already exported a member named 'TaskDefinitionArn'. Consider explicitly re-exporting to resolve the ambiguity. This seemed to make npm run build happy

Sets maxSize and desiredCapabity on ASG for ECS Cluster. NOTE: Should we set minSize to 0 or 1? See ecs-cli default)

This uses the @aws-cdk/aws-applicationautoscaling library to add Task AutoScaling facilities to ECS and Fargate services.

TaskExecutionRole not being set -- lazy evaluation of executionRole on TaskDef not working on private call to generateExecutionRole? Also weird issue with importing aws-iam

Add default deployment configuration

clareliguori · 2018-10-30T17:00:18Z

examples/cdk-examples-typescript/hello-cdk-ecs/cdk.json

+{
+  "app": "node index",
+  "context": {
+    "availability-zones:585695036304:us-east-1": [


Is it normal practice in the CDK to check in account IDs? Normally we avoid publicly sharing account IDs...

Nope. We should definitely delete

Yeah this should not have been checked in.

Do we expect customers to check this in to their repos, or should cdk.json generally be git'ignored? Should cdk init add that to a .gitignore file? Sounds like we need a separate issue to track that

fyi this stuff is online for all to see

clareliguori

Yay! Looking good, not much jumped out at me that has to be changed before merging. Most of my comments can be addressed in follow-up PRs

clareliguori · 2018-10-31T02:20:05Z

examples/cdk-examples-typescript/hello-cdk-ecs-declarative/cdk.json

@@ -0,0 +1,3 @@
+{
+    "app": "../node_modules/.bin/cdk-applet-js fargate-service.yml"


How do I get this magical cdk-applet-js file, because it is not appearing for me locally even after a build

packages/@aws-cdk/aws-ecs/README.md

clareliguori · 2018-10-31T02:49:22Z

packages/@aws-cdk/aws-ecs/lib/base/base-service.ts

+  /**
+   * Time after startup to ignore unhealthy load balancer checks.
+   *
+   * @default ??? FIXME


@nathanpeck any guidance here on a sane default?

I think this should maybe be removed from the service properties and only added if the customer specifies a loadBalancer.

clareliguori · 2018-10-31T03:25:52Z

packages/@aws-cdk/aws-ecs/lib/container-image.ts

+  /**
+   * Called when the image is used by a ContainerDefinition
+   */
+  public abstract bind(containerDefinition: ContainerDefinition): void;


What is the value of this method? It doesn't actually seem to bind to this specific image object; it just sets a flag back in the containerDefinition?

packages/@aws-cdk/aws-ecs/lib/container-image.ts

clareliguori · 2018-10-31T03:28:14Z

packages/@aws-cdk/aws-ecs/lib/ecs/ecs-cluster.ts

+    this.securityGroup = autoScalingGroup.connections.securityGroup!;
+
+    // Tie instances to cluster
+    autoScalingGroup.addUserData(`echo ECS_CLUSTER=${this.clusterName} >> /etc/ecs/ecs.config`);


This is so niiiiiiice

clareliguori · 2018-10-31T03:32:02Z

packages/@aws-cdk/aws-ecs/lib/ecs/ecs-cluster.ts

+    if (!props.containersAccessInstanceRole) {
+      // Deny containers access to instance metadata service
+      // Source: https://docs.aws.amazon.com/AmazonECS/latest/developerguide/instance_IAM_role.html
+      autoScalingGroup.addUserData('sudo iptables --insert FORWARD 1 --in-interface docker+ --destination 169.254.169.254/32 --jump DROP');


For awsvpc mode (which doesn't use docker0), you should also set ECS_AWSVPC_BLOCK_IMDS=true
https://docs.aws.amazon.com/AmazonECS/latest/developerguide/task-iam-roles.html

clareliguori · 2018-10-31T03:32:38Z

packages/@aws-cdk/aws-ecs/lib/ecs/ecs-cluster.ts

+      "ecr:BatchGetImage",
+      "logs:CreateLogStream",
+      "logs:PutLogEvents"
+    ).addAllResources()); // Conceivably we might do better than all resources and add targeted ARNs


+1 on the comment

PaulMaddox · 2018-10-31T04:18:44Z

packages/@aws-cdk/aws-ecs/README.md

+
+```ts
+// Create an ECS cluster (backed by an AutoScaling group)
+const cluster = new ecs.EcsCluster(this, 'Cluster', {


Do we need two separate constructs for ecs.EcsCluster and ecs.FargateCluster? I think this introduces some naming complexity (especially later down the line when we have Fargate for EKS). By having them so separate, it also hides the fact that customers can (and do) have mixed-mode clusters with some EC2 and some Fargate mode tasks.

The only real difference is how many instances (if any) are configured. Why not have a single new ecs.Cluster() construct, that accepts an optional instanceConfiguration prop.

While it's true that we wanted to supported mixed-mode clusters, these are an extremely low percentage of customers. We wanted the construct library to provide a more opinionated way to set up infrastructure. In addition provisioning capacity in an ECS cluster, there is some differences in the networking setup as well.

Unless you know of good reasons why customers wouldn't want to have a homogeneous cluster, keeping separate cluster types seemed to streamline the design of other ECS constructs.

PaulMaddox · 2018-10-31T04:23:22Z

packages/@aws-cdk/aws-ecs/README.md

+const cluster = new ecs.EcsCluster(this, 'Cluster', {
+  vpc,
+  size: 3,
+  instanceType: new InstanceType("t2.xlarge")


instanceCount is more descriptive than size.

I also think this API for just specifying instance type and count is too simplistic, and limits future options. For example, one nice thing we could do as a TODO is allow people to specify multiple instance types, and whether they want to use spot etc. The ECS L2 construct could then abstract away a lot of the complexity of creating this kind of diverse-bidding-strategy setup.

As above, I'd favour an instanceConfiguration type prop that could allow expressing more advanced configurations. We could then provide some sane-default examples of instance configurations that people could drop-in, or they could create their own.

PaulMaddox · 2018-10-31T04:24:54Z

packages/@aws-cdk/aws-ecs/README.md

+
+- Use the `EcsCluster`, `EcsTaskDefinition` and `EcsService` constructs to
+  run tasks on EC2 instances running in your account.
+- Use the `FargateCluster`, `FargateTaskDefinition` and `FargateService`


Again, is this naming future-proof/correct? EcsService vs FargateService is slightly confusing as both run on ECS. Maybe ecs.EC2Service and ecs.FargateService?

PaulMaddox · 2018-10-31T04:25:31Z

packages/@aws-cdk/aws-ecs/README.md

+## Service
+
+A `Service` instantiates a `TaskDefinition` on a `Cluster` a given number of
+times, optionally associating them with a load balnacer. Tasks that fail will


s/load balnacer/load balancer/

PaulMaddox · 2018-10-31T04:27:53Z

packages/@aws-cdk/aws-ecs/README.md

+Task AutoScaling is powered by *Application AutoScaling*. Refer to that for
+more information.
+
+## Instance AutoScaling


Setting up instance-autoscaling with proper draining (via lifecycle hook+lambda) is exactly the sort of undifferentiated heavy lifting that an L2 construct should be doing. From speaking to customers previously, reserved memory is normally a pretty sane default to scale on. For a drop-in example of the ASG hook/lambda function for draining, see https://github.com/aws-samples/ecs-refarch-cloudformation/blob/master/infrastructure/lifecyclehook.yaml

Aha, that is potentially more complicated than I had expected. Might hit you up next week as I'm trying to implement this.

rix0rrr · 2018-10-31T06:47:49Z

Closing in favor of newer one

SoManyHs and others added 30 commits September 28, 2018 11:42

Ignore swp files

939452c

Add ECS lib and example

7302a2f

Add service and task def constructs

4c7127a

WIP adding VPC/ASG

98749ec

Add more props on Service

bddfe56

Add TODO for AMI IDs

3574611

Update example to instantiate service separately

b00e0dd

WIP Add Container Definitions

80ba2e4

WIP Add other Task Def properties

0980f52

Merge remote-tracking branch 'official/master' into ecs-demo

8648540

Indentation 4 -> 2

2d53d89

Fix type errors

dbdab86

Use SSM parameter for ECS Optimized AMI

ee6b427

Move containerdefinition to its own file

2b16dad

Merge branch 'ecs-demo' of github.com:SoManyHs/aws-cdk into ecs-demo

16a92c0

Turning ContainerDefinition into a construct

ae881c2

Add enum for placement constraints

0672e6a

Rename fleet to autoScalingGroup

4f93a5b

Qualify "PlacementConstraintType" usage with package name

9b61b89

ECR can return itself as container image

98bf81b

Automatically generate execution and task roles

ae1d525

Make taskRole public

23072a9

Remove shellCommand from healthCheck

e5cc7f2

Rename toContainerDefinitionJson to renderContainerDefinition

047eed8

Some work on Service

65d3f79

Merge branch 'ecs-demo' of github.com:SoManyHs/aws-cdk into ecs-demo

11541d3

Separate Fargate and ECS clusters

fb17532

Separate ECS and Fargate Service

be8cb86

Rico Huijbers and others added 18 commits October 17, 2018 10:51

Fix build errors

64b6d06

Merge remote-tracking branch 'origin/master' into ecs-demo

af5a2a4

Fix ingressPort/containerPort tests

51097fd

Add LoadBalancedEcsService L3 Construct

7b19dd2

Add ability to specify cluster size

9bea6be

Sets maxSize and desiredCapabity on ASG for ECS Cluster. NOTE: Should we set minSize to 0 or 1? See ecs-cli default)

Merge remote-tracking branch 'origin/master' into ecs-demo

4ab455c

feat(aws-ecs): add Task AutoScaling to Service

be9f447

This uses the @aws-cdk/aws-applicationautoscaling library to add Task AutoScaling facilities to ECS and Fargate services.

Add unit tests for EcsCluster

2eab513

Merge branch 'master' into ecs-demo

57857a9

Unit tests for Cluster

4cf23fc

Set volumes on TaskDefinition

37d3c4e

Clarify doc string for memoryLimit and memoryReservation

1ad3669

WIP Unit tests on EcsTaskDefinition

78d2a49

TaskExecutionRole not being set -- lazy evaluation of executionRole on TaskDef not working on private call to generateExecutionRole? Also weird issue with importing aws-iam

WIP Unit tests for EcsService

ca61a74

Add default deployment configuration

WIP Unit tests for FargateService

98345b3

WIP unit tests Fargate Task Definition

35ebd9c

Merge branch 'huijbers/autoscaling' into ecs-demo

09a182a

Add docs and readme and a couple of tests

2fd2e7c

rix0rrr requested review from RomainMuller and eladb October 30, 2018 15:54

rix0rrr self-assigned this Oct 30, 2018

clareliguori reviewed Oct 30, 2018

View reviewed changes

Unit test for error case on service creation

9825127

clareliguori reviewed Oct 31, 2018

View reviewed changes

PaulMaddox suggested changes Oct 31, 2018

View reviewed changes

rix0rrr closed this Oct 31, 2018

This was referenced Oct 31, 2018

cdk-applet-js not installed on Mac? #342

Closed

feat: construct library for ECS #1058

Merged

NGL321 added the contribution/core This is a PR that came from AWS. label Sep 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: construct library for ECS #1041

feat: construct library for ECS #1041

rix0rrr commented Oct 30, 2018

clareliguori Oct 30, 2018

eladb Oct 30, 2018

rix0rrr Oct 30, 2018

clareliguori Oct 30, 2018

tom-wagner Mar 16, 2020

clareliguori left a comment

clareliguori Oct 31, 2018

clareliguori Oct 31, 2018

SoManyHs Oct 31, 2018

clareliguori Oct 31, 2018

clareliguori Oct 31, 2018

clareliguori Oct 31, 2018

clareliguori Oct 31, 2018

PaulMaddox Oct 31, 2018

SoManyHs Oct 31, 2018

PaulMaddox Oct 31, 2018 •

edited

Loading

PaulMaddox Oct 31, 2018

PaulMaddox Oct 31, 2018

PaulMaddox Oct 31, 2018

rix0rrr Nov 2, 2018

rix0rrr commented Oct 31, 2018

		@@ -0,0 +1,3 @@
		{
		"app": "../node_modules/.bin/cdk-applet-js fargate-service.yml"

feat: construct library for ECS #1041

feat: construct library for ECS #1041

Conversation

rix0rrr commented Oct 30, 2018

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

clareliguori 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PaulMaddox Oct 31, 2018 • edited Loading

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

rix0rrr commented Oct 31, 2018

PaulMaddox Oct 31, 2018 •

edited

Loading