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

Revert "imageDigest only for ecr" #3576

Merged
merged 1 commit into from
Mar 6, 2023
Merged

Revert "imageDigest only for ecr" #3576

merged 1 commit into from
Mar 6, 2023

Conversation

sparrc
Copy link
Contributor

@sparrc sparrc commented Feb 16, 2023

This reverts commit af54b2f.

This would address aws/containers-roadmap#1640

Summary

It was originally decided to supply image digests only for ECR because the image digests we supply do not match the dockerhub UI. This is because the dockerhub UI only shows individual "manifest" image digests for each particular image variant. ECR, on the other hand, displays the "index" digest, which is a single digest shared across all variants of a single image. Somewhat confusingly, the docker image inspect command shows the "index" image digest too.

What this means is that docker image pull and docker image inspect show a different image digest than the dockerhub UI. This is confusing for docker customers and fixing this is one of the most-upvoted feature requests for dockerhub: docker/hub-feedback#1925

Our solution to customer confusion to just simply omit the digest for non-ECR repos was probably not correct. Customers want image digests for all their images. If customers are confused why the docker "index" digest that they see in ecs.DescribeTasks does not match the "manifest" digest they see in dockerhub, then this is a documentation and education issue, not a code issue.

Testing

tested calling ecs.DescribeTasks before and after this change.

Before the change, can see imageDigest field only for private ECR repos:

private ECR (before change):

  "image": "XXXX.dkr.ecr.us-west-2.amazonaws.com/XXXX:XXXX",
  "imageDigest": "sha256:eab773b3c3916e73c19232cc854233a17b332754733f07dc75ccb6229a19b766",

public ECR (before change):

  "image": "public.ecr.aws/docker/library/busybox:latest",

dockerhub (before change):

  "image": "busybox:latest",

After this PR, can see imageDigest field for all images:

private ECR (after change):

  "image": "XXXX.dkr.ecr.us-west-2.amazonaws.com/XXXX:XXXX",
  "imageDigest": "sha256:eab773b3c3916e73c19232cc854233a17b332754733f07dc75ccb6229a19b766",

public ECR (after change):

  "image": "public.ecr.aws/docker/library/busybox:latest",
  "imageDigest": "sha256:7b3ccabffc97de872a30dfd234fd972a66d247c8cfc69b0550f276481852627c",

dockerhub (after change):

  "image": "busybox:latest",
  "imageDigest": "sha256:7b3ccabffc97de872a30dfd234fd972a66d247c8cfc69b0550f276481852627c",

Description for the changelog

Enhancement: Provide imageDigest for images from all container repositories

Licensing

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

@sparrc sparrc requested a review from a team as a code owner February 16, 2023 21:53
@holyspectral
Copy link

Any news on this? Would love to see image digest on other type of registries.

@sparrc
Copy link
Contributor Author

sparrc commented Mar 3, 2023

Any news on this? Would love to see image digest on other type of registries.

testing and getting it ready now, should be merged in a matter of weeks

@sparrc sparrc changed the title [wip] Revert "imageDigest only for ecr" Revert "imageDigest only for ecr" Mar 3, 2023
@sparrc sparrc added the bot/test label Mar 3, 2023
@sparrc sparrc merged commit 9e5cb26 into aws:dev Mar 6, 2023
@sparrc sparrc deleted the revert-pr-2201 branch March 6, 2023 16:52
@holyspectral
Copy link

So happy to see this PR is merged! I guess it's out of this repo's scope, but by any chance this change will also be in Fargate container agent?

@mye956 mye956 mentioned this pull request Mar 7, 2023
@ubhattacharjya ubhattacharjya mentioned this pull request Mar 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants