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 3 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
27 changes: 25 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,5 +1,7 @@
import { ICertificate } from '@aws-cdk/aws-certificatemanager';
import ec2 = require('@aws-cdk/aws-ec2');
import ecs = require('@aws-cdk/aws-ecs');
import { Cluster } from '@aws-cdk/aws-ecs';
import elbv2 = require('@aws-cdk/aws-elasticloadbalancingv2');
import cdk = require('@aws-cdk/cdk');

Expand All @@ -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; 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 @@ -75,9 +88,19 @@ export abstract class LoadBalancedServiceBase extends cdk.Construct {

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

public readonly cluster: ecs.ICluster;

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 Cluster(this, 'Cluster', { vpc: props.vpc });
}

// Load balancer
this.loadBalancerType = props.loadBalancerType !== undefined ? props.loadBalancerType : LoadBalancerType.Application;

Expand All @@ -88,7 +111,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 @@ -60,7 +60,7 @@ export class LoadBalancedEc2Service extends LoadBalancedServiceBase {
});

const service = new ecs.Ec2Service(this, "Service", {
cluster: props.cluster,
cluster: this.cluster,
desiredCount: props.desiredCount || 1,
taskDefinition
});
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