fix(batch): JobDefinition's ContainerDefinition's Image is synthesize…

…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*
aws · May 8, 2023 · b3d0d57 · b3d0d57
1 parent b76c182
commit b3d0d57
Show file tree

Hide file tree

Showing 13 changed files with 624 additions and 197 deletions.
diff --git a/docs/DESIGN_GUIDELINES.md b/docs/DESIGN_GUIDELINES.md
@@ -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

diff --git a/packages/@aws-cdk/aws-batch-alpha/lib/ecs-container-definition.ts b/packages/@aws-cdk/aws-batch-alpha/lib/ecs-container-definition.ts
@@ -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';
@@ -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,
@@ -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.
    *
@@ -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.
    *
@@ -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;
@@ -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) {
@@ -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,
@@ -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;
 }
 
 /**
@@ -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);
   }
 
   /**
@@ -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;
 }
 
 /**
@@ -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');
   }
 
   /**

diff --git a/packages/@aws-cdk/aws-batch-alpha/test/ecs-container-definition.test.ts b/packages/@aws-cdk/aws-batch-alpha/test/ecs-container-definition.test.ts
@@ -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';
@@ -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 = {
@@ -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', () => {

diff --git a/...finition.js.snapshot/BatchEcsJobDefinitionTestDefaultTestDeployAssertE5BAAC9B.assets.json b/...finition.js.snapshot/BatchEcsJobDefinitionTestDefaultTestDeployAssertE5BAAC9B.assets.json
@@ -1,5 +1,5 @@
 {
-  "version": "30.1.0",
+  "version": "31.0.0",
   "files": {
     "21fbb51d7b23f6a6c262b46a9caee79d744a3ac019fd45422d988b96d44b2a22": {
       "source": {

diff --git a/...napshot/asset.8b518243ecbfcfd08b4734069e7e74ff97b7889dfde0a60d16e7bdc96e6c593b/Dockerfile b/...napshot/asset.8b518243ecbfcfd08b4734069e7e74ff97b7889dfde0a60d16e7bdc96e6c593b/Dockerfile
@@ -0,0 +1,5 @@
+FROM public.ecr.aws/lambda/python:3.6
+EXPOSE 8000
+WORKDIR /src
+ADD . /src
+CMD python3 index.py
diff --git a/....snapshot/asset.8b518243ecbfcfd08b4734069e7e74ff97b7889dfde0a60d16e7bdc96e6c593b/index.py b/....snapshot/asset.8b518243ecbfcfd08b4734069e7e74ff97b7889dfde0a60d16e7bdc96e6c593b/index.py
@@ -0,0 +1,6 @@
+#!/usr/bin/python
+import os
+import pprint
+
+print('Hello from Batch!')
+pprint.pprint(dict(os.environ))
diff --git a/packages/@aws-cdk/aws-batch-alpha/test/integ.ecs-job-definition.js.snapshot/cdk.out b/packages/@aws-cdk/aws-batch-alpha/test/integ.ecs-job-definition.js.snapshot/cdk.out
@@ -1 +1 @@
-{"version":"30.1.0"}
+{"version":"31.0.0"}
diff --git a/packages/@aws-cdk/aws-batch-alpha/test/integ.ecs-job-definition.js.snapshot/integ.json b/packages/@aws-cdk/aws-batch-alpha/test/integ.ecs-job-definition.js.snapshot/integ.json
@@ -1,5 +1,5 @@
 {
-  "version": "30.1.0",
+  "version": "31.0.0",
   "testCases": {
     "BatchEcsJobDefinitionTest/DefaultTest": {
       "stacks": [