Fix tag-based manifest pulls 404ing while digest-based pulls succeed

CHANGES/+container-build-error-sanitization.bugfix

@@ -0,0 +1 @@
+Raised a proper `PulpException` subclass instead of a bare `Exception` when building or pushing an OCI image fails, so the error is not sanitized away by pulpcore in a future release.

CHANGES/2417.bugfix

@@ -0,0 +1 @@
+Fixed tag-based manifest pulls returning 404 when the client's `Accept` header listed the tagged manifest's media type together with parameters such as q-values, since the media type comparison did not strip them and had no support for the `*/*` wildcard.

pulp_container/app/exceptions.py

@@ -1,5 +1,7 @@
 from rest_framework.exceptions import APIException, NotFound, ParseError
 
+from pulpcore.plugin.exceptions import PulpException
+
 
 class BadGateway(APIException):
     status_code = 502
@@ -162,6 +164,19 @@ def __init__(self, digest):
         )
 
 
+class ContainerBuildError(PulpException):
+    """Exception to signal that building or pushing an OCI image failed."""
+
+    error_code = "CON0001"
+
+    def __init__(self, message):
+        """Initialize the exception with the decoded stderr of the failed podman command."""
+        self.message = message
+
+    def __str__(self):
+        return self.message
+
+
 class InvalidRequest(ParseError):
     """An exception to render an HTTP 400 response."""
 

pulp_container/app/registry_api.py

@@ -95,6 +95,7 @@
     get_accepted_media_types,
     get_full_path,
     has_task_completed,
+    is_media_type_accepted,
     validate_manifest,
 )
 from pulp_container.constants import (
@@ -166,11 +167,11 @@ def from_tag(cls, tag, request=None):
         manifest_media_type = manifest.media_type
         if request:
             accepted_media_types = get_accepted_media_types(request.headers)
-            if (
-                manifest_media_type not in accepted_media_types
-                and manifest_media_type != MEDIA_TYPE.MANIFEST_V1
+            if accepted_media_types and not is_media_type_accepted(
+                accepted_media_types, manifest_media_type
             ):
-                raise ManifestNotFound(reference=tag.name)
+                if manifest_media_type != MEDIA_TYPE.MANIFEST_V1:
+                    raise ManifestNotFound(reference=tag.name)
 
         # Schema v1 manifests are always returned with the signed content type
         if manifest_media_type == MEDIA_TYPE.MANIFEST_V1:

pulp_container/app/tasks/builder.py

@@ -13,6 +13,7 @@
 )
 from pulpcore.plugin.util import get_domain
 
+from pulp_container.app.exceptions import ContainerBuildError
 from pulp_container.app.models import (
     Blob,
     BlobManifest,
@@ -179,7 +180,7 @@ def build_image(
             stderr=subprocess.PIPE,
         )
         if bud_cp.returncode != 0:
-            raise Exception(bud_cp.stderr)
+            raise ContainerBuildError(bud_cp.stderr.decode())
         image_dir = os.path.join(working_directory, "image")
         os.makedirs(image_dir, exist_ok=True)
         push_cp = subprocess.run(
@@ -188,7 +189,7 @@ def build_image(
             stderr=subprocess.PIPE,
         )
         if push_cp.returncode != 0:
-            raise Exception(push_cp.stderr)
+            raise ContainerBuildError(push_cp.stderr.decode())
         repository_version = add_image_from_directory_to_repository(image_dir, repository, tag)
         if isinstance(containerfile_artifact, PulpTemporaryFile):
             containerfile_artifact.delete()
@@ -265,7 +266,7 @@ def build_image_from_containerfile(
             stderr=subprocess.PIPE,
         )
         if bud_cp.returncode != 0:
-            raise Exception(bud_cp.stderr)
+            raise ContainerBuildError(bud_cp.stderr.decode())
         image_dir = os.path.join(working_directory, "image")
         os.makedirs(image_dir, exist_ok=True)
         push_cp = subprocess.run(
@@ -274,7 +275,7 @@ def build_image_from_containerfile(
             stderr=subprocess.PIPE,
         )
         if push_cp.returncode != 0:
-            raise Exception(push_cp.stderr)
+            raise ContainerBuildError(push_cp.stderr.decode())
         repository_version = add_image_from_directory_to_repository(image_dir, repository, tag)
 
     return repository_version

pulp_container/app/utils.py

@@ -42,6 +42,12 @@ def get_full_path(base_path, pulp_domain=None):
     return base_path
 
 
+def _parse_accept_entry(value):
+    """Return the media type from an Accept header entry, without parameters such as q-values."""
+    media_type = value.strip().split(";", maxsplit=1)[0].strip()
+    return media_type or None
+
+
 def get_accepted_media_types(headers):
     """
     Returns a list of media types from the Accept headers.
@@ -56,12 +62,22 @@ def get_accepted_media_types(headers):
     """
     accepted_media_types = []
     for header, values in headers.items():
-        if header == "Accept":
-            values = [v.strip() for v in values.split(",")]
-            accepted_media_types.extend(values)
+        if header.lower() == "accept":
+            for value in values.split(","):
+                if media_type := _parse_accept_entry(value):
+                    accepted_media_types.append(media_type)
     return accepted_media_types
 
 
+def is_media_type_accepted(accepted_media_types, media_type):
+    """
+    Return whether a manifest media type satisfies an Accept header.
+
+    Supports exact media type matches and the `*/*` wildcard used by some clients.
+    """
+    return any(accepted in ("*/*", media_type) for accepted in accepted_media_types)
+
+
 def urlpath_sanitize(*args):
     """
     Join an arbitrary number of strings into a /-separated path.

pulp_container/tests/functional/api/test_domains.py

@@ -1,4 +1,5 @@
 import uuid
+from contextlib import suppress
 from subprocess import CalledProcessError
 
 import pytest
@@ -11,7 +12,7 @@
 
 
 @pytest.fixture
-def cdomain_factory(domain_factory, pulpcore_bindings):
+def cdomain_factory(domain_factory, pulpcore_bindings, container_bindings, monitor_task):
     domains = []
 
     def _domain_factory(*args, **kwargs):
@@ -22,9 +23,23 @@ def _domain_factory(*args, **kwargs):
     yield _domain_factory
 
     for domain in domains:
+        # ContainerNamespace has a PROTECT FK to Domain, so it must be cleared before
+        # domain_factory's own teardown deletes the domain, or that delete raises
+        # ProtectedError. Some tests already clean their own namespace up via
+        # add_to_cleanup; re-querying here is a no-op for those and a safety net for
+        # any test (e.g. one that creates multiple distributions) that doesn't.
+        namespaces = container_bindings.PulpContainerNamespacesApi.list(
+            pulp_domain=domain.name
+        ).results
+        for namespace in namespaces:
+            with suppress(Exception):
+                response = container_bindings.PulpContainerNamespacesApi.delete(namespace.pulp_href)
+                monitor_task(response.task)
+
         guards = pulpcore_bindings.ContentguardsContentRedirectApi.list(pulp_domain=domain.name)
         for guard in guards.results:
-            pulpcore_bindings.ContentguardsContentRedirectApi.delete(guard.pulp_href)
+            with suppress(Exception):
+                pulpcore_bindings.ContentguardsContentRedirectApi.delete(guard.pulp_href)
 
 
 def test_push_in_domain(

pulp_container/tests/functional/api/test_pull_content.py

@@ -76,9 +76,48 @@ def test_api_performes_schema_conversion(self, bindings_cfg, full_path, setup):
         content_response = requests.get(
             latest_image_url, auth=auth, headers={"Accept": MEDIA_TYPE.MANIFEST_V1}
         )
-        # I don't understand what this is testing
         assert 400 <= content_response.status_code < 500
 
+    def test_api_serves_tag_with_qualified_accept_header(self, bindings_cfg, full_path, setup):
+        """Verify a tag pull succeeds when the Accept header lists the tag's media type
+        alongside q-values, per https://github.com/pulp/pulp_container/issues/2417.
+
+        The tag pull path used to compare Accept header entries verbatim against the
+        manifest's media type, so an entry like ``<media-type>;q=0.9`` never matched even
+        though the client did list the media type. The digest pull path never performed
+        this comparison, so the very same manifest could always be fetched by digest.
+        """
+        _, distribution_with_repo, _ = setup
+        image_path = "/v2/{}/manifests/{}".format(full_path(distribution_with_repo), "latest")
+        latest_image_url = urljoin(bindings_cfg.host, image_path)
+        auth = get_auth_for_url(latest_image_url)
+
+        # Discover the tag's actual media type without constraining the Accept header.
+        unconstrained_response = requests.get(latest_image_url, auth=auth)
+        unconstrained_response.raise_for_status()
+        media_type = unconstrained_response.headers["Content-Type"]
+
+        qualified_accept = f"{media_type};q=0.9, */*;q=0.1"
+        content_response = requests.get(
+            latest_image_url, auth=auth, headers={"Accept": qualified_accept}
+        )
+        content_response.raise_for_status()
+
+        digest = content_response.headers["Docker-Content-Digest"]
+        digest_image_path = f"/v2/{full_path(distribution_with_repo)}/manifests/{digest}"
+        digest_image_url = urljoin(bindings_cfg.host, digest_image_path)
+        digest_response = requests.get(
+            digest_image_url, auth=auth, headers={"Accept": qualified_accept}
+        )
+        digest_response.raise_for_status()
+
+        assert content_response.headers["Content-Type"] == digest_response.headers["Content-Type"]
+        assert (
+            content_response.headers["Docker-Content-Digest"]
+            == digest_response.headers["Docker-Content-Digest"]
+        )
+        assert content_response.content == digest_response.content
+
     def test_create_empty_blob_on_the_fly(self, bindings_cfg, full_path, setup):
         """
         Test if empty blob getscreated and served on the fly.