From 055cb761b3c36f56686232baff0b06f9956ecd0c Mon Sep 17 00:00:00 2001 From: Gerrod Ubben Date: Thu, 10 Sep 2026 11:40:27 -0400 Subject: [PATCH] Fix pull-through distribution overlapping base-paths fixes: #2494 Generated-by: gpt-5.6-sol --- CHANGES/2494.bugfix | 1 + pulp_container/app/authorization.py | 12 +- pulp_container/app/cache.py | 16 +- pulp_container/app/checks.py | 30 +++ ...ainer-repair-pull-through-distributions.py | 80 ++++++++ pulp_container/app/pull_through.py | 77 ++++++++ pulp_container/app/registry_api.py | 16 +- pulp_container/app/serializers.py | 77 ++++++-- pulp_container/app/viewsets.py | 24 +++ pulp_container/constants.py | 3 + .../tests/functional/api/test_domains.py | 5 +- .../functional/api/test_pull_through_cache.py | 35 +++- .../api/test_remote_filter_pull_through.py | 2 +- pulp_container/tests/functional/conftest.py | 5 +- .../tests/unit/test_pull_through_repair.py | 124 ++++++++++++ pulp_container/tests/unit/test_serializers.py | 177 +++++++++++++++++- 16 files changed, 624 insertions(+), 60 deletions(-) create mode 100644 CHANGES/2494.bugfix create mode 100644 pulp_container/app/management/commands/container-repair-pull-through-distributions.py create mode 100644 pulp_container/app/pull_through.py create mode 100644 pulp_container/tests/unit/test_pull_through_repair.py diff --git a/CHANGES/2494.bugfix b/CHANGES/2494.bugfix new file mode 100644 index 000000000..dd8304ffd --- /dev/null +++ b/CHANGES/2494.bugfix @@ -0,0 +1 @@ +Prevent container distributions from overlapping other distribution base paths. Pull-through distributions **NEED** to be fixed before upgrading pulpcore; run `pulpcore-manager container-repair-pull-through-distributions` to repair existing distributions. diff --git a/pulp_container/app/authorization.py b/pulp_container/app/authorization.py index 058567de7..e19b4efe3 100644 --- a/pulp_container/app/authorization.py +++ b/pulp_container/app/authorization.py @@ -10,7 +10,6 @@ from cryptography.hazmat.backends import default_backend from cryptography.hazmat.primitives import serialization from django.conf import settings -from django.db.models import F, Value from django.http import HttpRequest from rest_framework.request import Request @@ -21,8 +20,8 @@ from pulp_container.app.models import ( ContainerDistribution, ContainerNamespace, - ContainerPullThroughDistribution, ) +from pulp_container.app.pull_through import get_pull_through_distribution TOKEN_EXPIRATION_TIME = getattr(settings, "TOKEN_EXPIRATION_TIME", 300) @@ -209,15 +208,6 @@ def generate_claim_set(issuer, issued_at, subject, audience, access): } -def get_pull_through_distribution(path, domain): - return ( - ContainerPullThroughDistribution.objects.annotate(path=Value(path)) - .filter(pulp_domain=domain, path__startswith=F("base_path")) - .order_by("-base_path") - .first() - ) - - class PermissionChecker: def __init__(self, user): self.user = user diff --git a/pulp_container/app/cache.py b/pulp_container/app/cache.py index c879cd335..1d7fb6341 100644 --- a/pulp_container/app/cache.py +++ b/pulp_container/app/cache.py @@ -1,11 +1,14 @@ from django.core.exceptions import ObjectDoesNotExist -from django.db.models import F, Value from pulpcore.plugin.cache import AsyncContentCache, CacheKeys, SyncContentCache from pulpcore.plugin.util import cache_key, get_domain from pulp_container.app.exceptions import RepositoryNotFound -from pulp_container.app.models import ContainerDistribution, ContainerPullThroughDistribution +from pulp_container.app.models import ContainerDistribution +from pulp_container.app.pull_through import ( + get_pull_through_distribution, + get_pull_through_distribution_path, +) ACCEPT_HEADER_KEY = "accept_header" QUERY_KEY = "query" @@ -83,15 +86,12 @@ def find_base_path_cached(request, cached): try: distro = ContainerDistribution.objects.get(base_path=path, pulp_domain=domain) except ObjectDoesNotExist: - distro = ( - ContainerPullThroughDistribution.objects.annotate(path=Value(path)) - .filter(path__startswith=F("base_path"), pulp_domain=domain) - .order_by("-base_path") - .first() - ) + distro = get_pull_through_distribution(path, domain) if not distro: raise RepositoryNotFound(name=path) + return cache_key(get_pull_through_distribution_path(distro)) + return cache_key(distro.base_path) diff --git a/pulp_container/app/checks.py b/pulp_container/app/checks.py index dd88ff9d7..86f0f67e3 100644 --- a/pulp_container/app/checks.py +++ b/pulp_container/app/checks.py @@ -1,6 +1,11 @@ from django.conf import settings from django.core.checks import Error as CheckError +from django.core.checks import Warning as CheckWarning from django.core.checks import register +from django.db import OperationalError, ProgrammingError + +from pulp_container.app.models import ContainerPullThroughDistribution +from pulp_container.app.pull_through import PULL_THROUGH_DISTRIBUTION_MARKER @register(deploy=True) @@ -45,3 +50,28 @@ def container_settings_check(app_configs, **kwargs): ) return errors + + +@register() +def pull_through_distribution_check(app_configs, **kwargs): + """Warn when legacy pull-through distributions need repair.""" + try: + needs_repair = ContainerPullThroughDistribution.objects.exclude( + pulp_labels__contains=PULL_THROUGH_DISTRIBUTION_MARKER + ).exists() + except (OperationalError, ProgrammingError): + return [] + + if not needs_repair: + return [] + + return [ + CheckWarning( + "Pull-through distributions need base-path repair.", + hint=( + "Run 'pulpcore-manager container-repair-pull-through-distributions' and " + "resolve any distributions it reports." + ), + id="pulp_container.W001", + ) + ] diff --git a/pulp_container/app/management/commands/container-repair-pull-through-distributions.py b/pulp_container/app/management/commands/container-repair-pull-through-distributions.py new file mode 100644 index 000000000..dd272506a --- /dev/null +++ b/pulp_container/app/management/commands/container-repair-pull-through-distributions.py @@ -0,0 +1,80 @@ +from gettext import gettext as _ +from uuid import uuid4 + +from django.conf import settings +from django.core.management import BaseCommand +from django.db import transaction + +from pulpcore.plugin.cache import SyncContentCache + +from pulp_container.app.models import ContainerPullThroughDistribution +from pulp_container.app.pull_through import ( + PULL_THROUGH_DISTRIBUTION_MARKER, + find_container_distribution_overlaps, +) + + +class Command(BaseCommand): + """Repair pull-through distributions whose registry path is stored as their base path.""" + + help = _("Repair pull-through distribution base paths and report remaining overlaps.") + + def handle(self, *args, **options): + """Move eligible pull-through base paths and report unresolved overlaps.""" + repaired = 0 + old_base_paths = [] + distributions = ContainerPullThroughDistribution.objects.exclude( + pulp_labels__contains=PULL_THROUGH_DISTRIBUTION_MARKER + ).select_related("pulp_domain") + + for distribution in distributions.iterator(): + if distribution.name != distribution.base_path: + continue + + old_base_paths.append(distribution.base_path) + distribution.base_path = str(uuid4()) + distribution.pulp_labels = { + **(distribution.pulp_labels or {}), + **PULL_THROUGH_DISTRIBUTION_MARKER, + } + with transaction.atomic(): + distribution.save(update_fields=["base_path", "pulp_labels"]) + repaired += 1 + + self.stdout.write(self.style.SUCCESS(f"Repaired {repaired} pull-through distributions.")) + + unresolved = ContainerPullThroughDistribution.objects.exclude( + pulp_labels__contains=PULL_THROUGH_DISTRIBUTION_MARKER + ).select_related("pulp_domain") + if unresolved: + self.stdout.write( + self.style.WARNING( + "The following pull-through distributions require manual repair:" + ) + ) + for distribution in unresolved: + self.stdout.write(self._format_distribution(distribution)) + + overlaps = find_container_distribution_overlaps() + if overlaps: + self.stdout.write( + self.style.WARNING( + "The following container distributions have overlapping base paths:" + ) + ) + for distribution, other in overlaps: + self.stdout.write( + f" {self._format_distribution(distribution).strip()} overlaps " + f"{self._format_distribution(other).strip()}" + ) + + if settings.CACHE_ENABLED and old_base_paths: + SyncContentCache().delete(base_key=old_base_paths) + self.stdout.write(self.style.SUCCESS("Flushed repaired distribution cache entries.")) + + @staticmethod + def _format_distribution(distribution): + return ( + f" domain={distribution.pulp_domain.name!r} pk={distribution.pk} " + f"name={distribution.name!r} base_path={distribution.base_path!r}" + ) diff --git a/pulp_container/app/pull_through.py b/pulp_container/app/pull_through.py new file mode 100644 index 000000000..913f564f9 --- /dev/null +++ b/pulp_container/app/pull_through.py @@ -0,0 +1,77 @@ +from django.db.models import F, Q, TextField, Value +from django.db.models.functions import Concat + +from pulpcore.plugin.models import Distribution + +from pulp_container.app.models import ContainerDistribution, ContainerPullThroughDistribution +from pulp_container.constants import ( + PULL_THROUGH_DISTRIBUTION_LABEL, + PULL_THROUGH_DISTRIBUTION_LABEL_VALUE, +) + +PULL_THROUGH_DISTRIBUTION_MARKER = { + PULL_THROUGH_DISTRIBUTION_LABEL: PULL_THROUGH_DISTRIBUTION_LABEL_VALUE +} + + +def is_pull_through_distribution_marked(distribution): + """Return whether a pull-through distribution uses its name as its registry path.""" + return all( + (distribution.pulp_labels or {}).get(key) == value + for key, value in PULL_THROUGH_DISTRIBUTION_MARKER.items() + ) + + +def get_pull_through_distribution_path(distribution): + """Return the registry path represented by a pull-through distribution.""" + if is_pull_through_distribution_marked(distribution): + return distribution.name + return distribution.base_path + + +def _match_path(queryset, path, field): + """Return the longest segment-aware path match from a queryset.""" + prefix_field = f"{field}_prefix" + return ( + queryset.annotate( + requested_path=Value(path, output_field=TextField()), + **{prefix_field: Concat(F(field), Value("/"), output_field=TextField())}, + ) + .filter(Q(requested_path=F(field)) | Q(requested_path__startswith=F(prefix_field))) + .order_by(f"-{field}") + .first() + ) + + +def get_pull_through_distribution(path, domain): + """Find a pull-through distribution using the repaired scheme, then the legacy scheme.""" + distributions = ContainerPullThroughDistribution.objects.filter(pulp_domain=domain) + marked = distributions.filter(pulp_labels__contains=PULL_THROUGH_DISTRIBUTION_MARKER) + if distribution := _match_path(marked, path, "name"): + return distribution + + legacy = distributions.exclude(pulp_labels__contains=PULL_THROUGH_DISTRIBUTION_MARKER) + return _match_path(legacy, path, "base_path") + + +def find_container_distribution_overlaps(): + """Return pairs where a container distribution overlaps any distribution.""" + container_ids = set(ContainerDistribution.objects.values_list("pk", flat=True)) + distributions = Distribution.objects.select_related("pulp_domain").order_by( + "pulp_domain_id", "base_path", "pk" + ) + by_domain_and_path = { + (distribution.pulp_domain_id, distribution.base_path): distribution + for distribution in distributions.iterator() + } + overlaps = [] + + for distribution in by_domain_and_path.values(): + parts = distribution.base_path.split("/") + for index in range(1, len(parts)): + parent_path = "/".join(parts[:index]) + if parent := by_domain_and_path.get((distribution.pulp_domain_id, parent_path)): + if parent.pk in container_ids or distribution.pk in container_ids: + overlaps.append((parent, distribution)) + + return overlaps diff --git a/pulp_container/app/registry_api.py b/pulp_container/app/registry_api.py index 442c74569..34101a5fa 100644 --- a/pulp_container/app/registry_api.py +++ b/pulp_container/app/registry_api.py @@ -75,6 +75,10 @@ RepositoryInvalid, RepositoryNotFound, ) +from pulp_container.app.pull_through import ( + get_pull_through_distribution, + get_pull_through_distribution_path, +) from pulp_container.app.redirects import ( AzureStorageRedirects, FileStorageRedirects, @@ -366,18 +370,12 @@ def get_drv_pull(self, path): def get_pull_through_drv(self, path): domain = get_domain() - pull_through_cache_distribution = ( - models.ContainerPullThroughDistribution.objects.annotate(path=Value(path)) - .filter(path__startswith=F("base_path"), pulp_domain=domain) - .order_by("-base_path") - .first() - ) + pull_through_cache_distribution = get_pull_through_distribution(path, domain) if not pull_through_cache_distribution or not self.request.user.is_authenticated: raise RepositoryNotFound(name=path) - upstream_name = path.split(pull_through_cache_distribution.base_path, maxsplit=1)[1].strip( - "/" - ) + pull_through_path = get_pull_through_distribution_path(pull_through_cache_distribution) + upstream_name = path.split(pull_through_path, maxsplit=1)[1].strip("/") try: pull_through_remote = models.ContainerPullThroughRemote.objects.get( pk=pull_through_cache_distribution.remote_id diff --git a/pulp_container/app/serializers.py b/pulp_container/app/serializers.py index 0fc49f985..002ffaf49 100644 --- a/pulp_container/app/serializers.py +++ b/pulp_container/app/serializers.py @@ -1,5 +1,6 @@ import re from gettext import gettext as _ +from uuid import uuid4 from django.core.validators import URLValidator from pulp_file.app.models import FileContent @@ -33,13 +34,32 @@ from pulp_container.app import fields, models from pulp_container.app.utils import get_full_path -from pulp_container.constants import SIGNATURE_TYPE +from pulp_container.constants import ( + PULL_THROUGH_DISTRIBUTION_LABEL, + PULL_THROUGH_DISTRIBUTION_LABEL_VALUE, + SIGNATURE_TYPE, +) VALID_SIGNATURE_NAME_REGEX = r"^sha256:[0-9a-f]{64}@[0-9a-f]{32}$" VALID_TAG_REGEX = r"^[A-Za-z0-9][A-Za-z0-9._-]*$" VALID_BASE_PATH_REGEX_COMPILED = re.compile(r"^[a-z0-9]+(?:(?:[._]|__|[-]*)[a-z0-9])*$") +def validate_registry_path(value): + """Validate a path that will be exposed by the container registry.""" + if len(value) > 255: + raise serializers.ValidationError( + _("The entered registry path cannot be longer than 255 characters.") + ) + + if not all(re.match(VALID_BASE_PATH_REGEX_COMPILED, part) for part in value.split("/")): + raise serializers.ValidationError( + _("The provided registry path contains forbidden characters.") + ) + + return value + + class TagSerializer(NoArtifactContentSerializer): """ Serializer for Tags. @@ -499,17 +519,9 @@ def validate(self, data): def validate_base_path(self, value): """Check whether the passed repository base path is valid or not.""" - if len(value) > 255: - raise serializers.ValidationError( - _("The entered base path cannot be longer than 255 characters.") - ) - - if not all(re.match(VALID_BASE_PATH_REGEX_COMPILED, p) for p in value.split("/")): - raise serializers.ValidationError( - _("The provided base path contains forbidden characters.") - ) - - return value + validate_registry_path(value) + # Ensure we follow pulpcore's cross-distribution overlap validation in addition to the OCI rules. + return super().validate_base_path(value) class Meta: model = models.ContainerDistribution @@ -556,18 +568,53 @@ class ContainerPullThroughDistributionSerializer(DistributionSerializer): description = serializers.CharField( help_text=_("An optional description."), required=False, allow_null=True ) + base_path = serializers.CharField( + help_text=_("The registry path. This must match the name when creating a distribution.") + ) + + def validate_name(self, value): + """Validate the public registry path supplied as the distribution name.""" + return validate_registry_path(value) def validate(self, data): validated_data = super().validate(data) + # This is the temporary fix for pull-through distros violating core's overlapping base-path rule + # The user must supply base-path and name as the same value, then after we validate they are good + # we set base-path to an UUID and use the name to construct the prefix of new distros' base-paths + # avoiding the overlapping problem; base-paths can share prefixes, but one must not be a subset of + # another. A proper fix should have us move off of core Distributions all together. + if self.instance is None: + if validated_data["name"] != validated_data["base_path"]: + raise serializers.ValidationError( + {"base_path": _("The name and base path must match.")} + ) + validated_data["base_path"] = str(uuid4()) + validated_data.setdefault("pulp_labels", {})[PULL_THROUGH_DISTRIBUTION_LABEL] = ( + PULL_THROUGH_DISTRIBUTION_LABEL_VALUE + ) + elif ( + "base_path" in validated_data and validated_data["base_path"] != self.instance.base_path + ): + is_marked = (self.instance.pulp_labels or {}).get( + PULL_THROUGH_DISTRIBUTION_LABEL + ) == PULL_THROUGH_DISTRIBUTION_LABEL_VALUE + if is_marked: + message = _("This value cannot be updated.") + else: + message = _( + "Run 'pulpcore-manager container-repair-pull-through-distributions' before " + "updating this value." + ) + raise serializers.ValidationError({"base_path": message}) + if "content_guard" not in validated_data: validated_data["content_guard"] = ContentRedirectContentGuardSerializer.get_or_create( {"name": "content redirect", "pulp_domain": get_domain()} ) - base_path = validated_data.get("base_path") - if base_path: - namespace_name = base_path.split("/")[0] + if name := validated_data.get("name"): + namespace_name = name.split("/")[0] validated_data["namespace"] = ContainerNamespaceSerializer.get_or_create( {"name": namespace_name, "pulp_domain": get_domain()} ) diff --git a/pulp_container/app/viewsets.py b/pulp_container/app/viewsets.py index d666c4818..f2c54f2e2 100644 --- a/pulp_container/app/viewsets.py +++ b/pulp_container/app/viewsets.py @@ -11,6 +11,7 @@ from drf_spectacular.utils import extend_schema from rest_framework import mixins from rest_framework.decorators import action +from rest_framework.exceptions import ValidationError from pulpcore.plugin.models import Content, PulpTemporaryFile from pulpcore.plugin.serializers import AsyncOperationResponseSerializer @@ -40,6 +41,7 @@ ) from pulp_container.app import models, serializers, tasks +from pulp_container.constants import PULL_THROUGH_DISTRIBUTION_LABEL log = logging.getLogger(__name__) @@ -1599,6 +1601,28 @@ class ContainerPullThroughDistributionViewSet(DistributionViewSet, RolesMixin): ], } + @action( + detail=True, + methods=["post"], + serializer_class=DistributionViewSet.set_label.kwargs["serializer_class"], + ) + def set_label(self, request, pk=None, **kwargs): + """Prevent clients from changing the internal pull-through marker.""" + if request.data.get("key") == PULL_THROUGH_DISTRIBUTION_LABEL: + raise ValidationError("This label is managed by pulp_container.") + return super().set_label(request, pk, **kwargs) + + @action( + detail=True, + methods=["post"], + serializer_class=DistributionViewSet.unset_label.kwargs["serializer_class"], + ) + def unset_label(self, request, pk=None, **kwargs): + """Prevent clients from removing the internal pull-through marker.""" + if request.data.get("key") == PULL_THROUGH_DISTRIBUTION_LABEL: + raise ValidationError("This label is managed by pulp_container.") + return super().unset_label(request, pk, **kwargs) + class ContainerNamespaceViewSet( NamedModelViewSet, diff --git a/pulp_container/constants.py b/pulp_container/constants.py index df9381b83..88eb38b4b 100644 --- a/pulp_container/constants.py +++ b/pulp_container/constants.py @@ -75,6 +75,9 @@ SIGNATURE_API_EXTENSION_VERSION = 2 +PULL_THROUGH_DISTRIBUTION_LABEL = "pulp_container.pull_through" +PULL_THROUGH_DISTRIBUTION_LABEL_VALUE = "true" + MANIFEST_TYPE = SimpleNamespace( ARTIFACT="artifact", BOOTABLE="bootable", diff --git a/pulp_container/tests/functional/api/test_domains.py b/pulp_container/tests/functional/api/test_domains.py index a47e7d7d7..828228b10 100644 --- a/pulp_container/tests/functional/api/test_domains.py +++ b/pulp_container/tests/functional/api/test_domains.py @@ -237,11 +237,12 @@ def test_cross_domain_pulp_apis( }, pulp_domain=domain1.name, ) + pt_distribution_name = str(uuid.uuid4()) with pytest.raises(container_bindings.ApiException) as e: container_bindings.DistributionsPullThroughApi.create( { - "name": str(uuid.uuid4()), - "base_path": str(uuid.uuid4()), + "name": pt_distribution_name, + "base_path": pt_distribution_name, "remote": pt_remote.pulp_href, }, pulp_domain=domain2.name, diff --git a/pulp_container/tests/functional/api/test_pull_through_cache.py b/pulp_container/tests/functional/api/test_pull_through_cache.py index d332e8a90..b85f20c2e 100644 --- a/pulp_container/tests/functional/api/test_pull_through_cache.py +++ b/pulp_container/tests/functional/api/test_pull_through_cache.py @@ -1,11 +1,15 @@ import subprocess from subprocess import CalledProcessError -from uuid import uuid4 +from uuid import UUID, uuid4 import pytest from django.conf import settings -from pulp_container.constants import MANIFEST_TYPE +from pulp_container.constants import ( + MANIFEST_TYPE, + PULL_THROUGH_DISTRIBUTION_LABEL, + PULL_THROUGH_DISTRIBUTION_LABEL_VALUE, +) from pulp_container.tests.functional.constants import ( PULP_FIXTURE_1, PULP_FIXTURE_1_MANIFEST_A_DIGEST, @@ -60,7 +64,7 @@ def _pull_and_verify(images, pull_through_distribution): tags_to_verify = [] for version, image_path in enumerate(images, start=1): remote_image_path = f"{REGISTRY_V2}/{image_path}" - local_image_path = f"{pull_through_distribution.base_path}/{image_path}" + local_image_path = f"{pull_through_distribution.name}/{image_path}" local_image_pull_path = full_path(local_image_path) # Handle if domain is enabled # 0. test if an anonymous user cannot pull new content through the pull-through cache @@ -122,6 +126,17 @@ def _pull_and_verify(images, pull_through_distribution): return _pull_and_verify +def test_pull_through_distribution_uses_internal_base_path(pull_through_distribution): + distribution = pull_through_distribution() + + UUID(distribution.base_path) + assert distribution.base_path != distribution.name + assert ( + distribution.pulp_labels[PULL_THROUGH_DISTRIBUTION_LABEL] + == PULL_THROUGH_DISTRIBUTION_LABEL_VALUE + ) + + def test_manifest_list_pull(delete_orphans_pre, pull_through_distribution, pull_and_verify): images = [f"{PULP_HELLO_WORLD_REPO}:latest", f"{PULP_HELLO_WORLD_REPO}:linux"] pull_and_verify(images, pull_through_distribution()) @@ -144,8 +159,8 @@ def test_pull_by_digest( # THIS TEST FAILS WHEN THE CONTENT IS ALREADY PRESENT IN PULP!! image1 = f"{PULP_HELLO_WORLD_REPO}@{PULP_HELLO_WORLD_LINUX_AMD64_DIGEST}" image2 = f"{PULP_FIXTURE_1}@{PULP_FIXTURE_1_MANIFEST_A_DIGEST}" - local_image_path1 = f"{full_path(pull_through_distribution())}/{image1}" - local_image_path2 = f"{full_path(pull_through_distribution())}/{image2}" + local_image_path1 = f"{full_path(pull_through_distribution().name)}/{image1}" + local_image_path2 = f"{full_path(pull_through_distribution().name)}/{image2}" with anonymous_user, pytest.raises(CalledProcessError): local_registry.pull(local_image_path1) @@ -186,15 +201,15 @@ def test_pull_from_private_distribution( image1 = f"{PULP_HELLO_WORLD_REPO}:latest" image2 = f"{PULP_FIXTURE_1}:manifest_a" - local_image_path1 = f"{full_path(pull_through_distribution)}/{image1}" - local_image_path2 = f"{full_path(pull_through_distribution)}/{image2}" + local_image_path1 = f"{full_path(pull_through_distribution.name)}/{image1}" + local_image_path2 = f"{full_path(pull_through_distribution.name)}/{image2}" with anonymous_user, pytest.raises(CalledProcessError): local_registry.pull(local_image_path1) local_registry.pull(local_image_path1) - namespace = container_namespace_api.list(name=pull_through_distribution.base_path).results[0] + namespace = container_namespace_api.list(name=pull_through_distribution.name).results[0] add_pull_through_entities_to_cleanup(local_image_path1.split(":")[0]) with anonymous_user, pytest.raises(CalledProcessError): @@ -216,7 +231,7 @@ def test_pull_from_private_distribution( with gen_user(model_roles=["container.containerdistribution_collaborator"]): local_registry.pull(local_image_path2) - distro1_name = f"{pull_through_distribution.base_path}/{image1}".split(":")[0] + distro1_name = f"{pull_through_distribution.name}/{image1}".split(":")[0] distro1 = container_distribution_api.list(name=distro1_name).results[0] distro_1_collaborator = gen_user( object_roles=[("container.containerdistribution_collaborator", distro1.pulp_href)] @@ -241,7 +256,7 @@ def test_conflicting_names_and_paths( monitor_task, ): pull_through_distribution = pull_through_distribution() - local_image_path = f"{pull_through_distribution.base_path}/{str(uuid4())}" + local_image_path = f"{pull_through_distribution.name}/{str(uuid4())}" local_image_pull_path = full_path(local_image_path) # Handle if domain is enabled remote = container_remote_factory(name=local_image_path) diff --git a/pulp_container/tests/functional/api/test_remote_filter_pull_through.py b/pulp_container/tests/functional/api/test_remote_filter_pull_through.py index 6ee06c876..06878a295 100644 --- a/pulp_container/tests/functional/api/test_remote_filter_pull_through.py +++ b/pulp_container/tests/functional/api/test_remote_filter_pull_through.py @@ -20,7 +20,7 @@ def _pull_and_verify(images, pull_through_distribution, includes, excludes, expe distr = pull_through_distribution(includes, excludes) for image_path in images: remote_image_path = f"{REGISTRY_V2}/{image_path}" - local_image_path = full_path(f"{distr.base_path}/{image_path}") + local_image_path = full_path(f"{distr.name}/{image_path}") if image_path not in expected: with pytest.raises(subprocess.CalledProcessError): diff --git a/pulp_container/tests/functional/conftest.py b/pulp_container/tests/functional/conftest.py index 990315f94..99d9f28de 100644 --- a/pulp_container/tests/functional/conftest.py +++ b/pulp_container/tests/functional/conftest.py @@ -511,9 +511,10 @@ def _pull_through_distribution(includes=None, excludes=None, private=False): }, ) + name = str(uuid4()) data = { - "name": str(uuid4()), - "base_path": str(uuid4()), + "name": name, + "base_path": name, "remote": remote.pulp_href, "private": private, } diff --git a/pulp_container/tests/unit/test_pull_through_repair.py b/pulp_container/tests/unit/test_pull_through_repair.py new file mode 100644 index 000000000..1ba9872f6 --- /dev/null +++ b/pulp_container/tests/unit/test_pull_through_repair.py @@ -0,0 +1,124 @@ +from importlib import import_module +from io import StringIO +from types import SimpleNamespace +from unittest.mock import patch +from uuid import uuid4 + +from django.core.management import call_command +from django.test import TestCase +from rest_framework.exceptions import ValidationError + +from pulpcore.plugin.models import Distribution + +from pulp_container.app.checks import pull_through_distribution_check +from pulp_container.app.models import ContainerDistribution, ContainerPullThroughDistribution +from pulp_container.app.pull_through import ( + PULL_THROUGH_DISTRIBUTION_MARKER, + get_pull_through_distribution, + get_pull_through_distribution_path, +) +from pulp_container.app.viewsets import ContainerPullThroughDistributionViewSet +from pulp_container.constants import PULL_THROUGH_DISTRIBUTION_LABEL + + +class TestPullThroughDistributionRepair(TestCase): + def test_command_repairs_eligible_distributions_and_reports_the_rest(self): + repairable = ContainerPullThroughDistribution.objects.create( + name="repairable", base_path="repairable" + ) + unresolved = ContainerPullThroughDistribution.objects.create( + name="unresolved", base_path="different" + ) + other = Distribution.objects.create(name="other", base_path="other") + container = ContainerDistribution.objects.create(name="container", base_path="container") + output = StringIO() + + command_module = import_module( + "pulp_container.app.management.commands.container-repair-pull-through-distributions" + ) + with patch.object( + command_module, + "find_container_distribution_overlaps", + return_value=[(container, other)], + ): + call_command("container-repair-pull-through-distributions", stdout=output) + + repairable.refresh_from_db() + unresolved.refresh_from_db() + self.assertNotEqual(repairable.base_path, repairable.name) + self.assertEqual( + repairable.pulp_labels, + PULL_THROUGH_DISTRIBUTION_MARKER, + ) + self.assertEqual(unresolved.base_path, "different") + self.assertIn("Repaired 1 pull-through distributions.", output.getvalue()) + self.assertIn("name='unresolved'", output.getvalue()) + self.assertIn(f"pk={container.pk}", output.getvalue()) + self.assertIn(f"pk={other.pk}", output.getvalue()) + + def test_check_warns_until_distribution_is_repaired(self): + ContainerPullThroughDistribution.objects.create(name="legacy", base_path="legacy") + + warnings = pull_through_distribution_check(None) + + self.assertEqual(warnings[0].id, "pulp_container.W001") + + call_command("container-repair-pull-through-distributions", stdout=StringIO()) + + self.assertEqual(pull_through_distribution_check(None), []) + + +class TestPullThroughDistributionLookup(TestCase): + def test_marked_distribution_matches_on_name(self): + distribution = ContainerPullThroughDistribution.objects.create( + name="registry-cache", + base_path=str(uuid4()), + pulp_labels=PULL_THROUGH_DISTRIBUTION_MARKER, + ) + + match = get_pull_through_distribution( + "registry-cache/library/busybox", distribution.pulp_domain + ) + + self.assertEqual(match, distribution) + self.assertEqual(get_pull_through_distribution_path(match), "registry-cache") + + def test_unmarked_distribution_falls_back_to_base_path(self): + distribution = ContainerPullThroughDistribution.objects.create( + name="legacy", base_path="legacy" + ) + + match = get_pull_through_distribution("legacy/library/busybox", distribution.pulp_domain) + + self.assertEqual(match, distribution) + self.assertEqual(get_pull_through_distribution_path(match), "legacy") + + def test_match_observes_path_segment_boundaries(self): + distribution = ContainerPullThroughDistribution.objects.create( + name="registry-cache", + base_path=str(uuid4()), + pulp_labels=PULL_THROUGH_DISTRIBUTION_MARKER, + ) + + match = get_pull_through_distribution( + "registry-cache-other/library/busybox", distribution.pulp_domain + ) + + self.assertIsNone(match) + + +class TestPullThroughDistributionLabels(TestCase): + def test_marker_label_actions_are_registered_and_protected(self): + actions = { + action.__name__: action + for action in ContainerPullThroughDistributionViewSet.get_extra_actions() + } + self.assertIn("set_label", actions) + self.assertIn("unset_label", actions) + + view = ContainerPullThroughDistributionViewSet() + request = SimpleNamespace(data={"key": PULL_THROUGH_DISTRIBUTION_LABEL}) + with self.assertRaises(ValidationError): + view.set_label(request) + with self.assertRaises(ValidationError): + view.unset_label(request) diff --git a/pulp_container/tests/unit/test_serializers.py b/pulp_container/tests/unit/test_serializers.py index 7f39a2717..79ba4735c 100644 --- a/pulp_container/tests/unit/test_serializers.py +++ b/pulp_container/tests/unit/test_serializers.py @@ -1,11 +1,27 @@ +from uuid import UUID, uuid4 + from django.conf import settings from django.contrib.auth import get_user_model from django.test import TestCase +from pulpcore.plugin.models import Distribution from pulpcore.plugin.util import set_current_user -from pulp_container.app.models import ContainerPushRepository, ContainerRepository -from pulp_container.app.serializers import ContainerDistributionSerializer, TagOperationSerializer +from pulp_container.app.models import ( + ContainerPullThroughDistribution, + ContainerPullThroughRemote, + ContainerPushRepository, + ContainerRepository, +) +from pulp_container.app.serializers import ( + ContainerDistributionSerializer, + ContainerPullThroughDistributionSerializer, + TagOperationSerializer, +) +from pulp_container.constants import ( + PULL_THROUGH_DISTRIBUTION_LABEL, + PULL_THROUGH_DISTRIBUTION_LABEL_VALUE, +) V3_API_ROOT = ( settings.API_ROOT + "default/api/v3/" if settings.DOMAIN_ENABLED else settings.V3_API_ROOT @@ -80,6 +96,34 @@ def test_invalid_push_version_data(self): self.assertIn("non_field_errors", serializer.errors) self.assertIn("cannot be distributed", str(serializer.errors["non_field_errors"][0])) + def test_base_path_cannot_contain_another_distribution(self): + """Test that the base path cannot contain another distribution's base path.""" + Distribution.objects.create(name="distribution", base_path="test") + data = { + "name": "container distribution", + "base_path": "test/container", + "repository": self.mirror_repository_href, + } + + serializer = ContainerDistributionSerializer(data=data) + + self.assertFalse(serializer.is_valid()) + self.assertIn("base_path", serializer.errors) + + def test_base_path_cannot_be_parent_of_another_distribution(self): + """Test that the base path cannot be the parent of another distribution's base path.""" + Distribution.objects.create(name="distribution", base_path="test/file") + data = { + "name": "container distribution", + "base_path": "test", + "repository": self.mirror_repository_href, + } + + serializer = ContainerDistributionSerializer(data=data) + + self.assertFalse(serializer.is_valid()) + self.assertIn("base_path", serializer.errors) + class TestTagOperationSerializer(TestCase): """Test TagOperationSerializer.""" @@ -103,3 +147,132 @@ def test_invalid_tag(self): self.assertFalse(serializer.is_valid()) self.assertIn("tag", serializer.errors) self.assertIn("tag is not valid", str(serializer.errors["tag"][0])) + + +class TestContainerPullThroughDistributionSerializer(TestCase): + """Test the temporary pull-through base-path handling.""" + + def setUp(self): + self.user = get_user_model().objects.create(username="pull-through-user") + set_current_user(self.user) + self.remote = ContainerPullThroughRemote.objects.create( + name="pull-through remote", + url="https://registry.example.com", + policy="on_demand", + ) + self.remote_href = f"{V3_API_ROOT}remotes/container/pull-through/{self.remote.pk}/" + + def tearDown(self): + super().tearDown() + self.user.delete() + set_current_user(None) + + def test_create_moves_base_path_and_marks_distribution(self): + data = { + "name": "registry-cache", + "base_path": "registry-cache", + "remote": self.remote_href, + } + + serializer = ContainerPullThroughDistributionSerializer(data=data) + + self.assertTrue(serializer.is_valid(), serializer.errors) + UUID(serializer.validated_data["base_path"]) + self.assertEqual( + serializer.validated_data["pulp_labels"][PULL_THROUGH_DISTRIBUTION_LABEL], + PULL_THROUGH_DISTRIBUTION_LABEL_VALUE, + ) + + def test_create_rejects_different_name_and_base_path(self): + data = { + "name": "registry-cache", + "base_path": "different-path", + "remote": self.remote_href, + } + + serializer = ContainerPullThroughDistributionSerializer(data=data) + + self.assertFalse(serializer.is_valid()) + self.assertIn("base_path", serializer.errors) + + def test_create_rejects_invalid_registry_path(self): + data = { + "name": "Invalid Path", + "base_path": "Invalid Path", + "remote": self.remote_href, + } + + serializer = ContainerPullThroughDistributionSerializer(data=data) + + self.assertFalse(serializer.is_valid()) + self.assertIn("name", serializer.errors) + + def test_create_replaces_an_exact_existing_base_path(self): + Distribution.objects.create(name="ordinary distribution", base_path="registry-cache") + data = { + "name": "registry-cache", + "base_path": "registry-cache", + "remote": self.remote_href, + } + + serializer = ContainerPullThroughDistributionSerializer(data=data) + + self.assertTrue(serializer.is_valid(), serializer.errors) + UUID(serializer.validated_data["base_path"]) + + def test_update_allows_non_base_path_change_for_unmarked_distribution(self): + distribution = ContainerPullThroughDistribution.objects.create( + name="registry-cache", + base_path="registry-cache", + remote=self.remote, + ) + serializer = ContainerPullThroughDistributionSerializer( + distribution, data={"description": "updated"}, partial=True + ) + + self.assertTrue(serializer.is_valid(), serializer.errors) + self.assertNotIn("base_path", serializer.validated_data) + self.assertNotIn("pulp_labels", serializer.validated_data) + + def test_update_rejects_unmarked_base_path_change_with_repair_instruction(self): + distribution = ContainerPullThroughDistribution.objects.create( + name="registry-cache", + base_path="different-path", + remote=self.remote, + ) + serializer = ContainerPullThroughDistributionSerializer( + distribution, data={"base_path": str(uuid4())}, partial=True + ) + + self.assertFalse(serializer.is_valid()) + self.assertIn("base_path", serializer.errors) + self.assertIn( + "container-repair-pull-through-distributions", str(serializer.errors["base_path"]) + ) + + def test_update_allows_unchanged_base_path(self): + distribution = ContainerPullThroughDistribution.objects.create( + name="registry-cache", + base_path="registry-cache", + remote=self.remote, + ) + serializer = ContainerPullThroughDistributionSerializer( + distribution, data={"base_path": distribution.base_path}, partial=True + ) + + self.assertTrue(serializer.is_valid(), serializer.errors) + + def test_update_rejects_repaired_base_path_change(self): + distribution = ContainerPullThroughDistribution.objects.create( + name="registry-cache", + base_path=str(uuid4()), + remote=self.remote, + pulp_labels={PULL_THROUGH_DISTRIBUTION_LABEL: PULL_THROUGH_DISTRIBUTION_LABEL_VALUE}, + ) + serializer = ContainerPullThroughDistributionSerializer( + distribution, data={"base_path": str(uuid4())}, partial=True + ) + + self.assertFalse(serializer.is_valid()) + self.assertIn("base_path", serializer.errors) + self.assertIn("cannot be updated", str(serializer.errors["base_path"]))