Fix c/image fails to pull OCI image with non-http(s):// urls
#1403
Conversation
|
LGTM. I think a pull test libimage would be nice but afaik @vrothberg @giuseppe @rhatdan PTAL |
There was a problem hiding this comment.
ACK overall.
(Reading the OCI spec, it might be possible to just always fall back to querying the registry. OTOH the docker/docker implementation fails the same way, and we can make the IPFS URLs possible without changing this, so let’s do the conservative thing now and not change that for HTTP.)
docker/docker_image_src.go
Outdated
| var ( | ||
| resp *http.Response | ||
| err error | ||
| ) | ||
| if len(urls) == 0 { | ||
| return nil, 0, errors.New("internal error: getExternalBlob called with no URLs") | ||
| return nil, 0, false, errors.New("internal error: getExternalBlob called with no URLs") // fatal error |
There was a problem hiding this comment.
Please drop the “fatal error” comment
- It’s not “fatal” as otherwise understood (e.g.
logrus.Fatalmeans “immediately terminate the whole process) - It’s completely unclear to me what the reader is supposed to do with the information in that comment. (We don’t generally have
count++ // increase countcomments — OTOH documenting the function API as this PR does is verry useful.)
There was a problem hiding this comment.
Thank you for the review. Fixed.
docker/docker_image_src.go
Outdated
| // This returns a bool value (named "fallback") which indicates whether the caller can fallback to the pull of | ||
| // the non-external blob (i.e. pull from the registry). | ||
| // This parameter becomes true only if no url is supported by this function. | ||
| // Do not pass zero-length "urls". This is considered a fatal error and returns an error without allowing fallback. |
There was a problem hiding this comment.
(Absolutely non-blocking: I wouldn’t overemphasize this. E.g. getExternalBlob returns the reader of the first available blob URL from urls, which must not be empty would be sufficient. See also the comment about error/fallback handling elsewhere, not having the (fallback == true, err != nil) special case would make documenting this simpler because “all errors are reported using err” is the default convention which doesn’t need documenting at all.)
docker/docker_image_src.go
Outdated
| } | ||
| for _, url := range urls { | ||
| noSupportedURL := true |
There was a problem hiding this comment.
Non-blocking: hasSupportedURL would be more direct than the double negation in noSupportedURL = false.
docker/docker_image_src.go
Outdated
| if noSupportedURL { | ||
| return nil, 0, true, errors.New("no supported url is specified") // fallback to non-external blob | ||
| } |
There was a problem hiding this comment.
So this now has:
err == nil: No error, blob stream availableerr != nil, fallback == true: Fall back (and theerrvalue is never used)err != nil, fallback == false: General error
As a thought experiment, consider
err == nil, fallback == false: No error, blob stream availableerr == nil, fallback == true: Fall backerr != nil: General error
where all errors are unusual, and we don’t have to worry about a “fallback” value on error paths (e.g. consider how the len(urls) case now needs extra documentation); and that can be turned into
err == nil, io.ReadCloser != nil: No error, blob stream availableerr == nil, io.ReadCloser == nil: Fall backerr != nil: General error
which is simpler and more direct.
(I prefer the last option but I don’t feel too strongly about this; if @vrothberg is fine with the code as is, let’s keep it.)
There was a problem hiding this comment.
Thank you for the suggestion. Fixed the logic.
| for _, url := range urls { | ||
| noSupportedURL := true | ||
| for _, u := range urls { | ||
| if u, err := url.Parse(u); err != nil || (u.Scheme != "http" && u.Scheme != "https") { |
There was a problem hiding this comment.
Absolutely non-blocking: Do we want to silently ignore err here? The combination of “a failing HTTP fetch is a hard error” and “a completely invalid URL is silently accepted” is a bit weird.
OTOH ignoring them is consistent with the idea of allowing IPFS or whatever else, so this is fine as is — just raising this design point so that others speak up if they feel strongly about it.
|
@mtrmac Thank you for the review. Fixed. |
Signed-off-by: Kohei Tokunaga <[email protected]>
|
All green. Thank you for the review! Could we move this forward? |
|
LGTM |
|
Thanks @ktock |
Fixes containers/podman#12231
Currently, c/image cannot pull OCI images that contain non-
http(s)://urls.This commit fixes this issue by allowing fallback when pull fails on non-
http(s)://urls.