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

[WIP] untangling auth-related code #5925

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

thaJeztah
Copy link
Member

Really (REALLY) work in progress; slowly untangling some of the auth-code which was wrapper-upon-wrapper-upon-wrapper; often because types like registry.IndexInfo or registry.RepositoryInfo were part of the signature.

Docker Content Trust added yet-another layer of abstraction on top of that, with trust.ImageRefAndAuth, which is a wrapper on its own to wrap all those bits.

In most cases, all we need is;

  • either the name of the registry, or an image-ref from which we can deduct the name
  • we DONT need to know about Mirrors, because the client doesn't configure those
  • for most situations we don't even need to know about "insecure registries", but we can deduct "defaults" there from the hostname (default is loopbacks are insecure, everything else isn't)

And of course, there's the "special cases" for docker hub;

  • docker.io or index.docker.io PREFIX means "docker hub registry" (actual registry is registry-1.docker.io (but there's other domains possible ⚠️ we still need to normalise those)
  • we currently use https://index.docker.io/v1/ as KEY to store credentials for those
  • ☝️ also something we should consider changing, because for other registries, we use hostname without scheme / path

But there's more to untangle, such as creds-helpers/stores converting "to hostname", but other paths don't, and likely corner-cases, where (e.g.) a trailing / is missing in https://index.docker.io/v1/, etc etc.

- Human readable description for the release notes

- A picture of a cute animal (not mandatory but encouraged)

@thaJeztah
Copy link
Member Author

FWIW, more untangling also happening in #5876 and #5921

I should probably look at basing this one on that, but wanted to see things before that

@codecov-commenter
Copy link

codecov-commenter commented Mar 12, 2025

Codecov Report

Attention: Patch coverage is 16.94915% with 49 lines in your changes missing coverage. Please review.

Project coverage is 59.02%. Comparing base (2b0631f) to head (c784802).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5925      +/-   ##
==========================================
- Coverage   59.08%   59.02%   -0.07%     
==========================================
  Files         354      354              
  Lines       29739    29764      +25     
==========================================
- Hits        17572    17568       -4     
- Misses      11197    11226      +29     
  Partials      970      970              
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@thaJeztah thaJeztah force-pushed the simplify_auth branch 3 times, most recently from df692c7 to 8a4f9b4 Compare March 19, 2025 14:37
@thaJeztah thaJeztah force-pushed the simplify_auth branch 3 times, most recently from 64e04a3 to 074b5a7 Compare March 23, 2025 14:34
Remove RepositoryInfo as intermediate struct in some places; we want
to remove the use of this additional abstration. More changes are
needed to fully remove it, but chipping away its use in small bits.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Lots to do here; too many wrappers everywhere, which may become easier
when content trust is removed (which adds another level of abstraction)

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
These options were moved to opts/swarmopts in ad21055
and have no known external consumers.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
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.

None yet

2 participants