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

feat(core): add volumes-from option to docker run command for bundling #22829

Merged
merged 30 commits into from Dec 12, 2022
Merged

feat(core): add volumes-from option to docker run command for bundling #22829

merged 30 commits into from Dec 12, 2022

Conversation

webratz
Copy link
Contributor

@webratz webratz commented Nov 8, 2022

relates to #8799
follow up to stale #21660

Describe the feature

Ability to add --volumes-from flag when bundling assets with docker.
This enabled people using Docker in Docker to use CDKs bundling functionality, which is currently not possible.

Use Case

CICD systems often run within a docker container already. Many systems mount the /var/run/docker.sock from the host system into the CICD container. When running bundling within such a container it currently breaks, as docker assume the path is from the host system, not within the CICD container.
The options allows to mount the data from any other container. Very often it will be the current one which can be used by using the HOSTNAME environment variable

Proposed Solution

Add optional property to DockerRunOptions and BundlingOptions that would translate into --volumes-from {user provided option}

This change would not reflect in any CloudFormation changes, but only with the docker commands performed when bundling.

Due to using the --volumes-from option, docker will instead of trying to find the path on the host (where it does not exist) try to use the volume that is created by the container C1 that is actually running the CDK. With that it is able to access the files from CDK and can continue the build.

Docker volumes from

The following plain docker steps show how this works from the docker side, and why we need to adjust the --volumes-from parameter.

docker volume create builds
docker run -v /var/run/docker.sock:/var/run/docker.sock -v builds:/builds -it docker

Now within the just created docker container, run the following commands.

echo "testfile" > /builds/my-share-file.txt
docker run --rm --name DinDContainer --volumes-from="${HOSTNAME}" ubuntu bash -c "ls -hla /builds"

We see that the second container C2 (here DinDContainer) has the same files available as the container C1.

Alternative solutions

I'm not aware of alternative solutions for this docker in docker use cases, besides of not relying on docker at all, which is out of scope for this MR.


All Submissions:

Adding new Unconventional Dependencies:

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

New Features

  • Have you added the new feature to an integration test?
    • Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?
      I ran it, but it seems not to have generated something, i might need some guidance there.

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

@gitpod-io
Copy link

gitpod-io bot commented Nov 8, 2022

@github-actions github-actions bot added the p2 label Nov 8, 2022
@aws-cdk-automation aws-cdk-automation requested a review from a team November 8, 2022 11:56
@github-actions github-actions bot added the beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK label Nov 8, 2022
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

@aws-cdk-automation aws-cdk-automation dismissed their stale review November 8, 2022 14:22

✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.

@webratz
Copy link
Contributor Author

webratz commented Nov 8, 2022

The failure is sort of expected:

@aws-cdk/aws-lambda-python: docker: Error response from daemon: No such container: 941e5a523af5.

This due to the fact, that the CodeBuild pipeline does not work like the CICD systems that this PR is targeting to enable to build.
Is there a way to either influence the calls to docker in the integration test or similar things?
Edit: found a way in the meanwhile. All tests are passing and are actually testing what they should

this is overall unfavourable as it makes the test very complex. its not reproducible locally so i need to push this to AWS ci
@webratz webratz marked this pull request as ready for review November 9, 2022 09:19
@mrgrain mrgrain self-assigned this Dec 8, 2022
Co-authored-by: Momo Kornher <mail@moritzkornher.de>
@mergify mergify bot dismissed mrgrain’s stale review December 9, 2022 07:21

Pull request has been modified.

mrgrain
mrgrain previously requested changes Dec 9, 2022
Copy link
Contributor

@mrgrain mrgrain left a comment

Choose a reason for hiding this comment

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

Glad you made it working, but I really don't see any point in the test. It literally just tests docker. Sorry about that!

To get this one merged asap, let's remove the integ test and revert customCommands as well.

I'd also think we should create parity of bundling options for all three functions we have (node, go, python). So what I'm pretty much asking you is to revert all python changes here and than create second PR that adds unified support for that as well as some of the other Docker options. Hope that's okay.

packages/@aws-cdk/core/test/bundling.test.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-lambda-python/lib/types.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-lambda-python/README.md Outdated Show resolved Hide resolved
@mergify mergify bot dismissed mrgrain’s stale review December 12, 2022 07:45

Pull request has been modified.

Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

@webratz
Copy link
Contributor Author

webratz commented Dec 12, 2022

this is all stripped down now to the minimum.
As discussed the integration tests and changes in the python module are now all removed.
This makes the PR checker unhappy, but i assume you know what to do it to grant exemptions there.

@mrgrain mrgrain added pr-linter/exempt-readme The PR linter will not require README changes pr-linter/exempt-integ-test The PR linter will not require integ test changes labels Dec 12, 2022
mrgrain
mrgrain previously approved these changes Dec 12, 2022
Copy link
Contributor

@mrgrain mrgrain left a comment

Choose a reason for hiding this comment

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

Thanks @webratz , I appreciate you working through this with me. If you're interested, you could look at extending/aligning the function bundling interfaces next.

@aws-cdk-automation aws-cdk-automation dismissed their stale review December 12, 2022 08:16

✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.

@mergify mergify bot dismissed mrgrain’s stale review December 12, 2022 08:23

Pull request has been modified.

@mrgrain
Copy link
Contributor

mrgrain commented Dec 12, 2022

@webratz I'll take it from here. No need to keep updating the branch (in fact makes everything slower).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@mergify
Copy link
Contributor

mergify bot commented Dec 12, 2022

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).

@mergify mergify bot merged commit 813c2f1 into aws:main Dec 12, 2022
mergify bot pushed a commit that referenced this pull request Dec 15, 2022
…all lambda variants (#23318)

This continues the work started in #22829 by exposing the underlying docker run options of the container bundling.
With that the bundling feature can be used in a wider range of setups that the current defaults can not support out of the box.

The removed properties are covered in the same way by the one the interface extends from.

----

### All Submissions:

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

### Adding new Construct Runtime Dependencies:

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

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] 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*
brennanho pushed a commit to brennanho/aws-cdk that referenced this pull request Jan 20, 2023
aws#22829)

relates to aws#8799 
follow up to stale aws#21660

## Describe the feature
Ability to add [--volumes-from](https://docs.docker.com/engine/reference/commandline/run/#mount-volumes-from-container---volumes-from) flag when bundling assets with docker.
This enabled people using Docker in Docker to use CDKs bundling functionality, which is currently not possible.

## Use Case
CICD systems often run within a docker container already. Many systems mount the ` /var/run/docker.sock` from the host system into the CICD container. When running bundling within such a container it currently breaks, as docker assume the path is from the host system, not within the CICD container.
The options allows to mount the data from any other container. Very often it will be the current one which can be used by using the `HOSTNAME` environment variable

## Proposed Solution
Add optional property to [DockerRunOptions](https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.DockerRunOptions.html) and [BundlingOptions](https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.BundlingOptions.html) that would translate into --volumes-from {user provided option}

This change would not reflect in any CloudFormation changes, but only with the docker commands performed when bundling.

Due to using the `--volumes-from` option, docker will instead of trying to find the path on the host (where it does not exist) try to use the volume that is created by the container C1 that is actually running the CDK. With that it is able to access the files from CDK and can continue the build.

![Docker volumes from](https://user-images.githubusercontent.com/2162832/193787498-de03c66c-7bce-458b-9776-7ba421b9d929.jpg)

The following plain docker steps show how this works from the docker side, and why we need to adjust the `--volumes-from` parameter.

```sh
docker volume create builds
docker run -v /var/run/docker.sock:/var/run/docker.sock -v builds:/builds -it docker
```
Now within the just created docker container, run the following commands.

```sh
echo "testfile" > /builds/my-share-file.txt
docker run --rm --name DinDContainer --volumes-from="${HOSTNAME}" ubuntu bash -c "ls -hla /builds"
```
We see that the second container C2 (here `DinDContainer`) has the same files available as the container C1. 

## Alternative solutions

I'm not aware of alternative solutions for this docker in docker use cases, besides of not relying on docker at all, which is out of scope for this MR.

----

### All Submissions:

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

### Adding new Unconventional Dependencies:

* [ ] 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

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [x] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?
I ran it, but it seems not to have generated something, i might need some guidance there.

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
brennanho pushed a commit to brennanho/aws-cdk that referenced this pull request Jan 20, 2023
…all lambda variants (aws#23318)

This continues the work started in aws#22829 by exposing the underlying docker run options of the container bundling.
With that the bundling feature can be used in a wider range of setups that the current defaults can not support out of the box.

The removed properties are covered in the same way by the one the interface extends from.

----

### All Submissions:

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

### Adding new Construct Runtime Dependencies:

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

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] 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*
brennanho pushed a commit to brennanho/aws-cdk that referenced this pull request Feb 22, 2023
aws#22829)

relates to aws#8799 
follow up to stale aws#21660

## Describe the feature
Ability to add [--volumes-from](https://docs.docker.com/engine/reference/commandline/run/#mount-volumes-from-container---volumes-from) flag when bundling assets with docker.
This enabled people using Docker in Docker to use CDKs bundling functionality, which is currently not possible.

## Use Case
CICD systems often run within a docker container already. Many systems mount the ` /var/run/docker.sock` from the host system into the CICD container. When running bundling within such a container it currently breaks, as docker assume the path is from the host system, not within the CICD container.
The options allows to mount the data from any other container. Very often it will be the current one which can be used by using the `HOSTNAME` environment variable

## Proposed Solution
Add optional property to [DockerRunOptions](https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.DockerRunOptions.html) and [BundlingOptions](https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.BundlingOptions.html) that would translate into --volumes-from {user provided option}

This change would not reflect in any CloudFormation changes, but only with the docker commands performed when bundling.

Due to using the `--volumes-from` option, docker will instead of trying to find the path on the host (where it does not exist) try to use the volume that is created by the container C1 that is actually running the CDK. With that it is able to access the files from CDK and can continue the build.

![Docker volumes from](https://user-images.githubusercontent.com/2162832/193787498-de03c66c-7bce-458b-9776-7ba421b9d929.jpg)

The following plain docker steps show how this works from the docker side, and why we need to adjust the `--volumes-from` parameter.

```sh
docker volume create builds
docker run -v /var/run/docker.sock:/var/run/docker.sock -v builds:/builds -it docker
```
Now within the just created docker container, run the following commands.

```sh
echo "testfile" > /builds/my-share-file.txt
docker run --rm --name DinDContainer --volumes-from="${HOSTNAME}" ubuntu bash -c "ls -hla /builds"
```
We see that the second container C2 (here `DinDContainer`) has the same files available as the container C1. 

## Alternative solutions

I'm not aware of alternative solutions for this docker in docker use cases, besides of not relying on docker at all, which is out of scope for this MR.

----

### All Submissions:

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

### Adding new Unconventional Dependencies:

* [ ] 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

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [x] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?
I ran it, but it seems not to have generated something, i might need some guidance there.

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
brennanho pushed a commit to brennanho/aws-cdk that referenced this pull request Feb 22, 2023
…all lambda variants (aws#23318)

This continues the work started in aws#22829 by exposing the underlying docker run options of the container bundling.
With that the bundling feature can be used in a wider range of setups that the current defaults can not support out of the box.

The removed properties are covered in the same way by the one the interface extends from.

----

### All Submissions:

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

### Adding new Construct Runtime Dependencies:

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

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] 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
beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK p2 pr-linter/exempt-integ-test The PR linter will not require integ test changes pr-linter/exempt-readme The PR linter will not require README changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants