From 9020ad246fc03c3bb40d7d16f2167601e11b198b Mon Sep 17 00:00:00 2001 From: Daniel Alley Date: Wed, 11 Nov 2026 10:28:09 -0500 Subject: [PATCH 1/2] Resolve storage paths through a backend registry Keep Domain storage identifiers compatible during rolling upgrades by resolving legacy paths through a backend registry without changing persisted or API values. This enables future releases to decouple the storage_class stored on a Domain from the literal Python class used to implement its backend. Capability helpers also let plugins avoid inspecting backend classes. No database migration is required, so new and old services can coexist during a rolling upgrade. Assisted-By: Codex 6 Sol --- .ci/ansible/inventory.yaml | 1 - .../+stable-domain-storage-backends.feature | 1 + .../storage_backend_implementations.md | 68 ++++ docs/user/guides/create-domains.md | 3 +- pulpcore/app/apps.py | 21 +- pulpcore/app/checks.py | 4 +- pulpcore/app/importexport.py | 6 +- pulpcore/app/models/domain.py | 23 +- pulpcore/app/serializers/domain.py | 121 ++++--- pulpcore/app/settings.py | 9 + pulpcore/app/storage_backends.py | 166 +++++++++ pulpcore/app/tasks/export.py | 8 +- pulpcore/app/util.py | 87 +++-- pulpcore/app/views/status.py | 3 +- pulpcore/app/viewsets/domain.py | 5 +- pulpcore/constants.py | 16 +- pulpcore/content/handler.py | 45 +-- pulpcore/plugin/util.py | 4 + pulpcore/responses.py | 38 ++- .../api/test_artifact_distribution.py | 31 +- .../tests/functional/api/test_crud_domains.py | 36 +- .../using_plugin/test_cdn_cache_validation.py | 11 +- pulpcore/tests/unit/test_storage_backends.py | 323 ++++++++++++++++++ 23 files changed, 855 insertions(+), 175 deletions(-) create mode 100644 CHANGES/+stable-domain-storage-backends.feature create mode 100644 docs/dev/learn/domains/storage_backend_implementations.md create mode 100644 pulpcore/app/storage_backends.py create mode 100644 pulpcore/tests/unit/test_storage_backends.py diff --git a/.ci/ansible/inventory.yaml b/.ci/ansible/inventory.yaml index ad2fbceacdc..b70b4d43a76 100644 --- a/.ci/ansible/inventory.yaml +++ b/.ci/ansible/inventory.yaml @@ -6,7 +6,6 @@ all: pulp: pulp-fixtures: rustfs: - ci-sftp: vars: ansible_connection: docker ... diff --git a/CHANGES/+stable-domain-storage-backends.feature b/CHANGES/+stable-domain-storage-backends.feature new file mode 100644 index 00000000000..0e6b2bb4d31 --- /dev/null +++ b/CHANGES/+stable-domain-storage-backends.feature @@ -0,0 +1 @@ +Storage backends can now be replaced without changing Domain records or the Domain API. diff --git a/docs/dev/learn/domains/storage_backend_implementations.md b/docs/dev/learn/domains/storage_backend_implementations.md new file mode 100644 index 00000000000..9670734e1bc --- /dev/null +++ b/docs/dev/learn/domains/storage_backend_implementations.md @@ -0,0 +1,68 @@ +# Replacing a Domain storage implementation + +Domain records retain historical storage class paths so older Pulp processes can read them during +a rolling upgrade. Pulp resolves those paths to stable backend IDs (`filesystem`, `s3`, `azure`, and +`gcs`), then selects the installed class through `DOMAIN_STORAGE_BACKENDS`. The API still advertises +the historical class paths and returns them on reads. It also accepts stable IDs in raw requests, +but generated clients may reject those IDs because the schema still lists the historical paths. +Existing `S3Boto3Storage` paths are preserved rather than rewritten to the newer S3 path. + +This boundary replaces the code for an existing storage type; it does not register a new type. +Only filesystem, S3, and Azure are currently offered for new Domains. GCS is recognized for +existing configurations, and SFTP is not supported. + +## Select an implementation + +Set the class path for each non-default Domain storage type you are replacing: + +```python +DOMAIN_STORAGE_BACKENDS = { + "filesystem": "pulpcore.app.models.storage.FileSystem", + "s3": "my_storage.backends.s3.S3Storage", + "azure": "storages.backends.azure_storage.AzureStorage", + "gcs": "storages.backends.gcloud.GoogleCloudStorage", +} +``` + +The default Domain instead uses Django's `STORAGES["default"]`. When replacing that class, set +both its `BACKEND` and the corresponding `DOMAIN_STORAGE_BACKENDS` entry. The mapping lets Pulp +identify its storage type without requiring the replacement's Python path in a Domain record. +An unrecognized default class can serve redirects only if it implements `pulp_url`. + +For direct content redirects, a replacement may implement +`pulp_url(name, headers=None, method=None)` to sign URLs with Pulp's response-header overrides. +Without that hook, Pulp calls the django-storages-style `url()` method for recognized object storage +types. For ranged content reads, a replacement may expose a synchronous +`open_stream(name, start=0, length=None)` context manager. Pulp falls back to Django's `open()` and +`seek()` if the hook is absent; that fallback bounds the returned bytes but may buffer a whole +remote object. The stream must release its resources when the context exits. + +## Use storage from a plugin + +Plugin code should use `pulpcore.plugin.util.get_local_storage_path(file, domain=None)` when it +needs an actual filesystem path. It returns `None` when the backend does not provide one; other +storage errors are propagated. Use `get_direct_storage_url(file, headers=None, http_method=None, +domain=None)` when a consumer needs a URL directly to object storage. It returns `None` when the +Domain uses filesystem storage or has redirects disabled, and raises `NotImplementedError` when +redirects are requested from an unsupported backend. Both functions accept a Pulp-managed file +such as `Artifact.file` or `PulpTemporaryFile.file`. If the file belongs to another Domain, pass +that Domain explicitly; otherwise they use the current Domain. + +The URL function accepts generic `Content-Type` and `Content-Disposition` response headers and +translates them into the selected provider's signed URL parameters. Plugins should not inspect +`storage_class` or pass provider-specific parameters to `storage.url()`. The existing +`get_artifact_url(artifact, ...)` differs: it always returns a URL, falling back to the content +app when a direct URL is unavailable. + +## Roll out a replacement + +Keep the Domain's storage type, bucket or container, prefix, object names, credentials, and URL +behavior compatible. Install the replacement and its dependencies on every API, content, and worker +process, and test reads, writes, range requests, and signed redirects against existing objects. +Then change the deployment settings. A code-only change needs no Domain data migration. Moving +objects to a different location still requires a storage migration. + +During a rolling upgrade, new processes continue to store and return historical class paths so +older processes can read the same Domain rows. Keep the old implementation installed until those +processes and the rollback window are gone. Do not switch persisted `storage_class` values to IDs +while any process still requires the historical paths. diff --git a/docs/user/guides/create-domains.md b/docs/user/guides/create-domains.md index ef8307add6b..f590da31578 100644 --- a/docs/user/guides/create-domains.md +++ b/docs/user/guides/create-domains.md @@ -85,8 +85,7 @@ Specific examples for different storage backends: !!! warning - Changing the `storage-class` or `storage-settings` of an in-use domain is dangerous and can result in a broken domain. - TODO Migration feature + Changing a domain's storage location without migrating its artifacts can break the domain. ## Using Domains diff --git a/pulpcore/app/apps.py b/pulpcore/app/apps.py index 374fa526d73..28275ea9b5c 100644 --- a/pulpcore/app/apps.py +++ b/pulpcore/app/apps.py @@ -11,6 +11,7 @@ from django.db.models.signals import post_migrate, pre_migrate from django.utils.module_loading import module_has_submodule +from pulpcore.constants import STORAGE_BACKENDS from pulpcore.exceptions.plugin import MissingPlugin VIEWSETS_MODULE_NAME = "viewsets" @@ -333,18 +334,30 @@ def _ensure_default_domain(sender, apps, **kwargs): with suppress(LookupError, ProgrammingError): table_names = connection.introspection.table_names() if "core_domain" in table_names: + from pulpcore.app.storage_backends import ( + backend_id, + default_backend_id, + legacy_backend_path, + ) from pulpcore.app.util import get_default_domain default = get_default_domain() # Cache the default domain + # A class-path change within one backend must not rewrite the legacy row. + storage_changed = default_backend_id() != ( + backend_id(default.storage_class) or default.storage_class + ) # Match the Pulp settings if ( settings.HIDE_GUARDED_DISTRIBUTIONS != default.hide_guarded_distributions or settings.REDIRECT_TO_OBJECT_STORAGE != default.redirect_to_object_storage - or settings.STORAGES["default"]["BACKEND"] != default.storage_class + or storage_changed ): default.hide_guarded_distributions = settings.HIDE_GUARDED_DISTRIBUTIONS default.redirect_to_object_storage = settings.REDIRECT_TO_OBJECT_STORAGE - default.storage_class = settings.STORAGES["default"]["BACKEND"] + if storage_changed: + default.storage_class = legacy_backend_path( + settings.STORAGES["default"]["BACKEND"] + ) default.save(skip_hooks=True) @@ -408,8 +421,10 @@ def _get_permission(perm): def _populate_artifact_serving_distribution(sender, apps, verbosity, **kwargs): + from pulpcore.app.storage_backends import default_backend_id + if ( - settings.STORAGES["default"]["BACKEND"] == "pulpcore.app.models.storage.FileSystem" + default_backend_id() == STORAGE_BACKENDS.FILESYSTEM or not settings.REDIRECT_TO_OBJECT_STORAGE ): try: diff --git a/pulpcore/app/checks.py b/pulpcore/app/checks.py index 36d8967eb7c..980aebeac91 100644 --- a/pulpcore/app/checks.py +++ b/pulpcore/app/checks.py @@ -38,7 +38,9 @@ def secret_key_check(app_configs, **kwargs): def storage_paths(app_configs, **kwargs): warnings = [] - if settings.STORAGES["default"]["BACKEND"] == "pulpcore.app.models.storage.FileSystem": + from pulpcore.app.storage_backends import default_backend_id + + if default_backend_id() == constants.STORAGE_BACKENDS.FILESYSTEM: try: media_root_dev = Path(settings.MEDIA_ROOT).stat().st_dev except OSError: diff --git a/pulpcore/app/importexport.py b/pulpcore/app/importexport.py index 68591a180b7..8b06328f532 100644 --- a/pulpcore/app/importexport.py +++ b/pulpcore/app/importexport.py @@ -17,7 +17,7 @@ from pulpcore.app.models.content import Artifact from pulpcore.app.models.progress import ProgressReport from pulpcore.app.models.repository import Repository -from pulpcore.constants import EXPORT_BATCH_SIZE, TASK_STATES +from pulpcore.constants import EXPORT_BATCH_SIZE, STORAGE_BACKENDS, TASK_STATES log = logging.getLogger(__name__) @@ -118,7 +118,9 @@ def export_artifacts(export, artifact_pks): with ProgressReport(**data) as pb: pb.BATCH_INTERVAL = 5000 - if settings.STORAGES["default"]["BACKEND"] != "pulpcore.app.models.storage.FileSystem": + from pulpcore.app.storage_backends import default_backend_id + + if default_backend_id() != STORAGE_BACKENDS.FILESYSTEM: with tempfile.TemporaryDirectory(dir=".") as temp_dir: for offset in range(0, len(artifact_pks), EXPORT_BATCH_SIZE): batch = artifact_pks[offset : offset + EXPORT_BATCH_SIZE] diff --git a/pulpcore/app/models/domain.py b/pulpcore/app/models/domain.py index e8e22eb76d1..efebf597d29 100644 --- a/pulpcore/app/models/domain.py +++ b/pulpcore/app/models/domain.py @@ -28,7 +28,7 @@ class Domain(BaseModel, AutoAddObjPermsMixin): name (models.SlugField): Unique name of domain pulp_labels (HStoreField): Dictionary of string values description (models.TextField): Optional description of domain - storage_class (models.TextField): Required storage class for backend + storage_class (models.TextField): Historical class-path identifier for the backend storage_settings (EncryptedJSONField): Settings needed to configure storage backend redirect_to_object_storage (models.BooleanField): Redirect to object storage in content app hide_guarded_distributions (models.BooleanField): Hide guarded distributions in content app @@ -41,7 +41,7 @@ class Domain(BaseModel, AutoAddObjPermsMixin): name = models.SlugField(null=False, unique=True) pulp_labels = HStoreField(default=dict) description = models.TextField(null=True) - # Storage class is required, optional settings are validated by serializer + # Class paths remain persisted so older Pulp processes can read Domain rows. storage_class = models.TextField(null=False) storage_settings = EncryptedJSONField(default=dict) # Pulp settings that are appropriate to be set on a "per domain" level @@ -53,21 +53,32 @@ class Domain(BaseModel, AutoAddObjPermsMixin): ) def get_storage(self): - """Returns this domain's instantiated storage class.""" + """Return the deployment-selected storage implementation for this domain.""" if self.name == "default": return default_storage + from pulpcore.app.storage_backends import backend_path + + implementation = backend_path(self.storage_class) if date_storage_tuple := storages.get(self.pulp_id): - last_updated, storage = date_storage_tuple - if self.pulp_last_updated == last_updated: + last_updated, path, storage = date_storage_tuple + # A settings-only implementation swap must invalidate the process cache. + if self.pulp_last_updated == last_updated and path == implementation: return storage from pulpcore.app.serializers import DomainSerializer storage = DomainSerializer(instance=self).create_storage() - storages[self.pulp_id] = (self.pulp_last_updated, storage) + storages[self.pulp_id] = (self.pulp_last_updated, implementation, storage) return storage + @property + def storage_backend(self): + """Return the stable backend ID for a historical class path or an ID.""" + from pulpcore.app.storage_backends import backend_id + + return backend_id(self.storage_class) + @hook(BEFORE_DELETE, when="name", is_now="default") @hook(BEFORE_UPDATE, when="name", was="default") def prevent_default_deletion(self): diff --git a/pulpcore/app/serializers/domain.py b/pulpcore/app/serializers/domain.py index dd15291e003..9986bf328c4 100644 --- a/pulpcore/app/serializers/domain.py +++ b/pulpcore/app/serializers/domain.py @@ -3,7 +3,6 @@ from django.conf import settings from django.core.exceptions import ImproperlyConfigured -from django.utils.module_loading import import_string from drf_spectacular.types import OpenApiTypes from drf_spectacular.utils import extend_schema_field from rest_framework import serializers @@ -17,16 +16,36 @@ ModelSerializer, pulp_labels_validator, ) +from pulpcore.app.storage_backends import ( + LEGACY_STORAGE_CLASSES, + backend_class, + backend_id, + legacy_backend_path, +) from pulpcore.app.util import get_prn +from pulpcore.constants import STORAGE_BACKENDS BACKEND_CHOICES = ( - ("pulpcore.app.models.storage.FileSystem", "Use local filesystem as storage"), - ("storages.backends.s3boto3.S3Boto3Storage", "Use Amazon S3 as storage [deprecated]"), - ("storages.backends.s3.S3Storage", "Use Amazon S3 as storage"), - ("storages.backends.azure_storage.AzureStorage", "Use Azure Blob as storage"), - # ("storages.backends.gcloud.GoogleCloudStorage", "Use Google Cloud as storage"), + (LEGACY_STORAGE_CLASSES.FILESYSTEM, "Use local filesystem as storage"), + (LEGACY_STORAGE_CLASSES.S3BOTO3, "Use Amazon S3 as storage [deprecated]"), + (LEGACY_STORAGE_CLASSES.S3, "Use Amazon S3 as storage"), + (LEGACY_STORAGE_CLASSES.AZURE, "Use Azure Blob as storage"), + # (LEGACY_STORAGE_CLASSES.GCS, "Use Google Cloud as storage"), ) + +class BackendChoiceField(serializers.ChoiceField): + """Keep the legacy API contract while accepting stable IDs as input.""" + + def to_internal_value(self, data): + """Store a class-path identifier understood by older Pulp processes.""" + return super().to_internal_value(legacy_backend_path(data)) + + def to_representation(self, value): + """Represent future ID-valued rows using the legacy API contract.""" + return super().to_representation(legacy_backend_path(value)) + + DEFAULT_CONTENT_TYPES = [ "text/css", "text/javascript", @@ -37,14 +56,14 @@ class BaseSettingsClass(HiddenFieldsMixin, serializers.Serializer): - """A Base Serializer class for different Storage Backend Settings.""" + """Validate the persisted settings shared by a backend's implementations.""" - STORAGE_CLASS = None + STORAGE_BACKEND = None SETTING_MAPPING = None def to_representation(self, instance): - """Handle getting settings values for default domain case.""" - # Should I convert back the saved settings to their Setting names for to_representation? + """Show the process settings used by the default Domain's Django storage.""" + # Process settings overlay the default Domain's stored settings on reads. if getattr(self.context.get("domain", None), "name", None) == "default": for setting_name, field in self.SETTING_MAPPING.items(): value = getattr(settings, setting_name, None) or settings.STORAGES["default"].get( @@ -55,7 +74,7 @@ def to_representation(self, instance): return super().to_representation(instance) def to_internal_value(self, data): - """Translate incoming data from storage setting name to storage init arg.""" + """Translate legacy setting names to the stored constructor arguments.""" init_keys = set(self.SETTING_MAPPING.values()) unknown_fields = set() fields_to_pop = set() @@ -79,19 +98,14 @@ def to_internal_value(self, data): return super().to_internal_value(data) def create(self, validated_data): - """ - Create a storage instance based on the stored settings. - - Subclasses can override this to instantiate objects needed for creation. - """ - storage_class = import_string(self.STORAGE_CLASS) - return storage_class(**validated_data) + """Create a storage instance from the validated settings.""" + return backend_class(self.STORAGE_BACKEND)(**validated_data) class FileSystemSettingsSerializer(BaseSettingsClass): - """A Serializer for FileSystem storage settings.""" + """Validate filesystem settings independently of the selected class path.""" - STORAGE_CLASS = "pulpcore.app.models.storage.FileSystem" + STORAGE_BACKEND = STORAGE_BACKENDS.FILESYSTEM SETTING_MAPPING = { "media_root": "location", "media_url": "base_url", @@ -127,9 +141,9 @@ def create(self, validated_data): class AmazonS3SettingsSerializer(BaseSettingsClass): - """A Serializer for Amazon S3 storage settings.""" + """Validate the existing S3 schema for compatible implementations.""" - STORAGE_CLASS = "storages.backends.s3.S3Storage" + STORAGE_BACKEND = STORAGE_BACKENDS.S3 SETTING_MAPPING = { "aws_s3_access_key_id": "access_key", "aws_access_key_id": "access_key", @@ -214,17 +228,18 @@ def validate(self, data): return data def create(self, validated_data): - """Special handling for TransferConfig if set.""" + """Convert the TransferConfig and create a storage instance.""" if transfer_config := validated_data.get("transfer_config"): + validated_data = dict(validated_data) transfer = self.fields["transfer_config"].create(transfer_config) validated_data["transfer_config"] = transfer return super().create(validated_data) class AzureSettingsSerializer(BaseSettingsClass): - """A Serializer for Azure storage settings.""" + """Validate the existing Azure schema for compatible implementations.""" - STORAGE_CLASS = "storages.backends.azure_storage.AzureStorage" + STORAGE_BACKEND = STORAGE_BACKENDS.AZURE SETTING_MAPPING = { "azure_account_name": "account_name", "azure_account_key": "account_key", @@ -267,9 +282,9 @@ class AzureSettingsSerializer(BaseSettingsClass): class GoogleSettingsSerializer(BaseSettingsClass): - """A Serializer for Google storage settings.""" + """Validate the existing GCS schema for compatible implementations.""" - STORAGE_CLASS = "storages.backends.gcloud.GoogleCloudStorage" + STORAGE_BACKEND = STORAGE_BACKENDS.GCS SETTING_MAPPING = { "gs_project_id": "project_id", # "gs_credentials": "credentials", @@ -310,24 +325,22 @@ class StorageSettingsSerializer(serializers.Serializer): """Serializer for converting a Domain's storage settings.""" STORAGE_MAPPING = { - "pulpcore.app.models.storage.FileSystem": FileSystemSettingsSerializer, - "storages.backends.s3.S3Storage": AmazonS3SettingsSerializer, - "storages.backends.s3boto3.S3Boto3Storage": AmazonS3SettingsSerializer, - "storages.backends.azure_storage.AzureStorage": AzureSettingsSerializer, - "storages.backends.gcloud.GoogleCloudStorage": GoogleSettingsSerializer, + STORAGE_BACKENDS.FILESYSTEM: FileSystemSettingsSerializer, + STORAGE_BACKENDS.S3: AmazonS3SettingsSerializer, + STORAGE_BACKENDS.AZURE: AzureSettingsSerializer, + STORAGE_BACKENDS.GCS: GoogleSettingsSerializer, } def to_representation(self, instance): - """Return the correct serializer based on the Domain's storage class.""" - serializer_class = self.STORAGE_MAPPING[instance.storage_class] + """Serialize settings by backend ID, not by the selected implementation path.""" + serializer_class = self.STORAGE_MAPPING[backend_id(instance.storage_class)] serializer = serializer_class( instance=instance.storage_settings, context={"domain": instance} ) return serializer.data def to_internal_value(self, data): - """Appropriately convert the incoming data based on the Domain's storage class.""" - # Handle Creating & Updating + """Validate settings against the backend type selected for this Domain.""" storage_settings = self.root.initial_data.get("storage_settings", {}) if not isinstance(storage_settings, dict): if isinstance(storage_settings, str): @@ -339,30 +352,30 @@ def to_internal_value(self, data): raise serializers.ValidationError("Storage settings should be a JSON object.") if self.root.instance: - # Use passed in values, if not present fallback onto current values of instance - storage_class = self.root.initial_data.get( - "storage_class", self.root.instance.storage_class + # Updates keep omitted settings only when the backend type has not changed. + storage_class = backend_id( + self.root.initial_data.get("storage_class", self.root.instance.storage_class) ) - if storage_class == self.root.instance.storage_class: + if storage_class == backend_id(self.root.instance.storage_class): storage_settings = {**self.root.instance.storage_settings, **storage_settings} else: - storage_class = self.root.initial_data["storage_class"] + storage_class = backend_id(self.root.initial_data["storage_class"]) ret = {} if serializer_class := self.STORAGE_MAPPING.get(storage_class): serializer = serializer_class(data=storage_settings) serializer.is_valid(raise_exception=True) ret.update(serializer.validated_data) - # Hack to get correct data for DomainSerializer since this field uses source="*" + # source="*" requires this field to return the parent serializer's key. return {"storage_settings": ret} def create_storage(self): - """Instantiate a storage class based on the Domain's storage class.""" + """Revalidate persisted settings before constructing the selected backend.""" if self.root.instance: - storage_class = self.root.instance.storage_class + storage_class = backend_id(self.root.instance.storage_class) storage_settings = self.root.instance.storage_settings else: - storage_class = self.root.initial_data["storage_class"] + storage_class = backend_id(self.root.initial_data["storage_class"]) storage_settings = self.root.initial_data["storage_settings"] serializer_class = self.STORAGE_MAPPING[storage_class] serializer = serializer_class(data=storage_settings) @@ -375,16 +388,20 @@ class BackendSettingsValidator: @staticmethod def _validate_storage_backend(storage_class, storage_settings): - """Ensure that the backend can be used.""" + """Check the deployment-selected class with the Domain's stored settings.""" try: - backend = import_string(storage_class) - except (ImportError, ImproperlyConfigured): + # The API value is a legacy identifier; it need not name the installed class. + backend_class(storage_class) + except (ImportError, AttributeError, KeyError, ValueError, ImproperlyConfigured): raise serializers.ValidationError( detail={"storage_class": _("Backend is not installed on Pulp.")} ) try: - backend(**storage_settings) + # Use the same serializer construction path as Domain.get_storage(), including + # backend-specific adaptation of validated settings for legacy implementations. + serializer_class = StorageSettingsSerializer.STORAGE_MAPPING[backend_id(storage_class)] + serializer_class().create(dict(storage_settings)) except ImproperlyConfigured as e: raise serializers.ValidationError( detail={ @@ -409,7 +426,7 @@ class DomainSerializer(BackendSettingsValidator, ModelSerializer): help_text=_("An optional description."), required=False, allow_null=True ) pulp_labels = serializers.HStoreField(required=False, validators=[pulp_labels_validator]) - storage_class = serializers.ChoiceField( + storage_class = BackendChoiceField( help_text=_("Backend storage class for domain."), choices=BACKEND_CHOICES, ) @@ -474,7 +491,7 @@ def validate(self, data): if self.instance and self.instance.name == "default": return data - storage_class = data.get("storage_class") or self.instance.storage_class + storage_class = data.get("storage_class") or backend_id(self.instance.storage_class) storage_settings = data.get("storage_settings") or self.instance.storage_settings self._validate_storage_backend(storage_class, storage_settings) return data @@ -497,7 +514,7 @@ class Meta: class DomainBackendMigratorSerializer(BackendSettingsValidator, serializers.Serializer): """Special serializer for performing a storage backend migration on a Domain.""" - storage_class = serializers.ChoiceField( + storage_class = BackendChoiceField( help_text=_("The new backend storage class to migrate to."), choices=BACKEND_CHOICES, ) diff --git a/pulpcore/app/settings.py b/pulpcore/app/settings.py index 8a4f1bf1413..e4722fd3e3f 100644 --- a/pulpcore/app/settings.py +++ b/pulpcore/app/settings.py @@ -77,6 +77,15 @@ }, } +# Domain class paths are treated as legacy backend identifiers. Override a path +# here to change the implementation without rewriting stored Domain rows. +DOMAIN_STORAGE_BACKENDS = { + constants.STORAGE_BACKENDS.FILESYSTEM: "pulpcore.app.models.storage.FileSystem", + constants.STORAGE_BACKENDS.S3: "storages.backends.s3.S3Storage", + constants.STORAGE_BACKENDS.AZURE: "storages.backends.azure_storage.AzureStorage", + constants.STORAGE_BACKENDS.GCS: "storages.backends.gcloud.GoogleCloudStorage", +} + REDIRECT_TO_OBJECT_STORAGE = True WORKING_DIRECTORY = DEPLOY_ROOT / "tmp" diff --git a/pulpcore/app/storage_backends.py b/pulpcore/app/storage_backends.py new file mode 100644 index 00000000000..763881dbda0 --- /dev/null +++ b/pulpcore/app/storage_backends.py @@ -0,0 +1,166 @@ +"""Pulp's stable identifiers and operations for domain storage backends. + +The identifiers in this module describe the storage service, not the Python class +used to access it. Changing an implementation must not change a Domain record. +""" + +from contextlib import contextmanager +from types import SimpleNamespace + +from django.conf import settings +from django.utils.module_loading import import_string + +from pulpcore.constants import STORAGE_BACKENDS, STORAGE_RESPONSE_MAP + +LEGACY_STORAGE_CLASSES = SimpleNamespace( + FILESYSTEM="pulpcore.app.models.storage.FileSystem", + S3BOTO3="storages.backends.s3boto3.S3Boto3Storage", + S3="storages.backends.s3.S3Storage", + AZURE="storages.backends.azure_storage.AzureStorage", + GCS="storages.backends.gcloud.GoogleCloudStorage", +) + +LEGACY_BACKENDS = { + LEGACY_STORAGE_CLASSES.FILESYSTEM: STORAGE_BACKENDS.FILESYSTEM, + LEGACY_STORAGE_CLASSES.S3BOTO3: STORAGE_BACKENDS.S3, + LEGACY_STORAGE_CLASSES.S3: STORAGE_BACKENDS.S3, + LEGACY_STORAGE_CLASSES.AZURE: STORAGE_BACKENDS.AZURE, + LEGACY_STORAGE_CLASSES.GCS: STORAGE_BACKENDS.GCS, +} + +LEGACY_BACKEND_PATHS = { + STORAGE_BACKENDS.FILESYSTEM: LEGACY_STORAGE_CLASSES.FILESYSTEM, + STORAGE_BACKENDS.S3: LEGACY_STORAGE_CLASSES.S3, + STORAGE_BACKENDS.AZURE: LEGACY_STORAGE_CLASSES.AZURE, + STORAGE_BACKENDS.GCS: LEGACY_STORAGE_CLASSES.GCS, +} + +BACKEND_IDS = frozenset(vars(STORAGE_BACKENDS).values()) +URL_RESPONSE_HEADERS = frozenset().union(*(mapping for mapping in STORAGE_RESPONSE_MAP.values())) + + +def backend_id(value): + """Return a stable ID for a built-in or configured implementation path. + + Unknown paths return `None`; they are not treated as importable Domain + backends merely because they happen to be present on `sys.path`. + """ + if not isinstance(value, str): + return None + if value in BACKEND_IDS: + return value + if value in LEGACY_BACKENDS: + return LEGACY_BACKENDS[value] + for kind, path in settings.DOMAIN_STORAGE_BACKENDS.items(): + if value == path and kind in BACKEND_IDS: + return kind + return None + + +def default_backend_id(): + """Identify the default Django storage without relying on its class path. + + Unrecognized custom default storages retain their path for compatibility, + but cannot use Pulp's provider-specific redirect behavior. + """ + path = settings.STORAGES["default"]["BACKEND"] + return backend_id(path) or path + + +def legacy_backend_path(value): + """Return a class-path identifier understood by older Pulp processes. + + Existing legacy paths, including the deprecated S3 alias, are preserved. + Stable IDs and configured implementation paths use a canonical legacy path. + """ + if not isinstance(value, str): + return value + if value in LEGACY_BACKENDS: + return value + kind = backend_id(value) + return LEGACY_BACKEND_PATHS.get(kind, value) + + +def backend_path(kind): + """Return the deployment-selected class path for a stable backend ID.""" + kind = backend_id(kind) + if kind is None: + raise ValueError("Unknown storage backend") + return settings.DOMAIN_STORAGE_BACKENDS[kind] + + +def backend_class(kind): + """Import the deployment-selected Django storage implementation.""" + return import_string(backend_path(kind)) + + +def signed_url(storage, kind, name, headers=None, method=None): + """Create a storage URL from Pulp's provider-neutral headers and HTTP method. + + Only non-empty response headers recognized by a supported provider are used; + each backend receives only the headers it supports. Replacement backends can + implement `pulp_url` to translate these headers and the method themselves. + Otherwise, this adapts the request to django-storages URL parameters; only + its S3 backend accepts `method` as `http_method`. Raise `NotImplementedError` + when the selected backend has no supported URL-signing behavior. + """ + response_headers = { + header: value + for header, value in (headers or {}).items() + if header in URL_RESPONSE_HEADERS and value is not None + } + if callable(pulp_url := getattr(storage, "pulp_url", None)): + return pulp_url(name, headers=response_headers, method=method) + + kind = backend_id(kind) + if kind not in STORAGE_RESPONSE_MAP: + raise NotImplementedError(f"Storage backend {kind!r} does not support redirects.") + parameters = { + provider_header: value + for header, provider_header in STORAGE_RESPONSE_MAP[kind].items() + if (value := response_headers.get(header)) is not None + } + if kind == STORAGE_BACKENDS.S3: + return storage.url(name, parameters=parameters, http_method=method) + return storage.url(name, parameters=parameters) + + +class _BoundedReader: + """Keep reads from a storage stream within the requested byte range. + + Clamp each read to the remaining range length so a caller's unbounded or + oversized `read()` cannot return bytes past the range end. The underlying + stream must already be positioned at the requested start offset. + """ + + def __init__(self, file, length): + self.file = file + self.remaining = length + + def read(self, size=-1): + """Read at most `size` bytes, never beyond the selected range.""" + if self.remaining is not None: + size = self.remaining if size is None or size < 0 else min(size, self.remaining) + data = self.file.read(size) + if self.remaining is not None: + self.remaining -= len(data) + return data + + +@contextmanager +def open_stream(storage, name, start=0, length=None): + """Open a bounded read stream and close it on exit, including after errors. + + Backends with native ranged reads should implement `open_stream`. The + Django `open` fallback bounds bytes returned but may still buffer the + entire object internally, depending on the installed implementation. + """ + if start < 0 or (length is not None and length < 0): + raise ValueError("Stream start and length must be non-negative") + if callable(native := getattr(storage, "open_stream", None)): + with native(name, start=start, length=length) as stream: + yield _BoundedReader(stream, length) + else: + with storage.open(name, "rb") as file: + file.seek(start) + yield _BoundedReader(file, length) diff --git a/pulpcore/app/tasks/export.py b/pulpcore/app/tasks/export.py index e5946b24015..3fba4f34a7b 100644 --- a/pulpcore/app/tasks/export.py +++ b/pulpcore/app/tasks/export.py @@ -32,7 +32,7 @@ from pulpcore.app.models.content import ContentArtifact, RemoteArtifact from pulpcore.app.serializers import PulpExportSerializer, relative_path_validator from pulpcore.app.util import Crc32Hasher, HashingFileWriter, compute_file_hash -from pulpcore.constants import FS_EXPORT_METHODS +from pulpcore.constants import FS_EXPORT_METHODS, STORAGE_BACKENDS log = logging.getLogger(__name__) @@ -171,9 +171,9 @@ def _export_to_file_system(path, relative_paths_to_artifacts, method=FS_EXPORT_M Raises: ValidationError: When path is not in the ALLOWED_EXPORT_PATHS setting """ - using_filesystem_storage = ( - settings.STORAGES["default"]["BACKEND"] == "pulpcore.app.models.storage.FileSystem" - ) + from pulpcore.app.storage_backends import default_backend_id + + using_filesystem_storage = default_backend_id() == STORAGE_BACKENDS.FILESYSTEM if method != FS_EXPORT_METHODS.WRITE and not using_filesystem_storage: raise RuntimeError(_("Only write is supported for non-filesystem storage.")) diff --git a/pulpcore/app/util.py b/pulpcore/app/util.py index 1ce6f70bf0a..02ce79a66e4 100644 --- a/pulpcore/app/util.py +++ b/pulpcore/app/util.py @@ -28,6 +28,8 @@ from pulpcore.app.apps import pulp_plugin_configs from pulpcore.app.contexts import _current_domain, _current_user_func, current_pulp_api_version from pulpcore.app.loggers import deprecation_logger +from pulpcore.app.storage_backends import legacy_backend_path, signed_url +from pulpcore.constants import STORAGE_BACKENDS from pulpcore.exceptions.validation import InvalidSignatureError # a little cache so viewset_for_model doesn't have to iterate over every app every time @@ -586,6 +588,41 @@ def _artifact_serving_distribution(): return models.ArtifactDistribution.objects.get() +def get_local_storage_path(file, domain=None): + """Return a Pulp-managed file's local path, or `None` if storage has no path. + + `domain` defaults to the current Domain. Other errors from the storage, such + as an invalid name, are not interpreted as a missing local-path capability. + """ + domain = domain if domain is not None else get_domain() + try: + return domain.get_storage().path(file.name) + except NotImplementedError: + return None + + +def get_direct_storage_url(file, headers=None, http_method=None, domain=None): + """Return a direct URL for a Pulp-managed file when redirects are enabled. + + Return `None` for filesystem storage or a Domain that serves content itself. + Unsupported redirect backends raise `NotImplementedError`. `domain` defaults + to the current Domain; callers handling a file from another Domain must pass it. + """ + domain = domain if domain is not None else get_domain() + if ( + domain.storage_backend == STORAGE_BACKENDS.FILESYSTEM + or not domain.redirect_to_object_storage + ): + return None + return signed_url( + domain.get_storage(), + domain.storage_backend, + file.name, + headers=headers, + method=http_method, + ) + + def get_artifact_url(artifact, headers=None, http_method=None): """Get artifact url. @@ -600,41 +637,18 @@ def get_artifact_url(artifact, headers=None, http_method=None): artifact_file = artifact.file content_disposition = f"attachment;filename={artifact.pk}" artifact_domain = artifact.pulp_domain - if ( - artifact_domain.storage_class == "pulpcore.app.models.storage.FileSystem" - or not artifact_domain.redirect_to_object_storage - ): - return _artifact_serving_distribution().artifact_url(artifact) - elif artifact_domain.storage_class in ( - "storages.backends.s3boto3.S3Boto3Storage", - "storages.backends.s3.S3Storage", - ): - parameters = {"ResponseContentDisposition": content_disposition} - if headers and headers.get("Content-Type"): - parameters["ResponseContentType"] = headers.get("Content-Type") - url = artifact_file.storage.url( - artifact_file.name, parameters=parameters, http_method=http_method - ) - elif artifact_domain.storage_class == "storages.backends.azure_storage.AzureStorage": - parameters = {"content_disposition": content_disposition} - if headers and headers.get("Content-Type"): - parameters["content_type"] = headers.get("Content-Type") - url = artifact_file.storage.url(artifact_file.name, parameters=parameters) - elif artifact_domain.storage_class == "storages.backends.gcloud.GoogleCloudStorage": - parameters = {"response_disposition": content_disposition} - if headers and headers.get("Content-Type"): - parameters["content_type"] = headers.get("Content-Type") - url = artifact_file.storage.url(artifact_file.name, parameters=parameters) - else: - if settings.DOMAIN_ENABLED: - loc = f"domain {artifact_domain.name}.storage_class" - else: - loc = "settings.STORAGES['default']['BACKEND']" - - raise NotImplementedError( - f"The value {loc}={artifact_domain.storage_class} does not allow redirecting." - ) - return url + response_headers = {"Content-Disposition": content_disposition} + if headers and headers.get("Content-Type"): + response_headers["Content-Type"] = headers["Content-Type"] + direct_url = get_direct_storage_url( + artifact_file, + headers=response_headers, + http_method=http_method, + domain=artifact_domain, + ) + if direct_url is not None: + return direct_url + return _artifact_serving_distribution().artifact_url(artifact) def get_current_user(): @@ -671,7 +685,8 @@ def get_default_domain(): default_domain = Domain.objects.get(name="default") except Domain.DoesNotExist: default_domain = Domain( - name="default", storage_class=settings.STORAGES["default"]["BACKEND"] + name="default", + storage_class=legacy_backend_path(settings.STORAGES["default"]["BACKEND"]), ) default_domain.save(skip_hooks=True) diff --git a/pulpcore/app/views/status.py b/pulpcore/app/views/status.py index 6ac28912c1c..c6521377029 100644 --- a/pulpcore/app/views/status.py +++ b/pulpcore/app/views/status.py @@ -16,6 +16,7 @@ from pulpcore.app.redis_connection import get_redis_connection from pulpcore.app.serializers.status import StatusSerializer, V4StatusSerializer from pulpcore.app.util import get_domain +from pulpcore.constants import STORAGE_BACKENDS _logger = logging.getLogger(__name__) StorageSpace = namedtuple("StorageSpace", ("total", "used", "free")) @@ -23,7 +24,7 @@ def _disk_usage(): domain = get_domain() - if domain.storage_class == "pulpcore.app.models.storage.FileSystem": + if domain.storage_backend == STORAGE_BACKENDS.FILESYSTEM: storage = domain.get_storage() try: return shutil.disk_usage(storage.location) diff --git a/pulpcore/app/viewsets/domain.py b/pulpcore/app/viewsets/domain.py index 677b2bf2963..6624c642b5c 100644 --- a/pulpcore/app/viewsets/domain.py +++ b/pulpcore/app/viewsets/domain.py @@ -154,14 +154,13 @@ def migrate(self, request, **kwargs): be backwards compatible. """ instance = request.pulp_domain - data = request.data if instance.name == "default": raise ValidationError(_("Default domain can not be migrated.")) - serializer = DomainBackendMigratorSerializer(data=data) + serializer = DomainBackendMigratorSerializer(data=request.data) serializer.is_valid(raise_exception=True) task = dispatch( migrate_backend, - args=(data,), + args=(serializer.validated_data,), exclusive_resources=[instance], ) return OperationPostponedResponse(task, request) diff --git a/pulpcore/constants.py b/pulpcore/constants.py index 9b48a5f91e1..2bcac13cf8f 100644 --- a/pulpcore/constants.py +++ b/pulpcore/constants.py @@ -106,12 +106,18 @@ "Content-Type": "response_type", } -# Storage-type mapped to storage-response-map +STORAGE_BACKENDS = SimpleNamespace( + FILESYSTEM="filesystem", + S3="s3", + AZURE="azure", + GCS="gcs", +) + +# Stable storage backend ID mapped to its legacy URL parameter names. STORAGE_RESPONSE_MAP = { - "storages.backends.s3boto3.S3Boto3Storage": S3_RESPONSE_HEADER_MAP, - "storages.backends.s3.S3Storage": S3_RESPONSE_HEADER_MAP, - "storages.backends.azure_storage.AzureStorage": AZURE_RESPONSE_HEADER_MAP, - "storages.backends.gcloud.GoogleCloudStorage": GCS_RESPONSE_HEADER_MAP, + STORAGE_BACKENDS.S3: S3_RESPONSE_HEADER_MAP, + STORAGE_BACKENDS.AZURE: AZURE_RESPONSE_HEADER_MAP, + STORAGE_BACKENDS.GCS: GCS_RESPONSE_HEADER_MAP, } # Message users receive when attempting to delete a protected repo version diff --git a/pulpcore/content/handler.py b/pulpcore/content/handler.py index 1bf9104960e..5f76f2afcf8 100644 --- a/pulpcore/content/handler.py +++ b/pulpcore/content/handler.py @@ -28,7 +28,7 @@ from multidict import CIMultiDict from yarl import URL -from pulpcore.constants import CHECKPOINT_TS_FORMAT, STORAGE_RESPONSE_MAP +from pulpcore.constants import CHECKPOINT_TS_FORMAT, STORAGE_BACKENDS from pulpcore.responses import ArtifactResponse os.environ.setdefault("DJANGO_SETTINGS_MODULE", "pulpcore.app.settings") @@ -58,6 +58,7 @@ Remote, RemoteArtifact, ) +from pulpcore.app.storage_backends import signed_url # noqa: E402 from pulpcore.app.util import ( # noqa: E402 cache_key, check_request_was_modified, @@ -1221,27 +1222,19 @@ def _set_secure_headers(self, filename, headers): def _build_response_from_content_artifact(self, content_artifact, headers, request): """Helper method to build the correct response to serve a ContentArtifact.""" - def _set_params_from_headers(hdrs, storage_domain): - # Map standard-response-headers to storage-object-specific keys - params = {} - if storage_domain in STORAGE_RESPONSE_MAP: - for a_key in STORAGE_RESPONSE_MAP[storage_domain]: - if hdrs.get(a_key, None): - params[STORAGE_RESPONSE_MAP[storage_domain][a_key]] = hdrs[a_key] - return params - - def _build_url(**kwargs): - parameters = _set_params_from_headers(headers, domain.storage_class) - storage_url = storage.url(artifact_name, parameters=parameters, **kwargs) + def _build_url(method=None): + storage_url = signed_url( + storage, domain.storage_backend, artifact_name, headers=headers, method=method + ) return URL(storage_url, encoded=True) - def _build_redirect_response(**kwargs): - url = _build_url(**kwargs) + def _build_redirect_response(method=None): + # The object-storage URL needs the disposition before it is signed. + self._set_secure_headers(os.path.basename(content_artifact.relative_path), headers) + url = _build_url(method=method) # Contains signed URL. Tell external caches to not store this headers["Cache-Control"] = NO_STORE_EDGE_CACHE_CONTROL - self._set_secure_headers(os.path.basename(content_artifact.relative_path), headers) - return HTTPFound(url, headers=headers) artifact_file = content_artifact.artifact.file @@ -1250,22 +1243,20 @@ def _build_redirect_response(**kwargs): storage = domain.get_storage() headers["X-PULP-ARTIFACT-SIZE"] = str(artifact_file.size) - if domain.storage_class == "pulpcore.app.models.storage.FileSystem": + if domain.storage_backend == STORAGE_BACKENDS.FILESYSTEM: path = storage.path(artifact_name) if not os.path.exists(path): raise Exception(_("Expected path '{}' is not found").format(path)) return FileResponse(path, headers=headers) elif not domain.redirect_to_object_storage: - return ArtifactResponse(content_artifact.artifact, headers=headers) - elif domain.storage_class in ( - "storages.backends.s3boto3.S3Boto3Storage", - "storages.backends.s3.S3Storage", - ): - return _build_redirect_response(http_method=request.method) - elif domain.storage_class in ( - "storages.backends.azure_storage.AzureStorage", - "storages.backends.gcloud.GoogleCloudStorage", + return ArtifactResponse(content_artifact.artifact, headers=headers, storage=storage) + elif domain.storage_backend in ( + STORAGE_BACKENDS.S3, + STORAGE_BACKENDS.AZURE, + STORAGE_BACKENDS.GCS, ): + return _build_redirect_response(method=request.method) + elif callable(getattr(storage, "pulp_url", None)): return _build_redirect_response() else: raise NotImplementedError() diff --git a/pulpcore/plugin/util.py b/pulpcore/plugin/util.py index b8e63db206f..e77b4ee2dc9 100644 --- a/pulpcore/plugin/util.py +++ b/pulpcore/plugin/util.py @@ -19,8 +19,10 @@ get_current_authenticated_user, get_current_user, get_default_domain, + get_direct_storage_url, get_domain, get_domain_pk, + get_local_storage_path, get_prn, get_url, gpg_verify, @@ -46,6 +48,8 @@ "batch_qs", "extract_pk", "get_artifact_url", + "get_direct_storage_url", + "get_local_storage_path", "get_prn", "get_url", "gpg_verify", diff --git a/pulpcore/responses.py b/pulpcore/responses.py index 1b1fac62a0d..5091cad72b9 100644 --- a/pulpcore/responses.py +++ b/pulpcore/responses.py @@ -8,6 +8,7 @@ ) from pulpcore.app.models import Artifact +from pulpcore.app.storage_backends import open_stream class ArtifactResponse(StreamResponse): @@ -23,6 +24,7 @@ def __init__( status=200, reason=None, headers=None, + storage=None, ): assert (artifact is not None) ^ (artifact_pk is not None) @@ -31,8 +33,9 @@ def __init__( self._artifact = artifact self._artifact_pk = artifact_pk self._chunk_size = chunk_size + self._storage = storage - async def _sendfile(self, request, fobj, offset, count): + async def _sendfile(self, request, storage, name, offset, count): # To keep memory usage low, fobj is transferred in chunks # controlled by the constructor's chunk_size argument. @@ -41,15 +44,17 @@ async def _sendfile(self, request, fobj, offset, count): loop = asyncio.get_event_loop() - await loop.run_in_executor(None, fobj.seek, offset) - - chunk = await loop.run_in_executor(None, fobj.read, min(self._chunk_size, count)) - while chunk: - await writer.write(chunk) - count = count - self._chunk_size - if count <= 0: - break - chunk = await loop.run_in_executor(None, fobj.read, min(self._chunk_size, count)) + context = open_stream(storage, name, start=offset, length=count) + stream = await loop.run_in_executor(None, context.__enter__) + try: + while count > 0: + chunk = await loop.run_in_executor(None, stream.read, min(self._chunk_size, count)) + if not chunk: + break + await writer.write(chunk) + count -= len(chunk) + finally: + await loop.run_in_executor(None, context.__exit__, None, None, None) await writer.drain() return writer @@ -62,8 +67,6 @@ async def prepare(self, request): self._file = self._artifact.file - loop = asyncio.get_event_loop() - if hdrs.CONTENT_TYPE not in self.headers: self.content_type = "application/octet-stream" @@ -150,7 +153,10 @@ async def prepare(self, request): else: offset = 0 - try: - return await self._sendfile(request, self._file, offset, count) - finally: - await loop.run_in_executor(None, self._file.close) + return await self._sendfile( + request, + self._storage or self._artifact.pulp_domain.get_storage(), + self._file.name, + offset, + count, + ) diff --git a/pulpcore/tests/functional/api/test_artifact_distribution.py b/pulpcore/tests/functional/api/test_artifact_distribution.py index 14b6dc7e862..0dfd249ffa5 100644 --- a/pulpcore/tests/functional/api/test_artifact_distribution.py +++ b/pulpcore/tests/functional/api/test_artifact_distribution.py @@ -3,34 +3,39 @@ import requests -OBJECT_STORAGES = ( - "storages.backends.s3boto3.S3Boto3Storage", - "storages.backends.s3.S3Storage", - "storages.backends.azure_storage.AzureStorage", - "storages.backends.gcloud.GoogleCloudStorage", -) - -def test_artifact_distribution(random_artifact, pulp_settings): - settings = pulp_settings +def test_artifact_distribution(random_artifact): + """Check artifact delivery and byte ranges through the configured serving path.""" artifact_uuid = random_artifact.pulp_href.split("/")[-2] commands = ( "from pulpcore.app.models import Artifact;" - "from pulpcore.app.util import get_artifact_url;" - f"print(get_artifact_url(Artifact.objects.get(pk='{artifact_uuid}')));" + "from pulpcore.app.util import get_artifact_url, get_direct_storage_url;" + f"artifact = Artifact.objects.get(pk='{artifact_uuid}');" + "headers = {'Content-Disposition': f'attachment;filename={artifact.pk}'};" + "print(get_artifact_url(artifact));" + "print(get_direct_storage_url(artifact.file, headers=headers, " + "domain=artifact.pulp_domain) is not None);" ) process = subprocess.run(["pulpcore-manager", "shell", "-c", commands], capture_output=True) assert process.returncode == 0 - artifact_url = process.stdout.decode().strip() + artifact_url, has_direct_storage_url = process.stdout.decode().splitlines() response = requests.get(artifact_url) response.raise_for_status() hasher = sha256() hasher.update(response.content) assert hasher.hexdigest() == random_artifact.sha256 - if settings.STORAGES["default"]["BACKEND"] in OBJECT_STORAGES: + if has_direct_storage_url == "True": content_disposition = response.headers.get("Content-Disposition") assert content_disposition is not None filename = artifact_uuid assert f"attachment;filename={filename}" == content_disposition + + # This URL is preauthenticated for content-app delivery or signed for object storage, + # depending on the Domain's configured serving path. + ranged = requests.get(artifact_url, headers={"Range": "bytes=1-3"}) + ranged.raise_for_status() + assert ranged.status_code == 206 + assert ranged.content == response.content[1:4] + assert ranged.headers["Content-Range"] == f"bytes 1-3/{len(response.content)}" diff --git a/pulpcore/tests/functional/api/test_crud_domains.py b/pulpcore/tests/functional/api/test_crud_domains.py index 2b427205f6a..9a043a2207c 100644 --- a/pulpcore/tests/functional/api/test_crud_domains.py +++ b/pulpcore/tests/functional/api/test_crud_domains.py @@ -5,6 +5,7 @@ import pytest +from pulpcore.app.storage_backends import backend_id, default_backend_id from pulpcore.client.pulpcore import ApiException from pulpcore.tests.functional.utils import PulpTaskError @@ -56,6 +57,36 @@ def test_crud_domains(pulpcore_bindings, monitor_task): monitor_task(response.task) +@pytest.mark.parallel +def test_backend_id_input_keeps_legacy_domain_value( + pulpcore_bindings, pulp_api_v3_url, monitor_task +): + """Accept stable backend IDs while retaining legacy class paths in Domain responses.""" + # DomainsApi validates against the legacy-only enum, so post directly to test server handling. + url = f"{pulp_api_v3_url}domains/" + headers = {"Content-Type": "application/json"} + pulpcore_bindings.client.update_params_for_auth(headers, {}, ["basicAuth"], url, "POST", None) + response = pulpcore_bindings.client.rest_client.request( + "POST", + url, + body={ + "name": str(uuid.uuid4()), + "storage_class": "filesystem", + "storage_settings": {"location": ""}, + }, + headers=headers, + ) + assert response.status == 201 + domain = json.loads(response.response.data) + assert domain["storage_class"] == "pulpcore.app.models.storage.FileSystem" + assert ( + pulpcore_bindings.DomainsApi.read(domain["pulp_href"]).storage_class + == "pulpcore.app.models.storage.FileSystem" + ) + response = pulpcore_bindings.DomainsApi.delete(domain["pulp_href"]) + monitor_task(response.task) + + @pytest.mark.parallel def test_default_domain(pulpcore_bindings, pulp_settings): """Test properties around the default domain.""" @@ -66,7 +97,10 @@ def test_default_domain(pulpcore_bindings, pulp_settings): # Read the default domain, ensure storage is set to default default_domain = domains.results[0] assert default_domain.name == "default" - assert default_domain.storage_class == settings.STORAGES["default"]["BACKEND"] + default_domain_backend = ( + backend_id(default_domain.storage_class) or default_domain.storage_class + ) + assert default_domain_backend == default_backend_id() assert default_domain.redirect_to_object_storage == settings.REDIRECT_TO_OBJECT_STORAGE assert default_domain.hide_guarded_distributions == settings.HIDE_GUARDED_DISTRIBUTIONS diff --git a/pulpcore/tests/functional/api/using_plugin/test_cdn_cache_validation.py b/pulpcore/tests/functional/api/using_plugin/test_cdn_cache_validation.py index 9e62ed3f581..96a039b26d7 100644 --- a/pulpcore/tests/functional/api/using_plugin/test_cdn_cache_validation.py +++ b/pulpcore/tests/functional/api/using_plugin/test_cdn_cache_validation.py @@ -8,7 +8,7 @@ import time from base64 import b64encode from math import ceil -from urllib.parse import urljoin +from urllib.parse import parse_qs, urljoin, urlparse from uuid import uuid4 import pytest @@ -218,7 +218,7 @@ def require_object_storage_redirect(self, pulp_settings): ): pytest.skip("Requires object-storage redirects") - def test_cold_request_without_if_modified_since(self, distribution_url): + def test_cold_request_without_if_modified_since(self, distribution_url, pulp_settings): """A signed redirect can be revalidated with Last-Modified.""" url = urljoin(distribution_url, "1.iso") @@ -227,6 +227,13 @@ def test_cold_request_without_if_modified_since(self, distribution_url): assert first.status_code == 302 assert first.headers.get("Location") assert first.headers.get("Cache-Control") == NO_STORE_EDGE_CACHE_CONTROL + assert first.headers["Content-Disposition"] == "attachment;filename=1.iso" + assert first.headers["Content-Security-Policy"] == "default-src 'none'; sandbox" + assert first.headers["X-Content-Type-Options"] == "nosniff" + endpoint = pulp_settings.STORAGES["default"].get("OPTIONS", {}).get("endpoint_url", "") + if urlparse(endpoint).hostname == "rustfs": + query = parse_qs(urlparse(first.headers["Location"]).query) + assert query["response-content-disposition"] == ["attachment;filename=1.iso"] last_modified = first.headers["Last-Modified"] # Regardless if CDN respected Cache-Control, Pulp follow the protocol diff --git a/pulpcore/tests/unit/test_storage_backends.py b/pulpcore/tests/unit/test_storage_backends.py new file mode 100644 index 00000000000..a3e9376ddba --- /dev/null +++ b/pulpcore/tests/unit/test_storage_backends.py @@ -0,0 +1,323 @@ +"""Tests for the stable Domain storage boundary.""" + +from io import BytesIO +from types import SimpleNamespace + +import pytest +from django.test import override_settings + +from pulpcore.app.models.domain import Domain, storages +from pulpcore.app.serializers.domain import ( + BACKEND_CHOICES, + BackendChoiceField, +) +from pulpcore.app.storage_backends import ( + backend_id, + backend_path, + legacy_backend_path, + open_stream, + signed_url, +) +from pulpcore.constants import STORAGE_BACKENDS +from pulpcore.content.handler import Handler +from pulpcore.plugin.util import get_artifact_url, get_direct_storage_url, get_local_storage_path + + +def test_backend_choice_preserves_legacy_api_values(): + """Older processes and clients continue to receive class-path identifiers.""" + field = BackendChoiceField(choices=BACKEND_CHOICES) + assert ( + field.to_internal_value("storages.backends.s3boto3.S3Boto3Storage") + == "storages.backends.s3boto3.S3Boto3Storage" + ) + assert field.to_representation("s3") == "storages.backends.s3.S3Storage" + assert field.to_internal_value("filesystem") == "pulpcore.app.models.storage.FileSystem" + assert "storages.backends.s3boto3.S3Boto3Storage" in dict(BACKEND_CHOICES) + + +class FirstStorage: + """A test implementation configured from Domain settings.""" + + def __init__(self, **settings): + self.settings = settings + + +class SecondStorage(FirstStorage): + """A replacement implementation using the same persisted Domain record.""" + + +@pytest.mark.django_db +def test_domain_implementation_can_change_without_updating_record(): + """Changing the backend mapping replaces its implementation without changing the row.""" + domain = Domain.objects.create( + name="other", + storage_class="pulpcore.app.models.storage.FileSystem", + storage_settings={"location": "/tmp/media"}, + ) + original_storage_class = domain.storage_class + original_storage_settings = domain.storage_settings.copy() + original_last_updated = domain.pulp_last_updated + first_path = "pulpcore.tests.unit.test_storage_backends.FirstStorage" + second_path = "pulpcore.tests.unit.test_storage_backends.SecondStorage" + try: + with override_settings(DOMAIN_STORAGE_BACKENDS={"filesystem": first_path}): + first = domain.get_storage() + assert isinstance(first, FirstStorage) + assert first.settings["location"] == "/tmp/media" + with override_settings(DOMAIN_STORAGE_BACKENDS={"filesystem": second_path}): + second = domain.get_storage() + assert isinstance(second, SecondStorage) + domain.refresh_from_db() + assert domain.storage_class == original_storage_class + assert domain.storage_settings == original_storage_settings + assert domain.pulp_last_updated == original_last_updated + finally: + storages.pop(domain.pulp_id, None) + + +@override_settings(DOMAIN_STORAGE_BACKENDS={"s3": "example.adapters.S3Storage"}) +def test_implementation_mapping_does_not_change_backend_id(): + """A deployment can replace code without updating a Domain row.""" + assert backend_id("example.adapters.S3Storage") == "s3" + assert backend_path("s3") == "example.adapters.S3Storage" + assert legacy_backend_path("example.adapters.S3Storage") == "storages.backends.s3.S3Storage" + + +def test_local_storage_path_uses_backend_capability(): + """A plugin can serve a local file without identifying its storage class.""" + file = SimpleNamespace(name="artifact/abc") + storage = SimpleNamespace(path=lambda name: f"/media/{name}") + domain = SimpleNamespace(get_storage=lambda: storage) + + assert get_local_storage_path(file, domain=domain) == "/media/artifact/abc" + + def unsupported_path(name): + raise NotImplementedError + + storage.path = unsupported_path + assert get_local_storage_path(file, domain=domain) is None + + def invalid_path(name): + raise ValueError("invalid name") + + storage.path = invalid_path + with pytest.raises(ValueError, match="invalid name"): + get_local_storage_path(file, domain=domain) + + +@pytest.mark.parametrize( + ("kind", "redirect"), + [("filesystem", True), ("s3", False)], +) +def test_direct_storage_url_is_absent_when_content_app_serves_file(kind, redirect): + """Local storage and Domains with redirects disabled have no direct URL.""" + domain = SimpleNamespace(storage_backend=kind, redirect_to_object_storage=redirect) + file = SimpleNamespace(name="artifact/abc") + assert get_direct_storage_url(file, domain=domain) is None + + +def test_direct_storage_url_uses_provider_neutral_hook(): + """A custom implementation signs generic headers without a plugin-side switch.""" + file = SimpleNamespace(name="artifact/abc") + + class Storage: + def pulp_url(self, name, headers, method): + assert name == "artifact/abc" + assert headers == { + "Content-Type": "application/octet-stream", + "Content-Disposition": "attachment;filename=archive.tar.gz", + } + assert method == "HEAD" + return "https://example.test/signed" + + domain = SimpleNamespace( + storage_backend=None, + redirect_to_object_storage=True, + get_storage=lambda: Storage(), + ) + assert ( + get_direct_storage_url( + file, + headers={ + "Content-Type": "application/octet-stream", + "Content-Disposition": "attachment;filename=archive.tar.gz", + }, + http_method="HEAD", + domain=domain, + ) + == "https://example.test/signed" + ) + + +def test_direct_storage_url_rejects_unsupported_redirect_backend(): + """An unsupported backend is not mistaken for an intentional content fallback.""" + domain = SimpleNamespace( + storage_backend=None, + redirect_to_object_storage=True, + get_storage=lambda: object(), + ) + with pytest.raises(NotImplementedError): + get_direct_storage_url(SimpleNamespace(name="artifact/abc"), domain=domain) + + +def test_artifact_url_falls_back_to_content_app(monkeypatch): + """Domains without direct storage URLs serve artifact links through the content app.""" + domain = SimpleNamespace(storage_backend="filesystem", redirect_to_object_storage=True) + artifact = SimpleNamespace( + pk="artifact-id", pulp_domain=domain, file=SimpleNamespace(name="artifact/abc") + ) + distribution = SimpleNamespace(artifact_url=lambda artifact: "/pulp/content/artifact/abc") + monkeypatch.setattr("pulpcore.app.util._artifact_serving_distribution", lambda: distribution) + + assert get_artifact_url(artifact) == "/pulp/content/artifact/abc" + + +def test_artifact_url_preserves_existing_direct_headers(): + """Pass the artifact filename, content type, and HTTP method to storage URL signing.""" + + class Storage: + def pulp_url(self, name, headers, method): + assert name == "artifact/abc" + assert headers == { + "Content-Disposition": "attachment;filename=artifact-id", + "Content-Type": "application/octet-stream", + } + assert method == "HEAD" + return "https://example.test/signed" + + domain = SimpleNamespace( + storage_backend=None, + redirect_to_object_storage=True, + get_storage=lambda: Storage(), + ) + artifact = SimpleNamespace( + pk="artifact-id", pulp_domain=domain, file=SimpleNamespace(name="artifact/abc") + ) + assert ( + get_artifact_url( + artifact, headers={"Content-Type": "application/octet-stream"}, http_method="HEAD" + ) + == "https://example.test/signed" + ) + + +def test_content_redirect_signs_disposition_and_keeps_secure_headers(monkeypatch): + """The object response override and Pulp redirect both get safe headers.""" + + class Storage: + def url(self, name, **kwargs): + assert name == "artifact/abc" + assert kwargs == { + "parameters": { + "ResponseContentDisposition": "attachment;filename=1.iso", + "ResponseContentType": "application/octet-stream", + }, + "http_method": "GET", + } + return "https://storage.example/artifact/abc" + + domain = SimpleNamespace( + storage_backend=STORAGE_BACKENDS.S3, + redirect_to_object_storage=True, + get_storage=Storage, + ) + monkeypatch.setattr("pulpcore.content.handler.get_domain", lambda: domain) + content_artifact = SimpleNamespace( + relative_path="nested/1.iso", + artifact=SimpleNamespace(file=SimpleNamespace(name="artifact/abc", size=3)), + ) + + response = Handler()._build_response_from_content_artifact( + content_artifact, + {"Content-Type": "application/octet-stream"}, + SimpleNamespace(method="GET"), + ) + + assert response.headers["Content-Disposition"] == "attachment;filename=1.iso" + assert response.headers["Content-Security-Policy"] == "default-src 'none'; sandbox" + assert response.headers["X-Content-Type-Options"] == "nosniff" + assert response.headers["Cache-Control"] == "no-store" + + +def test_s3_signed_url_translates_headers_and_method(): + """The django-storages adapter receives only provider-supported URL options.""" + + class Storage: + def url(self, name, **kwargs): + assert name == "artifact/abc" + assert kwargs == { + "parameters": { + "ResponseContentDisposition": "attachment;filename=abc", + "ResponseContentType": "text/plain", + }, + "http_method": "HEAD", + } + return "https://example.test/signed" + + assert ( + signed_url( + Storage(), + "s3", + "artifact/abc", + headers={ + "Content-Disposition": "attachment;filename=abc", + "Content-Type": "text/plain", + "Ignored": "value", + }, + method="HEAD", + ) + == "https://example.test/signed" + ) + + +def test_native_stream_is_bounded_and_closed_on_early_exit(): + """The Pulp boundary delegates ranges to implementations that support them.""" + calls = [] + streams = [] + + class Storage: + def open_stream(self, name, start=0, length=None): + from contextlib import contextmanager + + @contextmanager + def stream(): + calls.append((name, start, length)) + file = BytesIO(b"bc-extra") + streams.append(file) + try: + yield file + finally: + file.close() + calls.append("closed") + + return stream() + + storage = Storage() + with open_stream(storage, "artifact/abc", start=1, length=2) as stream: + assert stream.read(10) == b"bc" + assert stream.read(10) == b"" + with open_stream(storage, "artifact/abc", start=1, length=2) as stream: + assert stream.read(1) == b"b" + assert calls == [ + ("artifact/abc", 1, 2), + "closed", + ("artifact/abc", 1, 2), + "closed", + ] + assert all(file.closed for file in streams) + + +def test_fallback_stream_bounds_reads_and_closes_on_error(): + """The Django fallback cannot return bytes beyond the selected range.""" + file = BytesIO(b"abcdef") + + class Storage: + def open(self, name, mode): + assert (name, mode) == ("artifact/abc", "rb") + return file + + with pytest.raises(RuntimeError), open_stream(Storage(), "artifact/abc", 1, 2) as stream: + assert stream.read(10) == b"bc" + assert stream.read(10) == b"" + raise RuntimeError("stop") + assert file.closed From 50fe17c7537d282649d098aebe8911e0cc34e421 Mon Sep 17 00:00:00 2001 From: Daniel Alley Date: Wed, 11 Nov 2026 10:58:50 -0500 Subject: [PATCH 2/2] Validate default storage redirect support Move redirect compatibility validation from the settings validator to a Django system check. Accept built-in providers and custom backends that implement Pulp's pulp_url hook, and skip the check when object-storage redirects are disabled. Assisted-By: Codex 6 Sol --- pulpcore/app/checks.py | 35 ++++++++++++++++++ pulpcore/app/settings.py | 14 -------- pulpcore/tests/unit/test_storage_backends.py | 38 ++++++++++++++++++++ 3 files changed, 73 insertions(+), 14 deletions(-) diff --git a/pulpcore/app/checks.py b/pulpcore/app/checks.py index 980aebeac91..7bb27a8b63d 100644 --- a/pulpcore/app/checks.py +++ b/pulpcore/app/checks.py @@ -15,10 +15,45 @@ Warning as CheckWarning, ) from django.db.models import Q +from django.utils.module_loading import import_string from pulpcore import constants +@register() +def storage_redirect_backend(app_configs, **kwargs): + """Reject default storage that the content app cannot serve with redirects.""" + from pulpcore.app.storage_backends import backend_id + + if not settings.REDIRECT_TO_OBJECT_STORAGE: + return [] + path = settings.STORAGES["default"]["BACKEND"] + backend = backend_id(path) + if backend in { + constants.STORAGE_BACKENDS.FILESYSTEM, + constants.STORAGE_BACKENDS.S3, + constants.STORAGE_BACKENDS.AZURE, + constants.STORAGE_BACKENDS.GCS, + }: + return [] + + # An unregistered default backend can still redirect through Pulp's URL hook. + try: + implementation = import_string(path) + except (ImportError, AttributeError): + implementation = None + if callable(getattr(implementation, "pulp_url", None)): + return [] + + return [ + CheckError( + "STORAGES['default']['BACKEND'] must map to a supported storage type " + "or implement pulp_url when REDIRECT_TO_OBJECT_STORAGE is enabled.", + id="pulpcore.E003", + ) + ] + + @register(deploy=True) def secret_key_check(app_configs, **kwargs): messages = [] diff --git a/pulpcore/app/settings.py b/pulpcore/app/settings.py index e4722fd3e3f..443ced6ef9a 100644 --- a/pulpcore/app/settings.py +++ b/pulpcore/app/settings.py @@ -470,19 +470,6 @@ # Validators -storage_validator = ( - Validator("REDIRECT_TO_OBJECT_STORAGE", eq=False) - | Validator("STORAGES.default.BACKEND", eq="pulpcore.app.models.storage.FileSystem") - | Validator("STORAGES.default.BACKEND", eq="storages.backends.azure_storage.AzureStorage") - | Validator("STORAGES.default.BACKEND", eq="storages.backends.s3.S3Storage") - | Validator("STORAGES.default.BACKEND", eq="storages.backends.s3boto3.S3Boto3Storage") - | Validator("STORAGES.default.BACKEND", eq="storages.backends.gcloud.GoogleCloudStorage") -) -storage_validator.messages["combined"] = ( - "'REDIRECT_TO_OBJECT_STORAGE=True' is only supported with the local file, S3, GCP or Azure " - "storage backend configured in STORAGES['default']['BACKEND']." -) - cache_enabled_validator = Validator("CACHE_ENABLED", eq=True) redis_url_validator = Validator("REDIS_URL", must_exist=True, when=cache_enabled_validator) redis_host_validator = Validator("REDIS_HOST", must_exist=True, when=cache_enabled_validator) @@ -702,7 +689,6 @@ def validate_db_encryption_key_hook(settings): worker_type_allowed_values_validator, worker_type_redis_validator, sha256_validator, - storage_validator, unknown_algs_validator, json_header_auth_validator, authentication_json_header_openapi_security_scheme_validator, diff --git a/pulpcore/tests/unit/test_storage_backends.py b/pulpcore/tests/unit/test_storage_backends.py index a3e9376ddba..0406d0a64db 100644 --- a/pulpcore/tests/unit/test_storage_backends.py +++ b/pulpcore/tests/unit/test_storage_backends.py @@ -6,6 +6,7 @@ import pytest from django.test import override_settings +from pulpcore.app.checks import storage_redirect_backend from pulpcore.app.models.domain import Domain, storages from pulpcore.app.serializers.domain import ( BACKEND_CHOICES, @@ -46,6 +47,43 @@ class SecondStorage(FirstStorage): """A replacement implementation using the same persisted Domain record.""" +class CustomRedirectStorage: + """An unregistered default backend with Pulp's redirect hook.""" + + def pulp_url(self, name, headers=None, method=None): + """Return a URL using Pulp's provider-neutral arguments.""" + return f"https://example.test/{name}" + + +@pytest.mark.parametrize( + ("path", "expected_ids"), + [ + ("pulpcore.app.models.storage.FileSystem", []), + ("storages.backends.s3.S3Storage", []), + ("storages.backends.azure_storage.AzureStorage", []), + ("storages.backends.gcloud.GoogleCloudStorage", []), + ("pulpcore.tests.unit.test_storage_backends.CustomRedirectStorage", []), + ("pulpcore.tests.unit.test_storage_backends.FirstStorage", ["pulpcore.E003"]), + ("missing.storage.Backend", ["pulpcore.E003"]), + ], +) +def test_storage_redirect_backend_check(path, expected_ids): + """Only default classes with a supported content-serving path pass the check.""" + with override_settings( + STORAGES={"default": {"BACKEND": path}}, REDIRECT_TO_OBJECT_STORAGE=True + ): + assert [error.id for error in storage_redirect_backend(None)] == expected_ids + + +@override_settings( + STORAGES={"default": {"BACKEND": "pulpcore.tests.unit.test_storage_backends.FirstStorage"}}, + REDIRECT_TO_OBJECT_STORAGE=False, +) +def test_storage_redirect_backend_check_is_skipped_without_redirects(): + """A default backend needs no redirect hook when Pulp streams content itself.""" + assert storage_redirect_backend(None) == [] + + @pytest.mark.django_db def test_domain_implementation_can_change_without_updating_record(): """Changing the backend mapping replaces its implementation without changing the row."""