fix(ecr-assets): fix loading image tarballs with existing tags #18823

AndrewGuenther · 2022-02-04T01:13:30Z

The current logic for pushing tarball images fails if the tarball being
loaded uses a repository and tag that's already loaded into the docker
daemon. This is due to the way the ecr-assets module parses the output
of the docker load command. If the repository/tag combination already
exists in the daemon, it outputs a message about renaming it, which
breaks the sed command parsing its output.

This change updates the sed command used for extracting the
repository/tag for tarball images to make it more robust and will now
successfully parse docker load output in the case where the
repository/tag already exist.

fixes #18822

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

The current logic for pushing tarball images fails if the tarball being loaded uses a repository and tag that's already loaded into the docker daemon. This is due to the way the ecr-assets module parses the output of the docker load command. If the repository/tag combination already exists in the daemon, it outputs a message about renaming it, which breaks the `sed` command parsing its output. This change updates the `sed` command used for extracting the repository/tag for tarball images to make it more robust and will now successfully parse docker load output in the case where the repository/tag already exist. fixes aws#18822

gitpod-io · 2022-02-04T01:13:33Z

mergify · 2022-02-04T01:14:06Z

Title does not follow the guidelines of Conventional Commits. Please adjust title before merge.

madeline-k

Thanks for opening this PR, @AndrewGuenther! Do you have any ideas on how to test this a bit more thoroughly?

AndrewGuenther · 2022-02-23T23:24:22Z

So the tarball asset code isn't really tested much at all. The test I updated is the only one currently in place. We could add an integration test which specifically tests tarball loading, but the specific case covered here would require an image to already be loaded in the Docker daemon. I'm not sure what the best way to go about that is. Make a subprocess call during test setup?

I'd be happy to improve the overall test coverage of tarball functionality in a separate PR. The CDK testing setup is pretty complex and I've had major issues running it locally (sorry for all the failed CI revisions). So it would be a bit before I could really put my head down and improve that. I'd appreciate if we could land this seeing as it updates the existing tests and consider the general coverage improvements separately.

AndrewGuenther · 2022-03-10T17:54:55Z

@madeline-k Can we get this landed? It's a pretty small change and would help remove a lot of workaround cruft for my org.

aws-cdk-automation · 2022-03-16T19:20:23Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
Commit ID: ba70f39
Result: SUCCEEDED
Build Logs (available for 30 days)

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

rix0rrr · 2022-03-30T17:26:06Z

packages/@aws-cdk/aws-ecr-assets/lib/tarball-asset.ts

@@ -78,7 +78,7 @@ export class TarballImageAsset extends CoreConstruct implements IAsset {
 executable: [
 'sh',
 '-c',
- `docker load -i ${relativePathInOutDir} | sed "s/Loaded image: //g"`,
+ `docker load -i ${relativePathInOutDir} | sed -nr 's/^Loaded image: (.*)$/\\1/p'`,


This code will not work on MacOS, as -r is not a valid flag for sed on that platform.

Can you also exercise TarballImageAsset in the integ test integ.assets-docker.ts (and then run it to update the snapshot)?

Thanks.

Well that's a bit of a piss off...hmmmm. I'll work on figuring our an alternative. Technically what's really needed is to strip out any following lines. It isn't my ideal, but we could pipe this into head which should work better cross platform...

@rix0rrr For a test like that to work, I would need to communicate with the docker daemon directly in order to replicate the issue being fixed here. Is there an existing precedent for running shell commands in integ tests? I had pushed back earlier when @madeline-k suggested this as I don't think that this PR is the right place to backfill testing for a completely untested class.

github-actions · 2022-04-21T00:10:16Z

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.

AndrewGuenther · 2022-04-25T16:54:02Z

Will leaving a comment keep this open if I'm waiting on follow-up comments from reviewers? A bit frustrating that it takes so long to get a question answered that a bot will close my PR...

github-actions · 2022-04-28T00:15:09Z

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.

AndrewGuenther · 2022-04-28T00:25:35Z

Are you serious?

skinny85 · 2022-04-28T00:46:39Z

@AndrewGuenther apologies, our bots can be a little ham-fisted sometimes 🙂.

@rix0rrr can you reply to Andrew's comment in #18823 (comment)?

AndrewGuenther · 2022-04-28T00:49:03Z

Thanks @skinny85!

github-actions · 2022-04-29T00:08:27Z

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.

The current implementation fails because it assumes the output of `docker load` can only be ``` Loaded Image: {image} ``` However, if docker has to take any actions (such as untagging a previous image) the output will look like ``` {error message} {error message} Loaded Image: {image} ``` To fix this, we can simply take the last line of the output via `tail` before we attempt to extract the image. If the last line isn't of the correct form, this will fail in an effectively equivalent way to how it failed previously. A previous attempt at fixing this (aws#18823) used a more complicated `sed` command which used a cli flag that is not consistently available. This approach won't work on all platforms (ex. macos) and is also much less clear that it's correct. This change also adds an integration test. We vendor the tarball of the `hello-world` docker image, which results in ~10kb of additional data in the repo. This about as small as we can get a vendored image with as straightforward a vendoring process. fixes aws#18822

`docker load` can only be ``` Loaded Image: {image} ``` However, if docker has to take any actions (such as untagging a previous image) the output will look like ``` {error message} {error message} Loaded Image: {image} ``` To fix this, we can simply take the last line of the output via `tail` before we attempt to extract the image. If the last line isn't of the correct form, this will fail in an effectively equivalent way to how it failed previously. A previous attempt at fixing this (aws#18823) used a more complicated `sed` command which used a cli flag that is not consistently available. This approach won't work on all platforms (ex. macos) and is also much less clear that it's correct. This change also adds an integration test that tests the tarbell stack, though it doesn't test the repeated deploy automatically. I have tested that by running the integration test multiple times with an image modification in between. We also vendor the tarball of the hello-world docker image, which results in ~10kb of additional data in the repo. This is about as small as we can get a vendored image with as straightforward a vendoring process. fixes aws#18822

The current implementation fails because it assumes the output of `docker load` can only be ``` Loaded Image: {image} ``` However, if docker has to take any actions (such as untagging a previous image) the output will look like ``` {error message} {error message} Loaded Image: {image} ``` To fix this, we can simply take the last line of the output via `tail` before we attempt to extract the image. If the last line isn't of the correct form, this will fail in an effectively equivalent way to how it failed previously. A previous attempt at fixing this (aws#18823) used a more complicated `sed` command which used a cli flag that is not consistently available. This approach won't work on all platforms (ex. macos) and is also much less clear that it's correct. This change also adds an integration test that tests the tarbell stack, though it doesn't test the repeated deploy automatically. I have tested that by running the integration test multiple times with an image modification in between. We also vendor the tarball of the hello-world docker image, which results in ~10kb of additional data in the repo. This is about as small as we can get a vendored image with as straightforward a vendoring process. fixes aws#18822

The current implementation fails because it assumes the output of `docker load` can only be ``` Loaded Image: {image} ``` However, if docker has to take any actions (such as untagging a previous image) the output will look like ``` {error message} {error message} Loaded Image: {image} ``` To fix this, we can simply take the last line of the output via `tail` before we attempt to extract the image. If the last line isn't of the correct form, this will fail in an effectively equivalent way to how it failed previously. A previous attempt at fixing this (#18823) used a more complicated `sed` command which used a cli flag that is not consistently available. This approach won't work on all platforms (ex. macos) and is also much less clear that it's correct. This change also adds an integration test that tests the tarbell stack, though it doesn't test the repeated deploy automatically. I have tested that by running the integration test multiple times with an image modification in between. We also vendor the tarball of the hello-world docker image, which results in ~20kb of additional data in the repo. This is about as small as we can get a vendored image with as straightforward a vendoring process. fixes #18822 ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https:/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:/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-construct-runtime-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https:/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*

…23497) The current implementation fails because it assumes the output of `docker load` can only be ``` Loaded Image: {image} ``` However, if docker has to take any actions (such as untagging a previous image) the output will look like ``` {error message} {error message} Loaded Image: {image} ``` To fix this, we can simply take the last line of the output via `tail` before we attempt to extract the image. If the last line isn't of the correct form, this will fail in an effectively equivalent way to how it failed previously. A previous attempt at fixing this (aws#18823) used a more complicated `sed` command which used a cli flag that is not consistently available. This approach won't work on all platforms (ex. macos) and is also much less clear that it's correct. This change also adds an integration test that tests the tarbell stack, though it doesn't test the repeated deploy automatically. I have tested that by running the integration test multiple times with an image modification in between. We also vendor the tarball of the hello-world docker image, which results in ~20kb of additional data in the repo. This is about as small as we can get a vendored image with as straightforward a vendoring process. fixes aws#18822 ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https:/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:/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-construct-runtime-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https:/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*

github-actions bot added the @aws-cdk/aws-ecr-assets Related to AWS CDK Docker Image Assets label Feb 4, 2022

github-actions bot assigned madeline-k Feb 4, 2022

AndrewGuenther changed the title ~~fix(ecr-assets) fix loading image tarballs with existing tags~~ fix(ecr-assets): fix loading image tarballs with existing tags Feb 4, 2022

AndrewGuenther added 3 commits February 3, 2022 17:33

fix template syntax

8bfbaf5

Merge branch 'master' into fix-tarball-image-name-extraction

5b86818

fix template syntax in test

5252202

madeline-k reviewed Feb 23, 2022

View reviewed changes

rix0rrr added bug This issue is a bug. p2 and removed bug This issue is a bug. p2 @aws-cdk/aws-ecr-assets Related to AWS CDK Docker Image Assets labels Mar 4, 2022

Merge branch 'master' into fix-tarball-image-name-extraction

ba70f39

AndrewGuenther requested a review from madeline-k March 16, 2022 18:31

madeline-k removed their assignment Mar 23, 2022

rix0rrr requested changes Mar 30, 2022

View reviewed changes

github-actions bot added the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Apr 28, 2022

github-actions bot closed this Apr 28, 2022

skinny85 reopened this Apr 28, 2022

github-actions bot added the effort/small Small work item – less than a day of effort label Apr 28, 2022

github-actions bot closed this Apr 29, 2022

dastbe mentioned this pull request Dec 29, 2022

fix(ecr-assets): fix repeated deploys of stacks with tar assets #23497

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ecr-assets): fix loading image tarballs with existing tags #18823

fix(ecr-assets): fix loading image tarballs with existing tags #18823

AndrewGuenther commented Feb 4, 2022

gitpod-io bot commented Feb 4, 2022

mergify bot commented Feb 4, 2022

madeline-k left a comment

AndrewGuenther commented Feb 23, 2022

AndrewGuenther commented Mar 10, 2022

aws-cdk-automation commented Mar 16, 2022

rix0rrr Mar 30, 2022

rix0rrr Mar 30, 2022

AndrewGuenther Mar 30, 2022

AndrewGuenther Mar 30, 2022

github-actions bot commented Apr 21, 2022

AndrewGuenther commented Apr 25, 2022

github-actions bot commented Apr 28, 2022

AndrewGuenther commented Apr 28, 2022

skinny85 commented Apr 28, 2022

AndrewGuenther commented Apr 28, 2022

github-actions bot commented Apr 29, 2022

fix(ecr-assets): fix loading image tarballs with existing tags #18823

fix(ecr-assets): fix loading image tarballs with existing tags #18823

Conversation

AndrewGuenther commented Feb 4, 2022

gitpod-io bot commented Feb 4, 2022

mergify bot commented Feb 4, 2022

madeline-k left a comment

Choose a reason for hiding this comment

AndrewGuenther commented Feb 23, 2022

AndrewGuenther commented Mar 10, 2022

aws-cdk-automation commented Mar 16, 2022

AWS CodeBuild CI Report

rix0rrr Mar 30, 2022

Choose a reason for hiding this comment

rix0rrr Mar 30, 2022

Choose a reason for hiding this comment

AndrewGuenther Mar 30, 2022

Choose a reason for hiding this comment

AndrewGuenther Mar 30, 2022

Choose a reason for hiding this comment

github-actions bot commented Apr 21, 2022

AndrewGuenther commented Apr 25, 2022

github-actions bot commented Apr 28, 2022

AndrewGuenther commented Apr 28, 2022

skinny85 commented Apr 28, 2022

AndrewGuenther commented Apr 28, 2022

github-actions bot commented Apr 29, 2022