fix(batch): managed compute environment now properly works with compu…

…te resources and instanceRole has correct docstring and type definition (aws#6549)

packages/@aws-cdk/aws-batch/lib/compute-environment.ts

@@ -51,11 +51,33 @@ export enum AllocationStrategy {
  */
 export interface ComputeResources {
   /**
-   * The IAM role applied to EC2 resources in the compute environment.
+   * The allocation strategy to use for the compute resource in case not enough instances of the best
+   * fitting instance type can be allocated. This could be due to availability of the instance type in
+   * the region or Amazon EC2 service limits. If this is not specified, the default for the EC2
+   * ComputeResourceType is BEST_FIT, which will use only the best fitting instance type, waiting for
+   * additional capacity if it's not available. This allocation strategy keeps costs lower but can limit
+   * scaling. If you are using Spot Fleets with BEST_FIT then the Spot Fleet IAM Role must be specified.
+   * BEST_FIT_PROGRESSIVE will select an additional instance type that is large enough to meet the
+   * requirements of the jobs in the queue, with a preference for an instance type with a lower cost.
+   * The default value for the SPOT instance type is SPOT_CAPACITY_OPTIMIZED, which is only available for
+   * for this type of compute resources and will select an additional instance type that is large enough
+   * to meet the requirements of the jobs in the queue, with a preference for an instance type that is
+   * less likely to be interrupted.
+   *
+   * @default AllocationStrategy.BEST_FIT
+   */
+  readonly allocationStrategy?: AllocationStrategy;
+
+  /**
+   * The Amazon ECS instance profile applied to Amazon EC2 instances in a compute environment. You can specify
+   * the short name or full Amazon Resource Name (ARN) of an instance profile. For example, ecsInstanceRole or
+   * arn:aws:iam::<aws_account_id>:instance-profile/ecsInstanceRole . For more information, see Amazon ECS
+   * Instance Role in the AWS Batch User Guide.
    *
    * @default - a new role will be created.
+   * @link https://docs.aws.amazon.com/batch/latest/userguide/instance_IAM_role.html
    */
-  readonly instanceRole?: iam.IRole;
+  readonly instanceRole?: string;
 
   /**
    * The types of EC2 instances that may be launched in the compute environment. You can specify instance
@@ -70,7 +92,7 @@ export interface ComputeResources {
   /**
    * The EC2 security group(s) associated with instances launched in the compute environment.
    *
-   * @default AWS default security group.
+   * @default - AWS default security group.
    */
   readonly securityGroups?: ec2.ISecurityGroup[];
 
@@ -133,7 +155,7 @@ export interface ComputeResources {
    * The EC2 key pair that is used for instances launched in the compute environment.
    * If no key is defined, then SSH access is not allowed to provisioned compute resources.
    *
-   * @default - No SSH access will be possible.
+   * @default - no SSH access will be possible.
    */
   readonly ec2KeyPair?: string;
 
@@ -169,40 +191,25 @@ export interface ComputeResources {
  * Properties for creating a new Compute Environment
  */
 export interface ComputeEnvironmentProps {
-  /**
-   * The allocation strategy to use for the compute resource in case not enough instances ofthe best
-   * fitting instance type can be allocated. This could be due to availability of the instance type in
-   * the region or Amazon EC2 service limits. If this is not specified, the default is BEST_FIT, which
-   * will use only the best fitting instance type, waiting for additional capacity if it's not available.
-   * This allocation strategy keeps costs lower but can limit scaling. If you are using Spot Fleets with
-   * BEST_FIT then the Spot Fleet IAM Role must be specified. BEST_FIT_PROGRESSIVE will select an additional
-   * instance type that is large enough to meet the requirements of the jobs in the queue, with a preference
-   * for an instance type with a lower cost. SPOT_CAPACITY_OPTIMIZED is only available for Spot Instance
-   * compute resources and will select an additional instance type that is large enough to meet the requirements
-   * of the jobs in the queue, with a preference for an instance type that is less likely to be interrupted.
-   *
-   * @default AllocationStrategy.BEST_FIT
-   */
-  readonly allocationStrategy?: AllocationStrategy;
-
   /**
    * A name for the compute environment.
    *
    * Up to 128 letters (uppercase and lowercase), numbers, hyphens, and underscores are allowed.
    *
-   * @default Cloudformation-generated name
+   * @default - CloudFormation-generated name
    */
   readonly computeEnvironmentName?: string;
 
   /**
-   * The details of the compute resources managed by this environment.
+   * The details of the required compute resources for the managed compute environment.
    *
-   * If specified, and this is an managed compute environment, the property will be ignored.
+   * If specified, and this is an unmanaged compute environment, will throw an error.
    *
    * By default, AWS Batch managed compute environments use a recent, approved version of the
    * Amazon ECS-optimized AMI for compute resources.
    *
-   * @default - AWS-managed compute resources
+   * @default - CloudFormation defaults
+   * @link https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-batch-computeenvironment-computeresources.html
    */
   readonly computeResources?: ComputeResources;
 
@@ -302,19 +309,29 @@ export class ComputeEnvironment extends Resource implements IComputeEnvironment
     const spotFleetRole = this.getSpotFleetRole(props);
     let computeResources: CfnComputeEnvironment.ComputeResourcesProperty | undefined;
 
-    // Only allow compute resources to be set when using UNMANAGED type
-    if (props.computeResources && !this.isManaged(props)) {
+    // Only allow compute resources to be set when using MANAGED type
+    if (props.computeResources && this.isManaged(props)) {
       computeResources = {
-        allocationStrategy: props.allocationStrategy || AllocationStrategy.BEST_FIT,
+        allocationStrategy: props.computeResources.allocationStrategy
+         || (
+             props.computeResources.type === ComputeResourceType.SPOT
+             ? AllocationStrategy.SPOT_CAPACITY_OPTIMIZED
+             : AllocationStrategy.BEST_FIT
+            ),
         bidPercentage: props.computeResources.bidPercentage,
         desiredvCpus: props.computeResources.desiredvCpus,
         ec2KeyPair: props.computeResources.ec2KeyPair,
         imageId: props.computeResources.image && props.computeResources.image.getImage(this).imageId,
         instanceRole: props.computeResources.instanceRole
-          ? props.computeResources.instanceRole.roleArn
-          : new iam.Role(this, 'Resource-Instance-Role', {
-            assumedBy: new iam.ServicePrincipal('batch.amazonaws.com'),
-          }).roleArn,
+          ? props.computeResources.instanceRole
+          : new iam.CfnInstanceProfile(this, 'Instance-Profile', {
+              roles: [ new iam.Role(this, 'Ecs-Instance-Role', {
+                assumedBy: new iam.ServicePrincipal('ec2.amazonaws.com'),
+                managedPolicies: [
+                    iam.ManagedPolicy.fromAwsManagedPolicyName('service-role/AmazonEC2ContainerServiceforEC2Role')
+                    ]
+              }).roleName]
+          }).attrArn,
         instanceTypes: this.buildInstanceTypes(props.computeResources.instanceTypes),
         maxvCpus: props.computeResources.maxvCpus || 256,
         minvCpus: props.computeResources.minvCpus || 0,
@@ -338,7 +355,7 @@ export class ComputeEnvironment extends Resource implements IComputeEnvironment
           assumedBy: new iam.ServicePrincipal('batch.amazonaws.com'),
         }).roleArn,
       state: props.enabled === undefined ? 'ENABLED' : (props.enabled ? 'ENABLED' : 'DISABLED'),
-      type: this.isManaged(props) ? 'UNMANAGED' : 'MANAGED',
+      type: this.isManaged(props) ? 'MANAGED' : 'UNMANAGED',
     });
 
     if (props.computeResources && props.computeResources.vpc) {
@@ -365,12 +382,12 @@ export class ComputeEnvironment extends Resource implements IComputeEnvironment
       return;
     }
 
-    if (this.isManaged(props) && props.computeResources !== undefined) {
-      throw new Error('It is not allowed to set computeResources on an AWS managed compute environment');
+    if (!this.isManaged(props) && props.computeResources !== undefined) {
+      throw new Error('It is not allowed to set computeResources on an AWS unmanaged compute environment');
     }
 
-    if (!this.isManaged(props) && props.computeResources === undefined) {
-      throw new Error('computeResources is missing but required on an unmanaged compute environment');
+    if (this.isManaged(props) && props.computeResources === undefined) {
+      throw new Error('computeResources is missing but required on a managed compute environment');
     }
 
     // Setting a bid percentage is only allowed on SPOT resources +
@@ -385,7 +402,7 @@ export class ComputeEnvironment extends Resource implements IComputeEnvironment
         }
 
         // SPOT_CAPACITY_OPTIMIZED allocation is not allowed
-        if (props.allocationStrategy && props.allocationStrategy === AllocationStrategy.SPOT_CAPACITY_OPTIMIZED) {
+        if (props.computeResources.allocationStrategy && props.computeResources.allocationStrategy === AllocationStrategy.SPOT_CAPACITY_OPTIMIZED) {
           throw new Error('The SPOT_CAPACITY_OPTIMIZED allocation strategy is only allowed if the environment is a SPOT type compute environment');
         }
       } else {
@@ -440,7 +457,7 @@ export class ComputeEnvironment extends Resource implements IComputeEnvironment
    * @param props - the compute environment construct properties
    */
   private getSpotFleetRole(props: ComputeEnvironmentProps): iam.IRole | undefined {
-    if (props.allocationStrategy && props.allocationStrategy !== AllocationStrategy.BEST_FIT) {
+    if (props.computeResources?.allocationStrategy && props.computeResources.allocationStrategy !== AllocationStrategy.BEST_FIT) {
       return undefined;
     }
 

packages/@aws-cdk/aws-batch/test/compute-environment.test.ts

@@ -1,11 +1,11 @@
-import { expect, haveResource, haveResourceLike, ResourcePart } from '@aws-cdk/assert';
+import {expect, haveResource, haveResourceLike, ResourcePart} from '@aws-cdk/assert';
 import '@aws-cdk/assert/jest';
 import * as ec2 from '@aws-cdk/aws-ec2';
 import * as ecs from '@aws-cdk/aws-ecs';
 import * as iam from '@aws-cdk/aws-iam';
 import * as cdk from '@aws-cdk/core';
-import { throws } from 'assert';
-import * as batch from '../lib';
+import {throws} from 'assert';
+import * as batch from "../lib";
 
 describe('Batch Compute Evironment', () => {
   let expectedManagedDefaultComputeProps: any;
@@ -34,7 +34,7 @@ describe('Batch Compute Evironment', () => {
           AllocationStrategy: batch.AllocationStrategy.BEST_FIT,
           InstanceRole: {
             'Fn::GetAtt': [
-              'testcomputeenvResourceInstanceRole7FD819B9',
+              'testcomputeenvInstanceProfileCBD87EAB',
               'Arn'
             ]
           },
@@ -59,37 +59,39 @@ describe('Batch Compute Evironment', () => {
   });
 
   describe('when validating props', () => {
-    test('should deny setting compute resources when using type managed', () => {
+    test('should deny setting compute resources when using type unmanaged', () => {
       // THEN
       throws(() => {
         // WHEN
         new batch.ComputeEnvironment(stack, 'test-compute-env', {
+          managed: false,
           computeResources: {
-            vpc,
+            vpc
           },
         });
       });
     });
 
-    test('should deny if creating an unmanged environment with no provided compute resource props', () => {
+    test('should deny if creating a managed environment with no provided compute resource props', () => {
       // THEN
       throws(() => {
         // WHEN
         new batch.ComputeEnvironment(stack, 'test-compute-env', {
-          managed: false,
+          managed: true,
         });
       });
     });
   });
 
   describe('using spot resources', () => {
-    test('should provide a spotfleet role if one is not given', () => {
+    test('should provide a spot fleet role if one is not given and allocationStrategy is BEST_FIT', () => {
       // WHEN
       new batch.ComputeEnvironment(stack, 'test-compute-env', {
-        managed: false,
+        managed: true,
         computeResources: {
           type: batch.ComputeResourceType.SPOT,
-          vpc,
+          allocationStrategy: batch.AllocationStrategy.BEST_FIT,
+          vpc
         },
       });
 
@@ -124,7 +126,7 @@ describe('Batch Compute Evironment', () => {
         throws(() => {
           // WHEN
           new batch.ComputeEnvironment(stack, 'test-compute-env', {
-            managed: false,
+            managed: true,
             computeResources: {
               vpc,
               type: batch.ComputeResourceType.SPOT,
@@ -139,7 +141,7 @@ describe('Batch Compute Evironment', () => {
         throws(() => {
           // WHEN
           new batch.ComputeEnvironment(stack, 'test-compute-env', {
-            managed: false,
+            managed: true,
             computeResources: {
               vpc,
               type: batch.ComputeResourceType.SPOT,
@@ -155,9 +157,9 @@ describe('Batch Compute Evironment', () => {
     test('renders the correct cloudformation properties', () => {
       // WHEN
       const props = {
-        allocationStrategy: batch.AllocationStrategy.BEST_FIT,
         computeEnvironmentName: 'my-test-compute-env',
         computeResources: {
+          allocationStrategy: batch.AllocationStrategy.BEST_FIT,
           vpc,
           computeResourcesTags: new cdk.Tag('foo', 'bar'),
           desiredvCpus: 1,
@@ -166,9 +168,14 @@ describe('Batch Compute Evironment', () => {
             generation: ec2.AmazonLinuxGeneration.AMAZON_LINUX_2,
             hardwareType: ecs.AmiHardwareType.STANDARD,
           }),
-          instanceRole: new iam.Role(stack, 'test-compute-env-instance-role', {
-            assumedBy: new iam.ServicePrincipal('batch.amazonaws.com'),
-          }),
+          instanceRole:  new iam.CfnInstanceProfile(stack, 'Instance-Profile', {
+            roles: [ new iam.Role(stack, 'Ecs-Instance-Role', {
+              assumedBy: new iam.ServicePrincipal('ec2.amazonaws.com'),
+              managedPolicies: [
+                iam.ManagedPolicy.fromAwsManagedPolicyName('service-role/AmazonEC2ContainerServiceforEC2Role')
+              ]
+            }).roleName]
+          }).attrArn,
           instanceTypes: [
             ec2.InstanceType.of(ec2.InstanceClass.T2, ec2.InstanceSize.MICRO),
           ],
@@ -186,7 +193,7 @@ describe('Batch Compute Evironment', () => {
           },
         } as batch.ComputeResources,
         enabled: false,
-        managed: false,
+        managed: true,
       };
 
       new batch.ComputeEnvironment(stack, 'test-compute-env', props);
@@ -211,7 +218,7 @@ describe('Batch Compute Evironment', () => {
           },
           InstanceRole: {
             'Fn::GetAtt': [
-              props.computeResources.instanceRole ? `${props.computeResources.instanceRole.node.uniqueId}F3B86D94` : '',
+              props.computeResources.instanceRole ? "InstanceProfile" : '',
               'Arn'
             ]
           },
@@ -251,7 +258,7 @@ describe('Batch Compute Evironment', () => {
       test('should default to a best_fit strategy', () => {
         // WHEN
         new batch.ComputeEnvironment(stack, 'test-compute-env', {
-          managed: false,
+          managed: true,
           computeResources: {
             vpc,
           },
@@ -303,7 +310,7 @@ describe('Batch Compute Evironment', () => {
       test('should default to 0', () => {
         // WHEN
         new batch.ComputeEnvironment(stack, 'test-compute-env', {
-          managed: false,
+          managed: true,
           computeResources: {
             vpc,
           },
@@ -323,7 +330,7 @@ describe('Batch Compute Evironment', () => {
       test('should default to 256', () => {
         // WHEN
         new batch.ComputeEnvironment(stack, 'test-compute-env', {
-          managed: false,
+          managed: true,
           computeResources: {
             vpc,
           },
@@ -342,7 +349,7 @@ describe('Batch Compute Evironment', () => {
       test('should generate a role for me', () => {
         // WHEN
         new batch.ComputeEnvironment(stack, 'test-compute-env', {
-          managed: false,
+          managed: true,
           computeResources: {
             vpc,
           },
@@ -358,7 +365,7 @@ describe('Batch Compute Evironment', () => {
       test('should default to optimal matching', () => {
         // WHEN
         new batch.ComputeEnvironment(stack, 'test-compute-env', {
-          managed: false,
+          managed: true,
           computeResources: {
             vpc,
           },
@@ -377,7 +384,7 @@ describe('Batch Compute Evironment', () => {
       test('should default to EC2', () => {
         // WHEN
         new batch.ComputeEnvironment(stack, 'test-compute-env', {
-          managed: false,
+          managed: true,
           computeResources: {
             vpc,
           },