fix pull image that includes a sha #1085
Conversation
baude
force-pushed
the
shapull
branch
2 times, most recently
from
9d90db4 to
a1944cc
Compare
when pulling an image that includes a sha such as: centos/nginx-112-centos7@sha256:42330f7f29ba1ad67819f4ff3ae2472f62de13a827a74736a5098728462212e7 the final image name in libpod should not contain portions of the sha itself nor the sha identifier. and like docker, we provide a 'none' tag as well. this should fix containers#877 Signed-off-by: baude <[email protected]>
|
Can you add some tests? |
|
Might want to make this a second PR since we want to get this into the release today, if you don't have time. |
|
Other then the tests, this LGTM |
|
@mheon @giuseppe @mtrmac @umohnani8 PTAL |
|
@rhatdan there is a good test already for non fully qualified image pull by sha. |
|
Ok great. |
|
LGTM |
|
LGTM |
|
📌 Commit 5e08f8d has been approved by |
|
⚡ Test exempted: merge already tested. |
Which part of Docker does that? I see # docker pull alpine@sha256:7043076348bf5040220df6ad703798fd8593a0918d06d3ce30c6c93be117e430
…
Status: Downloaded newer image for docker.io/alpine@sha256:7043076348bf5040220df6ad703798fd8593a0918d06d3ce30c6c93be117e430
[root@f28 ~]# docker images
REPOSITORY TAG IMAGE ID CREATED SIZE
docker.io/alpine <none> 11cd0b38bc3c 7 days ago 4.41 MBwhere the |
| if i.HasShaInInputName() { | ||
| imageName = fmt.Sprintf("%s%s", decomposedImage.transport, i.InputName) | ||
| } else { | ||
| imageName = decomposedImage.assembleWithTransport() |
There was a problem hiding this comment.
This looks horribly irregular. Why not teach imageParts to actually support digest references fully?
| if err != nil { | ||
| return nil, errors.Wrapf(err, "unable to parse '%s'", i.InputName) | ||
| } | ||
| ps := pullStruct{ | ||
| image: i.InputName, | ||
| srcRef: srcRef, | ||
| } | ||
| if i.HasShaInInputName() { | ||
| ps.shaPullName = decomposedImage.assemble() |
There was a problem hiding this comment.
So, shaPullName actually means “a name which has no digest at all?! That’s very confusing.
| @@ -33,6 +34,9 @@ func decompose(input string) (imageParts, error) { | |||
| } | |||
| if !isTagged { | |||
| tag = "latest" | |||
| if strings.Contains(input, "@sha256:") { | |||
There was a problem hiding this comment.
Why hard-code sha256 here and not support all digests? And the parse called just above already knows the information, no need to touch string contents at all:
if _, ok := imgRef.(reference.Digested); ok {And, more importantly, reference.Parse accepts name:tag@digest input; so “has a digest” does not mean “does not have a tag”.
Overall, as I mentioned earlier , the tag = "none" behavior seems to have no precedent and seems entirely unnecessary to me.
| srcRef, err := alltransports.ParseImageName(decomposedImage.assembleWithTransport()) | ||
| imageName := decomposedImage.assembleWithTransport() | ||
| if i.HasShaInInputName() { | ||
| imageName = fmt.Sprintf("%s%s/%s", decomposedImage.transport, registry, i.InputName) |
There was a problem hiding this comment.
There are exactly two callers to assembleWithTransport, and both are now conditionalized with HasShaInInputName.
I think this very clearly shows that the digests should be the responsibility of the decompose().assembleWithTransport() function pair, and should be fixed in there instead of the callers fixing this up after them.
| destRef, err := is.Transport.ParseStoreReference(i.imageruntime.store, pStruct.image) | ||
| dstName := pStruct.image | ||
| if pStruct.shaPullName != "" { | ||
| dstName = pStruct.shaPullName |
There was a problem hiding this comment.
This will cause the containers-storage: destination to not be marked with the digest. Why is that desirable?
github-actions
bot
added
the
locked - please file new issue/PR
when pulling an image that includes a sha such as:
centos/nginx-112-centos7@sha256:42330f7f29ba1ad67819f4ff3ae2472f62de13a827a74736a5098728462212e7
the final image name in libpod should not contain portions of the sha itself nor the sha
identifier. and like docker, we provide a 'none' tag as well.
this should fix #877
Signed-off-by: baude [email protected]