Prefer daemon registry mirrors for Docker Hub digest checks#1555
Prefer daemon registry mirrors for Docker Hub digest checks#1555chenjia404 wants to merge 3 commits intonicholas-fedor:mainfrom
Conversation
WalkthroughExtends registry interactions to support explicit registry endpoints/mirrors: adds endpoint-aware token and challenge URL functions, builds manifest URLs with registry endpoint overrides, discovers daemon-configured mirrors, and retries digest fetches across ordered registry endpoints. Also adds tests and a small test HTTP mock adjustment. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as CompareDigest()
participant Daemon as Docker Daemon (Info)
participant Discovery as Mirror Discovery
participant Auth as Auth (GetTokenWithRegistryEndpoint)
participant Registry as Registry Endpoint
Caller->>Daemon: Request Info (RegistryConfig.Mirrors)
Daemon-->>Caller: Mirrors list
Caller->>Discovery: Build ordered endpoints (mirrors + fallback)
Discovery-->>Caller: [endpoint1, endpoint2, ...]
loop per endpoint
Caller->>Auth: GetTokenWithRegistryEndpoint(endpoint)
Auth->>Registry: Request challenge at endpoint (/v2/)
Registry-->>Auth: Challenge / Auth response
Auth-->>Caller: Token
Caller->>Registry: HEAD /manifests/<tag> (with token)
alt Success
Registry-->>Caller: Content-Digest -> return match
else Failure
Registry-->>Caller: Error -> log and continue
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Complexity | 7 medium |
🟢 Metrics 21 complexity · 1 duplication
Metric Results Complexity 21 Duplication 1
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/registry/auth/auth.go (1)
898-943: Consider consolidating duplicated URL helpers withdigest.go.
buildRegistryEndpointURLandjoinURLPathare duplicated betweenauth.goanddigest.gowith slightly different signatures:
auth.go: returns(url.URL, bool)digest.go: returns(*url.URL, error)This duplication increases maintenance burden and risks divergence. Consider extracting these helpers into a shared internal package (e.g.,
internal/urlutil).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/registry/auth/auth.go` around lines 898 - 943, buildRegistryEndpointURL and joinURLPath are duplicated in auth.go and digest.go with different return signatures (auth.go: buildRegistryEndpointURL, joinURLPath; digest.go: similar helpers returning (*url.URL, error)), so extract a single canonical implementation into a shared internal package (e.g., internal/urlutil) and update callers: move the logic for building and joining URL paths into functions like urlutil.BuildRegistryEndpointURL and urlutil.JoinURLPath with a stable signature (choose one return style, e.g., (*url.URL, error) or (url.URL, bool) and convert callers accordingly), remove the duplicates from auth.go and digest.go, update imports and adapt error handling in callers (replace boolean checks with error handling if you choose error returns) to ensure consistent behavior across buildRegistryEndpointURL and digest.go usages.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/registry/auth/auth.go`:
- Around line 898-943: buildRegistryEndpointURL and joinURLPath are duplicated
in auth.go and digest.go with different return signatures (auth.go:
buildRegistryEndpointURL, joinURLPath; digest.go: similar helpers returning
(*url.URL, error)), so extract a single canonical implementation into a shared
internal package (e.g., internal/urlutil) and update callers: move the logic for
building and joining URL paths into functions like
urlutil.BuildRegistryEndpointURL and urlutil.JoinURLPath with a stable signature
(choose one return style, e.g., (*url.URL, error) or (url.URL, bool) and convert
callers accordingly), remove the duplicates from auth.go and digest.go, update
imports and adapt error handling in callers (replace boolean checks with error
handling if you choose error returns) to ensure consistent behavior across
buildRegistryEndpointURL and digest.go usages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 33e9ee71-5534-4d6f-8b6d-90917fda793b
📒 Files selected for processing (4)
pkg/container/image_test.gopkg/registry/auth/auth.gopkg/registry/digest/digest.gopkg/registry/digest/digest_test.go
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/urlutil/registry_endpoint.go`:
- Around line 12-29: The parsing logic for registryEndpoint should first detect
whether the raw registryEndpoint string contains "://" and, if not, prepend
defaultScheme + "://" before calling url.Parse so inputs like
"mirror.example.com:5000" are parsed with a host rather than misinterpreted as a
scheme; update the code around endpointURL and url.Parse to remove the current
reparse fallback (the block that reparses when Scheme and Host are empty) and
instead always parse the possibly-prefixed string, then validate
endpointURL.Host (returning the existing "missing host" error if still empty)
and ensure endpointURL.Scheme is set to defaultScheme when missing.
In `@pkg/registry/digest/digest.go`:
- Around line 369-373: The function BuildManifestURLWithRegistryEndpoint (and
the similar overload around lines 595-606) currently accepts only a hostOverride
string, which causes path-prefixed mirror redirects to lose their path when
rebuilding manifest URLs; change the API to accept and thread a full redirected
base URL (e.g., *url.URL or a full base string) instead of just hostOverride,
update the URL construction logic inside BuildManifestURLWithRegistryEndpoint to
use the redirected base's Scheme/Host and Path prefix when resolving the
manifest path, and update all callers to pass the redirected base URL so mirror
prefixes are preserved during redirects.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d592d853-b062-47e4-afc2-ea25e4416b5a
📒 Files selected for processing (4)
internal/urlutil/registry_endpoint.gointernal/urlutil/registry_endpoint_test.gopkg/registry/auth/auth.gopkg/registry/digest/digest.go
| endpointURL, err := url.Parse(registryEndpoint) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if endpointURL.Scheme == "" && endpointURL.Host == "" && endpointURL.Path != "" { | ||
| endpointURL, err = url.Parse(defaultScheme + "://" + registryEndpoint) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } | ||
|
|
||
| if endpointURL.Scheme == "" { | ||
| endpointURL.Scheme = defaultScheme | ||
| } | ||
|
|
||
| if endpointURL.Host == "" { | ||
| return nil, fmt.Errorf("missing host") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the file
find . -name "registry_endpoint.go" -type fRepository: nicholas-fedor/watchtower
Length of output: 108
🏁 Script executed:
# Also test Go's url.Parse behavior with the claimed problematic input
cat > /tmp/test_parse.go << 'EOF'
package main
import (
"fmt"
"net/url"
)
func main() {
// Test cases from the review
testCases := []string{
"mirror.example.com:5000",
"example.com",
"example.com:8080",
"http://mirror.example.com:5000",
"mirror.example.com",
"http://example.com",
}
for _, tc := range testCases {
u, err := url.Parse(tc)
if err != nil {
fmt.Printf("Input: %q -> Error: %v\n", tc, err)
} else {
fmt.Printf("Input: %q -> Scheme: %q, Host: %q, Path: %q\n",
tc, u.Scheme, u.Host, u.Path)
}
}
}
EOF
go run /tmp/test_parse.goRepository: nicholas-fedor/watchtower
Length of output: 604
🏁 Script executed:
cat -n ./internal/urlutil/registry_endpoint.goRepository: nicholas-fedor/watchtower
Length of output: 1754
Handle scheme-less host:port endpoints by checking for :// before parsing.
The current heuristic on line 17 misses inputs like mirror.example.com:5000. Go's url.Parse treats that as Scheme="mirror.example.com", Host="", so the reparse condition fails and the function incorrectly returns missing host. This breaks valid mirror endpoints that include explicit ports. Check for the presence of :// before the first parse and prepend defaultScheme if absent, eliminating the need for the fallback reparse logic.
Proposed fix
func BuildRegistryEndpointURL(registryEndpoint, resourcePath, defaultScheme string) (*url.URL, error) {
+ if !strings.Contains(registryEndpoint, "://") {
+ registryEndpoint = defaultScheme + "://" + registryEndpoint
+ }
+
endpointURL, err := url.Parse(registryEndpoint)
if err != nil {
return nil, err
}
-
- if endpointURL.Scheme == "" && endpointURL.Host == "" && endpointURL.Path != "" {
- endpointURL, err = url.Parse(defaultScheme + "://" + registryEndpoint)
- if err != nil {
- return nil, err
- }
- }
if endpointURL.Scheme == "" {
endpointURL.Scheme = defaultScheme
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| endpointURL, err := url.Parse(registryEndpoint) | |
| if err != nil { | |
| return nil, err | |
| } | |
| if endpointURL.Scheme == "" && endpointURL.Host == "" && endpointURL.Path != "" { | |
| endpointURL, err = url.Parse(defaultScheme + "://" + registryEndpoint) | |
| if err != nil { | |
| return nil, err | |
| } | |
| } | |
| if endpointURL.Scheme == "" { | |
| endpointURL.Scheme = defaultScheme | |
| } | |
| if endpointURL.Host == "" { | |
| return nil, fmt.Errorf("missing host") | |
| if !strings.Contains(registryEndpoint, "://") { | |
| registryEndpoint = defaultScheme + "://" + registryEndpoint | |
| } | |
| endpointURL, err := url.Parse(registryEndpoint) | |
| if err != nil { | |
| return nil, err | |
| } | |
| if endpointURL.Scheme == "" { | |
| endpointURL.Scheme = defaultScheme | |
| } | |
| if endpointURL.Host == "" { | |
| return nil, fmt.Errorf("missing host") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/urlutil/registry_endpoint.go` around lines 12 - 29, The parsing
logic for registryEndpoint should first detect whether the raw registryEndpoint
string contains "://" and, if not, prepend defaultScheme + "://" before calling
url.Parse so inputs like "mirror.example.com:5000" are parsed with a host rather
than misinterpreted as a scheme; update the code around endpointURL and
url.Parse to remove the current reparse fallback (the block that reparses when
Scheme and Host are empty) and instead always parse the possibly-prefixed
string, then validate endpointURL.Host (returning the existing "missing host"
error if still empty) and ensure endpointURL.Scheme is set to defaultScheme when
missing.
| func BuildManifestURLWithRegistryEndpoint( | ||
| container types.Container, | ||
| hostOverride string, | ||
| registryEndpoint string, | ||
| ) (string, string, *url.URL, error) { |
There was a problem hiding this comment.
Carry a full redirected base URL here, not just hostOverride.
The new endpoint flow now supports mirror URLs with path prefixes (internal/urlutil/registry_endpoint_test.go, Lines 24-36), but this API can only swap the host. If a challenge redirect moves from something like /cache/v2/... to a canonical /v2/... endpoint, the rebuilt manifest URL keeps the mirror prefix and retries the wrong path on the redirect target. Thread the full redirected base URL/path through this flow instead of only the host.
Also applies to: 595-606
🧰 Tools
🪛 GitHub Check: Codacy Static Code Analysis
[warning] 369-369: pkg/registry/digest/digest.go#L369
Method BuildManifestURLWithRegistryEndpoint has 53 lines of code (limit is 50)
[warning] 369-369: pkg/registry/digest/digest.go#L369
Method BuildManifestURLWithRegistryEndpoint has a cyclomatic complexity of 9 (limit is 8)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/registry/digest/digest.go` around lines 369 - 373, The function
BuildManifestURLWithRegistryEndpoint (and the similar overload around lines
595-606) currently accepts only a hostOverride string, which causes
path-prefixed mirror redirects to lose their path when rebuilding manifest URLs;
change the API to accept and thread a full redirected base URL (e.g., *url.URL
or a full base string) instead of just hostOverride, update the URL construction
logic inside BuildManifestURLWithRegistryEndpoint to use the redirected base's
Scheme/Host and Path prefix when resolving the manifest path, and update all
callers to pass the redirected base URL so mirror prefixes are preserved during
redirects.
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #1555 +/- ##
==========================================
- Coverage 75.13% 74.95% -0.18%
==========================================
Files 58 59 +1
Lines 9763 9882 +119
==========================================
+ Hits 7335 7407 +72
- Misses 2168 2206 +38
- Partials 260 269 +9
🚀 New features to boost your workflow:
|
2 participants
You can use this as a PR description or upstream submission note:
Title
Prefer daemon registry mirrors for Docker Hub digest checks
Description
This change fixes Docker Hub update checks for environments where Docker is configured with
registry-mirrorsand direct access toindex.docker.iois unavailable.Today, Watchtower’s digest lookup path for Docker Hub images assumes the canonical registry endpoint (
index.docker.io) and sends challenge / manifest requests there during update checks. In practice, many users who configureregistry-mirrorsdo so specifically because they cannot reliably reach Docker Hub directly. In those setups, the current behavior causes update checks to fail before a pull is even attempted.This patch changes the Docker Hub digest-check flow to prefer the Docker daemon’s configured registry mirrors when querying remote image metadata.
What changed
For Docker Hub images only:
Info().RegistryConfig.Mirrorsis present, digest checks try those mirror endpoints first.The existing behavior for non-Docker-Hub registries is unchanged.
Implementation details
The change is intentionally scoped to the digest-check path rather than image pull behavior:
pkg/registry/auth/auth.goGetTokenWithRegistryEndpoint(...)so auth requests can target a mirror instead of the canonical registry host.pkg/registry/digest/digest.goInfo()and iterate over candidate endpoints.pkg/registry/digest/digest_test.gopkg/container/image_test.goInfo()lookup performed during Docker Hub digest checks.Why this approach
This uses the Docker daemon’s effective registry configuration instead of parsing
daemon.jsondirectly.That has a few advantages:
Behavioral impact
Before this change:
index.docker.iofirst.After this change:
index.docker.io.Testing
Verified with:
go test ./...If you want, I can also turn this into a shorter upstream-friendly PR body plus a separate “Problem / Solution / Testing” version.
#1102
Summary by CodeRabbit
New Features
Tests