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..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 = [] @@ -38,7 +73,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..443ced6ef9a 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" @@ -461,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) @@ -693,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/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..0406d0a64db --- /dev/null +++ b/pulpcore/tests/unit/test_storage_backends.py @@ -0,0 +1,361 @@ +"""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.checks import storage_redirect_backend +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.""" + + +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.""" + 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