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

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
108 changes: 108 additions & 0 deletions packages/@aws-cdk/aws-ecs/lib/base/_imported-task-definition.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
import { IRole } from '@aws-cdk/aws-iam';
import { Construct } from 'constructs';
import { IEc2TaskDefinition } from '../ec2/ec2-task-definition';
import { IFargateTaskDefinition } from '../fargate/fargate-task-definition';
import { Compatibility, NetworkMode, isEc2Compatible, isFargateCompatible } from './task-definition';
import { Resource } from '@aws-cdk/core';

/**
* The properties of ImportedTaskDefinition
*/
export interface ImportedTaskDefinitionProps {
/**
* The arn of the task definition
*/
readonly taskDefinitionArn: string;

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

/**
* The networking mode to use for the containers in the task.
*
* @default Network mode cannot be provided to the imported task.
*/
readonly networkMode?: NetworkMode;

/**
* The name of the IAM role that grants containers in the task permission to call AWS APIs on your behalf.
*
* @default Permissions cannot be granted to the imported task.
*/
readonly taskRole?: IRole;
}

/**
* Task definition reference of an imported task
*/
export class ImportedTaskDefinition extends Resource implements IEc2TaskDefinition, IFargateTaskDefinition {
/**
* What launch types this task definition should be compatible with.
*/
readonly compatibility: Compatibility;

/**
* ARN of this task definition
*/
readonly taskDefinitionArn: string;

/**
* Execution role for this task definition
*/
readonly executionRole?: IRole = undefined;

/**
* 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?: IRole;

constructor(scope: Construct, id: string, props: ImportedTaskDefinitionProps) {
super(scope, id);

this.compatibility = props.compatibility ?? Compatibility.EC2_AND_FARGATE;
this.taskDefinitionArn = props.taskDefinitionArn;
this._taskRole = props.taskRole;
this._networkMode = props.networkMode;
}

public get networkMode(): NetworkMode {
if (this._networkMode == undefined) {
throw new Error('This operation requires the networkMode in ImportedTaskDefinition to be defined. ' +
'Add the \'networkMode\' in ImportedTaskDefinitionProps to instantiate ImportedTaskDefinition');
} else {
return this._networkMode;
}
}

public get taskRole(): IRole {
if (this._taskRole == undefined) {
throw new Error('This operation requires the taskRole in ImportedTaskDefinition to be defined. ' +
'Add the \'taskRole\' in ImportedTaskDefinitionProps to instantiate ImportedTaskDefinition');
} else {
return this._taskRole;
}
}

/**
* Return true if the task definition can be run on an EC2 cluster
*/
public get isEc2Compatible(): boolean {
return isEc2Compatible(this.compatibility);
}

/**
* Return true if the task definition can be run on a Fargate cluster
*/
public get isFargateCompatible(): boolean {
return isFargateCompatible(this.compatibility);
}
}
73 changes: 64 additions & 9 deletions packages/@aws-cdk/aws-ecs/lib/base/task-definition.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { FirelensLogRouter, FirelensLogRouterDefinitionOptions, FirelensLogRoute
import { AwsLogDriver } from '../log-drivers/aws-log-driver';
import { PlacementConstraint } from '../placement';
import { ProxyConfiguration } from '../proxy-configuration/proxy-configuration';
import { ImportedTaskDefinition } from './_imported-task-definition';

/**
* The interface for all task definitions.
Expand Down Expand Up @@ -38,6 +39,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 +186,48 @@ 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 Network mode cannot be provided to the imported task.
*/
readonly networkMode?: NetworkMode;

/**
* The name of the IAM role that grants containers in the task permission to call AWS APIs on your behalf.
*
* @default Permissions cannot be granted to the imported task.
*/
readonly taskRole?: iam.IRole;
}

/**
* A reference to an existing task definition
*/
export interface TaskDefinitionAttributes extends CommonTaskDefinitionAttributes {
/**
* 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,13 +256,19 @@ export class TaskDefinition extends TaskDefinitionBase {
* The task will have a compatibility of EC2+Fargate.
*/
public static fromTaskDefinitionArn(scope: Construct, id: string, taskDefinitionArn: string): ITaskDefinition {
class Import extends TaskDefinitionBase {
public readonly taskDefinitionArn = taskDefinitionArn;
public readonly compatibility = Compatibility.EC2_AND_FARGATE;
public readonly executionRole?: iam.IRole = undefined;
}
return new ImportedTaskDefinition(scope, id, { taskDefinitionArn: taskDefinitionArn });
}

return new Import(scope, id);
/**
* Create a task definition from a task definition reference
*/
public static fromTaskDefinitionAttributes(scope: Construct, id: string, attrs: TaskDefinitionAttributes): ITaskDefinition {
return new ImportedTaskDefinition(scope, id, {
taskDefinitionArn: attrs.taskDefinitionArn,
compatibility: attrs.compatibility,
networkMode: attrs.networkMode,
taskRole: attrs.taskRole,
});
}

/**
Expand Down Expand Up @@ -248,7 +303,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 Expand Up @@ -890,13 +945,13 @@ export interface ITaskDefinitionExtension {
/**
* Return true if the given task definition can be run on an EC2 cluster
*/
function isEc2Compatible(compatibility: Compatibility): boolean {
export function isEc2Compatible(compatibility: Compatibility): boolean {
return [Compatibility.EC2, Compatibility.EC2_AND_FARGATE].includes(compatibility);
}

/**
* Return true if the given task definition can be run on a Fargate cluster
*/
function isFargateCompatible(compatibility: Compatibility): boolean {
export function isFargateCompatible(compatibility: Compatibility): boolean {
return [Compatibility.FARGATE, Compatibility.EC2_AND_FARGATE].includes(compatibility);
}
46 changes: 37 additions & 9 deletions packages/@aws-cdk/aws-ecs/lib/ec2/ec2-task-definition.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,16 @@
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';
import { ImportedTaskDefinition } from '../base/_imported-task-definition';

/**
* The properties for a task definition run on an EC2 cluster.
Expand Down Expand Up @@ -51,6 +60,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,13 +78,25 @@ 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 {
class Import extends Resource implements IEc2TaskDefinition {
public readonly taskDefinitionArn = ec2TaskDefinitionArn;
public readonly compatibility = Compatibility.EC2;
public readonly isEc2Compatible = true;
public readonly isFargateCompatible = false;
}
return new Import(scope, id);
return new ImportedTaskDefinition(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 {
return new ImportedTaskDefinition(scope, id, {
taskDefinitionArn: attrs.taskDefinitionArn,
compatibility: Compatibility.EC2,
networkMode: attrs.networkMode,
taskRole: attrs.taskRole,
});
}

/**
Expand Down
42 changes: 33 additions & 9 deletions packages/@aws-cdk/aws-ecs/lib/fargate/fargate-task-definition.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,14 @@
import { Resource, Tokenization } from '@aws-cdk/core';
import { 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';
import { ImportedTaskDefinition } from '../base/_imported-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,14 +77,23 @@ 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 {
class Import extends Resource implements IFargateTaskDefinition {
public readonly taskDefinitionArn = fargateTaskDefinitionArn;
public readonly compatibility = Compatibility.FARGATE;
public readonly isEc2Compatible = false;
public readonly isFargateCompatible = true;
}
return new ImportedTaskDefinition(scope, id, { taskDefinitionArn: fargateTaskDefinitionArn });
}

return new Import(scope, id);
/**
* Import an existing Fargate task definition from its attributes
*/
public static fromFargateTaskDefinitionAttributes(
scope: Construct,
id: string,
attrs: FargateTaskDefinitionAttributes,
): IFargateTaskDefinition {
return new ImportedTaskDefinition(scope, id, {
taskDefinitionArn: attrs.taskDefinitionArn,
compatibility: Compatibility.FARGATE,
networkMode: attrs.networkMode,
taskRole: attrs.taskRole,
});
}

/**
Expand Down
Loading