-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
compat API: allow enforcing short-names resolution to Docker Hub #12435
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 |
@edsantiago, I'd appreciate your eyes on the new tests |
1507347
to
95740d3
Compare
bc4b23c
to
bd478fe
Compare
Question: does this affect lookup of |
@@ -52,6 +52,13 @@ func CreateContainer(w http.ResponseWriter, r *http.Request) { | |||
return | |||
} | |||
|
|||
imageName, err := utils.NormalizeToDockerHub(r, body.Config.Image) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a thought (I didn’t review the PR otherwise at all!)
Several (but by no means all) instances might be a tiny bit shorter with a
if err := utils.NormalizeOverwriteToDockerHub(r, &body.Config.Image); err != nil { … }
with the obvious wrapper
func NormalizeOverwriteToDockerHub(r …, inPlace *string) error {
v, err := NormalizeToDockerHub(r, *inPlace)
if err != nil { return err }
*inPlace = v
return nil
}
(*grumble* about Go’s error handling ergonomics)
Yes. |
Alright. That's a breaking change (at least when the containers.conf flag is flipped on), so we'll need to document it. I threw on the correct label for that. |
Oh absolutely. It's changing the entire short-name resolution of the compat API, so it goes beyond the |
Updated now with c/image@main. Ready for reviewing/merging. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really thorough test suite, thank you. One nit, one question.
The Docker-compatible REST API has historically behaved just as the rest of Podman and Buildah (and the atomic Docker in older RHEL/Fedora) where `containers-registries.conf` is centrally controlling which registries a short name may resolve to during pull or local image lookups. Please refer to a blog for more details [1]. Docker, however, is only resolving short names to docker.io which has been reported (see containers#12320) to break certain clients who rely on this behavior. In order to support this scenario, `containers.conf(5)` received a new option to control whether Podman's compat API resolves to docker.io only or behaves as before. Most endpoints allow for directly normalizing parameters that represent an image. If set in containers.conf, Podman will then normalize the references directly to docker.io. The build endpoint is an outlier since images are also referenced in Dockerfiles. The Buildah API, however, supports specifying a custom `types.SystemContext` in which we can set a field that enforces short-name resolution to docker.io in `c/image/pkg/shortnames`. Notice that this a "hybrid" approach of doing the normalization directly in the compat endpoints *and* in `pkg/shortnames` by passing a system context. Doing such a hybrid approach is neccessary since the compat and the libpod endpoints share the same `libimage.Runtime` which makes a global enforcement via the `libimage.Runtime.systemContext` impossible. Having two separate runtimes for the compat and the libpod endpoints seems risky and not generally applicable to all endpoints. [1] https://www.redhat.com/sysadmin/container-image-short-names Fixes: containers#12320 Signed-off-by: Valentin Rothberg <rothberg@redhat.com>
LGTM and may i never work with images again ... |
/lgtm |
The unqualified registries setting is no longer needed, see containers/podman#12435
The unqualified registries setting is no longer needed, see containers/podman#12435
The unqualified registries setting is no longer needed, see containers/podman#12435
The unqualified registries setting is no longer needed, see containers/podman#12435
The unqualified registries setting is no longer needed, see containers/podman#12435
The unqualified registries setting is no longer needed, see containers/podman#12435
The unqualified registries setting is no longer needed, see containers/podman#12435
The unqualified registries setting is no longer needed, see containers/podman#12435
The Docker-compatible REST API has historically behaved just as the rest
of Podman and Buildah (and the atomic Docker in older RHEL/Fedora) where
containers-registries.conf
is centrally controlling which registriesa short name may resolve to during pull or local image lookups. Please
refer to a blog for more details [1].
Docker, however, is only resolving short names to docker.io which has
been reported (see #12320) to break certain clients who rely on this
behavior. In order to support this scenario,
containers.conf(5)
received a new option to control whether Podman's compat API resolves
to docker.io only or behaves as before.
Most endpoints allow for directly normalizing parameters that represent
an image. If set in containers.conf, Podman will then normalize the
references directly to docker.io. The build endpoint is an outlier
since images are also referenced in Dockerfiles. The Buildah API,
however, supports specifying a custom
types.SystemContext
in whichwe can set a field that enforces short-name resolution to docker.io
in
c/image/pkg/shortnames
.Notice that this a "hybrid" approach of doing the normalization directly
in the compat endpoints and in
pkg/shortnames
by passing a systemcontext. Doing such a hybrid approach is neccessary since the compat
and the libpod endpoints share the same
libimage.Runtime
which makesa global enforcement via the
libimage.Runtime.systemContext
impossible. Having two separate runtimes for the compat and the libpod
endpoints seems risky and not generally applicable to all endpoints.
[1] https://www.redhat.com/sysadmin/container-image-short-names
Fixes: #12320
Signed-off-by: Valentin Rothberg rothberg@redhat.com