image look up: consult registries.conf #7823
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vrothberg The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
|
@edsantiago maybe something to consider for the system tests as well? |
|
@containers/podman-maintainers PTAL |
vrothberg
force-pushed
the
fix-6381
branch
2 times, most recently
from
935e76f to
b693bfa
Compare
|
LGTM |
|
LGTM |
Yes, Valentin took over on this project. @vrothberg lmk if you'd like help with tests or additional features, I quite liked working on this |
It's unrelated to the "secure short names" work, which focuses on how we pull from registries. This PR focuses on how we locate images in the container storage. |
Much appreciated, thanks, @ParkerVR! I didn't start yet as we're still in bug-hunting mode but I expect to pick it up soon :) |
|
I now skip the tests for remote as |
|
/hold |
openshift-ci-robot
added
the
do-not-merge/hold
|
/hold cancel |
openshift-ci-robot
removed
the
do-not-merge/hold
The registries package should be retired. It was introduced as an easier to use wrapper around c/image `sysregistries` which has been replaced by `sysregistriesv2` a long while ago. Users should either use the `sysregistriesv2` package directly or, even better, we cache the config in libpod's image runtime to prevent redundant (and ~expensive) parsing of the registries.conf files. For now, just add a note in hope we'll not forgert about it when we find time in the future. Signed-off-by: Valentin Rothberg <[email protected]>
When looking up local images, take the unqualified-serach registries of the registries.conf into account (on top of "localhost/"). Also extend the integration tests to prevent future regressions. Fixes: containers#6381 Signed-off-by: Valentin Rothberg <[email protected]>
|
Looking good now :) Untag is fixed and the tests turn green. |
|
/lgtm |
openshift-ci-robot
added
the
do-not-merge/hold
|
/hold cancel |
openshift-ci-robot
removed
the
do-not-merge/hold
| // Check if the input name has a transport and if so strip it | ||
| dest, err := alltransports.ParseImageName(inputName) | ||
| if err == nil && dest.DockerReference() != nil { | ||
| inputName = dest.DockerReference().String() | ||
| } | ||
|
|
||
| img, err := ir.getImage(stripSha256(inputName)) |
There was a problem hiding this comment.
This removes the only caller of getImage with a name AFAICS; the remaining know that they are using a full image ID; so, getImage should become getImageByID and use either NewStoreReference(store, nil, id) (more explicit about the intent) or perhaps store.Image (bypasses c/image, but allows names as input), without the overhead/risk of ParseStoreReference allowing other inputs.
There was a problem hiding this comment.
Very good catch. I can open a follow-up PR to clean up 👍
| images, err := ir.GetImages() | ||
| if err != nil { | ||
| return "", nil, err | ||
| } | ||
|
|
||
| // check the repotags of all images for a match | ||
| decomposedImage, err = decompose(inputName) |
There was a problem hiding this comment.
This was already computed around line 430.
There was a problem hiding this comment.
Should have added a comment. This was needed as the decomposedImage.referenceWithRegistry above has side effects.
There was a problem hiding this comment.
I can’t see any side effects — referenceWithRegistry just reads ip.hasRegistry and calls ip.unnormalizedRef.String() AFAICS; if it had side effects, would it even be possible to call it in a loop?
| // TODO: this package should not exist anymore. Users should either use | ||
| // c/image's `sysregistriesv2` package directly OR, even better, we cache a | ||
| // config in libpod's image runtime so we don't need to parse the | ||
| // registries.conf files redundantly. |
There was a problem hiding this comment.
sysregistriesv2 is already caching the files.
github-actions
bot
added
the
locked - please file new issue/PR
When looking up local images, take the unqualified-serach registries of
the registries.conf into account (on top of "localhost/").
Also extend the integration tests to prevent future regressions.
Fixes: #6381
Signed-off-by: Valentin Rothberg [email protected]