crashvb / docker-registry-client-async

An AIOHTTP based Python REST client for the Docker Registry.
Apache License 2.0
5 stars 6 forks source link

Resolve image name with library prefix only when it's from DockerHub #31

Closed skrech closed 1 year ago

skrech commented 2 years ago

Hello crashvb,

I've encountered another interesting issue: At my company we're working with a lot of Artifactory-based registries and this uncovers a "bug" in the image resolution logic. The current logic is to always prefix images that don't contain "namespace" with library. However, Artifactory recommends to their users to move the "namespace" part of the repository as sub-domain part of the URL. In other words, usually one would expect to work with Artifactory as every other registry like so: some-artifactory.com/library/python, but Artifactory restructures this into library.some-artifactory.com/python (the namespace will never be called library, but it's good for illustration purposes). Obviously, if DRCA parses that reference, it will modify it to library.some-artifactory.com/library/python which would return 404.

It was my impression that if image doesn't have repository "namespace" it should be assigned to library repository. Apparently, DRCA thinks the same. But it turns out that's not the case -- there is nothing in the Docker Registry spec about that and surprisingly docker pull command copes just fine with library.some-artifactory.com/python.

So, I've spend some hours looking through Go code of containerd and registry-handling code in Docker to see what's going on and found these: containerd: https://github.com/containerd/containerd/blob/49a945b26b9d7bd485e49041b74aab3aab97c15a/reference/docker/reference.go#L666 Docker: https://github.com/distribution/distribution/blob/main/reference/normalize.go#L87 (They are the same)

So, it turns out that library should be only prefixed when working with DockerHub. Tomorrow I'll create a pull request with potential fix. Can you think of potential problems with that change?

stale[bot] commented 2 years ago

This discussion has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

stale[bot] commented 2 years ago

This discussion has been closed due to inactivity.

stale[bot] commented 1 year ago

This discussion has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

stale[bot] commented 1 year ago

This discussion has been closed due to inactivity.

skrech commented 1 year ago

@crashvb, did you have time to review the PR.

crashvb commented 1 year ago

@skrech If I'm following, am I correct in understanding that the library namespace is limited to only dockerhub (i.e. docker-library?

@kyatite heads up.

crashvb commented 1 year ago

@skrech, @kyatite,

I took a much closer look at the problem and pushed out a fix (v0.2.10) that should work if ImageName.DEFAULT_NAMESPACE is set to "", either directly or using the environment variable. This should have the expected behavior while still maintaining the position of defaulting official images to DockerHub.

I stopped short of bumping a major version and changing the DEFAULT_NAMESPACE to "" (with a note in the readme), because I think that would need more explicit testing downstream.

Side Note: I tested with both default=library,override="" and default="",override="library" and both of them seem to work; although it did expose some test data corruption that should be fixed now.