Skip to content

Commit

Permalink
fix(batch): JobDefinition's ContainerDefinition's Image is synthesize…
Browse files Browse the repository at this point in the history 
…d with `[Object object]` (#25466)

We were using `Tokenization.resolve()`, which uses a generic token resolver that does `${token1}${token2}` instead of creating an `Fn::Split` object. The only reason that token existed was to mutate the L1 object after synthesis so subclasses could fill in the `executionRole`. The `executionRole` was meant to be optional for EC2 jobs and required for Fargate jobs. Setting the `loggingConfig` being defined would force it to be defined it for EC2 Jobs. However, the `loggingConfig` was always defined, because it was set to a `Lazy` (which could result in `undefined`, but that doesn't matter: it was defined during synthesis). So the `executionRole` was always being set:

```
    this.executionRole = props.executionRole ?? (this.logDriverConfig ? createExecutionRole(this, 'ExecutionRole') : undefined);
```

This PR makes it clear that the `executionRole` is created by default.

Closes #25250

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
@comcalvi
comcalvi authored May 8, 2023
1 parent b76c182 commit b3d0d57
Show file tree
Hide file tree
Showing 13 changed files with 624 additions and 197 deletions.
21 changes: 21 additions & 0 deletions docs/DESIGN_GUIDELINES.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1523,6 +1523,27 @@ information that can be obtained from the stack trace.

* Do not use FnSub

### Lazys

Do not use a `Lazy` to perform a mutation on the construct tree. For example:

```ts
constructor(scope: Scope, id: string, props: MyConstructProps) {
this.lazyProperty = Lazy.any({
produce: () => {
return props.logging.bind(this, this);
},
});
}
```

`bind()` methods mutate the construct tree, and should not be called from a callback
in a `Lazy`.

* The why:
- `Lazy`s are called after the construct tree has already been sythesized. Mutating it
at this point could have not-obvious consequences.

## Documentation

Documentation style (copy from official AWS docs) No need to Capitalize Resource
Expand Down
106 changes: 36 additions & 70 deletions packages/@aws-cdk/aws-batch-alpha/lib/ecs-container-definition.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import * as ecs from 'aws-cdk-lib/aws-ecs';
import { IFileSystem } from 'aws-cdk-lib/aws-efs';
import * as iam from 'aws-cdk-lib/aws-iam';
import * as secretsmanager from 'aws-cdk-lib/aws-secretsmanager';
import { DefaultTokenResolver, Lazy, PhysicalName, Size, StringConcat, Tokenization } from 'aws-cdk-lib';
import { Lazy, PhysicalName, Size } from 'aws-cdk-lib';
import { Construct, IConstruct } from 'constructs';
import { CfnJobDefinition } from 'aws-cdk-lib/aws-batch';
import { LinuxParameters } from './linux-parameters';
Expand Down Expand Up @@ -237,6 +237,7 @@ export interface HostVolumeOptions extends EcsVolumeOptions {
*/
readonly hostPath?: string;
}

/**
* Creates a Host volume. This volume will persist on the host at the specified `hostPath`.
* If the `hostPath` is not specified, Docker will choose the host path. In this case,
Expand Down Expand Up @@ -306,6 +307,13 @@ export interface IEcsContainerDefinition extends IConstruct {
*/
readonly environment?: { [key:string]: string };

/**
* The role used by Amazon ECS container and AWS Fargate agents to make AWS API calls on your behalf.
*
* @see https://docs.aws.amazon.com/batch/latest/userguide/execution-IAM-role.html
*/
readonly executionRole: iam.IRole;

/**
* The role that the container can assume.
*
Expand Down Expand Up @@ -411,6 +419,15 @@ export interface EcsContainerDefinitionProps {
*/
readonly environment?: { [key:string]: string };

/**
* The role used by Amazon ECS container and AWS Fargate agents to make AWS API calls on your behalf.
*
* @see https://docs.aws.amazon.com/batch/latest/userguide/execution-IAM-role.html
*
* @default - a Role will be created
*/
readonly executionRole?: iam.IRole;

/**
* The role that the container can assume.
*
Expand Down Expand Up @@ -474,6 +491,7 @@ abstract class EcsContainerDefinitionBase extends Construct implements IEcsConta
public readonly memory: Size;
public readonly command?: string[];
public readonly environment?: { [key:string]: string };
public readonly executionRole: iam.IRole;
public readonly jobRole?: iam.IRole;
public readonly linuxParameters?: LinuxParameters;
public readonly logDriverConfig?: ecs.LogDriverConfig;
Expand All @@ -482,8 +500,6 @@ abstract class EcsContainerDefinitionBase extends Construct implements IEcsConta
public readonly user?: string;
public readonly volumes: EcsVolume[];

public abstract readonly executionRole?: iam.IRole;

private readonly imageConfig: ecs.ContainerImageConfig;

constructor(scope: Construct, id: string, props: EcsContainerDefinitionProps) {
Expand All @@ -493,52 +509,40 @@ abstract class EcsContainerDefinitionBase extends Construct implements IEcsConta
this.cpu = props.cpu;
this.command = props.command;
this.environment = props.environment;
this.executionRole = props.executionRole ?? createExecutionRole(this, 'ExecutionRole');
this.jobRole = props.jobRole;
this.linuxParameters = props.linuxParameters;
this.memory = props.memory;

// Lazy so this.executionRole can be filled by subclasses
this.logDriverConfig = Lazy.any({
produce: () => {
if (props.logging) {
return props.logging.bind(this, {
...this as any,
// TS!
taskDefinition: {
obtainExecutionRole: () => this.executionRole,
},
});
}

return undefined;
},
}) as any;
if (props.logging) {
this.logDriverConfig = props.logging.bind(this, {
...this as any,
// TS!
taskDefinition: {
obtainExecutionRole: () => this.executionRole,
},
});
}

this.readonlyRootFilesystem = props.readonlyRootFilesystem ?? false;
this.secrets = props.secrets;
this.user = props.user;
this.volumes = props.volumes ?? [];

// Lazy so this.executionRole can be filled by subclasses
this.imageConfig = Lazy.any({
produce: () => props.image.bind(this, {
...this as any,
taskDefinition: {
obtainExecutionRole: () => this.executionRole,
},
}),
}) as any;
this.imageConfig = props.image.bind(this, {
...this as any,
taskDefinition: {
obtainExecutionRole: () => this.executionRole,
},
});
}

/**
* @internal
*/
public _renderContainerDefinition(): CfnJobDefinition.ContainerPropertiesProperty {
return {
image: Tokenization.resolve(this.imageConfig, {
scope: this,
resolver: new DefaultTokenResolver(new StringConcat()),
}).imageName,
image: this.imageConfig.imageName,
command: this.command,
environment: Object.keys(this.environment ?? {}).map((envKey) => ({
name: envKey,
Expand Down Expand Up @@ -792,15 +796,6 @@ export interface EcsEc2ContainerDefinitionProps extends EcsContainerDefinitionPr
* @default - no gpus
*/
readonly gpu?: number;

/**
* The role used by Amazon ECS container and AWS Fargate agents to make AWS API calls on your behalf.
*
* @see https://docs.aws.amazon.com/batch/latest/userguide/execution-IAM-role.html
*
* @default - a Role will be created if logging is specified, no role otherwise
*/
readonly executionRole?: iam.IRole;
}

/**
Expand All @@ -811,21 +806,11 @@ export class EcsEc2ContainerDefinition extends EcsContainerDefinitionBase implem
public readonly ulimits: Ulimit[];
public readonly gpu?: number;

/**
* The role used by Amazon ECS container and AWS Fargate agents to make AWS API calls on your behalf.
*
* @see https://docs.aws.amazon.com/batch/latest/userguide/execution-IAM-role.html
*
* @default - a Role will be created if logging is specified, no role otherwise
*/
public readonly executionRole?: iam.IRole;

constructor(scope: Construct, id: string, props: EcsEc2ContainerDefinitionProps) {
super(scope, id, props);
this.privileged = props.privileged;
this.ulimits = props.ulimits ?? [];
this.gpu = props.gpu;
this.executionRole = props.executionRole ?? (this.logDriverConfig ? createExecutionRole(this, 'ExecutionRole') : undefined);
}

/**
Expand Down Expand Up @@ -919,15 +904,6 @@ export interface EcsFargateContainerDefinitionProps extends EcsContainerDefiniti
* @default LATEST
*/
readonly fargatePlatformVersion?: ecs.FargatePlatformVersion;

/**
* The role used by Amazon ECS container and AWS Fargate agents to make AWS API calls on your behalf.
*
* @see https://docs.aws.amazon.com/batch/latest/userguide/execution-IAM-role.html
*
* @default - a Role will be created
*/
readonly executionRole?: iam.IRole;
}

/**
Expand All @@ -937,20 +913,10 @@ export class EcsFargateContainerDefinition extends EcsContainerDefinitionBase im
public readonly fargatePlatformVersion?: ecs.FargatePlatformVersion;
public readonly assignPublicIp?: boolean;

/**
* The role used by Amazon ECS container and AWS Fargate agents to make AWS API calls on your behalf.
*
* @see https://docs.aws.amazon.com/batch/latest/userguide/execution-IAM-role.html
*
* @default - a Role will be created
*/
public readonly executionRole: iam.IRole;

constructor(scope: Construct, id: string, props: EcsFargateContainerDefinitionProps) {
super(scope, id, props);
this.assignPublicIp = props.assignPublicIp;
this.fargatePlatformVersion = props.fargatePlatformVersion;
this.executionRole = props.executionRole ?? createExecutionRole(this, 'ExecutionRole');
}

/**
Expand Down
81 changes: 81 additions & 0 deletions packages/@aws-cdk/aws-batch-alpha/test/ecs-container-definition.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { Template } from 'aws-cdk-lib/assertions';
import * as path from 'path';
import { Vpc } from 'aws-cdk-lib/aws-ec2';
import * as ecs from 'aws-cdk-lib/aws-ecs';
import * as ecr from 'aws-cdk-lib/aws-ecr';
import * as efs from 'aws-cdk-lib/aws-efs';
import { ArnPrincipal, Role } from 'aws-cdk-lib/aws-iam';
import * as logs from 'aws-cdk-lib/aws-logs';
Expand All @@ -11,6 +12,7 @@ import { Size, Stack } from 'aws-cdk-lib';
import { EcsContainerDefinitionProps, EcsEc2ContainerDefinition, EcsFargateContainerDefinition, EcsJobDefinition, EcsVolume, IEcsEc2ContainerDefinition, LinuxParameters, UlimitName } from '../lib';
import { CfnJobDefinitionProps } from 'aws-cdk-lib/aws-batch';
import { capitalizePropertyNames } from './utils';
import { DockerImageAsset } from 'aws-cdk-lib/aws-ecr-assets';

// GIVEN
const defaultContainerProps: EcsContainerDefinitionProps = {
Expand Down Expand Up @@ -528,6 +530,85 @@ describe.each([EcsEc2ContainerDefinition, EcsFargateContainerDefinition])('%p',
},
});
});

test('correctly renders docker images', () => {
// WHEN
new EcsJobDefinition(stack, 'ECSJobDefn', {
container: new ContainerDefinition(stack, 'EcsContainer', {
...defaultContainerProps,
image: ecs.ContainerImage.fromDockerImageAsset(new DockerImageAsset(stack, 'dockerImageAsset', {
directory: path.join(__dirname, 'batchjob-image'),
})),
}),
});

// THEN
Template.fromStack(stack).hasResourceProperties('AWS::Batch::JobDefinition', {
...pascalCaseExpectedProps,
ContainerProperties: {
...pascalCaseExpectedProps.ContainerProperties,
Image: {
'Fn::Sub': '${AWS::AccountId}.dkr.ecr.${AWS::Region}.${AWS::URLSuffix}/cdk-hnb659fds-container-assets-${AWS::AccountId}-${AWS::Region}:8b518243ecbfcfd08b4734069e7e74ff97b7889dfde0a60d16e7bdc96e6c593b',
},
},
});
});

test('correctly renders images from repositories', () => {
// GIVEN
const repo = new ecr.Repository(stack, 'Repo');

// WHEN
new EcsJobDefinition(stack, 'ECSJobDefn', {
container: new ContainerDefinition(stack, 'EcsContainer', {
...defaultContainerProps,
image: ecs.ContainerImage.fromEcrRepository(repo, 'my-tag'),
}),
});

// THEN
Template.fromStack(stack).hasResourceProperties('AWS::Batch::JobDefinition', {
...pascalCaseExpectedProps,
ContainerProperties: {
...pascalCaseExpectedProps.ContainerProperties,
Image: {
'Fn::Join': [
'',
[
{
'Fn::Select': [
4,
{
'Fn::Split': [
':',
{ 'Fn::GetAtt': ['Repo02AC86CF', 'Arn'] },
],
},
],
},
'.dkr.ecr.',
{
'Fn::Select': [
3,
{
'Fn::Split': [
':',
{ 'Fn::GetAtt': ['Repo02AC86CF', 'Arn'] },
],
},
],
},
'.',
{ Ref: 'AWS::URLSuffix' },
'/',
{ Ref: 'Repo02AC86CF' },
':my-tag',
],
],
},
},
});
});
});

describe('EC2 containers', () => {
Expand Down
2 changes: 1 addition & 1 deletion ...finition.js.snapshot/BatchEcsJobDefinitionTestDefaultTestDeployAssertE5BAAC9B.assets.json
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"version": "30.1.0",
"version": "31.0.0",
"files": {
"21fbb51d7b23f6a6c262b46a9caee79d744a3ac019fd45422d988b96d44b2a22": {
"source": {
Expand Down
5 changes: 5 additions & 0 deletions ...napshot/asset.8b518243ecbfcfd08b4734069e7e74ff97b7889dfde0a60d16e7bdc96e6c593b/Dockerfile
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
FROM public.ecr.aws/lambda/python:3.6
EXPOSE 8000
WORKDIR /src
ADD . /src
CMD python3 index.py
6 changes: 6 additions & 0 deletions ....snapshot/asset.8b518243ecbfcfd08b4734069e7e74ff97b7889dfde0a60d16e7bdc96e6c593b/index.py
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
#!/usr/bin/python
import os
import pprint

print('Hello from Batch!')
pprint.pprint(dict(os.environ))
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-batch-alpha/test/integ.ecs-job-definition.js.snapshot/cdk.out
View file
Original file line number Diff line number Diff line change
@@ -1 +1 @@
{"version":"30.1.0"}
{"version":"31.0.0"}
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-batch-alpha/test/integ.ecs-job-definition.js.snapshot/integ.json
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"version": "30.1.0",
"version": "31.0.0",
"testCases": {
"BatchEcsJobDefinitionTest/DefaultTest": {
"stacks": [
Expand Down
Loading

0 comments on commit b3d0d57

Please sign in to comment.