sts-sponsors team mailing list archive

[Igor Brovtsin <mp+433663@xxxxxxxxxxxxxxxxxx>]

Igor Brovtsin has proposed merging ~igor-brovtsin/maas:vault-refactor-cache into maas:master.

Commit message:
ref(vault): Don't cache `get_region_vault_client`; use fixture for MAAS_SECRET in startup tests
    
Since this method is only used in contexts that don't need caching
(and `get_region_vault_client_if_enabled` has its own cache), it's
safe to omit that cache and the related helpers.


Requested reviews:
  MAAS Maintainers (maas-maintainers)

For more details, see:
https://code.launchpad.net/~igor-brovtsin/maas/+git/maas/+merge/433663
-- 
Your team MAAS Maintainers is requested to review the proposed merge of ~igor-brovtsin/maas:vault-refactor-cache into maas:master.

diff --git a/src/maasserver/pytest_tests/test_vault.py b/src/maasserver/pytest_tests/test_vault.py
index d050999..caf437f 100644
--- a/src/maasserver/pytest_tests/test_vault.py
+++ b/src/maasserver/pytest_tests/test_vault.py
@@ -8,8 +8,6 @@ import pytest
 from maasserver import vault
 from maasserver.models import Config
 from maasserver.vault import (
-    _get_region_vault_client,
-    clear_vault_client_caches,
     get_region_vault_client,
     get_region_vault_client_if_enabled,
     UnknownSecretPath,
@@ -144,9 +142,7 @@ class TestVaultClient:
 @pytest.mark.django_db
 class TestGetRegionVaultClient:
     def test_cached(self, mocker):
-        mock_get_client = mocker.patch.object(
-            vault, "_get_region_vault_client"
-        )
+        mock_get_client = mocker.patch.object(vault, "get_region_vault_client")
         get_region_vault_client()
         get_region_vault_client()
         mock_get_client.assert_called_once()
@@ -155,7 +151,7 @@ class TestGetRegionVaultClient:
         # the secret is not set
         vault_regionconfig["vault_url"] = "http://vault:8200";
         vault_regionconfig["vault_approle_id"] = "x-y-z"
-        assert _get_region_vault_client() is None
+        assert get_region_vault_client() is None
 
     def test_get_client(self, factory, vault_regionconfig):
         approle_id = factory.make_name("uuid")
@@ -167,7 +163,7 @@ class TestGetRegionVaultClient:
         vault_regionconfig["vault_secret_id"] = secret_id
         vault_regionconfig["vault_secrets_path"] = secrets_path
         vault_regionconfig["vault_secrets_mount"] = secrets_mount
-        client = _get_region_vault_client()
+        client = get_region_vault_client()
         assert isinstance(client, VaultClient)
         assert client._role_id == approle_id
         assert client._secret_id == secret_id
@@ -183,15 +179,13 @@ class TestGetRegionVaultClient:
         vault_regionconfig["vault_approle_id"] = approle_id
         vault_regionconfig["vault_secret_id"] = secret_id
         vault_regionconfig["vault_secrets_mount"] = "other/secrets"
-        client = _get_region_vault_client()
+        client = get_region_vault_client()
         assert client._secrets_mount == "other/secrets"
 
 
 class TestGetRegionVaultClientIfEnabled:
     def test_cached(self, mocker):
-        mock_get_client = mocker.patch.object(
-            vault, "_get_region_vault_client"
-        )
+        mock_get_client = mocker.patch.object(vault, "get_region_vault_client")
         mock_get_config = mocker.patch.object(Config.objects, "get_config")
         mock_get_config.return_value = True
 
@@ -312,21 +306,3 @@ class TestCheckApprolePermissions:
         client.set.assert_called_once_with(expected_path, ANY)
         client.get.assert_called_once_with(expected_path)
         client.delete.assert_called_once_with(expected_path)
-
-
-class TestClearVaultCaches:
-    def test_clears_vault_caches(self, mocker):
-        mocker.patch.object(Config.objects, "get_config").return_value = True
-        mock_get_client = mocker.patch.object(
-            vault, "_get_region_vault_client"
-        )
-        first_client = MagicMock()
-        second_client = MagicMock()
-        mock_get_client.side_effect = [first_client, second_client]
-        assert get_region_vault_client() == first_client
-        assert get_region_vault_client() == first_client
-        assert get_region_vault_client_if_enabled() == first_client
-        assert get_region_vault_client_if_enabled() == first_client
-        clear_vault_client_caches()
-        assert get_region_vault_client() == second_client
-        assert get_region_vault_client_if_enabled() == second_client
diff --git a/src/maasserver/start_up.py b/src/maasserver/start_up.py
index c572b1f..91a040e 100644
--- a/src/maasserver/start_up.py
+++ b/src/maasserver/start_up.py
@@ -10,7 +10,7 @@ from django.db import connection
 from django.db.utils import DatabaseError
 from twisted.internet.defer import inlineCallbacks
 
-from maasserver import locks, security
+from maasserver import locks, security, vault
 from maasserver.config import get_db_creds_vault_path, RegionConfiguration
 from maasserver.deprecations import (
     log_deprecations,
@@ -32,11 +32,7 @@ from maasserver.utils.orm import (
     with_connection,
 )
 from maasserver.utils.threads import deferToDatabase
-from maasserver.vault import (
-    clear_vault_client_caches,
-    get_region_vault_client,
-    VaultClient,
-)
+from maasserver.vault import get_region_vault_client, VaultClient
 from metadataserver.builtin_scripts import load_builtin_scripts
 from provisioningserver.drivers.osystem.ubuntu import UbuntuOS
 from provisioningserver.logger import get_maas_logger, LegacyLogger
@@ -123,7 +119,7 @@ def start_up(master=False):
             # Since start_up now can be called multiple times in a process lifetime,
             # vault client caches should be cleared in order to re-read the configuration.
             # This prevents fetching shared secret from DB when Vault is already enabled.
-            clear_vault_client_caches()
+            vault.get_region_vault_client_if_enabled.cache_clear()
 
             # Ensure the shared secret is configured
             secret = yield security.get_shared_secret()
diff --git a/src/maasserver/tests/test_start_up.py b/src/maasserver/tests/test_start_up.py
index 13e806f..24a0dd3 100644
--- a/src/maasserver/tests/test_start_up.py
+++ b/src/maasserver/tests/test_start_up.py
@@ -5,6 +5,7 @@ from pathlib import Path
 import random
 from unittest.mock import call, MagicMock
 
+from fixtures import Fixture
 from testtools.matchers import HasLength
 
 from maasserver import deprecations, eventloop, locks, start_up, vault
@@ -45,6 +46,18 @@ from provisioningserver.utils.env import (
 from provisioningserver.utils.testing import MAASIDFixture, MAASUUIDFixture
 
 
+class MAASSecretFixture(Fixture):
+    def __init__(self, secret=None):
+        super(MAASSecretFixture, self).__init__()
+        self.secret = secret
+
+    def _setUp(self):
+        super(MAASSecretFixture, self)._setUp()
+        old_value = MAAS_SECRET.get()
+        MAAS_SECRET.set(self.secret)
+        self.addCleanup(MAAS_SECRET.set, old_value)
+
+
 class TestStartUp(MAASTransactionServerTestCase):
 
     """Tests for the `start_up` function.
@@ -100,13 +113,11 @@ class TestStartUp(MAASTransactionServerTestCase):
         self.expectThat(start_up.pause, MockCalledOnceWith(3.0))
 
     def test_start_up_fetches_secret_from_vault_after_migration(self):
-        vault.clear_vault_client_caches()
-        # Apparently, MAAS_SECRET state is shared between the tests
-        old_secret = MAAS_SECRET.get()
+        self.useFixture(MAASSecretFixture())
         # Prepare fake vault
         expected_shared_secret = b"EXPECTED"
         client = FakeVaultClient()
-        self.patch(vault, "_get_region_vault_client").return_value = client
+        self.patch(vault, "get_region_vault_client").return_value = client
         SecretManager(client).set_simple_secret(
             "rpc-shared", to_hex(expected_shared_secret)
         )
@@ -123,8 +134,6 @@ class TestStartUp(MAASTransactionServerTestCase):
             start_up.start_up()
 
         self.assertEqual(expected_shared_secret, MAAS_SECRET.get())
-        MAAS_SECRET.set(old_secret)
-        vault.clear_vault_client_caches()
 
 
 class TestInnerStartUp(MAASServerTestCase):
diff --git a/src/maasserver/vault.py b/src/maasserver/vault.py
index 269c983..1f58ca5 100644
--- a/src/maasserver/vault.py
+++ b/src/maasserver/vault.py
@@ -163,20 +163,7 @@ def get_region_vault_client_if_enabled() -> Optional[VaultClient]:
     return None
 
 
-@cache
 def get_region_vault_client() -> Optional[VaultClient]:
-    """Return a VaultClient configured for the region controller, if configured.
-
-    This is must be called once the Vault configuration (if any) is set, since
-    the result is cached.  This is done since Vault configuration is not
-    expected to change within the life of the region controller (a restart is
-    needed).
-
-    """
-    return _get_region_vault_client()
-
-
-def _get_region_vault_client() -> Optional[VaultClient]:
     """Return a VaultClient configured for the region controller.
 
     If configuration options for Vault are not set, None is returned.
@@ -265,10 +252,4 @@ def configure_region_with_vault(
         config.vault_secrets_path = secrets_path
         config.vault_secrets_mount = secrets_mount
     # ensure future calls to get the client use the updated config
-    clear_vault_client_caches()
-
-
-def clear_vault_client_caches():
-    """Clears cached vault clients, useful after reconfiguration"""
-    get_region_vault_client.cache_clear()
     get_region_vault_client_if_enabled.cache_clear()