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

assets: allow passing --output flag to Docker build #20566

Closed
2 tasks
vincent-dm opened this issue Jun 1, 2022 · 1 comment · Fixed by #23304
Closed
2 tasks

assets: allow passing --output flag to Docker build #20566

vincent-dm opened this issue Jun 1, 2022 · 1 comment · Fixed by #23304
Labels
@aws-cdk/assets Related to the @aws-cdk/assets package effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2

Comments

@vincent-dm
Copy link

vincent-dm commented Jun 1, 2022

Describe the feature

AssetImageProps should allow setting the docker build --output flag.

Use Case

Specifically, we need to set --output=type=docker. It could also be expressed by the shorthand --load flag, but I would prefer a solution which allows the user to pass any --output, to avoid having to adapt CDK every time Docker adds new output options.

Background

We use CDK to build and deploy an ARM64 image on an x86-based (i.e. AMD64) Linux EC2 instance. Only Docker desktop offers multi-platform builds out of the box, so we had to install it, as per Docker's guide. We make buildx the default builder by running docker buildx install and then configuring a builder for arm64. Invoking docker build ... (like CDK does) is then actually re-routed to use buildx instead of the legacy Docker builder.

BUT buildx by default doesn't publish the built image into the local registry and requires an additional --output flag (or a shorthand like --load) to actually store and use the image that was built. This is reflected by warnings shown by buildx when invoking docker build without setting the --output flag (or any of its shorthands like --push or --load:

No output specified for docker-container driver. Build result will only remain in the build cache. To push result image into registry use --push or to load image into docker use --load

Because CDK AssetImageProps builder API doesn't allow us to set the output for the docker build step, it fails after building because it tries to tag an image which doesn't exist:

docker tag cdkasset-da3d3b814c3ba23b82b05bcac15892e50aa87059a01ba3dbada59dfef2369aaf foo.dkr.ecr.eu-west-1.amazonaws.com/cdk-hnb659fds-container-assets-foo-eu-west-1:da3d3b814c3ba23b82b05bcac15892e50aa87059a01ba3dbada59dfef2369aaf

Error response from daemon: No such image: cdkasset-da3d3b814c3ba23b82b05bcac15892e50aa87059a01ba3dbada59dfef2369aaf:latest

Workarounds like @nathanpeck's #12472 (comment) are not an option for us, since we need to be able to synthesize our CDK stack without building all the Docker images (which requires long compilation etc).

Our current workaround is to rename the docker binary to docker_real and instead create a docker bash script on the path which adds the flag:

#!/usr/bin/env bash
if [ "$1" = "build" ]; then
  echo "Adding --load param to docker build command"
  docker_real build --load "${@:2}"
else
  docker_real "$@"
fi

But this is obviously a hack and the proper solution would be for CDK AssetImageProps builder to allow us to add the --output flag (or preferably: any flag) to its invocation of docker build.

Proposed Solution

The AssetImageProps builder should get a new method output, which works similar to the existingbuildArgs method.

For example, calling this new output method with a map like {"type": "local", "dest": "path"} would result in --output=type=local&dest=path.

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

Environment details (OS name and version, etc.)

Amazon Linux 2

@vincent-dm vincent-dm added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Jun 1, 2022
@github-actions github-actions bot added the @aws-cdk/assets Related to the @aws-cdk/assets package label Jun 1, 2022
@otaviomacedo otaviomacedo added p2 effort/small Small work item – less than a day of effort and removed needs-triage This issue or PR still needs to be triaged. labels Jun 30, 2022
@otaviomacedo otaviomacedo removed their assignment Jun 30, 2022
@mergify mergify bot closed this as completed in #23304 Jan 9, 2023
mergify bot pushed a commit that referenced this issue Jan 9, 2023
This adds the `--output` flag as an option when building docker containers. This fixes #20566.

----

### 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

* [x] 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`)?

*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 Jan 9, 2023

⚠️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.

DerkSchooltink pushed a commit to DerkSchooltink/aws-cdk that referenced this issue Jan 23, 2023
This adds the `--output` flag as an option when building docker containers. This fixes aws#20566.

----

### 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

* [x] 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`)?

*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/assets Related to the @aws-cdk/assets package effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants