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.

Sign up for GitHub

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

Allow selecting enabled plugins #5083

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Allow selecting enabled plugins #5083

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions CHANGES/5235.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Added ``ENABLED_PLUGINS`` option to allow selecting installed plugins to be enabled.
50 changes: 37 additions & 13 deletions pulpcore/app/settings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,9 @@

from contextlib import suppress
from importlib import import_module
from importlib.metadata import entry_points
from logging import getLogger
from pathlib import Path
from pkg_resources import iter_entry_points

from cryptography.fernet import Fernet
from django.core.exceptions import ImproperlyConfigured
Expand Down Expand Up @@ -81,14 +81,6 @@
"pulpcore.app",
]

# Enumerate the installed Pulp plugins during the loading process for use in the status API
INSTALLED_PULP_PLUGINS = []

for entry_point in iter_entry_points("pulpcore.plugin"):
plugin_app_config = entry_point.load()
INSTALLED_PULP_PLUGINS.append(entry_point.module_name)
INSTALLED_APPS.append(plugin_app_config)

# Optional apps that help with development, or augment Pulp in some non-critical way
OPTIONAL_APPS = [
"crispy_forms",
Expand Down Expand Up @@ -317,9 +309,17 @@

# HERE STARTS DYNACONF EXTENSION LOAD (Keep at the very bottom of settings.py)
# Read more at https://dynaconf.readthedocs.io/en/latest/guides/django.html
from dynaconf import DjangoDynaconf, Validator # noqa
from dynaconf import DjangoDynaconf, Dynaconf, Validator # noqa

# Validators

enabled_plugins_validator = Validator(
"ENABLED_PLUGINS",
is_type_of=list,
len_min=1,
when=Validator("ENABLED_PLUGINS", must_exist=True),
)

content_origin_validator = Validator(
"CONTENT_ORIGIN",
must_exist=True,
Expand Down Expand Up @@ -407,19 +407,43 @@
authentication_json_header_validator & authentication_json_header_jq_filter_validator
)


def load_plugin_config_hook(settings):
# Enumerate the installed Pulp plugins during the loading process for use in the status API
ENABLED_PLUGINS = getattr(settings, "ENABLED_PLUGINS", None)
Copy link
Member

Choose a reason for hiding this comment

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

does it make sense to add validatiion to the setting so that at least one plugin needs to be enabled?

Copy link
Member Author

Choose a reason for hiding this comment

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

Technically pulp would start just fine without it. So Maybe?

installed_plugins = []
installed_plugin_apps = []

for entry_point in entry_points().get("pulpcore.plugin", []):
if ENABLED_PLUGINS is not None and entry_point.name not in ENABLED_PLUGINS:
continue
installed_plugins.append(entry_point.module)
installed_plugin_apps.append(entry_point.load())

plugin_settings = Dynaconf(
PRELOAD_FOR_DYNACONF=[f"{module}.app.settings" for module in installed_plugins]
)

data = {"dynaconf_merge": True}
data.update(plugin_settings.as_dict())
data.update(settings.as_dict())
data["INSTALLED_APPS"].extend(installed_plugin_apps)
data["INSTALLED_APPS"].append("dynaconf_merge_unique")
return data


settings = DjangoDynaconf(
__name__,
ENVVAR_PREFIX_FOR_DYNACONF="PULP",
ENV_SWITCHER_FOR_DYNACONF="PULP_ENV",
PRELOAD_FOR_DYNACONF=[
"{}.app.settings".format(plugin_name) for plugin_name in INSTALLED_PULP_PLUGINS
],
ENVVAR_FOR_DYNACONF="PULP_SETTINGS",
post_hooks=(load_plugin_config_hook,),
Comment on lines -414 to +440
Copy link
Member

@rochacbruno rochacbruno Feb 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Where does ENABLED_PLUGINS comes from? an environment variable?

I see 2 simpler ways for doing it without requiring a hook

  1. read the env directly
PRELOAD_FOR_DYNACONF=[
    "{}.app.settings".format(plugin_name) 
     for plugin_name in INSTALLED_PULP_PLUGINS 
     if plugin_name in os.getenv("PULP_ENABLED_PLUGINS", [])
],

The downside of above strategy is that variable will need to come from envvar, not set on a settings.py file.

  1. Post load plugin data without the hook
settings = DjangoDynaconf(....)
# then imediatelly after
for plugin_name in INSTALLED_PULP_PLUGINS:
    if plugin_name not in settings.get("enabled_plugins", []):
        continue
    settings.load_file("{}.app.settings".format(plugin_name))

Copy link
Member

Choose a reason for hiding this comment

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

this will also require a late validation, so replace

validators= [...]

with

settings = DjangoDynaconf(...)
for plugin_name in INSTALLED_PULP_PLUGINS:
    ....

# invoke validators
settings.validators.register(**list_of_validators)
settings.validators.validade()

Copy link
Member Author

Choose a reason for hiding this comment

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

It can come from any outside of pulp code base config source. e.g. /etc/pulp/settings.py or PULP_ENABLED_PLUGINS var.

Does this method ensure that the chainloaded plugin settings (like pulp_containers TOKEN_SERVER) have lower precedence than external config sources?
My impression from reading the docs is that they would overwrite local settings.

Copy link
Member

Choose a reason for hiding this comment

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

You are right, my suggestion would put plugin loading at the end of the process and then override local settings.

That is why in the current implementation we are using PRELOAD to ensure plugins settings loads before other settings sources.

Looks like we needed to have support for pre hooks, but right now we only support post loading hooks.

One idea is to write a custom loader to bring plugin_settings and then change the order of loaders putting the plugin_loader before everything else.

ideas @pedro-psb ?

Copy link
Member

Choose a reason for hiding this comment

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

If taking approach 2, I believe an explicit load of the settings files should ensure the external file precedence:

settings = DjangoDynaconf(...)
# (plugins load)
settings.load_file(settings.get("PULP_SETTINGS"))
# (validators)

But this original hook solution seems to do the trick, is there a particular reason not to use it?

Copy link
Member Author

Choose a reason for hiding this comment

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

The thing is that we need to keep the order, because "everything else" is what may provide the ENABLED_PLUGINS.

load_dotenv=False,
validators=[
api_root_validator,
cache_validator,
content_origin_validator,
enabled_plugins_validator,
sha256_validator,
storage_validator,
unknown_algs_validator,
Expand Down
6 changes: 1 addition & 5 deletions pulpcore/tests/functional/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1071,11 +1071,7 @@ def _monitor_task_group(task_group_href, timeout=TASK_TIMEOUT):

@pytest.fixture(scope="session")
def pulp_settings():
import django

django.setup()

from django.conf import settings
from pulpcore.app import settings

return settings

Expand Down
2 changes: 1 addition & 1 deletion requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ djangorestframework-queryfields>=1.0,<=1.1.0
drf-access-policy>=1.1.2,<1.5.1
drf-nested-routers>=0.93.4,<=0.93.5
drf-spectacular==0.26.5 # We monkeypatch this so we need a very narrow requirement string
dynaconf>=3.1.12,<3.2.6
dynaconf>=3.2.5,<3.2.6
gunicorn>=20.1,<=21.2.0
importlib-metadata>=6.0.1,<=6.0.1 # Pinned to fix opentelemetry dependency solving issues with pip
jinja2>=3.1,<=3.1.3
Expand Down
5 changes: 5 additions & 0 deletions staging_docs/admin/learn/settings.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,11 @@ The password for Redis.
Pulp defines the following settings itself:


### ENABLED_PLUGINS

An optional list of plugin names.
If provided, Pulp will limit loading plugins to this list.
If omitted, Pulp will load all installed plugins.

### API_ROOT

Expand Down
Loading