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.

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): add external network modes to ExternalTaskDefinition and TaskDefinition #17762

Merged
merged 10 commits into from
Jun 14, 2022
4 changes: 2 additions & 2 deletions packages/@aws-cdk/aws-ecs/lib/base/task-definition.ts
Original file line number Diff line number Diff line change
Expand Up @@ -401,8 +401,8 @@ export class TaskDefinition extends TaskDefinitionBase {
throw new Error('Cannot use inference accelerators on tasks that run on Fargate');
}

if (this.isExternalCompatible && this.networkMode !== NetworkMode.BRIDGE) {
throw new Error(`External tasks can only have Bridge network mode, got: ${this.networkMode}`);
if (this.isExternalCompatible && ![NetworkMode.BRIDGE, NetworkMode.HOST, NetworkMode.NONE].includes(this.networkMode)) {
beezly marked this conversation as resolved.
Show resolved Hide resolved
throw new Error(`External tasks can only have Bridge, Host or None network mode, got: ${this.networkMode}`);
}

this._executionRole = props.executionRole;
Expand Down
21 changes: 17 additions & 4 deletions packages/@aws-cdk/aws-ecs/lib/external/external-task-definition.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,14 @@ import {
* The properties for a task definition run on an External cluster.
*/
export interface ExternalTaskDefinitionProps extends CommonTaskDefinitionProps {

/**
* The networking mode to use for the containers in the task.
*
* With ECS Anywhere, supported modes are bridge, host and none.
*
* @default - NetworkMode.Bridge
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @default - NetworkMode.Bridge
* @default NetworkMode.BRIDGE

*/
readonly networkMode?: NetworkMode;
}

/**
Expand All @@ -38,7 +45,6 @@ export interface ExternalTaskDefinitionAttributes extends CommonTaskDefinitionAt
* @resource AWS::ECS::TaskDefinition
*/
export class ExternalTaskDefinition extends TaskDefinition implements IExternalTaskDefinition {

/**
* Imports a task definition from the specified task definition ARN.
*/
Expand All @@ -59,20 +65,27 @@ export class ExternalTaskDefinition extends TaskDefinition implements IExternalT
return new ImportedTaskDefinition(scope, id, {
taskDefinitionArn: attrs.taskDefinitionArn,
compatibility: Compatibility.EXTERNAL,
networkMode: NetworkMode.BRIDGE,
networkMode: attrs.networkMode,
taskRole: attrs.taskRole,
});
}

/**
* The networking mode to use for the containers in the task.
*/
public readonly networkMode: NetworkMode;

Copy link
Contributor

Choose a reason for hiding this comment

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

This public property is not used anywhere, so let's not add it until there is a requirement for it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually this already exists in the base class, TaskDefinition. It should still be removed from here.

/**
* Constructs a new instance of the ExternalTaskDefinition class.
*/
constructor(scope: Construct, id: string, props: ExternalTaskDefinitionProps = {}) {
super(scope, id, {
...props,
compatibility: Compatibility.EXTERNAL,
networkMode: NetworkMode.BRIDGE,
networkMode: (props.networkMode ?? NetworkMode.BRIDGE),
});

this.networkMode = props.networkMode ?? NetworkMode.BRIDGE;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
this.networkMode = props.networkMode ?? NetworkMode.BRIDGE;

Copy link
Contributor

Choose a reason for hiding this comment

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

This is already done by passing in props.networkMode ?? NetworkMode.BRIDGE to the super constructor.

}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import * as secretsmanager from '@aws-cdk/aws-secretsmanager';
import * as ssm from '@aws-cdk/aws-ssm';
import * as cdk from '@aws-cdk/core';
import * as ecs from '../../lib';
import { NetworkMode } from '../../lib';
beezly marked this conversation as resolved.
Show resolved Hide resolved

describe('external task definition', () => {
describe('When creating an External TaskDefinition', () => {
Expand Down Expand Up @@ -37,6 +38,7 @@ describe('external task definition', () => {
),
}),
family: 'ecs-tasks',
networkMode: NetworkMode.HOST,
beezly marked this conversation as resolved.
Show resolved Hide resolved
taskRole: new iam.Role(stack, 'TaskRole', {
assumedBy: new iam.ServicePrincipal('ecs-tasks.amazonaws.com'),
}),
Expand All @@ -51,7 +53,7 @@ describe('external task definition', () => {
],
},
Family: 'ecs-tasks',
NetworkMode: ecs.NetworkMode.BRIDGE,
NetworkMode: ecs.NetworkMode.HOST,
Copy link
Contributor

@madeline-k madeline-k Mar 16, 2022

Choose a reason for hiding this comment

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

Let's test on the actual string value of the enum, so if someone changes the value of NetworkMode.HOST in the future, this test will catch it.

Suggested change
NetworkMode: ecs.NetworkMode.HOST,
NetworkMode: 'host',

RequiresCompatibilities: [
'EXTERNAL',
],
Expand All @@ -66,6 +68,18 @@ describe('external task definition', () => {

});

test('error when an invalid networkmode is set', () => {
// GIVEN
const stack = new cdk.Stack();

// THEN
expect(() => {
new ecs.ExternalTaskDefinition(stack, 'ExternalTaskDef', {
networkMode: NetworkMode.AWS_VPC,
beezly marked this conversation as resolved.
Show resolved Hide resolved
});
}).toThrow('External tasks can only have Bridge, Host or None network mode, got: awsvpc');
});

test('correctly sets containers', () => {
// GIVEN
const stack = new cdk.Stack();
Expand Down
19 changes: 18 additions & 1 deletion packages/@aws-cdk/aws-ecs/test/task-definition.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import * as ecs from '../lib';

describe('task definition', () => {
describe('When creating a new TaskDefinition', () => {
test('A task definition with both compatibilities defaults to networkmode AwsVpc', () => {
test('A task definition with EC2 and Fargate compatibilities defaults to networkmode AwsVpc', () => {
// GIVEN
const stack = new cdk.Stack();

Expand All @@ -23,6 +23,23 @@ describe('task definition', () => {


});

test('A task definition with External compatibility defaults to networkmode Bridge', () => {
// GIVEN
const stack = new cdk.Stack();

// WHEN
new ecs.TaskDefinition(stack, 'TD', {
cpu: '512',
memoryMiB: '512',
compatibility: ecs.Compatibility.EXTERNAL,
});

//THEN
expect(stack).toHaveResource('AWS::ECS::TaskDefinition', {
NetworkMode: 'bridge',
});
});
});

describe('When importing from an existing Task definition', () => {
Expand Down