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

webratz · 2022-08-18T11:49:22Z

relates to #8799

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.

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:

Have you followed the guidelines in our Contributing guide?

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 · 2022-08-18T11:49:30Z

webratz · 2022-08-18T13:43:38Z

I'm not sure how to make the tests work there, as they expect a specific way of how docker is set up, and in which the code is run to test it.
I assume we can not really change the build environment. Happy for any pointers how we could better test this. (The tests work locally, but of course they need to work in the CI env)

aws-cdk-automation · 2022-09-08T13:00:43Z

This PR has been in the BUILD FAILING state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

webratz · 2022-09-15T10:27:57Z

all tests, except for the probably not fixable docker integration tests, are now working.

TheRealAmazonKendra

Please make sure that your PR body describes the problem the PR is solving, and the design approach and alternatives considered. Explain why the PR solves the problem. A link to an issue is helpful, but does not replace an explanation of your thought process (See Contributing Guide, Pull Requests).

Additionally, we need integration tests for this. If they cannot be automatically run, we at least need steps for validation added to show how this was tested manually.

TheRealAmazonKendra · 2022-10-02T04:49:25Z

packages/@aws-cdk/core/test/bundling.test.ts

@@ -438,6 +438,39 @@ describe('bundling', () => {
    ], { stdio: ['ignore', process.stderr, 'inherit'] })).toEqual(true);
  });

+  // test('adding user provided volume options', () => {


We shouldn't have tests commented out. If the tests are failing, that generally indicates something isn't quite right here.

Hey,
the problem here is, that the fix solves a problem for specific docker in docker situations which require that specific environment to be there to be tested.
From my tries i was unable to mock this docker within docker situation in the CDK test suite, as this heavily depends on where the tests are run.
I would remove the test here, as it mostly would test basically the docker configuration, but not tell much about the functionality of the CDK code.

I added some notes on how to reproduce the steps solely with docker in the issues description.
I'm still trying to find a way to make that testable, but as the CI environment is different than what is available local its very time consuming back and forth of seeing if a change works or not

TheRealAmazonKendra · 2022-10-02T04:51:57Z

@Mergifyio update

mergify · 2022-10-02T04:52:12Z

update

✅ Branch has been successfully updated

aws-cdk-automation

The Pull Request Linter fails with the following errors:

❌ The title of this pull request does not follow the Conventional Commits format, see https://www.conventionalcommits.org/.

PRs must pass status checks before we can provide a meaningful review.

webratz · 2022-10-04T09:16:38Z

Contributing Guide, Pull Requests

The Pull Request Linter fails with the following errors:
❌ The title of this pull request does not follow the Conventional Commits format, see https://www.conventionalcommits.org/.
PRs must pass status checks before we can provide a meaningful review.

🤔 The name is: feat(core): add volumes-from option to docker run command for bundling
Looking at https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md#step-4-pull-request this seems to match the requirements. What am i missing here?

Pull Request updated. Dissmissing previous PRLinter Review.

aws-cdk-automation

The Pull Request Linter fails with the following errors:

❌ The title of this pull request does not follow the Conventional Commits format, see https://www.conventionalcommits.org/.

PRs must pass status checks before we can provide a meaningful review.

Pull Request updated. Dissmissing previous PRLinter Review.

aws-cdk-automation

The Pull Request Linter fails with the following errors:

❌ The title of this pull request does not follow the Conventional Commits format, see https://www.conventionalcommits.org/.

PRs must pass status checks before we can provide a meaningful review.

Naumel · 2022-10-04T14:09:54Z

The title issue can be easily written away, what is relevant is the build failing:

[jsii-rosetta] [INFO] Adding translations to /root/.s3buildcache/rosetta-cache.tabl.json
�[96m@aws-cdk.aws-lambda-python-alpha-README-L150.ts�[0m:�[93m15�[0m:�[93m5�[0m - �[91merror�[0m�[90m TS2304: �[0mCannot find name 'lambda'.

�[7m15�[0m new lambda.PythonFunction(this, 'function', {
�[7m  �[0m �[91m    ~~~~~~�[0m
[jsii-rosetta] [WARN] 1 diagnostics from assemblies with 'strict' mode on (and 410 more non-strict diagnostics)

Pull request has been modified.

aws-cdk-automation

The Pull Request Linter fails with the following errors:

❌ The title of this pull request does not follow the Conventional Commits format, see https://www.conventionalcommits.org/.

PRs must pass status checks before we can provide a meaningful review.

clean build for now, still looking for better ways to test

Pull request has been modified.

aws-cdk-automation

The Pull Request Linter fails with the following errors:

❌ The title of this pull request does not follow the Conventional Commits format, see https://www.conventionalcommits.org/.

PRs must pass status checks before we can provide a meaningful review.

its unclear how that would be reliably testable

alekitto · 2022-10-23T08:15:25Z

Hi @webratz,
This is a very useful addition, but unfortunately does not fix #8799, but only a very specific case.

In fact it is not useful when trying to build assets using a remote docker host (via docker-machine for example).
In addition you have to know exactly how the deployment environment is built and works and reflect it in your constructs, meaning that your constructs are dependent on a very specific environment.

Anyway, as I said, this is a useful addition as exposes another docker flag which is now hidden and that's good, but IMO is definitely not a fix of the linked issue.

aws-cdk-automation · 2022-10-26T15:17:11Z

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

webratz · 2022-11-02T14:24:18Z

Hi @webratz, This is a very useful addition, but unfortunately does not fix #8799, but only a very specific case.

In fact it is not useful when trying to build assets using a remote docker host (via docker-machine for example). In addition you have to know exactly how the deployment environment is built and works and reflect it in your constructs, meaning that your constructs are dependent on a very specific environment.

Anyway, as I said, this is a useful addition as exposes another docker flag which is now hidden and that's good, but IMO is definitely not a fix of the linked issue.

You are right, for the remote use case this one would not help

webratz · 2022-11-02T14:25:29Z

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

Several changes have been made since the comments added by @TheRealAmazonKendra - but no further review happened. So I'm not really sure what to add additionally

webratz · 2022-11-02T14:32:50Z

The pull request linter fails with the following errors:
❌ The title of this pull request does not follow the Conventional Commits format, see https://www.conventionalcommits.org/.
PRs must pass status checks before we can provide a meaningful review.

Its unclear to me what the problem here is actually.
The title of this is feat(core): add volumes-from option to docker run command for bundling which is to my understanding a valid title according to https://www.conventionalcommits.org/

Looking at previously merged PRs
https://github.com/aws/aws-cdk/pulls?q=is%3Apr+feat%28core%29+is%3Aclosed this actually seems to be following the same pattern. What am I missing here?

aws-cdk-automation · 2022-11-02T15:04:24Z

AWS CodeBuild CI Report

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

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

aws-cdk-automation · 2022-11-02T16:07:28Z

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

aws-cdk-automation · 2022-11-02T16:09:19Z

The pull request linter fails with the following errors:

❌ Features must contain a change to an integration test file and the resulting snapshot.

PRs must pass status checks before we can provide a meaningful review.

#22829) relates to #8799 follow up to stale #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*

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*

github-actions bot added effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2 labels Aug 18, 2022

aws-cdk-automation requested a review from a team August 18, 2022 11:49

webratz and others added 5 commits September 13, 2022 14:01

WIP add volumesFrom

9a18004

WIP add volumesFrom

020b825

adding tests and usage instruction

6bcb4ee

disable failing test for now

8473d45

add missing type

abdb9b5

TheRealAmazonKendra previously requested changes Oct 2, 2022

View reviewed changes

Merge branch 'main' into master

658e1dd

aws-cdk-automation previously requested changes Oct 2, 2022

View reviewed changes

aws-cdk-automation previously requested changes Oct 4, 2022

View reviewed changes

Merge branch 'aws:main' into master

a7fc600

aws-cdk-automation previously requested changes Oct 4, 2022

View reviewed changes

cleanup

74aae68

clean build for now, still looking for better ways to test

aws-cdk-automation requested changes Oct 4, 2022

View reviewed changes

webratz added 4 commits October 5, 2022 09:23

re-add adjusted test

2ee951b

test variant to allow the call to fail

9dbbd18

remove failing test

1fb4ada

its unclear how that would be reliably testable

whitespace cleanup

96c5614

Merge branch 'main' into master

d6b0bd4

webratz changed the title ~~feat(core): add volumes-from option to docker run command for bundling~~ feat(core): add volumes-from option to docker run command for bundling Nov 2, 2022

aws-cdk-automation added the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Nov 2, 2022

aws-cdk-automation closed this Nov 2, 2022

webratz mentioned this pull request Nov 8, 2022

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

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

webratz commented Aug 18, 2022 •

edited

Loading

gitpod-io bot commented Aug 18, 2022

webratz commented Aug 18, 2022

aws-cdk-automation commented Sep 8, 2022

webratz commented Sep 15, 2022

TheRealAmazonKendra left a comment

TheRealAmazonKendra Oct 2, 2022

webratz Oct 4, 2022

webratz Oct 5, 2022

TheRealAmazonKendra commented Oct 2, 2022

mergify bot commented Oct 2, 2022

aws-cdk-automation left a comment

webratz commented Oct 4, 2022

aws-cdk-automation left a comment

aws-cdk-automation left a comment

Naumel commented Oct 4, 2022

aws-cdk-automation left a comment

aws-cdk-automation left a comment

alekitto commented Oct 23, 2022

aws-cdk-automation commented Oct 26, 2022

webratz commented Nov 2, 2022

webratz commented Nov 2, 2022

webratz commented Nov 2, 2022

aws-cdk-automation commented Nov 2, 2022

aws-cdk-automation commented Nov 2, 2022

aws-cdk-automation commented Nov 2, 2022

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

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

Conversation

webratz commented Aug 18, 2022 • edited Loading

Describe the feature

Use Case

Proposed Solution

Alternative solutions

All Submissions:

Adding new Unconventional Dependencies:

New Features

gitpod-io bot commented Aug 18, 2022

webratz commented Aug 18, 2022

aws-cdk-automation commented Sep 8, 2022

webratz commented Sep 15, 2022

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

TheRealAmazonKendra Oct 2, 2022

Choose a reason for hiding this comment

webratz Oct 4, 2022

Choose a reason for hiding this comment

webratz Oct 5, 2022

Choose a reason for hiding this comment

TheRealAmazonKendra commented Oct 2, 2022

mergify bot commented Oct 2, 2022

✅ Branch has been successfully updated

aws-cdk-automation left a comment

Choose a reason for hiding this comment

webratz commented Oct 4, 2022

aws-cdk-automation left a comment

Choose a reason for hiding this comment

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Naumel commented Oct 4, 2022

aws-cdk-automation left a comment

Choose a reason for hiding this comment

aws-cdk-automation left a comment

Choose a reason for hiding this comment

alekitto commented Oct 23, 2022

aws-cdk-automation commented Oct 26, 2022

webratz commented Nov 2, 2022

webratz commented Nov 2, 2022

webratz commented Nov 2, 2022

aws-cdk-automation commented Nov 2, 2022

AWS CodeBuild CI Report

aws-cdk-automation commented Nov 2, 2022

aws-cdk-automation commented Nov 2, 2022

webratz commented Aug 18, 2022 •

edited

Loading