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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

flavianmissi
Copy link

@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.

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.

Signed-off-by: Flavian Missi <[email protected]>
@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

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