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(events): imported ECS Task Definition cannot be used as target #13293

Merged
merged 3 commits into from
Mar 3, 2021
Merged

fix(events): imported ECS Task Definition cannot be used as target #13293

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
829de1e
fix(aws-events-targets): Allow creating EcsTask from an imported task…
rgithubli Feb 25, 2021
0766d75
Address comments to 829de1e6b
rgithubli Feb 28, 2021
162561a
Merge branch 'master' into rli1994hi/ecsTask-using-ITaskDefinition
mergify[bot] Mar 3, 2021
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
86 changes: 82 additions & 4 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 @@ -38,6 +38,16 @@ export interface ITaskDefinition extends IResource {
* Return true if the task definition can be run on a Fargate cluster
*/
readonly isFargateCompatible: boolean;

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

/**
* The name of the IAM role that grants containers in the task permission to call AWS APIs on your behalf.
*/
readonly taskRole: iam.IRole;
}

/**
Expand Down Expand Up @@ -175,10 +185,55 @@ export interface TaskDefinitionProps extends CommonTaskDefinitionProps {
readonly pidMode?: PidMode;
}

/**
* The common task definition attributes used across all types of task definitions.
*/
export interface CommonTaskDefinitionAttributes {
/**
* The arn of the task definition
*/
readonly taskDefinitionArn: string;

/**
* The networking mode to use for the containers in the task.
*
* @default NetworkMode.BRIDGE
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mistake. Should be something like No network mode can be provided.

*/
readonly networkMode?: NetworkMode;

/**
* The name of the IAM role that grants containers in the task permission to call AWS APIs on your behalf.
*
* @default undefined.
Copy link
Contributor

Choose a reason for hiding this comment

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

While @default undefined here is technically true, it's not very useful. Anybody who knows JavaScript/TypeScript will know that if you don't pass a property somewhere, it will count as undefined.

What is more interesting is to write what the BEHAVIOR will be if you leave the value out. In this case something like @default - Permissions cannot be granted to the imported task.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.

*/
readonly taskRole?: iam.IRole;
}

/**
* A reference to an existing task definition
*/
export interface TaskDefinitionAttributes extends CommonTaskDefinitionAttributes {
/**
* Execution role for this task definition
*
* @default: undefined
*/
readonly executionRole?: iam.IRole;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why executionRole is here, and not in the EC2 TaskDefinitionAttributes 🤷‍♂️. An oversight on our part?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. We might be able to get rid of executionRole here.

fromTaskDefinitionArn returns an object containing executionRole to satisfy its parent class, TaskDefinitionBase, which containing executionRole?.

I was originally hoping to give users flexibility to define executionRole by themselves, but considering these methods are to import from existing resources, users might actually hope to give as minimal attributes as possible. I probably should keep executionRole to be undefined and drop it from TaskDefinitionAttributes.

Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

I probably should keep executionRole to be undefined and drop it from TaskDefinitionAttributes.

Agreed. iirc executionRole only makes sense for EC2 taskdefinitions (and is only on Base to satisfy some interface, but is never set there). For now, I think it's fine to take it out altogether.


/**
* What launch types this task definition should be compatible with.
*
* @default Compatibility.EC2_AND_FARGATE
*/
readonly compatibility?: Compatibility;
}

abstract class TaskDefinitionBase extends Resource implements ITaskDefinition {

public abstract readonly compatibility: Compatibility;
public abstract readonly networkMode: NetworkMode;
public abstract readonly taskDefinitionArn: string;
public abstract readonly taskRole: iam.IRole;
public abstract readonly executionRole?: iam.IRole;

/**
Expand Down Expand Up @@ -207,10 +262,33 @@ export class TaskDefinition extends TaskDefinitionBase {
* The task will have a compatibility of EC2+Fargate.
*/
public static fromTaskDefinitionArn(scope: Construct, id: string, taskDefinitionArn: string): ITaskDefinition {
return TaskDefinition.fromTaskDefinitionAttributes(scope, id, { taskDefinitionArn: taskDefinitionArn });
}

/**
* Create a task definition from a task definition reference
*/
public static fromTaskDefinitionAttributes(scope: Construct, id: string, attrs: TaskDefinitionAttributes): ITaskDefinition {
class Import extends TaskDefinitionBase {
public readonly taskDefinitionArn = taskDefinitionArn;
public readonly compatibility = Compatibility.EC2_AND_FARGATE;
public readonly executionRole?: iam.IRole = undefined;
public readonly taskDefinitionArn = attrs.taskDefinitionArn;
public readonly compatibility = attrs.compatibility ?? Compatibility.EC2_AND_FARGATE;
public readonly executionRole = attrs.executionRole;

public get networkMode(): NetworkMode {
if (attrs.networkMode == undefined) {
throw new Error('NetworkMode is available only if it is given when importing the TaskDefinition.');
Copy link
Contributor

Choose a reason for hiding this comment

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

Almost there. This error message makes sense to a user if they are calling taskDefinition.networkMode directly.

However, in 99% of cases the user will be PASSING this TaskDefinition to some other construct or function, and THAT one will be calling .networkMode. So think of it like this:

// User writes this:
const taskDefintiion = TaskDefintion.fromTaskDefinitionAttributes(...);
startNewService(taskDefinition);

// User gets error:
"NetworkMode is available only if it is given..."

And the user's thought will be "where is this NetworkMode coming from?"

A more accurate and useful message would be something like:

This operation requires the TaskDefinition's network mode to be known. Add the 'networkMode' parameter to the call to 'TaskDefinition.fromNetworkAttributes(...)'

Or somethign.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. Will update.

} else {
return attrs.networkMode;
}
}

public get taskRole(): iam.IRole {
if (attrs.taskRole == undefined) {
throw new Error('TaskRole is available only if it is given when importing the TaskDefinition.');
} else {
return attrs.taskRole;
}
}
}

return new Import(scope, id);
Expand Down Expand Up @@ -248,7 +326,7 @@ export class TaskDefinition extends TaskDefinitionBase {
public defaultContainer?: ContainerDefinition;

/**
* The task launch type compatiblity requirement.
* The task launch type compatibility requirement.
*/
public readonly compatibility: Compatibility;

Expand Down
51 changes: 49 additions & 2 deletions packages/@aws-cdk/aws-ecs/lib/ec2/ec2-task-definition.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,16 @@
import * as iam from '@aws-cdk/aws-iam';
import { Resource } from '@aws-cdk/core';
import { Construct } from 'constructs';
import { CommonTaskDefinitionProps, Compatibility, IpcMode, ITaskDefinition, NetworkMode, PidMode, TaskDefinition } from '../base/task-definition';
import {
CommonTaskDefinitionAttributes,
CommonTaskDefinitionProps,
Compatibility,
IpcMode,
ITaskDefinition,
NetworkMode,
PidMode,
TaskDefinition,
} from '../base/task-definition';
import { PlacementConstraint } from '../placement';

/**
Expand Down Expand Up @@ -51,6 +61,13 @@ export interface IEc2TaskDefinition extends ITaskDefinition {

}

/**
* Attributes used to import an existing EC2 task definition
*/
export interface Ec2TaskDefinitionAttributes extends CommonTaskDefinitionAttributes {

}

/**
* The details of a task definition run on an EC2 cluster.
*
Expand All @@ -62,12 +79,42 @@ export class Ec2TaskDefinition extends TaskDefinition implements IEc2TaskDefinit
* Imports a task definition from the specified task definition ARN.
*/
public static fromEc2TaskDefinitionArn(scope: Construct, id: string, ec2TaskDefinitionArn: string): IEc2TaskDefinition {
return Ec2TaskDefinition.fromEc2TaskDefinitionAttributes(
scope, id, { taskDefinitionArn: ec2TaskDefinitionArn },
);
}

/**
* Imports an existing Ec2 task definition from its attributes
*/
public static fromEc2TaskDefinitionAttributes(
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't this whole implementation just defer to TaskDefinition.fromTaskDefinitionAttributes() ?

Oh I guess it couldn't because the types are different...

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it might be helpful to define a private class that represents the Import class for all 3 types. This would be a file like:

aws-ecs-/lib/base/_imported-task-definition.ts
                  ^^^ underscore means we plan to not export this file to users

And it could contain a:

export class ImportedTaskDefinition extends Resource implements ITaskDefinition, IEc2TaskDefinition, IFargateTaskDefinition {
  // ...
}

To cut down on the duplication between these 3 methods.

If this sounds too daunting, I'm okay leaving it as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Will try.

A callout is that, there's a little difference in fromTaskDefinitionArn: It does have the return type of interface ITaskDefinition , but internally it's a TaskDefinitionBase (which implements ITaskDefinition) rather than a Resource like what the other two import methods return. Don't see the downside of unifying them into Resource for now tho.

scope: Construct,
id: string,
attrs: Ec2TaskDefinitionAttributes,
): IEc2TaskDefinition {
class Import extends Resource implements IEc2TaskDefinition {
public readonly taskDefinitionArn = ec2TaskDefinitionArn;
public readonly taskDefinitionArn = attrs.taskDefinitionArn;
public readonly compatibility = Compatibility.EC2;
public readonly isEc2Compatible = true;
public readonly isFargateCompatible = false;

public get networkMode(): NetworkMode {
if (attrs.networkMode == undefined) {
throw new Error('NetworkMode is available only if it is given when importing the Ec2 TaskDefinition.');
} else {
return attrs.networkMode;
}
}

public get taskRole(): iam.IRole {
if (attrs.taskRole == undefined) {
throw new Error('TaskRole is available only if it is given when importing the Ec2 TaskDefinition.');
} else {
return attrs.taskRole;
}
}
}

return new Import(scope, id);
}

Expand Down
48 changes: 46 additions & 2 deletions packages/@aws-cdk/aws-ecs/lib/fargate/fargate-task-definition.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,14 @@
import * as iam from '@aws-cdk/aws-iam';
import { Resource, Tokenization } from '@aws-cdk/core';
import { Construct } from 'constructs';
import { CommonTaskDefinitionProps, Compatibility, ITaskDefinition, NetworkMode, TaskDefinition } from '../base/task-definition';
import {
CommonTaskDefinitionAttributes,
CommonTaskDefinitionProps,
Compatibility,
ITaskDefinition,
NetworkMode,
TaskDefinition,
} from '../base/task-definition';

/**
* The properties for a task definition.
Expand Down Expand Up @@ -51,6 +59,13 @@ export interface IFargateTaskDefinition extends ITaskDefinition {

}

/**
* Attributes used to import an existing Fargate task definition
*/
export interface FargateTaskDefinitionAttributes extends CommonTaskDefinitionAttributes {

}

/**
* The details of a task definition run on a Fargate cluster.
*
Expand All @@ -62,11 +77,40 @@ export class FargateTaskDefinition extends TaskDefinition implements IFargateTas
* Imports a task definition from the specified task definition ARN.
*/
public static fromFargateTaskDefinitionArn(scope: Construct, id: string, fargateTaskDefinitionArn: string): IFargateTaskDefinition {
return FargateTaskDefinition.fromFargateTaskDefinitionAttributes(
scope, id, { taskDefinitionArn: fargateTaskDefinitionArn },
);
}

/**
* Import an existing Fargate task definition from its attributes
*/
public static fromFargateTaskDefinitionAttributes(
scope: Construct,
id: string,
attrs: FargateTaskDefinitionAttributes,
): IFargateTaskDefinition {
class Import extends Resource implements IFargateTaskDefinition {
public readonly taskDefinitionArn = fargateTaskDefinitionArn;
public readonly taskDefinitionArn = attrs.taskDefinitionArn;
public readonly compatibility = Compatibility.FARGATE;
public readonly isEc2Compatible = false;
public readonly isFargateCompatible = true;

public get networkMode(): NetworkMode {
if (attrs.networkMode == undefined) {
throw new Error('NetworkMode is available only if it is given when importing the Fargate TaskDefinition.');
} else {
return attrs.networkMode;
}
}

public get taskRole(): iam.IRole {
if (attrs.taskRole == undefined) {
throw new Error('TaskRole is available only if it is given when importing the Fargate TaskDefinition.');
} else {
return attrs.taskRole;
}
}
}

return new Import(scope, id);
Expand Down
75 changes: 75 additions & 0 deletions packages/@aws-cdk/aws-ecs/test/ec2/ec2-task-definition.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1059,6 +1059,81 @@ describe('ec2 task definition', () => {
});
});

describe('When importing from an existing Ec2 TaskDefinition', () => {
test('can succeed using TaskDefinition Arn', () => {
// GIVEN
const stack = new cdk.Stack();
const expectTaskDefinitionArn = 'TD_ARN';

// WHEN
const taskDefinition = ecs.Ec2TaskDefinition.fromEc2TaskDefinitionArn(stack, 'EC2_TD_ID', expectTaskDefinitionArn);

// THEN
expect(taskDefinition.taskDefinitionArn).toBe(expectTaskDefinitionArn);
});
});

describe('When importing from an existing Ec2 TaskDefinition using attributes', () => {
test('can set the imported task attribuets successfully', () => {
// GIVEN
const stack = new cdk.Stack();
const expectTaskDefinitionArn = 'TD_ARN';
const expectNetworkMode = ecs.NetworkMode.AWS_VPC;
const expectTaskRole = new iam.Role(stack, 'TaskRole', {
assumedBy: new iam.ServicePrincipal('ecs-tasks.amazonaws.com'),
});

// WHEN
const taskDefinition = ecs.Ec2TaskDefinition.fromEc2TaskDefinitionAttributes(stack, 'TD_ID', {
taskDefinitionArn: expectTaskDefinitionArn,
networkMode: expectNetworkMode,
taskRole: expectTaskRole,
});

// THEN
expect(taskDefinition.taskDefinitionArn).toBe(expectTaskDefinitionArn);
expect(taskDefinition.compatibility).toBe(ecs.Compatibility.EC2);
expect(taskDefinition.isEc2Compatible).toBeTruthy();
expect(taskDefinition.isFargateCompatible).toBeFalsy();
expect(taskDefinition.networkMode).toBe(expectNetworkMode);
expect(taskDefinition.taskRole).toBe(expectTaskRole);
});

test('returns an Ec2 TaskDefinition that will throw an error when trying to access its yet to defined networkMode', () => {
// GIVEN
const stack = new cdk.Stack();
const expectTaskDefinitionArn = 'TD_ARN';
const expectTaskRole = new iam.Role(stack, 'TaskRole', {
assumedBy: new iam.ServicePrincipal('ecs-tasks.amazonaws.com'),
});

// WHEN
const taskDefinition = ecs.Ec2TaskDefinition.fromEc2TaskDefinitionAttributes(stack, 'TD_ID', {
taskDefinitionArn: expectTaskDefinitionArn,
taskRole: expectTaskRole,
});

// THEN
expect(() => taskDefinition.networkMode).toThrow(/NetworkMode is available only if it is given when importing the Ec2 TaskDefinition./);
});

test('returns an Ec2 TaskDefinition that will throw an error when trying to access its yet to defined taskRole', () => {
// GIVEN
const stack = new cdk.Stack();
const expectTaskDefinitionArn = 'TD_ARN';
const expectNetworkMode = ecs.NetworkMode.AWS_VPC;

// WHEN
const taskDefinition = ecs.Ec2TaskDefinition.fromEc2TaskDefinitionAttributes(stack, 'TD_ID', {
taskDefinitionArn: expectTaskDefinitionArn,
networkMode: expectNetworkMode,
});

// THEN
expect(() => taskDefinition.taskRole).toThrow(/TaskRole is available only if it is given when importing the Ec2 TaskDefinition./);
});
});

test('throws when setting proxyConfiguration without networkMode AWS_VPC', () => {
// GIVEN
const stack = new cdk.Stack();
Expand Down
Loading