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

(aws-batch): ComputeEnvironment should implement IConnectable #20983

Closed
1 of 2 tasks
yoshizawa56 opened this issue Jul 4, 2022 · 2 comments · Fixed by #21458
Closed
1 of 2 tasks

(aws-batch): ComputeEnvironment should implement IConnectable #20983

yoshizawa56 opened this issue Jul 4, 2022 · 2 comments · Fixed by #21458
Assignees
Labels
@aws-cdk/aws-batch Related to AWS Batch feature/service-integration Add functionality to an L2 construct to enable easier integration with another service feature-request A feature should be added or improved. needs-design This feature request needs additional design work.

Comments

@yoshizawa56
Copy link
Contributor

Describe the feature

Once a ComputeEnvironment is created, there is no way to access the security group associated with it. We would like to implement IConectable interface in the ComputeEnvironment to more easily control connections from the batch container.

Use Case

Current implementation requires 3 steps below when creating batch with db connection.

  1. create a security group
  2. create the computing resource and associate with the security group created in 1.
  3. run db.connections.allowFrom(sg, ...)

If ComputeEnvironment implements IConnectable, it become more simple

  1. create computing environment without explicit security group association
  2. run db.connections.allowFrom(computeEnv, ...)

Proposed Solution

No response

Other Information

No response

Acknowledgements

  • I may be able to implement this feature request
  • This feature might incur a breaking change

CDK version used

2.24

Environment details (OS name and version, etc.)

ubuntu 18.04

@yoshizawa56 yoshizawa56 added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Jul 4, 2022
@github-actions github-actions bot added the @aws-cdk/aws-batch Related to AWS Batch label Jul 4, 2022
@indrora indrora added needs-design This feature request needs additional design work. feature/service-integration Add functionality to an L2 construct to enable easier integration with another service and removed needs-triage This issue or PR still needs to be triaged. labels Jul 20, 2022
@tcutts
Copy link
Contributor

tcutts commented Aug 4, 2022

I encountered this issue when working on a PoC for a customer.
In my case I was adding EFS volumes to the container definitions. The EFS filesystem needs to have appropriate connection changes to allow batch containers to access the filesystem. I found myself having to do this to get connectivity:

declare const fs: efs.FileSystem;

fs.connections.allowDefaultPortFromAnyIpv4();

which is clearly very bad practice, even if it's all in a private subnet. The security groups which which the Batch construct creates for each ComputeEnvironment need to be available, so I agree that ComputeEnvironment needs to implement IConnectable. I have implemented this, in a way which does not seem to have added any integration problems. I also added an integration test, but I'm not 💯 % comfortable this is the right design, it's not quite high level enough to make this really suitable for the user.

For a start, the computeEnvironments are not exposed by the JobQueue that contains them, so with my implementation the user still has to create the environments themselves and alter them before creating the batch queue as follows, which seems a bit cumbersome:

const computeEnvironments = [
  {
    computeEnvironment: new batch.ComputeEnvironment(stack, 'batch-demand-compute-env-launch-template', {
      managed: true,
      computeResources: {
        type: batch.ComputeResourceType.ON_DEMAND,
        vpc,
        launchTemplate: {
          launchTemplateName: launchTemplate.launchTemplateName as string,
        },
      },
    }),
    order: 2,
  },
  {
    computeEnvironment: new batch.ComputeEnvironment(stack, 'batch-spot-compute-env', {
      managed: true,
      computeResources: {
        type: batch.ComputeResourceType.SPOT,
        vpc,
        bidPercentage: 80,
      },
    }),
    order: 3,
  },
];

// at least with ComputeEnvironment implementing IConnectable, we now have sensible ingress rules:
computeEnvironments.forEach((ce) => {
  fs.connections.allowDefaultPortFrom(ce.computeEnvironment);
});

new batch.JobQueue(stack, 'batch-job-queue', { computeEnvironments });

I'll link a pull request to this issue, to show what I've done, although I'm not expecting it to be the final design that gets merged, more that it's food for thought about how this could be implemented.

@mergify mergify bot closed this as completed in #21458 Aug 5, 2022
mergify bot pushed a commit that referenced this issue Aug 5, 2022
closes #20983

ComputeEnvironments have embedded security groups in them.  These are currently difficult to reach and modify in CDK stacks, which forces users into undesirable practices when integrating batch queues with other services such as as EFS filesystems or RDS instances.  Ideally, it should be possible to use compute environments as a target:

```ts
something.connections.allowDefaultPortFrom(computeEnvironment);
```

but this isn't currently possible, so the user may try to work around it by allowing from any IPv4, or by having to use an escape hatch, which is not simple.

This pull request adds ec2.IConnectable to batch.ComputeEnvironment.  It still seems to pass all the existing tests and integration tests, so I don't think it's a breaking change, but I suspect it could be done in a better way than I've done it, to make things even simpler for the user.

----

### All Submissions:

* [yes] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [no] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [yes] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [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*
@github-actions
Copy link

github-actions bot commented Aug 5, 2022

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

josephedward pushed a commit to josephedward/aws-cdk that referenced this issue Aug 30, 2022
closes aws#20983

ComputeEnvironments have embedded security groups in them.  These are currently difficult to reach and modify in CDK stacks, which forces users into undesirable practices when integrating batch queues with other services such as as EFS filesystems or RDS instances.  Ideally, it should be possible to use compute environments as a target:

```ts
something.connections.allowDefaultPortFrom(computeEnvironment);
```

but this isn't currently possible, so the user may try to work around it by allowing from any IPv4, or by having to use an escape hatch, which is not simple.

This pull request adds ec2.IConnectable to batch.ComputeEnvironment.  It still seems to pass all the existing tests and integration tests, so I don't think it's a breaking change, but I suspect it could be done in a better way than I've done it, to make things even simpler for the user.

----

### All Submissions:

* [yes] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [no] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [yes] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [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*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-batch Related to AWS Batch feature/service-integration Add functionality to an L2 construct to enable easier integration with another service feature-request A feature should be added or improved. needs-design This feature request needs additional design work.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants