From 490db0822450241c8fe8781068255674e53c4b6b Mon Sep 17 00:00:00 2001 From: Chris Meyers Date: Thu, 5 Sep 2024 08:09:05 -0400 Subject: [PATCH] Register CredentialType(s) every time Django loads * Register all discovered CredentialType(s) after Django finishes loading * Protect parallel registrations using shared postgres advisory lock * The down-side of this is that this will run when it does not need to, adding overhead to the init process. * Only register discovered credential types in the database IF migrations have ran and are up-to-date. --- awx/main/apps.py | 25 ++++++++ .../setup_managed_credential_types.py | 7 ++- awx/main/models/credential.py | 57 +++++++++++++++++-- .../tests/functional/api/test_credential.py | 12 ---- awx/main/tests/functional/test_apps.py | 26 +++++++++ awx/main/tests/functional/test_credential.py | 22 +++++++ awx/main/tests/unit/models/test_credential.py | 31 ++++++++++ awx/main/utils/common.py | 8 +++ awx/main/utils/migration.py | 14 +++++ 9 files changed, 184 insertions(+), 18 deletions(-) create mode 100644 awx/main/tests/functional/test_apps.py create mode 100644 awx/main/utils/migration.py diff --git a/awx/main/apps.py b/awx/main/apps.py index 099caea96a6b..1dbfa4879b6d 100644 --- a/awx/main/apps.py +++ b/awx/main/apps.py @@ -1,5 +1,7 @@ from django.apps import AppConfig from django.utils.translation import gettext_lazy as _ +from awx.main.utils.common import bypass_in_test +from awx.main.utils.migration import is_database_synchronized from awx.main.utils.named_url_graph import _customize_graph, generate_graph from awx.conf import register, fields @@ -34,7 +36,30 @@ def load_named_url_feature(self): category_slug='named-url', ) + def _load_credential_types_feature(self): + """ + Create CredentialType records for any discovered credentials. + + Note that Django docs advise _against_ interacting with the database using + the ORM models in the ready() path. Specifically, during testing. + However, we explicitly use the @bypass_in_test decorator to avoid calling this + method during testing. + + Django also advises against running pattern because it runs everywhere i.e. + every management command. We use an advisory lock to ensure correctness and + we will deal performance if it becomes an issue. + """ + from awx.main.models.credential import CredentialType + + if is_database_synchronized(): + CredentialType.setup_tower_managed_defaults(app_config=self) + + @bypass_in_test + def load_credential_types_feature(self): + return self._load_credential_types_feature() + def ready(self): super().ready() + self.load_credential_types_feature() self.load_named_url_feature() diff --git a/awx/main/management/commands/setup_managed_credential_types.py b/awx/main/management/commands/setup_managed_credential_types.py index c0e566c3000b..1f9b74bf3efc 100644 --- a/awx/main/management/commands/setup_managed_credential_types.py +++ b/awx/main/management/commands/setup_managed_credential_types.py @@ -10,4 +10,9 @@ class Command(BaseCommand): help = 'Load default managed credential types.' def handle(self, *args, **options): - CredentialType.setup_tower_managed_defaults() + """ + Note that the call below is almost redundant. The same call as below is called in the Django ready() code path. The ready() code path runs + before every management command. The one difference in the below call is that the below call is _more_ likely to _actually_ run. The ready() code path + version _can_ be a NOOP if the lock is not acquired. The below version waits to acquire the lock. This can be useful for recreating bugs or pdb. + """ + CredentialType.setup_tower_managed_defaults(wait_for_lock=True) diff --git a/awx/main/models/credential.py b/awx/main/models/credential.py index 852c1c5cdfec..1d6e4c1501f1 100644 --- a/awx/main/models/credential.py +++ b/awx/main/models/credential.py @@ -1,5 +1,6 @@ # Copyright (c) 2015 Ansible, Inc. # All Rights Reserved. +from contextlib import nullcontext import functools import inspect import logging @@ -14,6 +15,8 @@ from jinja2 import sandbox # Django +from django.apps.config import AppConfig +from django.apps.registry import Apps from django.db import models from django.utils.translation import gettext_lazy as _ from django.core.exceptions import ValidationError @@ -24,6 +27,7 @@ from django.contrib.auth.models import User # DRF +from awx.main.utils.pglock import advisory_lock from rest_framework.serializers import ValidationError as DRFValidationError # AWX @@ -332,6 +336,8 @@ def validate_role_assignment(self, actor, role_definition): class CredentialType(CommonModelNameNotUnique): + CREDENTIAL_REGISTRATION_ADVISORY_LOCK_NAME = 'setup_tower_managed_defaults' + """ A reusable schema for a credential. @@ -414,11 +420,29 @@ def defaults(cls): return dict((k, functools.partial(v.create)) for k, v in ManagedCredentialType.registry.items()) @classmethod - def setup_tower_managed_defaults(cls, apps=None): - if apps is not None: - ct_class = apps.get_model('main', 'CredentialType') - else: - ct_class = CredentialType + def _get_credential_type_class(cls, apps: Apps = None, app_config: AppConfig = None): + """ + Legacy code passing in apps while newer code should pass only the specific 'main' app config. + """ + if apps and app_config: + raise ValueError('Expected only apps or app_config to be defined, not both') + + if not any( + ( + apps, + app_config, + ) + ): + return CredentialType + + if apps: + app_config = apps.get_app_config('main') + + return app_config.get_model('CredentialType') + + @classmethod + def _setup_tower_managed_defaults(cls, apps: Apps = None, app_config: AppConfig = None): + ct_class = cls._get_credential_type_class(apps=apps, app_config=app_config) for default in ManagedCredentialType.registry.values(): existing = ct_class.objects.filter(name=default.name, kind=default.kind).first() if existing is not None: @@ -436,6 +460,29 @@ def setup_tower_managed_defaults(cls, apps=None): created.inputs = created.injectors = {} created.save() + @classmethod + def setup_tower_managed_defaults(cls, apps: Apps = None, app_config: AppConfig = None, lock: bool = True, wait_for_lock: bool = False): + """ + Create a CredentialType for discovered credential plugins. + + By default, this function will attempt to acquire the globally distributed lock (postgres advisory lock). + If the lock is acquired the method will call the underlying method. + If the lock is NOT acquired the method will NOT call the underlying method. + + lock=False will set acquired to True and appear to acquire the lock. + lock=True, wait_for_lock=False will attempt to acquire the lock and NOT block. + lock=True, wait_for_lock=True will attempt to acquire the lock and will block until the exclusive lock is acquired. + + :param lock(optional[bool]): Attempt to acquire the postgres advisory lock. + :param wait_for_lock(optional[bool]): Block and wait forever for the postgres advisory lock. + """ + if apps and not apps.ready: + return + + with advisory_lock(cls.CREDENTIAL_REGISTRATION_ADVISORY_LOCK_NAME, wait=wait_for_lock) if lock else nullcontext(True) as acquired: + if acquired: + cls._setup_tower_managed_defaults(apps=apps, app_config=app_config) + @classmethod def load_plugin(cls, ns, plugin): ManagedCredentialType(namespace=ns, name=plugin.name, kind='external', inputs=plugin.inputs) diff --git a/awx/main/tests/functional/api/test_credential.py b/awx/main/tests/functional/api/test_credential.py index 65e807424175..d3efb2133a80 100644 --- a/awx/main/tests/functional/api/test_credential.py +++ b/awx/main/tests/functional/api/test_credential.py @@ -12,18 +12,6 @@ EXAMPLE_PRIVATE_KEY = '-----BEGIN PRIVATE KEY-----\nxyz==\n-----END PRIVATE KEY-----' EXAMPLE_ENCRYPTED_PRIVATE_KEY = '-----BEGIN PRIVATE KEY-----\nProc-Type: 4,ENCRYPTED\nxyz==\n-----END PRIVATE KEY-----' - -@pytest.mark.django_db -def test_idempotent_credential_type_setup(): - assert CredentialType.objects.count() == 0 - CredentialType.setup_tower_managed_defaults() - total = CredentialType.objects.count() - assert total > 0 - - CredentialType.setup_tower_managed_defaults() - assert CredentialType.objects.count() == total - - # # user credential creation # diff --git a/awx/main/tests/functional/test_apps.py b/awx/main/tests/functional/test_apps.py new file mode 100644 index 000000000000..a52d4aa723ed --- /dev/null +++ b/awx/main/tests/functional/test_apps.py @@ -0,0 +1,26 @@ +import pytest + +from django.apps import apps + + +@pytest.fixture +def mock_setup_tower_managed_defaults(mocker): + return mocker.patch('awx.main.models.credential.CredentialType.setup_tower_managed_defaults') + + +@pytest.mark.django_db +def test_load_credential_types_feature_migrations_ran(mocker, mock_setup_tower_managed_defaults): + mocker.patch('awx.main.apps.is_database_synchronized', return_value=True) + + apps.get_app_config('main')._load_credential_types_feature() + + mock_setup_tower_managed_defaults.assert_called_once() + + +@pytest.mark.django_db +def test_load_credential_types_feature_migrations_not_ran(mocker, mock_setup_tower_managed_defaults): + mocker.patch('awx.main.apps.is_database_synchronized', return_value=False) + + apps.get_app_config('main')._load_credential_types_feature() + + mock_setup_tower_managed_defaults.assert_not_called() diff --git a/awx/main/tests/functional/test_credential.py b/awx/main/tests/functional/test_credential.py index 97cf2beb2d81..177bed97dedf 100644 --- a/awx/main/tests/functional/test_credential.py +++ b/awx/main/tests/functional/test_credential.py @@ -339,3 +339,25 @@ def test_credential_get_input(organization_factory): # verify return values for encrypted secret fields are decrypted assert cred.inputs['vault_password'].startswith('$encrypted$') assert cred.get_input('vault_password') == 'testing321' + + +@pytest.mark.django_db +def test_idempotent_credential_type_setup(): + """ + awx main app ready() calls `setup_tower_managed_defaults()` to register CredentialType(s). + This is problematic in our testing system. pytest_django deviates from the production ready() call path. pytest_django calls our apps ready() function + before migrations run. This is a problem since we interact with tables in the database that do not yet exist. + + Now forget about what you just read because we do not _actually_ want to register CredentialType(s) in our test at all. So then + you would expect this bit of code to spy on `setup_tower_managed_defaults` and assert it was not called BUT registering a spy early + enough is hard. The call to ready() from pytest_django happens via pytest hooks very early https://github.com/pytest-dev/pytest-django/blob/1157a7c5c74f4b4e0f4aca8312f3fe67eb00568e/pytest_django/plugin.py#L266C5-L266C34 + + Instead of ensuring that `setup_tower_managed_defaults()` is explicitly not called, we check it _implicitly_ by observing that no credential type records are created. + """ + assert CredentialType.objects.count() == 0 + CredentialType.setup_tower_managed_defaults() + total = CredentialType.objects.count() + assert total > 0 + + CredentialType.setup_tower_managed_defaults() + assert CredentialType.objects.count() == total diff --git a/awx/main/tests/unit/models/test_credential.py b/awx/main/tests/unit/models/test_credential.py index 0dc8daff3356..81f243ddefd5 100644 --- a/awx/main/tests/unit/models/test_credential.py +++ b/awx/main/tests/unit/models/test_credential.py @@ -4,6 +4,8 @@ from awx.main.models import Credential, CredentialType +from django.apps import apps + @pytest.mark.django_db def test_unique_hash_with_unicode(): @@ -16,3 +18,32 @@ def test_custom_cred_with_empty_encrypted_field(): ct = CredentialType(name='My Custom Cred', kind='custom', inputs={'fields': [{'id': 'some_field', 'label': 'My Field', 'secret': True}]}) cred = Credential(id=4, name='Testing 1 2 3', credential_type=ct, inputs={}) assert cred.encrypt_field('some_field', None) is None + + +@pytest.mark.parametrize( + ( + 'apps', + 'app_config', + ), + [ + ( + apps, + None, + ), + ( + None, + apps.get_app_config('main'), + ), + ], +) +def test__get_credential_type_class(apps, app_config): + ct = CredentialType._get_credential_type_class(apps=apps, app_config=app_config) + assert ct.__name__ == 'CredentialType' + + +def test__get_credential_type_class_invalid_params(): + with pytest.raises(ValueError) as e: + CredentialType._get_credential_type_class(apps=apps, app_config=apps.get_app_config('main')) + + assert type(e.value) is ValueError + assert str(e.value) == 'Expected only apps or app_config to be defined, not both' diff --git a/awx/main/utils/common.py b/awx/main/utils/common.py index bdd7465b907d..f9ba62433963 100644 --- a/awx/main/utils/common.py +++ b/awx/main/utils/common.py @@ -147,6 +147,14 @@ def is_testing(argv=None): return False +def bypass_in_test(func): + def fn(*args, **kwargs): + if not is_testing(): + return func(*args, **kwargs) + + return fn + + class RequireDebugTrueOrTest(logging.Filter): """ Logging filter to output when in DEBUG mode or running tests. diff --git a/awx/main/utils/migration.py b/awx/main/utils/migration.py new file mode 100644 index 000000000000..10396e7ea358 --- /dev/null +++ b/awx/main/utils/migration.py @@ -0,0 +1,14 @@ +from django.db.migrations.executor import MigrationExecutor +from django.db import connections, DEFAULT_DB_ALIAS + + +def is_database_synchronized(database=DEFAULT_DB_ALIAS): + """_summary_ + Ensure all migrations have ran + https://stackoverflow.com/questions/31838882/check-for-pending-django-migrations + """ + connection = connections[database] + connection.prepare_database() + executor = MigrationExecutor(connection) + targets = executor.loader.graph.leaf_nodes() + return not executor.migration_plan(targets)