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(batch): JobDefinition's ContainerDefinition's Image is synthesized with [Object object] #25466

Merged
merged 4 commits into from
May 8, 2023
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
21 changes: 21 additions & 0 deletions docs/DESIGN_GUIDELINES.md
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
Copy link
Contributor

Choose a reason for hiding this comment

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

👍


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
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
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
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
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
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))
Original file line number Diff line number Diff line change
@@ -1 +1 @@
{"version":"30.1.0"}
{"version":"31.0.0"}
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