Paginate Docker tag listing and classify registry error responses#15651
Paginate Docker tag listing and classify registry error responses#15651robaiken wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds resilient Docker registry tag retrieval and clearer registry error classification.
Changes:
- Falls back to paginated tag retrieval after registry HTTP failures.
- Maps 403 and persistent HTTP failures to appropriate Dependabot errors.
- Adds coverage for pagination fallback and error classification.
Show a summary per file
| File | Description |
|---|---|
docker/lib/dependabot/docker/update_checker.rb |
Implements pagination fallback and registry exception handling. |
docker/spec/dependabot/docker/update_checker_spec.rb |
Tests 403 and 504 behavior. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Medium
Images with very large tag counts (e.g. hexpm/elixir has ~1M tags) make registries such as Docker Hub time out (HTTP 504) when Dependabot requests the full, unpaginated tags/list, which surfaced as an unknown_error. - Fall back to a paginated tags/list request (?n=100) when the registry can't return the full list at once, following the registry's pagination links to collect the remaining tags. The common case still makes a single unpaginated request. - Classify the gem's RegistryHTTPException (5xx, including 504) as a RegistryError carrying the HTTP status instead of unknown_error. This is a public-registry-neutral error, so Docker Hub is not mislabelled as a private source. - Classify RegistryAuthorizationException (HTTP 403) as a PrivateSourceAuthenticationFailure alongside the existing 401 handling, at every registry-touching rescue site. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9d5f2c82-4124-4996-99cd-e6fd2e11874d
75d3fd2 to
31e9bd0
Compare
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4e918f38-f3b9-4353-995d-0740e8a32bb6
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4e918f38-f3b9-4353-995d-0740e8a32bb6
There was a problem hiding this comment.
Review details
Comments suppressed due to low confidence (1)
docker/lib/dependabot/docker/update_checker.rb:765
- In
docker_registry21.19, HTTP 429 is also wrapped asRegistryHTTPException, so this rescue immediately sends another request with?n=100while the registry is rate-limiting us. Pagination cannot resolve throttling and the extra request can prolong it; re-raise 429 so the outer handler classifies it without making another call.
rescue DockerRegistry2::RegistryHTTPException
raise if page_size
fetch_tags_from_registry(page_size: TAGS_PAGE_SIZE)
- Files reviewed: 3/5 changed files
- Comments generated: 2
- Review effort level: Medium
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4e918f38-f3b9-4353-995d-0740e8a32bb6
|
Addressed the suppressed 429 concern in 3579b14 as well: tag-list HTTP 429 responses are re-raised for immediate RegistryError classification without issuing a paginated follow-up request, and the spec verifies only one request is made. |
There was a problem hiding this comment.
Review details
Comments suppressed due to low confidence (3)
docker/lib/dependabot/docker/update_checker.rb:502
- With
docker_registry21.19,digest/doheadwrap non-special HTTP failures such as 500, 502, 503, and 504 inRegistryHTTPException. This best-effort cooldown path does not rescue that class, so those responses now escape asunknown_errorinstead of being logged and skipped as they were when the transient RestClient exceptions were caught. Include the wrapper here; status-aware retrying for transient 5xx responses can be handled separately.
DockerRegistry2::RegistryAuthorizationException,
docker/lib/dependabot/docker/update_checker.rb:1180
- The 1.19 client also raises
RegistryHTTPExceptionfrom the manifest and config-blob requests used by this timestamp fallback. Because this rescue omits it, a registry 5xx now aborts version/date validation instead of following this method's documented fail-open behavior and returningnil. Rescue the wrapper alongside the other registry failures.
DockerRegistry2::RegistryAuthorizationException,
docker/lib/dependabot/docker/update_checker.rb:1587
- Per-platform manifest/config requests can now raise
RegistryHTTPExceptionfor HTTP failures, but that wrapper is not caught here. A 5xx on any one platform therefore escapes and fails the update instead of using the intended empty-hash fallback. Rescue the HTTP wrapper with the other registry failures.
DockerRegistry2::RegistryAuthorizationException,
- Files reviewed: 3/5 changed files
- Comments generated: 1
- Review effort level: Medium
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Review details
Comments suppressed due to low confidence (2)
docker/lib/dependabot/docker/update_checker.rb:504
docker_registry21.19 converts server responses such as 500/502/503/504 toRegistryHTTPException, which is not intransient_docker_errors. Either the manifest GET or publication HEAD can therefore escape this fail-open cooldown path asunknown_errorinstead of logging and skipping cooldown. Include the new HTTP exception here (and add a server-response regression case) to preserve this method's existing failure behavior.
rescue *transient_docker_errors,
DockerRegistry2::RegistryAuthenticationException,
DockerRegistry2::RegistryAuthorizationException,
RestClient::Forbidden,
RestClient::TooManyRequests => e
docker/lib/dependabot/docker/update_checker.rb:1588
- A 1.19 server error while fetching any per-platform manifest/blob is now
RegistryHTTPException, so it bypasses this fail-open rescue and aborts timestamp validation asunknown_error. This path previously handled 500/502/503 through the RestClient classes intransient_docker_errors; include the new exception so failed platform metadata still degrades to{}.
rescue *transient_docker_errors, DockerRegistry2::RegistryAuthenticationException,
DockerRegistry2::RegistryAuthorizationException,
RestClient::Forbidden, JSON::ParserError => e
- Files reviewed: 3/5 changed files
- Comments generated: 0 new
- Review effort level: Medium
Summary
Docker/
docker_composeupdate jobs fail withunknown_errorfor images whose registry can't return the full tag list in one response. The clearest case is images with very large tag counts — e.g.hexpm/elixirhas ~993k tags, so Docker Hub'sGET /v2/hexpm/elixir/tags/list(requested unpaginated) times out with an HTTP 504 after 30s on every run.Root cause:
Dependabot::Docker::UpdateChecker#tags_from_registryrequests the tag list without a page size, and thedocker_registry2gem wraps the resulting 504 inDockerRegistry2::RegistryHTTPException, which wasn't rescued — so it surfaced asunknown_error. Separately, a 403 raisesRegistryAuthorizationException(sibling of the already-handledRegistryAuthenticationException), which was also unclassified.Changes
tags_from_registrymakes a single unpaginated request for the common case, and falls back to a paginated request (?n=100) when the registry returns an HTTP error trying to serve the full list, following the registry's pagination links. This keeps the efficient single call for normal images while letting large-but-servable images succeed via pagination.RegistryHTTPException(incl. 504) now raisesRegistryErrorcarrying the HTTP status, instead ofunknown_error.RegistryErroris registry-neutral, so a public registry like Docker Hub is not mislabelled as a private source.RegistryAuthorizationExceptionnow raisesPrivateSourceAuthenticationFailurealongside the existing 401 handling, at every registry-touching rescue site.This also fixes the
docker_composeecosystem, which reusesDependabot::Docker::UpdateChecker.Tests
?n=100request is made).RegistryErrorwithstatus == 504.PrivateSourceAuthenticationFailure.Full docker suite: 573 examples, 0 failures; RuboCop clean; Sorbet
No errors!.Verified with a dry-run against the real failing repo
docker_composein/docker-compose/hexpm(imagehexpm/elixir):Before
After
Note on
hexpm/elixirspecificallyPagination works for normal images (verified:
library/alpinereturns a bounded page fine). Forhexpm/elixir(~1M tags), Docker Hub returns 504 even for a bounded?n=100page, so pagination can't rescue this specific image — but the failure is now a graceful, classifiedregistry_error(with the HTTP status) instead of an opaqueunknown_error. Pagination still benefits the broad middle ground of large-but-servable images that currently 504 only on the unpaginated request.