yodle / docker-registry-client

A Python REST client for the Docker Registry
Apache License 2.0
58 stars 58 forks source link

Fix auth URL #53

Open myw opened 5 years ago

myw commented 5 years ago

The way that the auth url was being used by AuthorizationService was too restrictive and made assumptions about the format of the URL that are not part of the standard and are violated by some existing authorization schemes: GitLab's authorization, for example, uses a different URL format (no v2/token path) and does not name the service GET parameter sent to the authorization URL with the same format as DockerHub does (it usescontainer_registry, instead of the registry host name).

The changes in this PR do two main things:

  1. It adds a parameter to the client constructor, auth_service_name, which allows client consumer code to provide custom service names; if not provided, the code defaults to the previous behavior (using the registry hostname).
  2. It adds a new auth service URL parameter auth_service_url_full. In this parameter, the /v2/token path is no longer automatically assumed on all URLs and must be provided explicitly if needed. This new parameter deprecates the older auth_service_url parameter, which will now raise a DeprecationWarning if provided.

As a minor additional improvement, the PR also makes the docker-registry-show.py script releasted to users' PATHs upon package installation (via the scripts argument in setup.py).

myw commented 5 years ago

hello @graingert — would you be able to review and merge in this PR if it's acceptable?

graingert commented 5 years ago

can you add a test case for this change?

myw commented 5 years ago

@graingert thanks for your prompt and helpful review! In my next round of changes, I added backwards-compatible interfaces, both at the API and command-line level, with deprecation warnings; I also added unit tests to check the functionality, and did some other minor cleanup. I'm updating the PR description to reflect that it does a deprecation, rather than a backwards-incompatible change.

Currently, the base client integration test fails for me on put_manifest in both python 2.7 and 3.6: the registry responds with HTTP 500 for the PUT request. However, this is also happening for me in master as well, so I do not think this PR introduces the failure.

Please let me know if there are any further changes you'd like me to make for this PR—I'd be happy to discuss.

On a related note, I will likely add the capability for the BaseClient to make cross-repository blob mounts in an upcoming PR. It should be straightforward enough, and enables efficiently "moving" Docker images on a registry from one repo to another without downloading, retagging, and uploading the entire image.

armstrongli commented 5 years ago

@graingert any update on this request?

graingert commented 5 years ago

I don't have permissions to merge PRs

djmattyg007 commented 3 years ago

Hello. Since this package appears to be abandoned, I forked it and made several improvements. This includes dropping support for v1 registries, and overhauling how the auth service works.

If you're still feeling up to it, I'd be happy to review an updated PR against my fork:

https://github.com/djmattyg007/dreg-client

myw commented 3 years ago

@djmattyg007 sure;seems easy enough; i’ll take a look when I get back to my laptop; if you haven’t heard from me or seen a PR by 9/13, feel free to ping me again.

djmattyg007 commented 3 years ago

@myw ping :slightly_smiling_face:

djmattyg007 commented 2 years ago

Hi @myw just checking in to ask if you'd had a chance to look at dreg-client yet