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

@@ -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()

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)

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)

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
 #

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()

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

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'

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.

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)