Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PULP-210] Update deprecated django storage config #6058

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 21 additions & 10 deletions .ci/ansible/settings.py.j2
Original file line number Diff line number Diff line change
Expand Up @@ -27,19 +27,30 @@ API_ROOT = {{ api_root | repr }}
{% endif %}

{% if s3_test | default(false) %}
DEFAULT_FILE_STORAGE = "storages.backends.s3boto3.S3Boto3Storage"
MEDIA_ROOT = ""
AWS_ACCESS_KEY_ID = "{{ minio_access_key }}"
AWS_SECRET_ACCESS_KEY = "{{ minio_secret_key }}"
AWS_S3_REGION_NAME = "eu-central-1"
AWS_S3_ADDRESSING_STYLE = "path"
MEDIA_ROOT: ""
S3_USE_SIGV4 = True
AWS_S3_SIGNATURE_VERSION = "s3v4"
AWS_STORAGE_BUCKET_NAME = "pulp3"
AWS_S3_ENDPOINT_URL = "http://minio:9000"
AWS_DEFAULT_ACL = "@none None"
STORAGES = {
"default": {
"BACKEND": "storages.backends.s3boto3.S3Boto3Storage",
"OPTIONS": {
"access_key": "{{ minio_access_key }}",
"secret_key": "{{ minio_secret_key }}",
"region_name": "eu-central-1",
"addressing_style": "path",
"signature_version": "s3v4",
"bucket_name": "pulp3",
"endpoint_url": "http://minio:9000",
"default_acl": "@none None",
},
},
"staticfiles": {
"BACKEND": "django.contrib.staticfiles.storage.StaticFilesStorage",
},
}
{% endif %}

# This is using DEFAULT_FILE_STORAGE to test both usages.
# Remove when DEFAULT_FILE_STORAGE is completely removed.
{% if azure_test | default(false) %}
DEFAULT_FILE_STORAGE = "storages.backends.azure_storage.AzureStorage"
MEDIA_ROOT = ""
Expand Down
11 changes: 11 additions & 0 deletions CHANGES/5404.removal
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
Marked django's `DEFAULT_FILE_STORAGE` and `STATIC_FILE_STORAGE` settings to be
removed in pulpcore 3.85. Users should upgrade to use the
[`STORAGES`](https://docs.djangoproject.com/en/4.2/ref/settings/#std-setting-STORAGES)
setting instead.

The [django-upgrade](https://github.com/adamchainz/django-upgrade?tab=readme-ov-file#django-42)
tool can handle simple cases. If cloud storages are being used, refer to django-storages
to adapt their specific storage options. E.g:

* <https://django-storages.readthedocs.io/en/latest/backends/amazon-S3.html>
* <https://django-storages.readthedocs.io/en/latest/backends/azure.html>
9 changes: 9 additions & 0 deletions CHANGES/plugin_api/5404.removal
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
Started using `settings.STORAGES` internally instead of `settings.DEFAULT_FILE_STORAGE` and `settings.STATICFILES_STORAGE`,
which was deprecated in Django 4.2.

For compatibility, plugins must replace access to:

* `settings.DEFAULT_FILE_STORAGE` with `settings.STORAGES["default"]["BACKEND"]`
* `settings.STATICFILES_STORAGE` with `settings.STORAGES["staticfiles"]["BACKEND"]`

See the new storage structure in [Django docs](https://docs.djangoproject.com/en/4.2/ref/settings/#std-setting-STORAGES).
8 changes: 4 additions & 4 deletions pulp_file/tests/functional/api/test_download_policies.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@

from pulpcore.tests.functional.utils import get_files_in_manifest, download_file

from pulpcore.app import settings
Copy link
Member

Choose a reason for hiding this comment

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

Oh dear, this test was mixing both "imports"...?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it was

from pulpcore.client.pulp_file import FileFilePublication, RepositorySyncURL


Expand Down Expand Up @@ -54,7 +53,8 @@ def test_download_policy(
download_policy,
):
"""Test that "on_demand" and "streamed" download policies work as expected."""
if download_policy == "on_demand" and "SFTP" in pulp_settings.DEFAULT_FILE_STORAGE:
settings = pulp_settings
if download_policy == "on_demand" and "SFTP" in settings.STORAGES["default"]["BACKEND"]:
pytest.skip("This storage technology is not properly supported.")

remote = file_remote_ssl_factory(
Expand Down Expand Up @@ -148,7 +148,7 @@ def test_download_policy(
assert expected_checksum == actual_checksum
if (
download_policy == "immediate"
and settings.DEFAULT_FILE_STORAGE != "pulpcore.app.models.storage.FileSystem"
and settings.STORAGES["default"]["BACKEND"] != "pulpcore.app.models.storage.FileSystem"
and settings.REDIRECT_TO_OBJECT_STORAGE
):
content_disposition = downloaded_file.response_obj.headers.get("Content-Disposition")
Expand Down Expand Up @@ -187,7 +187,7 @@ def test_download_policy(
content_unit = expected_files_list[4]
content_unit_url = urljoin(distribution.base_url, content_unit[0])
# The S3 test API project doesn't handle invalid Range values correctly
if settings.DEFAULT_FILE_STORAGE == "pulpcore.app.models.storage.FileSystem":
if settings.STORAGES["default"]["BACKEND"] == "pulpcore.app.models.storage.FileSystem":
with pytest.raises(ClientResponseError) as exc:
range_header = {"Range": "bytes=-1-11"}
download_file(content_unit_url, headers=range_header)
Expand Down
18 changes: 14 additions & 4 deletions pulpcore/app/apps.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
from importlib import import_module

from django import apps
from django.conf import settings
from django.core.exceptions import ImproperlyConfigured
from django.db import connection, transaction
from django.db.models.signals import post_migrate
Expand Down Expand Up @@ -68,6 +67,15 @@ class PulpPluginAppConfig(apps.AppConfig):

def __init__(self, app_name, app_module):
super().__init__(app_name, app_module)
# begin Compatilibity layer for DEFAULT_FILE_STORAGE deprecation
# Remove on pulpcore=3.85 or pulpcore=4.0
# * Workaround for getting the up-to-date settings instance, otherwise is doesnt
# get the patch in settings.py
# * Update code in signal handlers to use module-level imports again
from django.conf import settings

self.settings = settings
# end

try:
self.version
Expand Down Expand Up @@ -314,6 +322,7 @@ def _populate_system_id(sender, apps, verbosity, **kwargs):


def _ensure_default_domain(sender, **kwargs):
settings = sender.settings
table_names = connection.introspection.table_names()
if "core_domain" in table_names:
from pulpcore.app.util import get_default_domain
Expand All @@ -323,11 +332,11 @@ def _ensure_default_domain(sender, **kwargs):
if (
settings.HIDE_GUARDED_DISTRIBUTIONS != default.hide_guarded_distributions
or settings.REDIRECT_TO_OBJECT_STORAGE != default.redirect_to_object_storage
or settings.DEFAULT_FILE_STORAGE != default.storage_class
or settings.STORAGES["default"]["BACKEND"] != default.storage_class
):
default.hide_guarded_distributions = settings.HIDE_GUARDED_DISTRIBUTIONS
default.redirect_to_object_storage = settings.REDIRECT_TO_OBJECT_STORAGE
default.storage_class = settings.DEFAULT_FILE_STORAGE
default.storage_class = settings.STORAGES["default"]["BACKEND"]
default.save(skip_hooks=True)


Expand Down Expand Up @@ -393,8 +402,9 @@ def _get_permission(perm):


def _populate_artifact_serving_distribution(sender, apps, verbosity, **kwargs):
settings = sender.settings
if (
settings.DEFAULT_FILE_STORAGE == "pulpcore.app.models.storage.FileSystem"
settings.STORAGES["default"]["BACKEND"] == "pulpcore.app.models.storage.FileSystem"
or not settings.REDIRECT_TO_OBJECT_STORAGE
):
try:
Expand Down
2 changes: 1 addition & 1 deletion pulpcore/app/checks.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ def secret_key_check(app_configs, **kwargs):
def storage_paths(app_configs, **kwargs):
warnings = []

if settings.DEFAULT_FILE_STORAGE == "pulpcore.app.models.storage.FileSystem":
if settings.STORAGES["default"]["BACKEND"] == "pulpcore.app.models.storage.FileSystem":
try:
media_root_dev = Path(settings.MEDIA_ROOT).stat().st_dev
except OSError:
Expand Down
2 changes: 1 addition & 1 deletion pulpcore/app/importexport.py
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ def export_artifacts(export, artifact_pks):
with ProgressReport(**data) as pb:
pb.BATCH_INTERVAL = 5000

if settings.DEFAULT_FILE_STORAGE != "pulpcore.app.models.storage.FileSystem":
if settings.STORAGES["default"]["BACKEND"] != "pulpcore.app.models.storage.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]
Expand Down
5 changes: 4 additions & 1 deletion pulpcore/app/serializers/domain.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,10 @@ def to_representation(self, instance):
# Should I convert back the saved settings to their Setting names for to_representation?
if getattr(self.context.get("domain", None), "name", None) == "default":
for setting_name, field in self.SETTING_MAPPING.items():
if value := getattr(settings, setting_name.upper(), None):
value = getattr(settings, setting_name, None) or settings.STORAGES["default"].get(
"OPTIONS", {}
).get(field)
if value:
instance[field] = value
return super().to_representation(instance)

Expand Down
48 changes: 40 additions & 8 deletions pulpcore/app/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
from pathlib import Path

from cryptography.fernet import Fernet
from django.core.files.storage import storages
from django.conf import global_settings
from django.core.exceptions import ImproperlyConfigured
from django.db import connection

Expand Down Expand Up @@ -56,7 +58,28 @@
STATIC_URL = "/assets/"
STATIC_ROOT = DEPLOY_ROOT / STATIC_URL.strip("/")

DEFAULT_FILE_STORAGE = "pulpcore.app.models.storage.FileSystem"
# begin compatilibity layer for DEFAULT_FILE_STORAGE
# Remove on pulpcore=3.85 or pulpcore=4.0

# - What is this?
# We shouldnt use STORAGES or DEFAULT_FILE_STORAGE directly because those are
# mutually exclusive by django, which constraints users to use whatever we use.
# This is a hack/workaround to set Pulp's default while still enabling users to choose
# the legacy or the new storage setting.
_DEFAULT_FILE_STORAGE = "pulpcore.app.models.storage.FileSystem"
_STORAGES = {
"default": {
"BACKEND": "pulpcore.app.models.storage.FileSystem",
},
"staticfiles": {
"BACKEND": "django.contrib.staticfiles.storage.StaticFilesStorage",
},
}

setattr(global_settings, "DEFAULT_FILE_STORAGE", _DEFAULT_FILE_STORAGE)
setattr(global_settings, "STORAGES", _STORAGES)
Comment on lines +79 to +80
Copy link
Member

Choose a reason for hiding this comment

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

Setting them both here seems to contradict that they are, as you say, "mutually exclusive".

Copy link
Member Author

@pedro-psb pedro-psb Dec 18, 2024

Choose a reason for hiding this comment

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

The "global setting" settings is the only place where django won't complain about exclusivity. It will perform this check on a different context.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds weird, but I'll take your word that it works.

# end DEFAULT_FILE_STORAGE deprecation layer

REDIRECT_TO_OBJECT_STORAGE = True

WORKING_DIRECTORY = DEPLOY_ROOT / "tmp"
Expand Down Expand Up @@ -371,16 +394,18 @@
from dynaconf import DjangoDynaconf, Validator # noqa

# Validators
storage_keys = ("STORAGES.default.BACKEND", "DEFAULT_FILE_STORAGE")
storage_validator = (
Validator("REDIRECT_TO_OBJECT_STORAGE", eq=False)
| Validator("DEFAULT_FILE_STORAGE", eq="pulpcore.app.models.storage.FileSystem")
| Validator("DEFAULT_FILE_STORAGE", eq="storages.backends.azure_storage.AzureStorage")
| Validator("DEFAULT_FILE_STORAGE", eq="storages.backends.s3boto3.S3Boto3Storage")
| Validator("DEFAULT_FILE_STORAGE", eq="storages.backends.gcloud.GoogleCloudStorage")
| Validator(*storage_keys, eq="pulpcore.app.models.storage.FileSystem")
| Validator(*storage_keys, eq="storages.backends.azure_storage.AzureStorage")
| Validator(*storage_keys, eq="storages.backends.s3boto3.S3Boto3Storage")
| Validator(*storage_keys, 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 DEFAULT_FILE_STORAGE."
"'REDIRECT_TO_OBJECT_STORAGE=True' is only supported with the local file, S3, GCP or Azure "
"storage backend configured in STORAGES['default']['BACKEND'] "
"(deprecated DEFAULT_FILE_STORAGE)."
)

cache_enabled_validator = Validator("CACHE_ENABLED", eq=True)
Expand Down Expand Up @@ -485,7 +510,14 @@ def otel_middleware_hook(settings):
],
post_hooks=otel_middleware_hook,
)
# HERE ENDS DYNACONF EXTENSION LOAD (No more code below this line)

# begin compatilibity layer for DEFAULT_FILE_STORAGE
# Remove on pulpcore=3.85 or pulpcore=4.0

# Ensures the cached property storage.backends uses the the right value
storages._backends = settings.STORAGES.copy()
storages.backends
Comment on lines +518 to +519
Copy link
Member

Choose a reason for hiding this comment

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

Is this the "magic" to prevent django storages to see both items we set before?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, Django in fact has both set in their default global_settings.
But the previous hack caused a sife-effect on a specific point of django startup where it tries to get those values with a method that can't see the patch we made. The storages is a cached property, so it would hold the wrong value forever.

Line 518 does exactly what they do here.
Line 519 triggers the value to be cached.

All this magic is to give users a larger time span to upgrade.

# end compatibility layer

_logger = getLogger(__name__)

Expand Down
2 changes: 1 addition & 1 deletion pulpcore/app/tasks/export.py
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ def _export_to_file_system(path, relative_paths_to_artifacts, method=FS_EXPORT_M
ValidationError: When path is not in the ALLOWED_EXPORT_PATHS setting
"""
using_filesystem_storage = (
settings.DEFAULT_FILE_STORAGE == "pulpcore.app.models.storage.FileSystem"
settings.STORAGES["default"]["BACKEND"] == "pulpcore.app.models.storage.FileSystem"
)

if method != FS_EXPORT_METHODS.WRITE and not using_filesystem_storage:
Expand Down
6 changes: 4 additions & 2 deletions pulpcore/app/util.py
Original file line number Diff line number Diff line change
Expand Up @@ -536,7 +536,7 @@ def get_artifact_url(artifact, headers=None, http_method=None):
if settings.DOMAIN_ENABLED:
loc = f"domain {artifact_domain.name}.storage_class"
else:
loc = "settings.DEFAULT_FILE_STORAGE"
loc = "settings.STORAGES['default']['BACKEND']"

raise NotImplementedError(
f"The value {loc}={artifact_domain.storage_class} does not allow redirecting."
Expand Down Expand Up @@ -582,7 +582,9 @@ def get_default_domain():
try:
default_domain = Domain.objects.get(name="default")
except Domain.DoesNotExist:
default_domain = Domain(name="default", storage_class=settings.DEFAULT_FILE_STORAGE)
default_domain = Domain(
name="default", storage_class=settings.STORAGES["default"]["BACKEND"]
)
default_domain.save(skip_hooks=True)

return default_domain
Expand Down
28 changes: 18 additions & 10 deletions pulpcore/pytest_plugin.py
Original file line number Diff line number Diff line change
Expand Up @@ -603,13 +603,13 @@ def _settings_factory(storage_class=None, storage_settings=None):
keys = dict()
keys["pulpcore.app.models.storage.FileSystem"] = ["MEDIA_ROOT", "MEDIA_URL"]
keys["storages.backends.s3boto3.S3Boto3Storage"] = [
"AWS_ACCESS_KEY_ID",
"AWS_SECRET_ACCESS_KEY",
"AWS_S3_ENDPOINT_URL",
"AWS_S3_ADDRESSING_STYLE",
"AWS_S3_SIGNATURE_VERSION",
"AWS_S3_REGION_NAME",
"AWS_STORAGE_BUCKET_NAME",
"access_key",
"secret_key",
"endpoint_url",
"addressing_style",
"signature_version",
"region_name",
"bucket_name",
]
keys["storages.backends.azure_storage.AzureStorage"] = [
"AZURE_ACCOUNT_NAME",
Expand All @@ -621,9 +621,17 @@ def _settings_factory(storage_class=None, storage_settings=None):
"AZURE_CONNECTION_STRING",
]
settings = storage_settings or dict()
backend = storage_class or pulp_settings.DEFAULT_FILE_STORAGE
for key in keys[backend]:
if key not in settings:
backend = storage_class or pulp_settings.STORAGES["default"]["BACKEND"]
not_defined_settings = (k for k in keys[backend] if k not in settings)
# The CI configures s3 with STORAGES and Azure with legacy
# Move all to STORAGES structure on DEFAULT_FILE_STORAGE removal
if backend == "storages.backends.s3boto3.S3Boto3Storage":
storages_dict = getattr(pulp_settings, "STORAGES", {})
storage_options = storages_dict.get("default", {}).get("OPTIONS", {})
for key in not_defined_settings:
settings[key] = storage_options.get(key)
else:
for key in not_defined_settings:
settings[key] = getattr(pulp_settings, key, None)
return backend, settings

Expand Down
7 changes: 3 additions & 4 deletions pulpcore/tests/functional/api/test_artifact_distribution.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@
import subprocess
from hashlib import sha256

from django.conf import settings


OBJECT_STORAGES = (
"storages.backends.s3boto3.S3Boto3Storage",
Expand All @@ -12,7 +10,8 @@
)


def test_artifact_distribution(random_artifact):
def test_artifact_distribution(random_artifact, pulp_settings):
settings = pulp_settings
artifact_uuid = random_artifact.pulp_href.split("/")[-2]

commands = (
Expand All @@ -29,7 +28,7 @@ def test_artifact_distribution(random_artifact):
hasher = sha256()
hasher.update(response.content)
assert hasher.hexdigest() == random_artifact.sha256
if settings.DEFAULT_FILE_STORAGE in OBJECT_STORAGES:
if settings.STORAGES["default"]["BACKEND"] in OBJECT_STORAGES:
content_disposition = response.headers.get("Content-Disposition")
assert content_disposition is not None
filename = artifact_uuid
Expand Down
7 changes: 3 additions & 4 deletions pulpcore/tests/functional/api/test_crd_artifacts.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@
import uuid
import pytest

from django.conf import settings


@pytest.fixture
def pulpcore_random_file(tmp_path):
Expand Down Expand Up @@ -142,10 +140,11 @@ def test_upload_mixed_attrs(pulpcore_bindings, pulpcore_random_file):


@pytest.mark.parallel
def test_delete_artifact(pulpcore_bindings, pulpcore_random_file, gen_user):
def test_delete_artifact(pulpcore_bindings, pulpcore_random_file, gen_user, pulp_settings):
"""Verify that the deletion of artifacts is prohibited for both regular users and
administrators."""
if settings.DEFAULT_FILE_STORAGE != "pulpcore.app.models.storage.FileSystem":
settings = pulp_settings
if settings.STORAGES["default"]["BACKEND"] != "pulpcore.app.models.storage.FileSystem":
pytest.skip("this test only works for filesystem storage")
media_root = settings.MEDIA_ROOT

Expand Down
Loading
Loading