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.

Sign up for GitHub

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): make cluster and vpc optional for higher level constructs #2773

Merged
merged 25 commits into from
Jul 29, 2019
Merged

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

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
7b6dece
feat(ecs): make cluster and vpc optional for higher level constructs
hoegertn Jun 6, 2019
10708f4
chore(ecs): review feedback
hoegertn Jun 6, 2019
f7896ee
chore(ecs): improve commenst after review
hoegertn Jun 7, 2019
da87dd9
Merge branch 'master' into optional-cluster-props
hoegertn Jun 11, 2019
deee9cc
Merge branch 'master' into optional-cluster-props
hoegertn Jun 17, 2019
bbeb602
Merge branch 'master' into optional-cluster-props
hoegertn Jun 17, 2019
b61209d
fix(ecs-patterns): refactor tests after merge
hoegertn Jun 17, 2019
8d73f37
fix(ecs-patterns): whitespace
hoegertn Jun 17, 2019
c6fdd60
fix(ecs-patterns): memory in MB
hoegertn Jun 17, 2019
66ee656
Merge branch 'master' into optional-cluster-props
hoegertn Jun 24, 2019
a213221
fix(ecs): merge conflicts
hoegertn Jun 24, 2019
02f9dd9
Merge branch 'master' into optional-cluster-props
hoegertn Jun 24, 2019
ea2b992
Merge branch 'master' into optional-cluster-props
hoegertn Jun 24, 2019
0f538c5
feat(ecs-patterns): use default cluster per stack
hoegertn Jun 25, 2019
493fdec
fix(ecs-patterns): tests
hoegertn Jun 25, 2019
23282d0
feat(ecs-patterns): add magic id
hoegertn Jun 25, 2019
d11fd6c
Merge branch 'master' into optional-cluster-props
hoegertn Jun 25, 2019
50510ec
Merge branch 'master' into optional-cluster-props
hoegertn Jun 27, 2019
4c6855b
Merge branch 'master' into optional-cluster-props
hoegertn Jun 30, 2019
d60c06c
Merge branch 'master' into optional-cluster-props
hoegertn Jun 30, 2019
d0bd65a
Merge remote-tracking branch 'origin/master' into optional-cluster-props
hoegertn Jul 9, 2019
e5ef6de
chore: PR review
hoegertn Jul 9, 2019
a892832
Merge branch 'master' into optional-cluster-props
Jul 23, 2019
ce23026
chore: add README
hoegertn Jul 23, 2019
0498958
Merge branch 'master' into optional-cluster-props
piradeepk Jul 29, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 24 additions & 2 deletions packages/@aws-cdk/aws-ecs-patterns/lib/base/load-balanced-service-base.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { ICertificate } from '@aws-cdk/aws-certificatemanager';
import ec2 = require('@aws-cdk/aws-ec2');
import ecs = require('@aws-cdk/aws-ecs');
import elbv2 = require('@aws-cdk/aws-elasticloadbalancingv2');
import { AddressRecordTarget, ARecord, IHostedZone } from '@aws-cdk/aws-route53';
Expand All @@ -16,8 +17,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; if you do not specify a cluster nor a vpc, a new VPC will be created for you as well
*/
readonly cluster?: ecs.ICluster;

/**
* 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
*/
readonly cluster: ecs.ICluster;
readonly vpc?: ec2.IVpc;

/**
* The image to start.
Expand Down Expand Up @@ -108,11 +120,21 @@ export abstract class LoadBalancedServiceBase extends cdk.Construct {

public readonly targetGroup: elbv2.ApplicationTargetGroup | elbv2.NetworkTargetGroup;

public readonly cluster: ecs.ICluster;

public readonly logDriver?: ecs.LogDriver;

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

if (props.cluster && props.vpc) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move this logic to LoadBalancedFargateService as discussed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

The base class is abstract, isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@piradeepk piradeepk Jul 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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

throw new Error(`You can only specify either vpc or cluster. Alternatively, you can leave both blank`);
} else if (props.cluster) {
hoegertn marked this conversation as resolved.
Show resolved Hide resolved
this.cluster = props.cluster;
} else {
this.cluster = new ecs.Cluster(this, 'Cluster', { vpc: props.vpc });
Copy link
Contributor

@piradeepk piradeepk Jun 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@clareliguori clareliguori Jun 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}

// Create log driver if logging is enabled
const enableLogging = props.enableLogging !== undefined ? props.enableLogging : true;
this.logDriver = enableLogging ? this.createAWSLogDriver(this.node.id) : undefined;
Expand All @@ -127,7 +149,7 @@ export abstract class LoadBalancedServiceBase extends cdk.Construct {
const internetFacing = props.publicLoadBalancer !== undefined ? props.publicLoadBalancer : true;

const lbProps = {
vpc: props.cluster.vpc,
vpc: this.cluster.vpc,
internetFacing
};

Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-ecs-patterns/lib/ecs/load-balanced-ecs-service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ export class LoadBalancedEc2Service extends LoadBalancedServiceBase {

const assignPublicIp = props.publicTasks !== undefined ? props.publicTasks : false;
const service = new ecs.Ec2Service(this, "Service", {
cluster: props.cluster,
cluster: this.cluster,
desiredCount: props.desiredCount || 1,
taskDefinition,
assignPublicIp
Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-ecs-patterns/lib/fargate/load-balanced-fargate-service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ export class LoadBalancedFargateService extends LoadBalancedServiceBase {
});
const assignPublicIp = props.publicTasks !== undefined ? props.publicTasks : false;
const service = new ecs.FargateService(this, "Service", {
cluster: props.cluster,
cluster: this.cluster,
desiredCount: props.desiredCount || 1,
taskDefinition,
assignPublicIp,
Expand Down
17 changes: 17 additions & 0 deletions packages/@aws-cdk/aws-ecs-patterns/test/ec2/test.l3s.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,23 @@ export = {
test.done();
},

'setting vpc and cluster throws error'(test: Test) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

// GIVEN
const stack = new cdk.Stack();
const vpc = new ec2.Vpc(stack, 'VPC');
const cluster = new ecs.Cluster(stack, 'Cluster', { vpc });

// WHEN
test.throws(() => new ecsPatterns.LoadBalancedEc2Service(stack, 'Service', {
cluster,
vpc,
loadBalancerType: ecsPatterns.LoadBalancerType.NETWORK,
image: ecs.ContainerImage.fromRegistry("/aws/aws-example-app")
}));

test.done();
},

'test ECS loadbalanced construct with memoryReservationMiB'(test: Test) {
// GIVEN
const stack = new cdk.Stack();
Expand Down
Loading