feat(ecs): make cluster and vpc optional for higher level constructs #2773

hoegertn · 2019-06-06T14:54:13Z

Pull Request Checklist

Testing
- Unit test added (prefer not to modify an existing test, otherwise, it's probably a breaking change)
- CLI change?: coordinate update of integration tests with team
- cdk-init template change?: coordinated update of integration tests with team
Docs
- jsdocs: All public APIs documented
- README: README and/or documentation topic updated
- Design: For significant features, design document added to design folder
Title and Description
- Change type: title prefixed with fix, feat and module name in parens, which will appear in changelog
- Title: use lower-case and doesn't end with a period
- Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
- Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
Sensitive Modules (requires 2 PR approvers)
- IAM Policy Document (in @aws-cdk/aws-iam)
- EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
- Grant APIs (only if not based on official documentation with a reference)

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

jogold · 2019-06-06T15:07:46Z

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

@@ -65,12 +68,16 @@ export class Cluster extends Resource implements ICluster {
   */
  private _hasEc2Capacity: boolean = false;

-  constructor(scope: Construct, id: string, props: ClusterProps) {
+  constructor(scope: Construct, id: string, props?: ClusterProps) {


Suggested change

constructor(scope: Construct, id: string, props?: ClusterProps) {

constructor(scope: Construct, id: string, props: ClusterProps = {}) {

jogold · 2019-06-07T07:26:15Z

If I might suggest something here: I think that you need some kind of "singleton" VPC and "singleton" cluster (less important but also nice) otherwise you are going to create as many VPCs as services in your stack, no?

hoegertn · 2019-06-07T07:39:34Z

I see your idea, but I think that is exactly why you can provide the VPC as a prop. If you do not care about VPC (not specify one), you do not care about the number. You can still use the vpc property of the first service for the other ones.

eladb · 2019-06-07T08:26:16Z

@jogold I really like this idea generally for VPC. Seems like a recurring pattern. Can you raise an issue and let's explore the design/API for this? I have some ideas.

In the meantime, this PR is great.

eladb

LGTM, @rix0rrr @SoManyHs please review

eladb · 2019-06-07T08:27:44Z

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

@@ -11,8 +13,19 @@ export enum LoadBalancerType {
 export interface LoadBalancedServiceBaseProps {
  /**
   * The cluster where your service will be deployed
+   * You can only specify either vpc or cluster. Alternatively, you can leave both blank
+   *
+   * @default create a new cluster


Set the default "value" as -:

@default - create a new cluster

Also, mentioned that the cluster will be created with a default VPC.

eladb · 2019-06-07T08:29:14Z

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

+   * VPC that the cluster instances or tasks are running in
+   * You can only specify either vpc or cluster. Alternatively, you can leave both blank
+   *
+   * @default use vpc of cluster or create a new one


eladb · 2019-06-07T08:29:41Z

packages/@aws-cdk/aws-ecs-patterns/test/fargate/integ.l3-autocreate.ts

+const app = new cdk.App();
+const stack = new cdk.Stack(app, 'aws-ecs-integ');
+
+new ecsPatterns.LoadBalancedFargateService(stack, 'L3', {


nice is nice!

eladb · 2019-06-07T08:30:12Z

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

@@ -20,8 +21,10 @@ export interface ClusterProps {

  /**
   * The VPC where your ECS instances will be running or your ENIs will be deployed
+   *
+   * @default creates a new VPC


mention that maxAZ setting

piradeepk · 2019-06-07T23:25:36Z

@eladb please hold off merging this in as there's an ongoing discussion about cluster/vpc/network constructs. @clareliguori @uttarasridhar

eladb

Holding this off temporarily. I will follow up shortly.

hoegertn · 2019-06-12T17:43:15Z

Any info if this will continue or if I should change something?

eladb · 2019-06-17T11:57:27Z

Stay tuned. I am on it.

hoegertn · 2019-06-24T06:47:28Z

@eladb Can you elaborate why this is on hold and #3022 is created in the meantime?

piradeepk · 2019-06-24T17:11:38Z

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

+    } else if (props.cluster) {
+      this.cluster = props.cluster;
+    } else {
+      this.cluster = new ecs.Cluster(this, 'Cluster', { vpc: props.vpc });


I like what you're doing here, but this would better fit in the high level FargateService constructs, not in the BaseService constructs. Cluster should be mandatory for any high level Ec2Service constructs as you'll need to add capacity to the cluster. What you have would work for FargateService constructs, but not Ec2Service constructs.

Also, can you use the default cluster here? If one exists use it, otherwise create the default cluster?

+1 that this makes much more sense for Fargate then for EC2, so I rather optimize for this use case.

As for default cluster, I am not aware that we have a concept like that, but should be fairly easy to introduce:

function getCreateDefaultCluster(scope: Construct, vpc?: ec2.Vpc): ecs.Cluster { const DEFAULT_CLUSTER_ID = 'EcsDefaultCluster<plug-in-some-uuid>'; const stack = Stack.of(this); return stack.tryGetChild(id) as ecs.Cluster || new ecs.Cluster(stack, id, { vpc }); }

As the properties are set and only available in the base I added this code here. I am open to moving this to the FargateService.

As I understood it, the concept of defaults is part of another issue or PR

@eladb ECS has a concept of a cluster literally called 'default'. The console first-run will create it, as will any ECS-optimized AMI-based instances that don't have another cluster name configured. So, many but not all accounts have a 'default' cluster already.

Could we have something like the ImportedVpc and Vpc.fromLookup concepts for ECS clusters? i.e. a way to check if a cluster named 'default' exists in the account/region, and if not, add it to the stack.

+1 that this makes much more sense for Fargate then for EC2, so I rather optimize for this use case.

As for default cluster, I am not aware that we have a concept like that, but should be fairly easy to introduce:

function getCreateDefaultCluster(scope: Construct, vpc?: ec2.Vpc): ecs.Cluster { const DEFAULT_CLUSTER_ID = 'EcsDefaultCluster<plug-in-some-uuid>'; const stack = Stack.of(this); return stack.tryGetChild(id) as ecs.Cluster || new ecs.Cluster(stack, id, { vpc }); }

https://docs.aws.amazon.com/AmazonECS/latest/APIReference/API_CreateCluster.html -

clusterName - The name of your cluster. If you do not specify a name for your cluster, you create a cluster named default. Up to 255 letters (uppercase and lowercase), numbers, and hyphens are allowed.

I fully agree with this. If there is a great way to have this default cluster I am totally fine with it. But I like if CDK is not calling AWS before creating the template.

Since the default cluster has a static name 'default', another option could be to always use the 'default' cluster for the Fargate service if the cluster is not provided. Con: this would require the user to create the 'default' cluster manually in the account/region (through console first-run, etc), otherwise the stack creation will fail due to non-existent cluster.

FWIW, the VPC construct is already describing stuff about the account, like availability zones

We could implement a context provider for ECS clusters, probably worth to do generally. However, for this use case, since the default cluster must be created manually, I'd argue that we can simply define a CDK default cluster per stack in the method I described, as it will seamlessly work for any user, and will not create any conflicts between stacks/apps/users within the same account/region.

Ok, I will implement the default cluster per stack approach. I think one cluster per stack is a reasonable trade-off.

eladb

Looks great

eladb · 2019-06-25T14:05:17Z

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

@@ -198,6 +198,12 @@ export abstract class LoadBalancedServiceBase extends cdk.Construct {
    new cdk.CfnOutput(this, 'LoadBalancerDNS', { value: this.loadBalancer.loadBalancerDnsName });
  }

+  protected getCreateDefaultCluster(scope: cdk.Construct, vpc?: ec2.IVpc): ecs.Cluster {
+    const DEFAULT_CLUSTER_ID = `EcsDefaultCluster${vpc ? vpc.node.id : ''}`;


"salt" the cluster ID with some uuid or something that will prevent people from accidentally defining constructs that will collide with this name, especially if it's EcsDefaultCluster.

Do you mean a variable value or some magic string in the code to prevent collision?

magic string

eladb · 2019-06-25T14:06:22Z

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

  public readonly logDriver?: ecs.LogDriver;

  constructor(scope: cdk.Construct, id: string, props: LoadBalancedServiceBaseProps) {
    super(scope, id);

+    if (props.cluster && props.vpc) {


Please move this logic to LoadBalancedFargateService as discussed.

I am not sure how this should work, as the constructor needs the cluster and I cannot do stuff before calling super...

you are right. OK, let's leave this here, but make sure that for EC2 cluster is required.

And how? The base class does not know about EC2 or Fargate, does it?

Why shouldn't it be possible to do:

const service = new LoadBalanced....; service.cluster.addCapacity(...);

I am open for any hint.

The base class is abstract, isn't it?

This is the same when you provide a cluster but do not add capacity to it ...

@hoegertn is correct. We have other similar cases for “late validation”, like codepipeline (you can create a pipeline with no stages and then add them, but at least one is required). This essentially the only reason we have the “validate” hook.

@hoegertn is correct. We have other similar cases for “late validation”, like codepipeline (you can create a pipeline with no stages and then add them, but at least one is required). This essentially the only reason we have the “validate” hook.

@eladb the intent behind the constructs in ecs-patterns is to provide an opinionated approach to creating ecs services following what is considered best practices. I definitely agree that this works, but by relying on late validation, and not requiring capacity to be an input prop, we're not providing an opinion (allowing the user to decide if they want to add capacity when we know that they need to provide capacity), we're straying away from the intentions of the ecs-patterns module. @SoManyHs thoughts?

I would suggest that AddCapacityOptions (if cluster is not provided) or cluster with capacity already added to it should be required.

While I agree with this opinion I do not see how this PR changes the current behavior as it currently possible to add clusters without capacity. So I think we could have this PR as is and create an extra issue about late vs. instant validation of capacity whether providing a cluster or not. Any thoughts?

I'm okay with this, can you create an issue and add it?

hoegertn · 2019-06-30T12:49:05Z

How do we proceed here? @eladb

piradeepk

@hoegertn Sorry for the delay in reviewing this PR. Overall, I like the approach, thanks for taking the time to separate cluster and vpc!

piradeepk · 2019-07-08T16:58:11Z

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

@@ -82,7 +84,11 @@ export class Cluster extends Resource implements ICluster {
    });
    this.clusterName = this.getResourceNameAttribute(cluster.ref);

-    this.vpc = props.vpc;
+    if (props.vpc) {


Can you change this into a one-line conditional (to maintain consistency with the way that we set properties in the other constructs).

this.vpc = props.vpc !== undefined ? props.vpc : new ec2.Vpc(this, 'Vpc', { maxAZs: 2 });

piradeepk · 2019-07-09T12:54:29Z

packages/@aws-cdk/aws-ecs-patterns/test/ec2/test.l3s.ts

@@ -56,6 +56,23 @@ export = {
    test.done();
  },

+  'setting vpc and cluster throws error'(test: Test) {


Can you also add a test here for providing a vpc directly?

@hoegertn LoadBalancedFargateService needs a vpc or a cluster with vpc specified, but it doesn't actually require capacity, where as a LoadBalancedEc2Service needs capacity which requires a cluster to be present with capacity in it.

In this case, you want cluster to be required and ensure that capacity is added, fail if vpc is provided without capacity.

piradeepk · 2019-07-09T12:59:18Z

packages/@aws-cdk/aws-ecs-patterns/test/fargate/test.load-balanced-fargate-service.ts

@@ -51,11 +51,29 @@ export = {
    test.done();
  },

+  'setting vpc and cluster throws error'(test: Test) {


Can you add a test for the scenario when only vpc is provided and ensure that capacity isn't added?

piradeepk · 2019-07-09T13:02:03Z

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

  public readonly logDriver?: ecs.LogDriver;

  constructor(scope: cdk.Construct, id: string, props: LoadBalancedServiceBaseProps) {
    super(scope, id);

+    if (props.cluster && props.vpc) {


@hoegertn LoadBalancedFargateService needs a vpc or a cluster with vpc specified, but it doesn't actually require capacity, where as a LoadBalancedEc2Service needs capacity which requires a cluster to be present with capacity in it.

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

piradeepk

This looks great @hoegertn! Thanks for taking the time to separate the vpc from within cluster.

@eladb approving on the condition that we create and address the issue regarding clusters with capacity being required for higher level ec2 service constructs in a separate PR.

piradeepk · 2019-07-18T09:44:29Z

@rix0rrr @eladb please review and merge

eladb

Can you please update the README?

hoegertn · 2019-07-23T08:28:16Z

Can you please update the README?

Done.

hoegertn · 2019-07-27T21:10:24Z

@SoManyHs Hi, any chance to get this merged?

piradeepk · 2019-07-29T01:56:15Z

@eladb can we merge this in?

feat(ecs): make cluster and vpc optional for higher level constructs

7b6dece

hoegertn requested review from SoManyHs and a team as code owners June 6, 2019 14:54

jogold reviewed Jun 6, 2019

View reviewed changes

chore(ecs): review feedback

10708f4

eladb approved these changes Jun 7, 2019

View reviewed changes

chore(ecs): improve commenst after review

f7896ee

jogold mentioned this pull request Jun 7, 2019

Analyze singleton VPC #2785

Closed

eladb suggested changes Jun 8, 2019

View reviewed changes

Merge branch 'master' into optional-cluster-props

da87dd9

hoegertn and others added 2 commits June 17, 2019 10:01

Merge branch 'master' into optional-cluster-props

deee9cc

Merge branch 'master' into optional-cluster-props

bbeb602

hoegertn added 3 commits June 17, 2019 15:42

fix(ecs-patterns): refactor tests after merge

b61209d

fix(ecs-patterns): whitespace

8d73f37

fix(ecs-patterns): memory in MB

c6fdd60

eladb mentioned this pull request Jun 24, 2019

fix(aws-ecs): make vpc optional in cluster #3022

Closed

hoegertn added 3 commits June 24, 2019 11:18

Merge branch 'master' into optional-cluster-props

66ee656

fix(ecs): merge conflicts

a213221

Merge branch 'master' into optional-cluster-props

02f9dd9

piradeepk requested changes Jun 24, 2019

View reviewed changes

hoegertn added 3 commits June 25, 2019 01:42

Merge branch 'master' into optional-cluster-props

ea2b992

feat(ecs-patterns): use default cluster per stack

0f538c5

fix(ecs-patterns): tests

493fdec

eladb reviewed Jun 25, 2019

View reviewed changes

hoegertn and others added 4 commits June 25, 2019 19:09

feat(ecs-patterns): add magic id

23282d0

Merge branch 'master' into optional-cluster-props

d11fd6c

Merge branch 'master' into optional-cluster-props

50510ec

Merge branch 'master' into optional-cluster-props

4c6855b

Merge branch 'master' into optional-cluster-props

d60c06c

piradeepk requested changes Jul 9, 2019

View reviewed changes

hoegertn added 2 commits July 9, 2019 15:39

Merge remote-tracking branch 'origin/master' into optional-cluster-props

d0bd65a

chore: PR review

e5ef6de

piradeepk approved these changes Jul 11, 2019

View reviewed changes

Merge branch 'master' into optional-cluster-props

a892832

eladb requested a review from a team as a code owner July 23, 2019 08:11

eladb approved these changes Jul 23, 2019

View reviewed changes

eladb reviewed Jul 23, 2019

View reviewed changes

chore: add README

ce23026

Merge branch 'master' into optional-cluster-props

0498958

eladb merged commit 979f6fd into aws:master Jul 29, 2019

piradeepk mentioned this pull request Aug 2, 2019

ecs-patterns: make "cluster" optional in "LoadBalancedFargateService" #2754

Closed

hoegertn deleted the optional-cluster-props branch September 27, 2019 07:09

	constructor(scope: Construct, id: string, props?: ClusterProps) {
	constructor(scope: Construct, id: string, props: ClusterProps = {}) {

feat(ecs): make cluster and vpc optional for higher level constructs #2773

feat(ecs): make cluster and vpc optional for higher level constructs #2773

Conversation

hoegertn commented Jun 6, 2019

Pull Request Checklist

Choose a reason for hiding this comment

jogold commented Jun 7, 2019

hoegertn commented Jun 7, 2019

eladb commented Jun 7, 2019

eladb 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

piradeepk commented Jun 7, 2019

eladb left a comment

Choose a reason for hiding this comment

hoegertn commented Jun 12, 2019

eladb commented Jun 17, 2019

hoegertn commented Jun 24, 2019 • edited Loading

piradeepk Jun 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clareliguori Jun 24, 2019 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eladb 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piradeepk Jul 11, 2019 • edited Loading

Choose a reason for hiding this comment

hoegertn commented Jun 30, 2019

piradeepk 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

piradeepk Jul 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piradeepk left a comment

Choose a reason for hiding this comment

piradeepk commented Jul 18, 2019

eladb left a comment

Choose a reason for hiding this comment

hoegertn commented Jul 23, 2019

hoegertn commented Jul 27, 2019

piradeepk commented Jul 29, 2019

hoegertn commented Jun 24, 2019 •

edited

Loading

piradeepk Jun 24, 2019 •

edited

Loading

clareliguori Jun 24, 2019 •

edited

Loading

piradeepk Jul 11, 2019 •

edited

Loading

piradeepk Jul 9, 2019 •

edited

Loading