use local image name for pull policy checks #2908
Conversation
Some pull policies require to first look up a local image and compare that to the remote counter part. When looking up the remote image, we need to make sure to use the name of the local image, if it exists. This fixes a bug where a short name resolved to an image with the "localhost/" prefix. This prefix is only used for local image look ups via `shortnames.ResolveLocally`. Hence, when looking up the remote counter part, we must preserve this prefix. Fixes: containers#2904 Signed-off-by: Valentin Rothberg <[email protected]>
|
LGTM |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: giuseppe, 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 |
| if localImageRef != nil { | ||
| fromImage = localImageName | ||
| } | ||
|
|
||
| resolved, err := shortnames.Resolve(systemContext, fromImage) |
There was a problem hiding this comment.
This is now stuttering and hard to follow.
How about
if options.PullPolicy == PullNever && localImage == nil { /* fail*/ }
if localImage != nil {
// the three checks
}| @@ -145,7 +145,7 @@ func resolveImage(ctx context.Context, systemContext *types.SystemContext, store | |||
| } | |||
| } | |||
|
|
|||
| localImageRef, _, localImage, err := resolveLocalImage(systemContext, store, options) | |||
| localImageRef, _, localImageName, localImage, err := resolveLocalImage(systemContext, store, options) | |||
There was a problem hiding this comment.
Unrelated? This is the only caller, yet we ignore the second return value. It should be dropped, then.
| // If we found a local image, we must use it's name. | ||
| // See #2904. | ||
| if localImageRef != nil { | ||
| fromImage = localImageName |
There was a problem hiding this comment.
(Non-blocking: It might be useful to add a debug log if fromImage != localImageName, so that users can figure out where the fully-qualified variant came from.)
| // See #2904. | ||
| if localImageRef != nil { | ||
| fromImage = localImageName | ||
| } |
There was a problem hiding this comment.
Neat workaround, hadn't thought about trying this.
…9 branch Cherry pick @vrothberg's "use local image name for pull policy checks" containers#2908 and update the cirrus and git validations so the test will run on this new(ish) branch. From @vrothberg: Some pull policies require to first look up a local image and compare that to the remote counter part. When looking up the remote image, we need to make sure to use the name of the local image, if it exists. This fixes a bug where a short name resolved to an image with the "localhost/" prefix. This prefix is only used for local image look ups via shortnames.ResolveLocally. Hence, when looking up the remote counter part, we must preserve this prefix. Fixes: containers#2904 Signed-off-by: TomSweeneyRedHat <[email protected]>
Some pull policies require to first look up a local image and compare
that to the remote counter part. When looking up the remote image, we
need to make sure to use the name of the local image, if it exists.
This fixes a bug where a short name resolved to an image with the
"localhost/" prefix. This prefix is only used for local image look ups
via
shortnames.ResolveLocally. Hence, when looking up the remotecounter part, we must preserve this prefix.
Fixes: #2904
Signed-off-by: Valentin Rothberg [email protected]