fix(aws-batch): Support omitting ComputeEnvironment security groups so that they can be specified in Launch Template

[tcutts]

HPC Batch applications frequently require Elastic Fabric Adapters for low-latency networking. Currently, the ComputeEnvironment construct always automatically defines a set of SecurityGroupIds in the CloudFormation it generates, and this prevents the stack deploying if the LaunchTemplate contains network interface definitions; Batch does not allow SecurityGroups at the ComputeEnvironment level if there are network interfaces defined in the CfnLaunchTemplate.

Since we do not currently have support for network interfaces this PR adds a new boolean property in launchTemplate called useNetworkInterfaceSecurityGroups. When this is enabled we will assume that security groups are being provided by the launch template.

A long term solution may be to:

• Add support for network interfaces in the L2 ec2.LaunchTemplate construct.
• Update the batch.ComputeEnvironment construct to take a ILaunchTemplate instead of the name/id.
• Check the ILaunchTemplate for whether the ComputeEnvironment needs to create any security groups.

closes #21577

---

All Submissions:

• [yes] Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

• [no] This PR adds new unconventional dependencies following the process described here

New Features

• [yes] Have you added the new feature to an integration test?
  • [yes] Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

[gitpod-io]

[corymhall]

@tcutts thanks for taking the time to create this PR! Unfortunately the union type is not currently supported in CDK so I don't think this solution will work.

Without looking into this too much, I think the ideal solution is probably a much bigger effort. I'm thinking that we need to:

1. Add support for network interfaces in the L2 ec2.LaunchTemplate construct.
2. Update the batch.ComputeEnvironment construct to take a ILaunchTemplate instead of the name/id.
3. Check the ILaunchTemplate for whether the ComputeEnvironment needs to create any security groups.

[tcutts]

On 15 Aug 2022, at 13:30, Cory Hall ***@***.***> wrote: @corymhall requested changes on this pull request. @tcutts <https://github.com/tcutts> thanks for taking the time to create this PR! Unfortunately the union type is not currently supported in CDK so I don't think this solution will work.

Ah, I didn’t know that. It did feel a bit dirty. :-)

Without looking into this too much, I think the ideal solution is probably a much bigger effort. I'm thinking that we need to: Add support for network interfaces in the L2 ec2.LaunchTemplate construct Update the batch.ComputeEnvironment construct to take a ILaunchTemplate instead of the name/id.

That would be a breaking change, wouldn’t it? I suppose the API is currently experimental, so that doesn’t matter quite so much...

Check the ILaunchTemplate for whether the ComputeEnvironment needs to create any security groups.

… I agree with this, because the combination of all three steps allows us to put a validation in.

…

In ***@***.***/aws-batch/lib/compute-environment.ts <#21579 (comment)>: > @@ -142,7 +161,7 @@ export interface ComputeResources { * * @default - AWS default security group. */ - readonly securityGroups?: ec2.ISecurityGroup[]; + readonly securityGroups?: ec2.ISecurityGroup[] | ComputeEnvironmentSecurityGroups; Unfortunately we can't use union types due to limitations with JSII. — Reply to this email directly, view it on GitHub <#21579 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AASQ7CWR6NMK7FQZDAJXUDDVZIZ4VANCNFSM56LNXTZQ>. You are receiving this because you were mentioned.

[tcutts]

This is potentially an enormous can of worms. CDK has no abstraction for networkInterfaces at all, anywhere. That needs to be done first before anything else that ought to support them can do so (Instance, AutoScalingGroup). This is way beyond my relatively hobbyist ability to implement.

[tcutts]

What I could do is revert to a simpler change, which is to change what happens if the user specifies [] as the list of security groups. If, in that case, we emit no SecurityGroupIds line in the resource, we still fix the bug, and we've no longer created the unsupported union type. That does leave us with slightly messy and misleading end user code:

const computeEnvironmentEFA = new batch.ComputeEnvironment(stack, 'EFAComputeEnv', {
  managed: true,
  computeResources: {
    securityGroups: [],
    vpc,
    launchTemplate: {
      launchTemplateName: launchTemplateEFA.launchTemplateName as string,
    },
  },
});

which I don't like very much, but would achieve the desired outcome. Arguably, ComputeEnvironment needs updating to do this anyway, because emitting an empty list in a set SecurityGroupIds property doesn't seem to be the right behaviour to me.

[corymhall]

What I could do is revert to a simpler change, which is to change what happens if the user specifies [] as the list of security groups. If, in that case, we emit no SecurityGroupIds line in the resource, we still fix the bug, and we've no longer created the unsupported union type. That does leave us with slightly messy and misleading end user code code:

I agree that this isn't what we want, but since this package is experimental and given the complexity of the preferred solution I think I'm fine with this as an interim solution.

Pull request has been modified.

[corymhall]

Looks good! just 1 minor change requested.

Pull request has been modified.

[mergify]

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
• Commit ID: 6d3668a
• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[mergify]

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).