Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGES/+head-based-manifest-version-check.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Pull-through manifest resolution now performs a HEAD-based version check and serves locally
stored manifests without downloading the manifest body. Per Docker Hub's pull definition, version
checks do not count toward rate limits, substantially reducing 429 errors for repeat tag
resolutions.
Comment on lines +1 to +4

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you put this on one line and make it more concise?

36 changes: 30 additions & 6 deletions pulp_container/app/registry_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -1478,14 +1478,43 @@ def fetch_manifest(self, remote, pk):
Fetch a manifest from the upstream remote.
Returns the local manifest, if it exists in Pulp, and the full response from upstream.
Raises response errors if manifest is not found or the download fails.

A HEAD request is issued first to check the manifest's digest against what is already
stored in Pulp. Per Docker Hub's pull-rate accounting, a HEAD ("version check") does not
count as a pull, unlike a GET. If the manifest is already stored locally, the HEAD
response is returned as-is and the counted GET is skipped entirely.
"""
relative_url = "/v2/{name}/manifests/{pk}".format(
name=remote.namespaced_upstream_name, pk=pk
)
tag_url = urljoin(remote.url, relative_url)

head_response = self._fetch_manifest_response(remote, tag_url, pk, http_method="head")
digest = head_response.headers.get("docker-content-digest")
if digest:
manifest = models.Manifest.objects.filter(
digest=digest, pulp_domain=get_domain()
).first()
if manifest is not None:
return manifest, head_response

# The manifest is not stored locally yet, or the upstream did not report a digest on the
# HEAD response; fall back to a full GET, identical to the previous behavior.
response = self._fetch_manifest_response(remote, tag_url, pk, http_method="get")
digest = response.headers.get("docker-content-digest")
return models.Manifest.objects.filter(
digest=digest, pulp_domain=get_domain()
).first(), response

def _fetch_manifest_response(self, remote, tag_url, pk, http_method):
"""
Issue a HEAD or GET request for a manifest and map response errors consistently.
"""
downloader = remote.get_downloader(url=tag_url)
try:
response = downloader.fetch(extra_data={"headers": V2_ACCEPT_HEADERS})
return downloader.fetch(
extra_data={"headers": V2_ACCEPT_HEADERS, "http_method": http_method}
)
except ClientResponseError as response_error:
if response_error.status == 429:
# the client could request the manifest outside the docker hub pull limit;
Expand All @@ -1498,11 +1527,6 @@ def fetch_manifest(self, remote, pk):
except (ClientConnectionError, TimeoutException):
# The remote server is not available at the moment
raise GatewayTimeout()
else:
digest = response.headers.get("docker-content-digest")
return models.Manifest.objects.filter(
digest=digest, pulp_domain=get_domain()
).first(), response

def put(self, request, path, pk=None):
"""
Expand Down
137 changes: 137 additions & 0 deletions pulp_container/tests/unit/test_registry_api.py

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not sure we need a unit test for this fix. As long as the functional tests pass, I think we are fine. The change uses the same pattern as sync, so I'm confident it'll work.

Original file line number Diff line number Diff line change
@@ -0,0 +1,137 @@
"""Unit tests for HEAD-based manifest version checking in pull-through caching."""

import unittest
from unittest.mock import MagicMock, patch

from aiohttp.client_exceptions import ClientConnectionError, ClientResponseError
from rest_framework.exceptions import Throttled

from pulp_container.app.exceptions import BadGateway, GatewayTimeout, ManifestNotFound
from pulp_container.app.registry_api import Manifests
from pulpcore.plugin.exceptions import TimeoutException


def _mock_response(digest):
response = MagicMock()
response.headers = {"docker-content-digest": digest} if digest else {}
return response


class TestFetchManifest(unittest.TestCase):
"""Exercise the HEAD-first manifest resolution flow."""

def setUp(self):
self.view = Manifests()
self.remote = MagicMock()
self.remote.namespaced_upstream_name = "library/test"
self.remote.url = "https://registry.example/"

def _set_downloaders(self, *downloaders):
self.remote.get_downloader = MagicMock(side_effect=list(downloaders))

@patch("pulp_container.app.registry_api.get_domain")
@patch("pulp_container.app.registry_api.models.Manifest.objects")
def test_local_hit_issues_only_head(self, mock_manifest_objects, mock_get_domain):
digest = "sha256:" + "a" * 64
local_manifest = MagicMock()
mock_manifest_objects.filter.return_value.first.return_value = local_manifest

head_downloader = MagicMock()
head_downloader.fetch.return_value = _mock_response(digest)
self._set_downloaders(head_downloader)

manifest, response = self.view.fetch_manifest(self.remote, "latest")

self.assertIs(manifest, local_manifest)
self.remote.get_downloader.assert_called_once()
head_downloader.fetch.assert_called_once()
_, kwargs = head_downloader.fetch.call_args
self.assertEqual(kwargs["extra_data"]["http_method"], "head")

@patch("pulp_container.app.registry_api.get_domain")
@patch("pulp_container.app.registry_api.models.Manifest.objects")
def test_local_miss_falls_back_to_get(self, mock_manifest_objects, mock_get_domain):
digest = "sha256:" + "b" * 64
mock_manifest_objects.filter.return_value.first.return_value = None

head_downloader = MagicMock()
head_downloader.fetch.return_value = _mock_response(digest)
get_downloader = MagicMock()
get_downloader.fetch.return_value = _mock_response(digest)
self._set_downloaders(head_downloader, get_downloader)

manifest, response = self.view.fetch_manifest(self.remote, "latest")

self.assertIsNone(manifest)
self.assertIs(response, get_downloader.fetch.return_value)
self.assertEqual(self.remote.get_downloader.call_count, 2)
head_downloader.fetch.assert_called_once()
get_downloader.fetch.assert_called_once()
_, kwargs = get_downloader.fetch.call_args
self.assertEqual(kwargs["extra_data"]["http_method"], "get")

@patch("pulp_container.app.registry_api.get_domain")
@patch("pulp_container.app.registry_api.models.Manifest.objects")
def test_missing_digest_header_falls_back_to_get(self, mock_manifest_objects, mock_get_domain):
mock_manifest_objects.filter.return_value.first.return_value = None

head_downloader = MagicMock()
head_downloader.fetch.return_value = _mock_response(digest=None)
get_downloader = MagicMock()
get_downloader.fetch.return_value = _mock_response("sha256:" + "c" * 64)
self._set_downloaders(head_downloader, get_downloader)

self.view.fetch_manifest(self.remote, "latest")

self.assertEqual(self.remote.get_downloader.call_count, 2)
get_downloader.fetch.assert_called_once()

def test_404_on_head_raises_manifest_not_found(self):
head_downloader = MagicMock()
head_downloader.fetch.side_effect = ClientResponseError(
request_info=MagicMock(), history=(), status=404
)
self._set_downloaders(head_downloader)

with self.assertRaises(ManifestNotFound):
self.view.fetch_manifest(self.remote, "missing-tag")

def test_429_on_head_raises_throttled(self):
head_downloader = MagicMock()
head_downloader.fetch.side_effect = ClientResponseError(
request_info=MagicMock(), history=(), status=429
)
self._set_downloaders(head_downloader)

with self.assertRaises(Throttled):
self.view.fetch_manifest(self.remote, "latest")

def test_other_status_on_head_raises_bad_gateway(self):
head_downloader = MagicMock()
head_downloader.fetch.side_effect = ClientResponseError(
request_info=MagicMock(), history=(), status=500, message="boom"
)
self._set_downloaders(head_downloader)

with self.assertRaises(BadGateway):
self.view.fetch_manifest(self.remote, "latest")

def test_connection_error_on_head_raises_gateway_timeout(self):
head_downloader = MagicMock()
head_downloader.fetch.side_effect = ClientConnectionError()
self._set_downloaders(head_downloader)

with self.assertRaises(GatewayTimeout):
self.view.fetch_manifest(self.remote, "latest")

def test_timeout_on_head_raises_gateway_timeout(self):
head_downloader = MagicMock()
head_downloader.fetch.side_effect = TimeoutException()
self._set_downloaders(head_downloader)

with self.assertRaises(GatewayTimeout):
self.view.fetch_manifest(self.remote, "latest")


if __name__ == "__main__":
unittest.main()
Loading