Skip to content
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

docker: drop use of external distribution challenge pkg #2629

Merged
merged 2 commits into from
Nov 18, 2024

Conversation

flavianmissi
Copy link
Contributor

@flavianmissi flavianmissi commented Nov 8, 2024

in distribution v3, the registry/client package became internal. this change copies responseChallenges from upstream, removing the last reference to registry/client in this repo, making it ready for the distribution v3 bump, whenever that comes.


NOTE: I expected this change to cause go mod tidy && go mod vendor to remove the challenge package from the vendor dir, but that didn't happen - not sure why.

@flavianmissi flavianmissi force-pushed the use-local-docker-challenge branch from 3cfd7bb to f0b1ae1 Compare November 8, 2024 12:28
@mtrmac
Copy link
Collaborator

mtrmac commented Nov 12, 2024

NOTE: I expected this change to cause go mod tidy && go mod vendor to remove the challenge package from the vendor dir, but that didn't happen - not sure why.

% go mod why github.com/docker/distribution/registry/client
# github.com/docker/distribution/registry/client
github.com/containers/image/v5/pkg/docker/config
github.com/containers/image/v5/pkg/docker/config.test
github.com/docker/docker/registry
github.com/docker/distribution/registry/client/auth
github.com/docker/distribution/registry/client

Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! distribution/distribution/v3 would be a separate Go module from docker/distribution v2.…+incompatible , so this is not immediately urgent, but removing duplicated code is always nice.

Could you restructure the code to make the handling of StatusCode more explicit, please? We don’t really need the separate responseChallenges function, and it seems somewhat non-obvious to me that a function with that name does not just parse challenges, but conditions that on a status (“A server MAY generate a WWW-Authenticate header field in other response messages” per RFC 7235)

Something like

switch {
case resp.StatusCode == http.StatusUnauthorized:
   // …
   for _, c := range parseAuthHeader(…) {
   }
   fallthrough
case resp.StatusCode >= 400 && resp.StatusCode < 500:
   …
default:
  …
}

maybe

@flavianmissi flavianmissi force-pushed the use-local-docker-challenge branch from f0b1ae1 to 3a8a059 Compare November 18, 2024 09:41
@flavianmissi
Copy link
Contributor Author

I did as you suggested and also added some more unit tests for handleErrorResponse to make sure my changes didn't change existing behavior. The new tests pass on the main branch as well as on this one.
PTAL @mtrmac

in distribution v3, the registry/client package became internal. this
change drops usage of dockerChallenge.ResponseChallenges in favor of
parseAuthHeader directly, merging some of ResponseChallenges
functionality into handleErrorResponse (where it was being called).

Signed-off-by: Flavian Missi <[email protected]>
@flavianmissi flavianmissi force-pushed the use-local-docker-challenge branch from 3a8a059 to 2aee541 Compare November 18, 2024 09:44
Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@mtrmac mtrmac merged commit 57c4be9 into containers:main Nov 18, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants