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

fix(ecs): nat network mode for windows tasks #4317

Merged
merged 11 commits into from
Oct 3, 2019
Merged

fix(ecs): nat network mode for windows tasks #4317

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
ab86b56
fix(ecs): nat network mode for windows tasks
Oct 1, 2019
f50e3ac
remove unnecessary whitespaces
Oct 1, 2019
c6812c3
PR review feedback - consolidate the comments.
Oct 2, 2019
1be7f87
Merge branch 'master' into kolomied/nat-networkmode
kolomied Oct 2, 2019
6eae70a
Merge branch 'master' into kolomied/nat-networkmode
piradeepk Oct 2, 2019
cb71a29
Merge branch 'master' into kolomied/nat-networkmode
piradeepk Oct 2, 2019
63e09ff
Merge branch 'master' into kolomied/nat-networkmode
piradeepk Oct 2, 2019
fd83aef
PR feedback - simplify ephemeral port mapping logic
Oct 2, 2019
88eedd3
Merge branch 'kolomied/nat-networkmode' of github.com:kolomied/aws-cd…
Oct 2, 2019
0fc3e07
Merge branch 'master' into kolomied/nat-networkmode
piradeepk Oct 3, 2019
c6a1ea7
Merge branch 'master' into kolomied/nat-networkmode
mergify[bot] Oct 3, 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
23 changes: 21 additions & 2 deletions packages/@aws-cdk/aws-ecs/lib/base/task-definition.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,7 @@ export class TaskDefinition extends TaskDefinitionBase {
...(isEc2Compatible(props.compatibility) ? ["EC2"] : []),
...(isFargateCompatible(props.compatibility) ? ["FARGATE"] : []),
],
networkMode: this.networkMode,
networkMode: this.renderNetworkMode(this.networkMode),
piradeepk marked this conversation as resolved.
Show resolved Hide resolved
placementConstraints: Lazy.anyValue({ produce: () =>
!isFargateCompatible(this.compatibility) ? this.placementConstraints : undefined
}, { omitEmptyArray: true }),
Expand Down Expand Up @@ -326,7 +326,7 @@ export class TaskDefinition extends TaskDefinitionBase {
if (portMapping.hostPort !== undefined && portMapping.hostPort !== 0) {
return portMapping.protocol === Protocol.UDP ? ec2.Port.udp(portMapping.hostPort) : ec2.Port.tcp(portMapping.hostPort);
}
if (this.networkMode === NetworkMode.BRIDGE) {
if (this.isEphemeralPortMappingSupported(this.networkMode)) {
return EPHEMERAL_PORT_RANGE;
}
return portMapping.protocol === Protocol.UDP ? ec2.Port.udp(portMapping.containerPort) : ec2.Port.tcp(portMapping.containerPort);
Expand Down Expand Up @@ -428,6 +428,18 @@ export class TaskDefinition extends TaskDefinitionBase {
private findContainer(containerName: string): ContainerDefinition | undefined {
return this.containers.find(c => c.containerName === containerName);
}

private isEphemeralPortMappingSupported(networkMode: NetworkMode): boolean {
return networkMode === NetworkMode.BRIDGE || networkMode === NetworkMode.NAT;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: personally, I'd prefer these conditions in the if statement itself, just to allow for ease of debugging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem, will address that

}

private renderNetworkMode(networkMode: NetworkMode): string | undefined {
// 'NAT' is the only supported network mode for Windows containers,
Copy link
Contributor

Choose a reason for hiding this comment

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

Since it is also explained in the ENUM def, I think you can combine those lines of comment into the ENUM one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, makes sense. Thanks!

// which is represented as omitted 'networkMode' property in CF template.
// See https://docs.aws.amazon.com/AmazonECS/latest/developerguide/windows_task_IAM_roles.html

return (networkMode === NetworkMode.NAT) ? undefined : networkMode;
}
}

/**
Expand Down Expand Up @@ -461,6 +473,13 @@ export enum NetworkMode {
* single container instance when port mappings are used.
*/
HOST = 'host',

/**
* The task utilizes NAT network mode required by Windows containers.
*
* This is the only network mode supported for Windows containers.
*/
NAT = 'nat'
}

/**
Expand Down
160 changes: 82 additions & 78 deletions packages/@aws-cdk/aws-ecs/test/ec2/test.ec2-service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1088,94 +1088,98 @@ export = {
},

'correctly setting ingress and egress port': {
'with bridge network mode and 0 host port'(test: Test) {
// GIVEN
const stack = new cdk.Stack();
const vpc = new ec2.Vpc(stack, 'MyVpc', {});
const cluster = new ecs.Cluster(stack, 'EcsCluster', { vpc });
cluster.addCapacity('DefaultAutoScalingGroup', { instanceType: new ec2.InstanceType('t2.micro') });
const taskDefinition = new ecs.Ec2TaskDefinition(stack, 'Ec2TaskDef');
const container = taskDefinition.addContainer('MainContainer', {
image: ContainerImage.fromRegistry('hello'),
memoryLimitMiB: 512,
});
container.addPortMappings({ containerPort: 8000 });
container.addPortMappings({ containerPort: 8001 });

const service = new ecs.Ec2Service(stack, 'Service', {
cluster,
taskDefinition
});
'with bridge/NAT network mode and 0 host port'(test: Test) {
[NetworkMode.BRIDGE, NetworkMode.NAT].forEach((networkMode: NetworkMode) => {
// GIVEN
const stack = new cdk.Stack();
const vpc = new ec2.Vpc(stack, 'MyVpc', {});
const cluster = new ecs.Cluster(stack, 'EcsCluster', { vpc });
cluster.addCapacity('DefaultAutoScalingGroup', { instanceType: new ec2.InstanceType('t2.micro') });
const taskDefinition = new ecs.Ec2TaskDefinition(stack, 'Ec2TaskDef', { networkMode });
const container = taskDefinition.addContainer('MainContainer', {
image: ContainerImage.fromRegistry('hello'),
memoryLimitMiB: 512,
});
container.addPortMappings({ containerPort: 8000 });
container.addPortMappings({ containerPort: 8001 });

// WHEN
const lb = new elbv2.ApplicationLoadBalancer(stack, "lb", { vpc });
const listener = lb.addListener("listener", { port: 80 });
listener.addTargets("target", {
port: 80,
targets: [service.loadBalancerTarget({
containerName: "MainContainer",
containerPort: 8001
})]
});
const service = new ecs.Ec2Service(stack, 'Service', {
cluster,
taskDefinition
});

// THEN
expect(stack).to(haveResource('AWS::EC2::SecurityGroupIngress', {
Description: "Load balancer to target",
FromPort: 32768,
ToPort: 65535
}));
// WHEN
const lb = new elbv2.ApplicationLoadBalancer(stack, "lb", { vpc });
const listener = lb.addListener("listener", { port: 80 });
listener.addTargets("target", {
port: 80,
targets: [service.loadBalancerTarget({
containerName: "MainContainer",
containerPort: 8001
})]
});

expect(stack).to(haveResource('AWS::EC2::SecurityGroupEgress', {
Description: "Load balancer to target",
FromPort: 32768,
ToPort: 65535
}));
// THEN
expect(stack).to(haveResource('AWS::EC2::SecurityGroupIngress', {
Description: "Load balancer to target",
FromPort: 32768,
ToPort: 65535
}));

expect(stack).to(haveResource('AWS::EC2::SecurityGroupEgress', {
Description: "Load balancer to target",
FromPort: 32768,
ToPort: 65535
}));
});

test.done();
},

'with bridge network mode and host port other than 0'(test: Test) {
// GIVEN
const stack = new cdk.Stack();
const vpc = new ec2.Vpc(stack, 'MyVpc', {});
const cluster = new ecs.Cluster(stack, 'EcsCluster', { vpc });
cluster.addCapacity('DefaultAutoScalingGroup', { instanceType: new ec2.InstanceType('t2.micro') });
const taskDefinition = new ecs.Ec2TaskDefinition(stack, 'Ec2TaskDef');
const container = taskDefinition.addContainer('MainContainer', {
image: ContainerImage.fromRegistry('hello'),
memoryLimitMiB: 512,
});
container.addPortMappings({ containerPort: 8000 });
container.addPortMappings({ containerPort: 8001, hostPort: 80 });

const service = new ecs.Ec2Service(stack, 'Service', {
cluster,
taskDefinition
});
'with bridge/NAT network mode and host port other than 0'(test: Test) {
[NetworkMode.BRIDGE, NetworkMode.NAT].forEach((networkMode: NetworkMode) => {
// GIVEN
const stack = new cdk.Stack();
const vpc = new ec2.Vpc(stack, 'MyVpc', {});
const cluster = new ecs.Cluster(stack, 'EcsCluster', { vpc });
cluster.addCapacity('DefaultAutoScalingGroup', { instanceType: new ec2.InstanceType('t2.micro') });
const taskDefinition = new ecs.Ec2TaskDefinition(stack, 'Ec2TaskDef', { networkMode });
const container = taskDefinition.addContainer('MainContainer', {
image: ContainerImage.fromRegistry('hello'),
memoryLimitMiB: 512,
});
container.addPortMappings({ containerPort: 8000 });
container.addPortMappings({ containerPort: 8001, hostPort: 80 });

// WHEN
const lb = new elbv2.ApplicationLoadBalancer(stack, "lb", { vpc });
const listener = lb.addListener("listener", { port: 80 });
listener.addTargets("target", {
port: 80,
targets: [service.loadBalancerTarget({
containerName: "MainContainer",
containerPort: 8001
})]
});
const service = new ecs.Ec2Service(stack, 'Service', {
cluster,
taskDefinition
});

// THEN
expect(stack).to(haveResource('AWS::EC2::SecurityGroupIngress', {
Description: "Load balancer to target",
FromPort: 80,
ToPort: 80,
}));
// WHEN
const lb = new elbv2.ApplicationLoadBalancer(stack, "lb", { vpc });
const listener = lb.addListener("listener", { port: 80 });
listener.addTargets("target", {
port: 80,
targets: [service.loadBalancerTarget({
containerName: "MainContainer",
containerPort: 8001
})]
});

expect(stack).to(haveResource('AWS::EC2::SecurityGroupEgress', {
Description: "Load balancer to target",
FromPort: 80,
ToPort: 80
}));
// THEN
expect(stack).to(haveResource('AWS::EC2::SecurityGroupIngress', {
Description: "Load balancer to target",
FromPort: 80,
ToPort: 80,
}));

expect(stack).to(haveResource('AWS::EC2::SecurityGroupEgress', {
Description: "Load balancer to target",
FromPort: 80,
ToPort: 80
}));
});

test.done();
},
Expand Down
27 changes: 26 additions & 1 deletion packages/@aws-cdk/aws-ecs/test/test.container-definition.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { expect, haveResource, haveResourceLike } from '@aws-cdk/assert';
import { expect, haveResource, haveResourceLike, InspectionFailure } from '@aws-cdk/assert';
import secretsmanager = require('@aws-cdk/aws-secretsmanager');
import ssm = require('@aws-cdk/aws-ssm');
import cdk = require('@aws-cdk/core');
Expand Down Expand Up @@ -274,6 +274,31 @@ export = {

test.done();
},
},

"With network mode NAT": {
"produces undefined CF networkMode property"(test: Test) {
// GIVEN
const stack = new cdk.Stack();

// WHEN
new ecs.TaskDefinition(stack, 'TD', {
compatibility: ecs.Compatibility.EC2,
networkMode: ecs.NetworkMode.NAT
});

// THEN
expect(stack).to(haveResource('AWS::ECS::TaskDefinition', (props: any, inspection: InspectionFailure) => {
if (props.NetworkMode === undefined) {
return true;
}

inspection.failureReason = 'CF template should not have NetworkMode defined for a task definition that relies on NAT network mode.';
return false;
}));

test.done();
}
}
},

Expand Down