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(ecs): update logic for secret JSON field with fargate task to fail #7317

Merged
merged 9 commits into from
May 6, 2020
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-ecs/lib/base/task-definition.ts
Original file line number Diff line number Diff line change
Expand Up @@ -497,7 +497,7 @@ export class TaskDefinition extends TaskDefinitionBase {
}
}

return this.containers.map(x => x.renderContainerDefinition(this));
return this.containers.map(x => x.renderContainerDefinition());
}
}

Expand Down
44 changes: 32 additions & 12 deletions packages/@aws-cdk/aws-ecs/lib/container-definition.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,24 @@ export abstract class Secret {
public static fromSecretsManager(secret: secretsmanager.ISecret, field?: string): Secret {
return {
arn: field ? `${secret.secretArn}:${field}::` : secret.secretArn,
hasField: !!field,
grantRead: grantee => secret.grantRead(grantee),
};
}

/**
* The ARN of the secret
*/
public abstract readonly arn: string;

/**
* Whether this secret uses a specific JSON field
*/
public abstract readonly hasField?: boolean;

/**
* Grants reading the secret to a principal
*/
Comment on lines +54 to +56
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for adding comments to the public fields!

public abstract grantRead(grantee: iam.IGrantable): iam.Grant;
}

Expand Down Expand Up @@ -348,6 +361,8 @@ export class ContainerDefinition extends cdk.Construct {

private readonly imageConfig: ContainerImageConfig;

private readonly secrets?: CfnTaskDefinition.SecretProperty[];

/**
* Constructs a new instance of the ContainerDefinition class.
*/
Expand All @@ -369,6 +384,20 @@ export class ContainerDefinition extends cdk.Construct {
this.logDriverConfig = props.logging.bind(this, this);
}
props.taskDefinition._linkContainer(this);

if (props.secrets) {
this.secrets = [];
for (const [name, secret] of Object.entries(props.secrets)) {
if (this.taskDefinition.isFargateCompatible && secret.hasField) {
throw new Error(`Cannot specify secret JSON field for a task using the FARGATE launch type: '${name}' in container '${this.node.id}'`);
}
secret.grantRead(this.taskDefinition.obtainExecutionRole());
this.secrets.push({
name,
valueFrom: secret.arn,
});
}
}
}

/**
Expand Down Expand Up @@ -519,9 +548,9 @@ export class ContainerDefinition extends cdk.Construct {
/**
* Render this container definition to a CloudFormation object
*
* @param taskDefinition [disable-awslint:ref-via-interface] (made optional to avoid breaking change)
* @param _taskDefinition [disable-awslint:ref-via-interface] (unused but kept to avoid breaking change)
*/
public renderContainerDefinition(taskDefinition?: TaskDefinition): CfnTaskDefinition.ContainerDefinitionProperty {
public renderContainerDefinition(_taskDefinition?: TaskDefinition): CfnTaskDefinition.ContainerDefinitionProperty {
return {
command: this.props.command,
cpu: this.props.cpu,
Expand Down Expand Up @@ -551,16 +580,7 @@ export class ContainerDefinition extends cdk.Construct {
workingDirectory: this.props.workingDirectory,
logConfiguration: this.logDriverConfig,
environment: this.props.environment && renderKV(this.props.environment, 'name', 'value'),
secrets: this.props.secrets && Object.entries(this.props.secrets)
.map(([k, v]) => {
if (taskDefinition) {
v.grantRead(taskDefinition.obtainExecutionRole());
}
return {
name: k,
valueFrom: v.arn,
};
}),
secrets: this.secrets,
extraHosts: this.props.extraHosts && renderKV(this.props.extraHosts, 'hostname', 'ipAddress'),
healthCheck: this.props.healthCheck && renderHealthCheck(this.props.healthCheck),
links: cdk.Lazy.listValue({ produce: () => this.links }, { omitEmpty: true }),
Expand Down
4 changes: 2 additions & 2 deletions packages/@aws-cdk/aws-ecs/lib/firelens-log-router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -236,9 +236,9 @@ export class FirelensLogRouter extends ContainerDefinition {
/**
* Render this container definition to a CloudFormation object
*/
public renderContainerDefinition(taskDefinition?: TaskDefinition): CfnTaskDefinition.ContainerDefinitionProperty {
public renderContainerDefinition(_taskDefinition?: TaskDefinition): CfnTaskDefinition.ContainerDefinitionProperty {
return {
...(super.renderContainerDefinition(taskDefinition)),
...(super.renderContainerDefinition()),
firelensConfiguration: this.firelensConfig && renderFirelensConfig(this.firelensConfig),
};
}
Expand Down
2 changes: 0 additions & 2 deletions packages/@aws-cdk/aws-ecs/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -135,8 +135,6 @@
"docs-public-apis:@aws-cdk/aws-ecs.GelfCompressionType.GZIP",
"docs-public-apis:@aws-cdk/aws-ecs.WindowsOptimizedVersion.SERVER_2016",
"docs-public-apis:@aws-cdk/aws-ecs.WindowsOptimizedVersion.SERVER_2019",
"docs-public-apis:@aws-cdk/aws-ecs.Secret.arn",
"docs-public-apis:@aws-cdk/aws-ecs.Secret.grantRead",
"props-default-doc:@aws-cdk/aws-ecs.AppMeshProxyConfigurationProps.egressIgnoredIPs",
"props-default-doc:@aws-cdk/aws-ecs.AppMeshProxyConfigurationProps.egressIgnoredPorts",
"props-default-doc:@aws-cdk/aws-ecs.AppMeshProxyConfigurationProps.ignoredGID",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
{
"Essential": true,
"Image": "amazon/amazon-ecs-sample",
"Memory": 256,
"Name": "web",
"Secrets": [
{
Expand All @@ -52,18 +53,16 @@
]
}
],
"Cpu": "512",
"ExecutionRoleArn": {
"Fn::GetAtt": [
"TaskDefExecutionRoleB4775C97",
"Arn"
]
},
"Family": "awsecsintegsecretjsonkeyTaskDefC01C0E99",
"Memory": "1024",
"NetworkMode": "awsvpc",
"Family": "awsecsintegsecretjsonfieldTaskDef1C2EE990",
"NetworkMode": "bridge",
"RequiresCompatibilities": [
"FARGATE"
"EC2"
],
"TaskRoleArn": {
"Fn::GetAtt": [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import * as cdk from '@aws-cdk/core';
import * as ecs from '../../lib';

const app = new cdk.App();
const stack = new cdk.Stack(app, 'aws-ecs-integ-secret-json-key');
const stack = new cdk.Stack(app, 'aws-ecs-integ-secret-json-field');

const secret = new secretsmanager.Secret(stack, 'Secret', {
generateSecretString: {
Expand All @@ -12,13 +12,11 @@ const secret = new secretsmanager.Secret(stack, 'Secret', {
},
});

const taskDefinition = new ecs.FargateTaskDefinition(stack, 'TaskDef', {
memoryLimitMiB: 1024,
cpu: 512,
});
const taskDefinition = new ecs.Ec2TaskDefinition(stack, 'TaskDef');

taskDefinition.addContainer('web', {
image: ecs.ContainerImage.fromRegistry('amazon/amazon-ecs-sample'),
memoryLimitMiB: 256,
secrets: {
PASSWORD: ecs.Secret.fromSecretsManager(secret, 'password'),
},
Expand Down
109 changes: 109 additions & 0 deletions packages/@aws-cdk/aws-ecs/test/fargate/integ.secret.expected.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
{
"Resources": {
"SecretA720EF05": {
"Type": "AWS::SecretsManager::Secret",
"Properties": {
"GenerateSecretString": {
"GenerateStringKey": "password",
"SecretStringTemplate": "{\"username\":\"user\"}"
}
}
},
"TaskDefTaskRole1EDB4A67": {
"Type": "AWS::IAM::Role",
"Properties": {
"AssumeRolePolicyDocument": {
"Statement": [
{
"Action": "sts:AssumeRole",
"Effect": "Allow",
"Principal": {
"Service": "ecs-tasks.amazonaws.com"
}
}
],
"Version": "2012-10-17"
}
}
},
"TaskDef54694570": {
"Type": "AWS::ECS::TaskDefinition",
"Properties": {
"ContainerDefinitions": [
{
"Essential": true,
"Image": "amazon/amazon-ecs-sample",
"Name": "web",
"Secrets": [
{
"Name": "SECRET",
"ValueFrom": {
"Ref": "SecretA720EF05"
}
}
]
}
],
"Cpu": "256",
"ExecutionRoleArn": {
"Fn::GetAtt": [
"TaskDefExecutionRoleB4775C97",
"Arn"
]
},
"Family": "awsecsintegsecretTaskDef58AA207D",
"Memory": "512",
"NetworkMode": "awsvpc",
"RequiresCompatibilities": [
"FARGATE"
],
"TaskRoleArn": {
"Fn::GetAtt": [
"TaskDefTaskRole1EDB4A67",
"Arn"
]
}
}
},
"TaskDefExecutionRoleB4775C97": {
"Type": "AWS::IAM::Role",
"Properties": {
"AssumeRolePolicyDocument": {
"Statement": [
{
"Action": "sts:AssumeRole",
"Effect": "Allow",
"Principal": {
"Service": "ecs-tasks.amazonaws.com"
}
}
],
"Version": "2012-10-17"
}
}
},
"TaskDefExecutionRoleDefaultPolicy0DBB737A": {
"Type": "AWS::IAM::Policy",
"Properties": {
"PolicyDocument": {
"Statement": [
{
"Action": "secretsmanager:GetSecretValue",
"Effect": "Allow",
"Resource": {
"Ref": "SecretA720EF05"
}
}
],
"Version": "2012-10-17"
},
"PolicyName": "TaskDefExecutionRoleDefaultPolicy0DBB737A",
"Roles": [
{
"Ref": "TaskDefExecutionRoleB4775C97"
}
]
}
}
}
}
24 changes: 24 additions & 0 deletions packages/@aws-cdk/aws-ecs/test/fargate/integ.secret.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import * as secretsmanager from '@aws-cdk/aws-secretsmanager';
import * as cdk from '@aws-cdk/core';
import * as ecs from '../../lib';

const app = new cdk.App();
const stack = new cdk.Stack(app, 'aws-ecs-integ-secret');

const secret = new secretsmanager.Secret(stack, 'Secret', {
generateSecretString: {
generateStringKey: 'password',
secretStringTemplate: JSON.stringify({ username: 'user' }),
},
});

const taskDefinition = new ecs.FargateTaskDefinition(stack, 'TaskDef');

taskDefinition.addContainer('web', {
image: ecs.ContainerImage.fromRegistry('amazon/amazon-ecs-sample'),
secrets: {
SECRET: ecs.Secret.fromSecretsManager(secret),
},
});

app.synth();
19 changes: 19 additions & 0 deletions packages/@aws-cdk/aws-ecs/test/test.container-definition.ts
Original file line number Diff line number Diff line change
Expand Up @@ -843,6 +843,25 @@ export = {

},

'throws when using a specific secret JSON field as environment variable for a Fargate task'(test: Test) {
// GIVEN
const stack = new cdk.Stack();
const taskDefinition = new ecs.FargateTaskDefinition(stack, 'TaskDef');

const secret = new secretsmanager.Secret(stack, 'Secret');

// THEN
test.throws(() => taskDefinition.addContainer('cont', {
image: ecs.ContainerImage.fromRegistry('test'),
memoryLimitMiB: 1024,
secrets: {
SECRET_KEY: ecs.Secret.fromSecretsManager(secret, 'specificKey'),
},
}), /Cannot specify secret JSON field for a task using the FARGATE launch type/);

test.done();
},

'can add AWS logging to container definition'(test: Test) {
// GIVEN
const stack = new cdk.Stack();
Expand Down