From fc877c996d92a5cee38316732cabe927aa915c7f Mon Sep 17 00:00:00 2001
From: Peter Novotnak <peter.novotnak@reddit.com>
Date: Mon, 26 Feb 2024 20:49:32 +0000
Subject: [PATCH 01/17] Vault CSI support v2

---
 baseplate/lib/secrets.py                      |  79 ++++++-
 .../unit/lib/secrets/store_directory_tests.py |   9 -
 tests/unit/lib/secrets/vault_csi_tests.py     | 194 ++++++++++++++++++
 3 files changed, 272 insertions(+), 10 deletions(-)
 create mode 100644 tests/unit/lib/secrets/vault_csi_tests.py

diff --git a/baseplate/lib/secrets.py b/baseplate/lib/secrets.py
index 41b5d04c4..9750734bf 100644
--- a/baseplate/lib/secrets.py
+++ b/baseplate/lib/secrets.py
@@ -3,6 +3,7 @@
 import binascii
 import json
 import logging
+import os
 
 from pathlib import Path
 from typing import Any
@@ -464,6 +465,82 @@ def _get_file_data(self, filename: str) -> Tuple[Dict, float]:
         return result
 
 
+class VaultCSIEntry(NamedTuple):
+    mtime: float
+    data: Any
+    # A mutex to prevent possible duplicate work from event loops
+    updating: bool
+
+
+class VaultCSISecretsStore(SecretsStore):
+    """Access to secret tokens with automatic refresh when changed.
+
+    This local vault allows access to the secrets cached on disk given a path to
+    a directory. It will automatically reload the cache when it is changed. Do not
+    cache or store the values returned by this class's methods but rather get
+    them from this class each time you need them. The secrets are served from
+    memory so there's little performance impact to doing so and you will be
+    sure to always have the current version in the face of key rotation etc.
+    """
+
+    path: Path
+    data_symlink: Path
+    cache: Dict[str, VaultCSIEntry]
+
+    def __init__(
+        self,
+        path: str,
+        parser: SecretParser,
+        timeout: Optional[int] = None,
+        backoff: Optional[float] = None,
+    ):  # pylint: disable=super-init-not-called
+        self.path = Path(path)
+        self.cache = {}
+        self.data_symlink = self.path.joinpath("..data")
+
+    def get_vault_url(self) -> str:
+        raise NotImplementedError
+
+    def get_vault_token(self) -> str:
+        raise NotImplementedError
+
+    def _get_mtime(self) -> float:
+        """Modification time is store-wide for CSI secrets."""
+        return os.path.getmtime(self.data_symlink)
+
+    def _raw_secret(self, name: str) -> Any:
+        try:
+            with open(self.data_symlink.joinpath(name), "r", encoding="UTF-8") as fp:
+                return json.load(fp)["data"]
+        except FileNotFoundError:
+            # Try a second time in case the file was deleted while we tried to read it
+            with open(self.data_symlink.joinpath(name), "r", encoding="UTF-8") as fp:
+                return json.load(fp)["data"]
+
+    def get_raw_and_mtime(self, secret_path: str) -> Tuple[Dict[str, str], float]:
+        mtime = self._get_mtime()
+        cache_entry = self.cache.get(secret_path, VaultCSIEntry(mtime=0, data=None, updating=False))
+        if cache_entry.mtime == mtime or cache_entry.updating is True:
+            return cache_entry.data, mtime
+        # Prevent multiple requests from updating the cache at the same time by setting the
+        # `updating` flag
+        self.cache[secret_path] = VaultCSIEntry(
+            mtime=cache_entry.mtime, data=cache_entry.data, updating=True
+        )
+        secret_data = self._raw_secret(secret_path)
+        self.cache[secret_path] = VaultCSIEntry(
+            # Note that there is a potential data race here around mtime, but it's
+            # not a big deal since we'll just update it at the next interval.
+            mtime=mtime,
+            data=secret_data,
+            updating=False,
+        )
+        return secret_data, mtime
+
+    def make_object_for_context(self, name: str, span: Span) -> "SecretsStore":
+        return self
+
+
 def secrets_store_from_config(
     app_config: config.RawConfig, timeout: Optional[int] = None, prefix: str = "secrets."
 ) -> SecretsStore:
@@ -510,7 +587,7 @@ def secrets_store_from_config(
 
     if options.provider == "vault_csi":
         parser = parse_vault_csi
-        return DirectorySecretsStore(options.path, parser, timeout=timeout, backoff=backoff)
+        return VaultCSISecretsStore(options.path, parser=parser, timeout=timeout, backoff=backoff)
 
     return SecretsStore(
         options.path, timeout=timeout, backoff=backoff, parser=parse_secrets_fetcher
diff --git a/tests/unit/lib/secrets/store_directory_tests.py b/tests/unit/lib/secrets/store_directory_tests.py
index d4b5b07b0..1e0b375c4 100644
--- a/tests/unit/lib/secrets/store_directory_tests.py
+++ b/tests/unit/lib/secrets/store_directory_tests.py
@@ -5,7 +5,6 @@
 from baseplate.lib.secrets import DirectorySecretsStore
 from baseplate.lib.secrets import parse_vault_csi
 from baseplate.lib.secrets import SecretNotFoundError
-from baseplate.lib.secrets import secrets_store_from_config
 from baseplate.testing.lib.file_watcher import FakeFileWatcher
 
 
@@ -212,11 +211,3 @@ def test_credential_secrets(self):
         }
         with self.assertRaises(CorruptSecretError):
             self.store.get_credentials("secret2")
-
-
-class StoreFromConfigTests(unittest.TestCase):
-    def test_make_store(self):
-        secrets = secrets_store_from_config(
-            {"secrets.path": "/tmp", "secrets.provider": "vault_csi"}
-        )
-        self.assertIsInstance(secrets, DirectorySecretsStore)
diff --git a/tests/unit/lib/secrets/vault_csi_tests.py b/tests/unit/lib/secrets/vault_csi_tests.py
new file mode 100644
index 000000000..86e4e28e7
--- /dev/null
+++ b/tests/unit/lib/secrets/vault_csi_tests.py
@@ -0,0 +1,194 @@
+import datetime
+import json
+import shutil
+import tempfile
+import typing
+import unittest
+
+from pathlib import Path
+
+import gevent
+import typing_extensions
+
+from baseplate.lib.secrets import secrets_store_from_config
+from baseplate.lib.secrets import SecretsStore
+from baseplate.lib.secrets import VaultCSISecretsStore
+
+SecretType: typing_extensions.TypeAlias = typing.Dict[str, any]
+
+
+def write_secrets(secrets_data_path: Path, data: typing.Dict[str, SecretType]) -> None:
+    """Write secrets to the current data directory."""
+    for key, value in data.items():
+        secret_path = secrets_data_path.joinpath(key)
+        secret_path.parent.mkdir(parents=True, exist_ok=True)
+        with open(secret_path, "w") as fp:
+            json.dump(value, fp)
+
+
+def write_symlinks(data_path: Path) -> None:
+    csi_path = data_path.parent
+    # This path can be monitored for changes
+    # https://github.com/kubernetes-sigs/secrets-store-csi-driver/blob/c697863c35d5431ec048b440d36550eb3ceb338f/pkg/util/fileutil/atomic_writer.go#L60-L62
+    data_link = Path(csi_path, "..data")
+    if data_link.exists():
+        # Simulate atomic update
+        new_data_link = Path(csi_path, "..data-new")
+        new_data_link.symlink_to(data_path)
+        new_data_link.rename(data_link)
+    else:
+        data_link.symlink_to(data_path)
+    human_path = Path(csi_path, "secret")
+    if not human_path.exists():
+        human_path.symlink_to(csi_path.joinpath("..data/secret"))
+
+
+def new_fake_csi(data: typing.Dict[str, SecretType]) -> Path:
+    """Creates a simulated CSI directory with data and symlinks.
+    Note that this would already be configured before the pod starts."""
+    csi_dir = Path(tempfile.mkdtemp())
+    # Closely resembles but doesn't precisely match the actual CSI plugin
+    data_path = Path(csi_dir, f'..{datetime.datetime.today().strftime("%Y_%m_%d_%H_%M_%S.%f")}')
+    write_secrets(data_path, data)
+    write_symlinks(data_path)
+    return csi_dir
+
+
+def simulate_secret_update(
+    csi_dir: Path, updated_data: typing.Optional[typing.Dict[str, SecretType]] = None
+) -> None:
+    """Simulates either TTL expiry / a secret update."""
+    old_data_path = csi_dir.joinpath("..data").resolve()
+    # Clone the data directory
+    new_data_path = Path(csi_dir, f'..{datetime.datetime.today().strftime("%Y_%m_%d_%H_%M_%S.%f")}')
+    # Update the secret
+    if updated_data:
+        write_secrets(new_data_path, updated_data)
+    else:
+        shutil.copytree(old_data_path, new_data_path)
+    write_symlinks(new_data_path)
+    shutil.rmtree(old_data_path)
+
+
+def get_secrets_store(csi_dir: str) -> SecretsStore:
+    store = secrets_store_from_config({"secrets.path": csi_dir, "secrets.provider": "vault_csi"})
+    assert isinstance(store, VaultCSISecretsStore)
+    return store
+
+
+EXAMPLE_SECRETS_DATA = {
+    "secret/example-service/example-secret": {
+        "request_id": "8487d906-2154-0151-d07e-57f41447326a",
+        "lease_id": "",
+        "lease_duration": 2764800,
+        "renewable": False,
+        "data": {"password": "password", "type": "credential", "username": "reddit"},
+        "warnings": None,
+    },
+    "secret/example-service/nested/example-nested-secret": {
+        "request_id": "8487d906-2154-0151-d07e-57f41447326a",
+        "lease_id": "",
+        "lease_duration": 2764800,
+        "renewable": False,
+        "data": {"password": "password", "type": "credential", "username": "reddit"},
+        "warnings": None,
+    },
+    "secret/bare-secret": {
+        "request_id": "8487d906-2154-0151-d07e-57f41447326a",
+        "lease_id": "",
+        "lease_duration": 2764800,
+        "renewable": False,
+        "data": {"password": "password", "type": "credential", "username": "reddit"},
+        "warnings": None,
+    },
+}
+
+EXAMPLE_UPDATED_SECRETS = EXAMPLE_SECRETS_DATA.copy()
+EXAMPLE_UPDATED_SECRETS.update(
+    {
+        "secret/example-service/example-secret": {
+            "request_id": "8487d906-2154-0151-d07e-57f41447326a",
+            "lease_id": "",
+            "lease_duration": 2764800,
+            "renewable": False,
+            "data": {
+                "password": "new_password",
+                "type": "credential",
+                "username": "new_reddit",
+            },
+            "warnings": None,
+        },
+    }
+)
+
+
+class StoreTests(unittest.TestCase):
+    def setUp(self):
+        self.csi_dir = new_fake_csi(EXAMPLE_SECRETS_DATA)
+
+    def tearDown(self):
+        shutil.rmtree(self.csi_dir)
+
+    def test_can_load_credential_secret(self):
+        secrets_store = get_secrets_store(str(self.csi_dir))
+        data = secrets_store.get_credentials("secret/example-service/example-secret")
+        assert data.username == "reddit"
+        assert data.password == "password"
+
+    def test_symlink_updated(self):
+        original_data_path = self.csi_dir.joinpath("..data").resolve()
+        secrets_store = get_secrets_store(str(self.csi_dir))
+        data = secrets_store.get_credentials("secret/example-service/example-secret")
+        gevent.sleep(0.1)  # prevent gevent from making execution out-of-order
+        assert data.username == "reddit"
+        assert data.password == "password"
+        simulate_secret_update(self.csi_dir)
+        assert original_data_path != self.csi_dir.joinpath("..data").resolve()
+        data = secrets_store.get_credentials("secret/example-service/example-secret")
+        assert data.username == "reddit"
+        assert data.password == "password"
+
+    def test_secret_updated(self):
+        secrets_store = get_secrets_store(str(self.csi_dir))
+        data = secrets_store.get_credentials("secret/example-service/example-secret")
+        gevent.sleep(0.1)  # prevent gevent from making execution out-of-order
+        assert data.username == "reddit"
+        assert data.password == "password"
+        simulate_secret_update(
+            self.csi_dir,
+            updated_data=EXAMPLE_UPDATED_SECRETS,
+        )
+        data = secrets_store.get_credentials("secret/example-service/example-secret")
+        assert data.username == "new_reddit", f"{data.username} != new_reddit"
+        assert data.password == "new_password", f"{data.password} != new_password"
+
+    def test_multiple_requests_during_symlink_update(self):
+        original_data_path = self.csi_dir.joinpath("..data").resolve()
+        secrets_store = get_secrets_store(str(self.csi_dir))
+        # Populate the cache
+        secrets_store.get_credentials("secret/example-service/example-secret")
+        gevent.sleep(0.1)  # prevent gevent from making execution out-of-order
+        original_raw_secret_callable = secrets_store._raw_secret
+
+        def mock_raw_secret(*args):
+            """Inverts control back to the test during the symlink update."""
+            second_request_result = secrets_store.get_credentials(
+                "secret/example-service/example-secret"
+            )
+            # We should get stale data back from the store
+            assert second_request_result.username == "reddit"
+            assert second_request_result.password == "password"
+            return original_raw_secret_callable(*args)
+
+        secrets_store._raw_secret = mock_raw_secret
+        simulate_secret_update(
+            self.csi_dir,
+            EXAMPLE_UPDATED_SECRETS,
+        )
+        first_request_result = secrets_store.get_credentials(
+            "secret/example-service/example-secret"
+        )
+        gevent.sleep(0.1)  # prevent gevent from making execution out-of-order
+        assert first_request_result.username == "new_reddit"
+        assert first_request_result.password == "new_password"
+        assert original_data_path != self.csi_dir.joinpath("..data").resolve()

From b979cdc62868ae38c845c6af5bfa923bb94d8d87 Mon Sep 17 00:00:00 2001
From: Tyler Lubeck <tyler@tylerlubeck.com>
Date: Tue, 27 Feb 2024 14:05:39 -0500
Subject: [PATCH 02/17] CSI Additions (#829)

* Ensure VaultCSI secrets are sourcing from a directory

* Utilize the parser mechanisms

* Update error messages
---
 baseplate/lib/secrets.py | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/baseplate/lib/secrets.py b/baseplate/lib/secrets.py
index 9750734bf..72f98ada7 100644
--- a/baseplate/lib/secrets.py
+++ b/baseplate/lib/secrets.py
@@ -495,8 +495,13 @@ def __init__(
         backoff: Optional[float] = None,
     ):  # pylint: disable=super-init-not-called
         self.path = Path(path)
+        self.parser = parser
         self.cache = {}
         self.data_symlink = self.path.joinpath("..data")
+        if not self.path.is_dir():
+            raise ValueError(f"Expected {self.path} to be a directory.")
+        if not self.data_symlink.is_dir():
+            raise ValueError(f"Expected {self.data_symlink} to be a directory. Verify {self.path} is the root of the Vault CSI mount.")
 
     def get_vault_url(self) -> str:
         raise NotImplementedError
@@ -511,11 +516,11 @@ def _get_mtime(self) -> float:
     def _raw_secret(self, name: str) -> Any:
         try:
             with open(self.data_symlink.joinpath(name), "r", encoding="UTF-8") as fp:
-                return json.load(fp)["data"]
+                return self.parser(json.load(fp))
         except FileNotFoundError:
             # Try a second time in case the file was deleted while we tried to read it
             with open(self.data_symlink.joinpath(name), "r", encoding="UTF-8") as fp:
-                return json.load(fp)["data"]
+                return self.parser(json.load(fp))
 
     def get_raw_and_mtime(self, secret_path: str) -> Tuple[Dict[str, str], float]:
         mtime = self._get_mtime()
@@ -586,8 +591,7 @@ def secrets_store_from_config(
         backoff = None
 
     if options.provider == "vault_csi":
-        parser = parse_vault_csi
-        return VaultCSISecretsStore(options.path, parser=parser, timeout=timeout, backoff=backoff)
+        return VaultCSISecretsStore(options.path, parser=parse_vault_csi, timeout=timeout, backoff=backoff)
 
     return SecretsStore(
         options.path, timeout=timeout, backoff=backoff, parser=parse_secrets_fetcher

From 8e9d91a4cf6f134a2db6ab802095454e759202bd Mon Sep 17 00:00:00 2001
From: Peter Novotnak <peter.novotnak@reddit.com>
Date: Tue, 27 Feb 2024 15:04:44 -0800
Subject: [PATCH 03/17] Fix data race causing null return (#830)

---
 baseplate/lib/secrets.py                  | 33 ++++++++++++++---------
 tests/unit/lib/secrets/vault_csi_tests.py | 10 +++++++
 2 files changed, 31 insertions(+), 12 deletions(-)

diff --git a/baseplate/lib/secrets.py b/baseplate/lib/secrets.py
index 72f98ada7..e5c03d4d3 100644
--- a/baseplate/lib/secrets.py
+++ b/baseplate/lib/secrets.py
@@ -14,6 +14,8 @@
 from typing import Optional
 from typing import Tuple
 
+from typing_extensions import Protocol
+
 from baseplate import Span
 from baseplate.clients import ContextFactory
 from baseplate.lib import cached_property
@@ -120,7 +122,9 @@ def _decode_secret(path: str, encoding: str, value: str) -> bytes:
     raise CorruptSecretError(path, f"unknown encoding: {encoding!r}")
 
 
-SecretParser = Callable[[Dict[str, Any], str], Dict[str, str]]
+class SecretParser(Protocol):
+    def __call__(self, data: Dict[str, Any], secret_path: Optional[str] = None) -> Dict[str, str]:
+        ...
 
 
 def parse_secrets_fetcher(data: Dict[str, Any], secret_path: str = "") -> Dict[str, str]:
@@ -501,7 +505,9 @@ def __init__(
         if not self.path.is_dir():
             raise ValueError(f"Expected {self.path} to be a directory.")
         if not self.data_symlink.is_dir():
-            raise ValueError(f"Expected {self.data_symlink} to be a directory. Verify {self.path} is the root of the Vault CSI mount.")
+            raise ValueError(
+                f"Expected {self.data_symlink} to be a directory. Verify {self.path} is the root of the Vault CSI mount."
+            )
 
     def get_vault_url(self) -> str:
         raise NotImplementedError
@@ -524,14 +530,15 @@ def _raw_secret(self, name: str) -> Any:
 
     def get_raw_and_mtime(self, secret_path: str) -> Tuple[Dict[str, str], float]:
         mtime = self._get_mtime()
-        cache_entry = self.cache.get(secret_path, VaultCSIEntry(mtime=0, data=None, updating=False))
-        if cache_entry.mtime == mtime or cache_entry.updating is True:
-            return cache_entry.data, mtime
-        # Prevent multiple requests from updating the cache at the same time by setting the
-        # `updating` flag
-        self.cache[secret_path] = VaultCSIEntry(
-            mtime=cache_entry.mtime, data=cache_entry.data, updating=True
-        )
+        if cache_entry := self.cache.get(secret_path):
+            if cache_entry.mtime == mtime or cache_entry.updating is True:
+                return cache_entry.data, mtime
+            # Prevent multiple requests from updating the cache at the same time by setting the
+            # `updating` flag. Unfortunately we can only prevent this when the cache currently
+            # has a valid entry.
+            self.cache[secret_path] = VaultCSIEntry(
+                mtime=cache_entry.mtime, data=cache_entry.data, updating=True
+            )
         secret_data = self._raw_secret(secret_path)
         self.cache[secret_path] = VaultCSIEntry(
             # Note that there is a potential data race here around mtime, but it's
@@ -542,7 +549,7 @@ def get_raw_and_mtime(self, secret_path: str) -> Tuple[Dict[str, str], float]:
         )
         return secret_data, mtime
 
-    def make_object_for_context(self, name: str, span: Span) -> "SecretsStore":
+    def make_object_for_context(self, name: str, span: Span) -> SecretsStore:
         return self
 
 
@@ -591,7 +598,9 @@ def secrets_store_from_config(
         backoff = None
 
     if options.provider == "vault_csi":
-        return VaultCSISecretsStore(options.path, parser=parse_vault_csi, timeout=timeout, backoff=backoff)
+        return VaultCSISecretsStore(
+            options.path, parser=parse_vault_csi, timeout=timeout, backoff=backoff
+        )
 
     return SecretsStore(
         options.path, timeout=timeout, backoff=backoff, parser=parse_secrets_fetcher
diff --git a/tests/unit/lib/secrets/vault_csi_tests.py b/tests/unit/lib/secrets/vault_csi_tests.py
index 86e4e28e7..567475e38 100644
--- a/tests/unit/lib/secrets/vault_csi_tests.py
+++ b/tests/unit/lib/secrets/vault_csi_tests.py
@@ -8,6 +8,7 @@
 from pathlib import Path
 
 import gevent
+import pytest
 import typing_extensions
 
 from baseplate.lib.secrets import secrets_store_from_config
@@ -192,3 +193,12 @@ def mock_raw_secret(*args):
         assert first_request_result.username == "new_reddit"
         assert first_request_result.password == "new_password"
         assert original_data_path != self.csi_dir.joinpath("..data").resolve()
+
+    def test_invalid_secret_raises(self):
+        self.csi_dir.joinpath("..data").resolve()
+        secrets_store = get_secrets_store(str(self.csi_dir))
+        with pytest.raises(FileNotFoundError):
+            secrets_store.get_credentials("secret/example-service/does-not-exist")
+        # While cache is updating we should still fail
+        with pytest.raises(FileNotFoundError):
+            secrets_store.get_credentials("secret/example-service/does-not-exist")

From a9015bf662fe7f901e52a64329b169bc4900beeb Mon Sep 17 00:00:00 2001
From: Peter Novotnak <peter.novotnak@reddit.com>
Date: Tue, 27 Feb 2024 23:11:50 +0000
Subject: [PATCH 04/17] lint and make fmt

---
 baseplate/lib/secrets.py | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/baseplate/lib/secrets.py b/baseplate/lib/secrets.py
index e5c03d4d3..6b4aff700 100644
--- a/baseplate/lib/secrets.py
+++ b/baseplate/lib/secrets.py
@@ -7,15 +7,13 @@
 
 from pathlib import Path
 from typing import Any
-from typing import Callable
 from typing import Dict
 from typing import Iterator
 from typing import NamedTuple
 from typing import Optional
+from typing import Protocol
 from typing import Tuple
 
-from typing_extensions import Protocol
-
 from baseplate import Span
 from baseplate.clients import ContextFactory
 from baseplate.lib import cached_property
@@ -123,7 +121,7 @@ def _decode_secret(path: str, encoding: str, value: str) -> bytes:
 
 
 class SecretParser(Protocol):
-    def __call__(self, data: Dict[str, Any], secret_path: Optional[str] = None) -> Dict[str, str]:
+    def __call__(self, data: Dict[str, Any], secret_path: str = "") -> Dict[str, str]:
         ...
 
 
@@ -576,7 +574,6 @@ def secrets_store_from_config(
     :param provider: The secrets provider, acceptable values are 'vault' and 'vault_csi'. Defaults to 'vault'
 
     """
-    parser: SecretParser
     assert prefix.endswith(".")
     config_prefix = prefix[:-1]
 

From d7ed31ca5fd5f8a2f10a38707ef64f66623d4b84 Mon Sep 17 00:00:00 2001
From: Peter Novotnak <peter.novotnak@reddit.com>
Date: Wed, 28 Feb 2024 17:10:15 +0000
Subject: [PATCH 05/17] remove useless comment

---
 baseplate/lib/secrets.py | 2 --
 1 file changed, 2 deletions(-)

diff --git a/baseplate/lib/secrets.py b/baseplate/lib/secrets.py
index 6b4aff700..cd8afa2b6 100644
--- a/baseplate/lib/secrets.py
+++ b/baseplate/lib/secrets.py
@@ -539,8 +539,6 @@ def get_raw_and_mtime(self, secret_path: str) -> Tuple[Dict[str, str], float]:
             )
         secret_data = self._raw_secret(secret_path)
         self.cache[secret_path] = VaultCSIEntry(
-            # Note that there is a potential data race here around mtime, but it's
-            # not a big deal since we'll just update it at the next interval.
             mtime=mtime,
             data=secret_data,
             updating=False,

From a696704b6e6d786b3e43b09f7b79470404c436ed Mon Sep 17 00:00:00 2001
From: Peter Novotnak <peter.novotnak@reddit.com>
Date: Wed, 28 Feb 2024 23:02:59 +0000
Subject: [PATCH 06/17] KA and TL review points

---
 baseplate/lib/secrets.py                  | 27 ++++++++++++-----------
 docs/pyproject.toml                       |  2 +-
 tests/unit/lib/secrets/vault_csi_tests.py |  5 +++--
 3 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/baseplate/lib/secrets.py b/baseplate/lib/secrets.py
index cd8afa2b6..df7935e1b 100644
--- a/baseplate/lib/secrets.py
+++ b/baseplate/lib/secrets.py
@@ -475,13 +475,13 @@ class VaultCSIEntry(NamedTuple):
 
 
 class VaultCSISecretsStore(SecretsStore):
-    """Access to secret tokens with automatic refresh when changed.
+    """Access to secret tokens using a vault CSI mount with automatic refresh.
 
-    This local vault allows access to the secrets cached on disk given a path to
-    a directory. It will automatically reload the cache when it is changed. Do not
-    cache or store the values returned by this class's methods but rather get
-    them from this class each time you need them. The secrets are served from
-    memory so there's little performance impact to doing so and you will be
+    This store allows access to secrets stored in vault through the CSI interface.
+    It performs caching and will automatically reload secrets from disk. Generally
+    do not cache or store the values returned by this class's methods, rather get
+    them from this class each time you need them. The secrets are served from memory
+    when possible, so there's little performance impact in doing so, and you will be
     sure to always have the current version in the face of key rotation etc.
     """
 
@@ -493,8 +493,6 @@ def __init__(
         self,
         path: str,
         parser: SecretParser,
-        timeout: Optional[int] = None,
-        backoff: Optional[float] = None,
     ):  # pylint: disable=super-init-not-called
         self.path = Path(path)
         self.parser = parser
@@ -508,9 +506,11 @@ def __init__(
             )
 
     def get_vault_url(self) -> str:
+        """Deprecated and will be removed in 3.0.0"""
         raise NotImplementedError
 
     def get_vault_token(self) -> str:
+        """Deprecated and will be removed in 3.0.0"""
         raise NotImplementedError
 
     def _get_mtime(self) -> float:
@@ -523,8 +523,11 @@ def _raw_secret(self, name: str) -> Any:
                 return self.parser(json.load(fp))
         except FileNotFoundError:
             # Try a second time in case the file was deleted while we tried to read it
-            with open(self.data_symlink.joinpath(name), "r", encoding="UTF-8") as fp:
-                return self.parser(json.load(fp))
+            try:
+                with open(self.data_symlink.joinpath(name), "r", encoding="UTF-8") as fp:
+                    return self.parser(json.load(fp))
+            except FileNotFoundError:
+                raise SecretNotFoundError(name)
 
     def get_raw_and_mtime(self, secret_path: str) -> Tuple[Dict[str, str], float]:
         mtime = self._get_mtime()
@@ -593,9 +596,7 @@ def secrets_store_from_config(
         backoff = None
 
     if options.provider == "vault_csi":
-        return VaultCSISecretsStore(
-            options.path, parser=parse_vault_csi, timeout=timeout, backoff=backoff
-        )
+        return VaultCSISecretsStore(options.path, parser=parse_vault_csi)
 
     return SecretsStore(
         options.path, timeout=timeout, backoff=backoff, parser=parse_secrets_fetcher
diff --git a/docs/pyproject.toml b/docs/pyproject.toml
index 0f57bb01b..8b89f3d71 100644
--- a/docs/pyproject.toml
+++ b/docs/pyproject.toml
@@ -5,4 +5,4 @@ line-length = 74
 
 # Always use our latest supported version here since we want code snippets in
 # docs to use the most up-to-date syntax.
-target-version = ['py312']
+target-version = ['py39']
diff --git a/tests/unit/lib/secrets/vault_csi_tests.py b/tests/unit/lib/secrets/vault_csi_tests.py
index 567475e38..b1c1b0e44 100644
--- a/tests/unit/lib/secrets/vault_csi_tests.py
+++ b/tests/unit/lib/secrets/vault_csi_tests.py
@@ -11,6 +11,7 @@
 import pytest
 import typing_extensions
 
+from baseplate.lib.secrets import SecretNotFoundError
 from baseplate.lib.secrets import secrets_store_from_config
 from baseplate.lib.secrets import SecretsStore
 from baseplate.lib.secrets import VaultCSISecretsStore
@@ -197,8 +198,8 @@ def mock_raw_secret(*args):
     def test_invalid_secret_raises(self):
         self.csi_dir.joinpath("..data").resolve()
         secrets_store = get_secrets_store(str(self.csi_dir))
-        with pytest.raises(FileNotFoundError):
+        with pytest.raises(SecretNotFoundError):
             secrets_store.get_credentials("secret/example-service/does-not-exist")
         # While cache is updating we should still fail
-        with pytest.raises(FileNotFoundError):
+        with pytest.raises(SecretNotFoundError):
             secrets_store.get_credentials("secret/example-service/does-not-exist")

From fba0109be0e688f254ef57066938ef0949a810ca Mon Sep 17 00:00:00 2001
From: Peter Novotnak <peter.novotnak@reddit.com>
Date: Thu, 29 Feb 2024 11:17:10 -0800
Subject: [PATCH 07/17] TL review point: enrich stack trace

Co-authored-by: Tyler Lubeck <tyler@tylerlubeck.com>
---
 baseplate/lib/secrets.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/baseplate/lib/secrets.py b/baseplate/lib/secrets.py
index df7935e1b..7d394e067 100644
--- a/baseplate/lib/secrets.py
+++ b/baseplate/lib/secrets.py
@@ -526,8 +526,8 @@ def _raw_secret(self, name: str) -> Any:
             try:
                 with open(self.data_symlink.joinpath(name), "r", encoding="UTF-8") as fp:
                     return self.parser(json.load(fp))
-            except FileNotFoundError:
-                raise SecretNotFoundError(name)
+            except FileNotFoundError as exc:
+                raise SecretNotFoundError(name) from exc
 
     def get_raw_and_mtime(self, secret_path: str) -> Tuple[Dict[str, str], float]:
         mtime = self._get_mtime()

From 9e39e3b28b89222fd6501255dba7d488f1255389 Mon Sep 17 00:00:00 2001
From: Peter Novotnak <peter.novotnak@reddit.com>
Date: Fri, 1 Mar 2024 18:36:25 +0000
Subject: [PATCH 08/17] KL review points

---
 baseplate/lib/secrets.py                  |  7 +-
 docs/pyproject.toml                       |  2 +-
 tests/unit/lib/secrets/vault_csi_tests.py | 97 ++++++++++++++++++++---
 3 files changed, 95 insertions(+), 11 deletions(-)

diff --git a/baseplate/lib/secrets.py b/baseplate/lib/secrets.py
index 7d394e067..3a74e4fd9 100644
--- a/baseplate/lib/secrets.py
+++ b/baseplate/lib/secrets.py
@@ -522,7 +522,12 @@ def _raw_secret(self, name: str) -> Any:
             with open(self.data_symlink.joinpath(name), "r", encoding="UTF-8") as fp:
                 return self.parser(json.load(fp))
         except FileNotFoundError:
-            # Try a second time in case the file was deleted while we tried to read it
+            # Every few minutes, the Vault CSI will create a new directory and swap over to it.
+            # There is a race condition where we could resolve the symlink and then the file could
+            # There is a race condition where we could resolve the symlink and then the file could
+            # be deleted. Given the timeframe between such deletions, a single retry is sufficient
+            # to double-check whether the FileNotFoundError was due to this race or an
+            # actually-nonexistent path.
             try:
                 with open(self.data_symlink.joinpath(name), "r", encoding="UTF-8") as fp:
                     return self.parser(json.load(fp))
diff --git a/docs/pyproject.toml b/docs/pyproject.toml
index 8b89f3d71..0f57bb01b 100644
--- a/docs/pyproject.toml
+++ b/docs/pyproject.toml
@@ -5,4 +5,4 @@ line-length = 74
 
 # Always use our latest supported version here since we want code snippets in
 # docs to use the most up-to-date syntax.
-target-version = ['py39']
+target-version = ['py312']
diff --git a/tests/unit/lib/secrets/vault_csi_tests.py b/tests/unit/lib/secrets/vault_csi_tests.py
index b1c1b0e44..f6e278e28 100644
--- a/tests/unit/lib/secrets/vault_csi_tests.py
+++ b/tests/unit/lib/secrets/vault_csi_tests.py
@@ -1,6 +1,7 @@
 import datetime
 import json
 import shutil
+import string
 import tempfile
 import typing
 import unittest
@@ -103,6 +104,43 @@ def get_secrets_store(csi_dir: str) -> SecretsStore:
         "data": {"password": "password", "type": "credential", "username": "reddit"},
         "warnings": None,
     },
+    "secret/simple-secret": {
+        "request_id": "8487d906-2154-0151-d07e-57f41447326a",
+        "lease_id": "",
+        "lease_duration": 2764800,
+        "renewable": False,
+        "data": {"type": "simple", "value": "simply a secret"},
+        "warnings": None,
+    },
+    "secret/simple-encoded-secret": {
+        "request_id": "8487d906-2154-0151-d07e-57f41447326a",
+        "lease_id": "",
+        "lease_duration": 2764800,
+        "renewable": False,
+        "data": {"type": "simple", "encoding": "base64", "value": "MTMzNw=="},
+        "warnings": None,
+    },
+    "secret/versioned-secret": {
+        "request_id": "8487d906-2154-0151-d07e-57f41447326a",
+        "lease_id": "",
+        "lease_duration": 2764800,
+        "renewable": False,
+        "data": {"type": "versioned", "current": "current value", "previous": "previous value"},
+        "warnings": None,
+    },
+    "secret/versioned-encoded-secret": {
+        "request_id": "8487d906-2154-0151-d07e-57f41447326a",
+        "lease_id": "",
+        "lease_duration": 2764800,
+        "renewable": False,
+        "data": {
+            "type": "versioned",
+            "encoding": "base64",
+            "current": "Y3VycmVudCBlbmNvZGVkIHZhbHVl",
+            "previous": "cHJldmlvdXMgZW5jb2RlZCB2YWx1ZQ==",
+        },
+        "warnings": None,
+    },
 }
 
 EXAMPLE_UPDATED_SECRETS = EXAMPLE_SECRETS_DATA.copy()
@@ -137,16 +175,43 @@ def test_can_load_credential_secret(self):
         assert data.username == "reddit"
         assert data.password == "password"
 
+    def test_get_raw_secret(self):
+        secrets_store = get_secrets_store(str(self.csi_dir))
+        data = secrets_store.get_raw("secret/example-service/nested/example-nested-secret")
+        assert data == {"password": "password", "type": "credential", "username": "reddit"}
+
+    def test_get_simple_secret(self):
+        secrets_store = get_secrets_store(str(self.csi_dir))
+        data = secrets_store.get_simple("secret/simple-secret")
+        assert data == b"simply a secret"
+
+    def test_get_versioned_secret(self):
+        secrets_store = get_secrets_store(str(self.csi_dir))
+        data = secrets_store.get_versioned("secret/versioned-secret")
+        assert data.current == b"current value"
+        assert data.previous == b"previous value"
+
+    def test_simple_encoding(self):
+        secrets_store = get_secrets_store(str(self.csi_dir))
+        data = secrets_store.get_simple("secret/simple-encoded-secret")
+        assert data == b"1337"
+
+    def test_versioned_encoding(self):
+        secrets_store = get_secrets_store(str(self.csi_dir))
+        data = secrets_store.get_versioned("secret/versioned-encoded-secret")
+        assert data.current == b"current encoded value"
+        assert data.previous == b"previous encoded value"
+
     def test_symlink_updated(self):
         original_data_path = self.csi_dir.joinpath("..data").resolve()
         secrets_store = get_secrets_store(str(self.csi_dir))
-        data = secrets_store.get_credentials("secret/example-service/example-secret")
+        data = secrets_store.get_credentials("secret/bare-secret")
         gevent.sleep(0.1)  # prevent gevent from making execution out-of-order
         assert data.username == "reddit"
         assert data.password == "password"
         simulate_secret_update(self.csi_dir)
         assert original_data_path != self.csi_dir.joinpath("..data").resolve()
-        data = secrets_store.get_credentials("secret/example-service/example-secret")
+        data = secrets_store.get_credentials("secret/bare-secret")
         assert data.username == "reddit"
         assert data.password == "password"
 
@@ -154,15 +219,29 @@ def test_secret_updated(self):
         secrets_store = get_secrets_store(str(self.csi_dir))
         data = secrets_store.get_credentials("secret/example-service/example-secret")
         gevent.sleep(0.1)  # prevent gevent from making execution out-of-order
+
         assert data.username == "reddit"
         assert data.password == "password"
-        simulate_secret_update(
-            self.csi_dir,
-            updated_data=EXAMPLE_UPDATED_SECRETS,
-        )
-        data = secrets_store.get_credentials("secret/example-service/example-secret")
-        assert data.username == "new_reddit", f"{data.username} != new_reddit"
-        assert data.password == "new_password", f"{data.password} != new_password"
+
+        printable_ascii = list(string.printable)
+        for i in range(0, len(printable_ascii), 6):
+            chars = printable_ascii[i : i + 6]
+            expected_username = "".join(chars[:3])
+            expected_password = "".join(chars[3:])
+            new_secrets = EXAMPLE_UPDATED_SECRETS.copy()
+            new_secrets["secret/example-service/example-secret"]["data"][
+                "username"
+            ] = expected_username
+            new_secrets["secret/example-service/example-secret"]["data"][
+                "password"
+            ] = expected_password
+            simulate_secret_update(
+                self.csi_dir,
+                updated_data=EXAMPLE_UPDATED_SECRETS,
+            )
+            data = secrets_store.get_credentials("secret/example-service/example-secret")
+            assert data.username == expected_username, f"{data.username} != {expected_username}"
+            assert data.password == expected_password, f"{data.password} != {expected_password}"
 
     def test_multiple_requests_during_symlink_update(self):
         original_data_path = self.csi_dir.joinpath("..data").resolve()

From cdc7d381bd12d9e0d2a89a0e3bf7558474149c15 Mon Sep 17 00:00:00 2001
From: Peter Novotnak <peter.novotnak@reddit.com>
Date: Fri, 1 Mar 2024 18:44:35 +0000
Subject: [PATCH 09/17] prevent gevent returning values out-of-order

---
 tests/unit/lib/secrets/vault_csi_tests.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/unit/lib/secrets/vault_csi_tests.py b/tests/unit/lib/secrets/vault_csi_tests.py
index f6e278e28..e48f91212 100644
--- a/tests/unit/lib/secrets/vault_csi_tests.py
+++ b/tests/unit/lib/secrets/vault_csi_tests.py
@@ -223,7 +223,7 @@ def test_secret_updated(self):
         assert data.username == "reddit"
         assert data.password == "password"
 
-        printable_ascii = list(string.printable)
+        printable_ascii = list(string.printable)[:60]
         for i in range(0, len(printable_ascii), 6):
             chars = printable_ascii[i : i + 6]
             expected_username = "".join(chars[:3])
@@ -242,6 +242,7 @@ def test_secret_updated(self):
             data = secrets_store.get_credentials("secret/example-service/example-secret")
             assert data.username == expected_username, f"{data.username} != {expected_username}"
             assert data.password == expected_password, f"{data.password} != {expected_password}"
+            gevent.sleep(0.05)  # prevent gevent from making execution out-of-order
 
     def test_multiple_requests_during_symlink_update(self):
         original_data_path = self.csi_dir.joinpath("..data").resolve()

From 88e0e072da5e7428d0513e10e699eda668d71a6f Mon Sep 17 00:00:00 2001
From: Peter Novotnak <peter.novotnak@reddit.com>
Date: Fri, 1 Mar 2024 19:05:39 +0000
Subject: [PATCH 10/17] remove cache mutex

---
 baseplate/lib/secrets.py                  | 11 +-----
 tests/unit/lib/secrets/vault_csi_tests.py | 48 +++++++----------------
 2 files changed, 16 insertions(+), 43 deletions(-)

diff --git a/baseplate/lib/secrets.py b/baseplate/lib/secrets.py
index 3a74e4fd9..478360452 100644
--- a/baseplate/lib/secrets.py
+++ b/baseplate/lib/secrets.py
@@ -470,8 +470,6 @@ def _get_file_data(self, filename: str) -> Tuple[Dict, float]:
 class VaultCSIEntry(NamedTuple):
     mtime: float
     data: Any
-    # A mutex to prevent possible duplicate work from event loops
-    updating: bool
 
 
 class VaultCSISecretsStore(SecretsStore):
@@ -537,19 +535,12 @@ def _raw_secret(self, name: str) -> Any:
     def get_raw_and_mtime(self, secret_path: str) -> Tuple[Dict[str, str], float]:
         mtime = self._get_mtime()
         if cache_entry := self.cache.get(secret_path):
-            if cache_entry.mtime == mtime or cache_entry.updating is True:
+            if cache_entry.mtime == mtime:
                 return cache_entry.data, mtime
-            # Prevent multiple requests from updating the cache at the same time by setting the
-            # `updating` flag. Unfortunately we can only prevent this when the cache currently
-            # has a valid entry.
-            self.cache[secret_path] = VaultCSIEntry(
-                mtime=cache_entry.mtime, data=cache_entry.data, updating=True
-            )
         secret_data = self._raw_secret(secret_path)
         self.cache[secret_path] = VaultCSIEntry(
             mtime=mtime,
             data=secret_data,
-            updating=False,
         )
         return secret_data, mtime
 
diff --git a/tests/unit/lib/secrets/vault_csi_tests.py b/tests/unit/lib/secrets/vault_csi_tests.py
index e48f91212..69af899c0 100644
--- a/tests/unit/lib/secrets/vault_csi_tests.py
+++ b/tests/unit/lib/secrets/vault_csi_tests.py
@@ -7,8 +7,9 @@
 import unittest
 
 from pathlib import Path
+from unittest.mock import mock_open
+from unittest.mock import patch
 
-import gevent
 import pytest
 import typing_extensions
 
@@ -206,7 +207,6 @@ def test_symlink_updated(self):
         original_data_path = self.csi_dir.joinpath("..data").resolve()
         secrets_store = get_secrets_store(str(self.csi_dir))
         data = secrets_store.get_credentials("secret/bare-secret")
-        gevent.sleep(0.1)  # prevent gevent from making execution out-of-order
         assert data.username == "reddit"
         assert data.password == "password"
         simulate_secret_update(self.csi_dir)
@@ -218,7 +218,6 @@ def test_symlink_updated(self):
     def test_secret_updated(self):
         secrets_store = get_secrets_store(str(self.csi_dir))
         data = secrets_store.get_credentials("secret/example-service/example-secret")
-        gevent.sleep(0.1)  # prevent gevent from making execution out-of-order
 
         assert data.username == "reddit"
         assert data.password == "password"
@@ -242,38 +241,21 @@ def test_secret_updated(self):
             data = secrets_store.get_credentials("secret/example-service/example-secret")
             assert data.username == expected_username, f"{data.username} != {expected_username}"
             assert data.password == expected_password, f"{data.password} != {expected_password}"
-            gevent.sleep(0.05)  # prevent gevent from making execution out-of-order
 
-    def test_multiple_requests_during_symlink_update(self):
-        original_data_path = self.csi_dir.joinpath("..data").resolve()
+    def test_cache_works(self):
+        self.csi_dir.joinpath("..data").resolve()
         secrets_store = get_secrets_store(str(self.csi_dir))
-        # Populate the cache
-        secrets_store.get_credentials("secret/example-service/example-secret")
-        gevent.sleep(0.1)  # prevent gevent from making execution out-of-order
-        original_raw_secret_callable = secrets_store._raw_secret
-
-        def mock_raw_secret(*args):
-            """Inverts control back to the test during the symlink update."""
-            second_request_result = secrets_store.get_credentials(
-                "secret/example-service/example-secret"
-            )
-            # We should get stale data back from the store
-            assert second_request_result.username == "reddit"
-            assert second_request_result.password == "password"
-            return original_raw_secret_callable(*args)
-
-        secrets_store._raw_secret = mock_raw_secret
-        simulate_secret_update(
-            self.csi_dir,
-            EXAMPLE_UPDATED_SECRETS,
-        )
-        first_request_result = secrets_store.get_credentials(
-            "secret/example-service/example-secret"
-        )
-        gevent.sleep(0.1)  # prevent gevent from making execution out-of-order
-        assert first_request_result.username == "new_reddit"
-        assert first_request_result.password == "new_password"
-        assert original_data_path != self.csi_dir.joinpath("..data").resolve()
+        with patch(
+            "builtins.open",
+            mock_open(
+                read_data=json.dumps(EXAMPLE_SECRETS_DATA["secret/example-service/example-secret"])
+            ),
+        ) as mock_open_:
+            secrets_store.get_credentials("secret/example-service/example-secret")
+            secrets_store.get_credentials("secret/example-service/example-secret")
+            secrets_store.get_credentials("secret/example-service/example-secret")
+            secrets_store.get_credentials("secret/example-service/example-secret")
+        assert mock_open_.call_count == 1
 
     def test_invalid_secret_raises(self):
         self.csi_dir.joinpath("..data").resolve()

From ddbc377d72b7b5780903cb4050f1d3f56fef5243 Mon Sep 17 00:00:00 2001
From: Peter Novotnak <peter.novotnak@reddit.com>
Date: Fri, 1 Mar 2024 19:09:44 +0000
Subject: [PATCH 11/17] reintroduce sleep

---
 tests/unit/lib/secrets/vault_csi_tests.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/unit/lib/secrets/vault_csi_tests.py b/tests/unit/lib/secrets/vault_csi_tests.py
index 69af899c0..8c6b80c3d 100644
--- a/tests/unit/lib/secrets/vault_csi_tests.py
+++ b/tests/unit/lib/secrets/vault_csi_tests.py
@@ -10,6 +10,7 @@
 from unittest.mock import mock_open
 from unittest.mock import patch
 
+import gevent
 import pytest
 import typing_extensions
 
@@ -241,6 +242,7 @@ def test_secret_updated(self):
             data = secrets_store.get_credentials("secret/example-service/example-secret")
             assert data.username == expected_username, f"{data.username} != {expected_username}"
             assert data.password == expected_password, f"{data.password} != {expected_password}"
+            gevent.sleep(0.05)  # prevent gevent shenanigans
 
     def test_cache_works(self):
         self.csi_dir.joinpath("..data").resolve()

From d6967d6a15ca786680e62e936d9891c90a356332 Mon Sep 17 00:00:00 2001
From: Peter Novotnak <peter.novotnak@reddit.com>
Date: Fri, 1 Mar 2024 19:21:52 +0000
Subject: [PATCH 12/17] more sleep

---
 tests/unit/lib/secrets/vault_csi_tests.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/unit/lib/secrets/vault_csi_tests.py b/tests/unit/lib/secrets/vault_csi_tests.py
index 8c6b80c3d..17eb8f59e 100644
--- a/tests/unit/lib/secrets/vault_csi_tests.py
+++ b/tests/unit/lib/secrets/vault_csi_tests.py
@@ -219,6 +219,7 @@ def test_symlink_updated(self):
     def test_secret_updated(self):
         secrets_store = get_secrets_store(str(self.csi_dir))
         data = secrets_store.get_credentials("secret/example-service/example-secret")
+        gevent.sleep(0.1)  # prevent gevent shenanigans
 
         assert data.username == "reddit"
         assert data.password == "password"
@@ -242,7 +243,7 @@ def test_secret_updated(self):
             data = secrets_store.get_credentials("secret/example-service/example-secret")
             assert data.username == expected_username, f"{data.username} != {expected_username}"
             assert data.password == expected_password, f"{data.password} != {expected_password}"
-            gevent.sleep(0.05)  # prevent gevent shenanigans
+            gevent.sleep(0.075)  # prevent gevent shenanigans
 
     def test_cache_works(self):
         self.csi_dir.joinpath("..data").resolve()

From 3bdb034ee7862cc73c8451bf4146c78216adc25f Mon Sep 17 00:00:00 2001
From: Peter Novotnak <peter.novotnak@reddit.com>
Date: Fri, 1 Mar 2024 19:26:52 +0000
Subject: [PATCH 13/17] add sleep for cache test

---
 tests/unit/lib/secrets/vault_csi_tests.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/unit/lib/secrets/vault_csi_tests.py b/tests/unit/lib/secrets/vault_csi_tests.py
index 17eb8f59e..c49cfc87d 100644
--- a/tests/unit/lib/secrets/vault_csi_tests.py
+++ b/tests/unit/lib/secrets/vault_csi_tests.py
@@ -258,6 +258,7 @@ def test_cache_works(self):
             secrets_store.get_credentials("secret/example-service/example-secret")
             secrets_store.get_credentials("secret/example-service/example-secret")
             secrets_store.get_credentials("secret/example-service/example-secret")
+        gevent.sleep(0.1)  # prevent gevent shenanigans
         assert mock_open_.call_count == 1
 
     def test_invalid_secret_raises(self):

From 1f6bda5551d7089670564b1a1bce7d87d498373f Mon Sep 17 00:00:00 2001
From: Peter Novotnak <peter.novotnak@reddit.com>
Date: Tue, 5 Mar 2024 09:10:37 -0800
Subject: [PATCH 14/17] simpler test secret writer

Co-authored-by: Chris Kuehl <chris.kuehl@reddit.com>
---
 tests/unit/lib/secrets/vault_csi_tests.py | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tests/unit/lib/secrets/vault_csi_tests.py b/tests/unit/lib/secrets/vault_csi_tests.py
index c49cfc87d..d580ea2fe 100644
--- a/tests/unit/lib/secrets/vault_csi_tests.py
+++ b/tests/unit/lib/secrets/vault_csi_tests.py
@@ -27,8 +27,7 @@ def write_secrets(secrets_data_path: Path, data: typing.Dict[str, SecretType]) -
     for key, value in data.items():
         secret_path = secrets_data_path.joinpath(key)
         secret_path.parent.mkdir(parents=True, exist_ok=True)
-        with open(secret_path, "w") as fp:
-            json.dump(value, fp)
+        secret_path.write_text(json.dumps(value))
 
 
 def write_symlinks(data_path: Path) -> None:

From 23c31ae2a8fcb369423f6eb0d61b7c3fd2be5468 Mon Sep 17 00:00:00 2001
From: Peter Novotnak <peter.novotnak@reddit.com>
Date: Tue, 5 Mar 2024 09:16:51 -0800
Subject: [PATCH 15/17] simplify link management in test

Co-authored-by: Chris Kuehl <chris.kuehl@reddit.com>
---
 tests/unit/lib/secrets/vault_csi_tests.py | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/tests/unit/lib/secrets/vault_csi_tests.py b/tests/unit/lib/secrets/vault_csi_tests.py
index d580ea2fe..a55880022 100644
--- a/tests/unit/lib/secrets/vault_csi_tests.py
+++ b/tests/unit/lib/secrets/vault_csi_tests.py
@@ -35,13 +35,10 @@ def write_symlinks(data_path: Path) -> None:
     # This path can be monitored for changes
     # https://github.com/kubernetes-sigs/secrets-store-csi-driver/blob/c697863c35d5431ec048b440d36550eb3ceb338f/pkg/util/fileutil/atomic_writer.go#L60-L62
     data_link = Path(csi_path, "..data")
-    if data_link.exists():
-        # Simulate atomic update
-        new_data_link = Path(csi_path, "..data-new")
-        new_data_link.symlink_to(data_path)
-        new_data_link.rename(data_link)
-    else:
-        data_link.symlink_to(data_path)
+    # Simulate atomic update
+    new_data_link = Path(csi_path, "..data-new")
+    new_data_link.symlink_to(data_path)
+    new_data_link.rename(data_link)
     human_path = Path(csi_path, "secret")
     if not human_path.exists():
         human_path.symlink_to(csi_path.joinpath("..data/secret"))

From 2c7dcdb08b223408064902cb328adcc322fbce50 Mon Sep 17 00:00:00 2001
From: Peter Novotnak <peter.novotnak@reddit.com>
Date: Tue, 5 Mar 2024 17:23:06 +0000
Subject: [PATCH 16/17] CK review point: remove race handling

---
 baseplate/lib/secrets.py | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/baseplate/lib/secrets.py b/baseplate/lib/secrets.py
index 478360452..1a18a8fa7 100644
--- a/baseplate/lib/secrets.py
+++ b/baseplate/lib/secrets.py
@@ -519,18 +519,8 @@ def _raw_secret(self, name: str) -> Any:
         try:
             with open(self.data_symlink.joinpath(name), "r", encoding="UTF-8") as fp:
                 return self.parser(json.load(fp))
-        except FileNotFoundError:
-            # Every few minutes, the Vault CSI will create a new directory and swap over to it.
-            # There is a race condition where we could resolve the symlink and then the file could
-            # There is a race condition where we could resolve the symlink and then the file could
-            # be deleted. Given the timeframe between such deletions, a single retry is sufficient
-            # to double-check whether the FileNotFoundError was due to this race or an
-            # actually-nonexistent path.
-            try:
-                with open(self.data_symlink.joinpath(name), "r", encoding="UTF-8") as fp:
-                    return self.parser(json.load(fp))
-            except FileNotFoundError as exc:
-                raise SecretNotFoundError(name) from exc
+        except FileNotFoundError as exc:
+            raise SecretNotFoundError(name) from exc
 
     def get_raw_and_mtime(self, secret_path: str) -> Tuple[Dict[str, str], float]:
         mtime = self._get_mtime()

From b073d140c62dec290e96b2ae23c7d47f6b03d5b8 Mon Sep 17 00:00:00 2001
From: Peter Novotnak <peter.novotnak@reddit.com>
Date: Thu, 7 Mar 2024 00:18:41 +0000
Subject: [PATCH 17/17] remove old implementation based on discussion

---
 baseplate/lib/secrets.py                      |  82 -------
 .../unit/lib/secrets/store_directory_tests.py | 213 ------------------
 2 files changed, 295 deletions(-)
 delete mode 100644 tests/unit/lib/secrets/store_directory_tests.py

diff --git a/baseplate/lib/secrets.py b/baseplate/lib/secrets.py
index 1a18a8fa7..820fb6c3a 100644
--- a/baseplate/lib/secrets.py
+++ b/baseplate/lib/secrets.py
@@ -385,88 +385,6 @@ def _get_data(self) -> Tuple[Dict, float]:
         return self._data
 
 
-class DirectorySecretsStore(SecretsStore):
-    """Access to secret tokens with automatic refresh when changed.
-
-    This local vault allows access to the secrets cached on disk given a path to
-    a directory. It will automatically reload the cache when it is changed. Do not
-    cache or store the values returned by this class's methods but rather get
-    them from this class each time you need them. The secrets are served from
-    memory so there's little performance impact to doing so and you will be
-    sure to always have the current version in the face of key rotation etc.
-    """
-
-    def __init__(
-        self,
-        path: str,
-        parser: SecretParser,
-        timeout: Optional[int] = None,
-        backoff: Optional[float] = None,
-    ):  # pylint: disable=super-init-not-called
-        self.parser = parser
-        self._filewatchers = {}
-        root = Path(path)
-        for p in root.glob("**"):
-            file_path = str(p.relative_to(path))
-            self._filewatchers[file_path] = FileWatcher(
-                file_path, json.load, timeout=timeout, backoff=backoff
-            )
-
-    def get_vault_url(self) -> str:
-        raise NotImplementedError
-
-    def get_vault_token(self) -> str:
-        raise NotImplementedError
-
-    def _get_file_data(self, filename: str) -> Tuple[Any, float]:
-        try:
-            return self._filewatchers[filename].get_data_and_mtime()
-        except KeyError:
-            raise SecretNotFoundError(filename)
-        except WatchedFileNotAvailableError as exc:
-            raise SecretsNotAvailableError(exc)
-
-    def get_raw_and_mtime(self, secret_path: str) -> Tuple[Dict[str, str], float]:
-        """Return raw secret and modification time.
-        This returns the same data as :py:meth:`get_raw` as well as a UNIX
-        epoch timestamp indicating the last time the secrets data was updated.
-        This modification time can be used to know when to invalidate
-        downstream caching.
-        .. versionadded:: 1.5
-        """
-        data, mtime = self._get_file_data(secret_path)
-        return self.parser(data, secret_path), mtime
-
-    def make_object_for_context(self, name: str, span: Span) -> "DirectorySecretsStore":
-        """Return an object that can be added to the context object.
-        This allows the secret store to be used with
-        :py:meth:`~baseplate.Baseplate.add_to_context`::
-           secrets = SecretsStore("/var/local/secrets.json")
-           baseplate.add_to_context("secrets", secrets)
-        """
-        return _CachingDirectorySecretsStore(self._filewatchers, self.parser)
-
-
-# pylint: disable=abstract-method
-class _CachingDirectorySecretsStore(DirectorySecretsStore):
-    """Lazily load and cache the parsed data until the server span ends."""
-
-    def __init__(
-        self, filewatchers: Dict[str, FileWatcher], parser: SecretParser
-    ):  # pylint: disable=super-init-not-called
-        self._filewatchers = filewatchers
-        self.parser = parser
-        self._cached_data: Dict[str, Tuple[Dict, float]] = {}
-
-    def _get_file_data(self, filename: str) -> Tuple[Dict, float]:
-        try:
-            result = self._cached_data[filename]
-        except KeyError:
-            result = super()._get_file_data(filename)
-            self._cached_data[filename] = result
-        return result
-
-
 class VaultCSIEntry(NamedTuple):
     mtime: float
     data: Any
diff --git a/tests/unit/lib/secrets/store_directory_tests.py b/tests/unit/lib/secrets/store_directory_tests.py
deleted file mode 100644
index 1e0b375c4..000000000
--- a/tests/unit/lib/secrets/store_directory_tests.py
+++ /dev/null
@@ -1,213 +0,0 @@
-import unittest
-
-from baseplate.lib.secrets import CorruptSecretError
-from baseplate.lib.secrets import CredentialSecret
-from baseplate.lib.secrets import DirectorySecretsStore
-from baseplate.lib.secrets import parse_vault_csi
-from baseplate.lib.secrets import SecretNotFoundError
-from baseplate.testing.lib.file_watcher import FakeFileWatcher
-
-
-class StoreDirectoryTests(unittest.TestCase):
-    def setUp(self):
-        self.fake_filewatcher_1 = FakeFileWatcher()
-        self.fake_filewatcher_2 = FakeFileWatcher()
-        self.fake_filewatcher_3 = FakeFileWatcher()
-        self.store = DirectorySecretsStore("/whatever", parse_vault_csi)
-        self.store._filewatchers["secret1"] = self.fake_filewatcher_1
-        self.store._filewatchers["secret2"] = self.fake_filewatcher_2
-        self.store._filewatchers["secret3"] = self.fake_filewatcher_3
-
-    def test_file_not_found(self):
-        with self.assertRaises(SecretNotFoundError):
-            self.store.get_raw("test")
-
-    def test_vault_info(self):
-        with self.assertRaises(NotImplementedError):
-            self.store.get_vault_token()
-
-        with self.assertRaises(NotImplementedError):
-            self.store.get_vault_url()
-
-    def test_raw_secrets(self):
-        self.fake_filewatcher_1.data = {
-            "data": {"something": "exists"},
-        }
-
-        self.assertEqual(self.store.get_raw("secret1"), {"something": "exists"})
-
-        with self.assertRaises(SecretNotFoundError):
-            self.store.get_raw("secret0")
-
-    def test_simple_secrets(self):
-        # simple test
-        self.fake_filewatcher_1.data = {
-            "data": {"type": "simple", "value": "easy"},
-        }
-        self.assertEqual(self.store.get_simple("secret1"), b"easy")
-
-        # test base64
-        self.fake_filewatcher_2.data = {
-            "data": {"type": "simple", "value": "aHVudGVyMg==", "encoding": "base64"},
-        }
-        self.assertEqual(self.store.get_simple("secret2"), b"hunter2")
-
-        # test unknown encoding
-        self.fake_filewatcher_3.data = {
-            "data": {"type": "simple", "value": "sdlfkj", "encoding": "mystery"},
-        }
-        with self.assertRaises(CorruptSecretError):
-            self.store.get_simple("secret3")
-
-        # test not simple
-        self.fake_filewatcher_1.data = {
-            "data": {"something": "else"},
-        }
-        with self.assertRaises(CorruptSecretError):
-            self.store.get_simple("secret1")
-
-        # test no value
-        self.fake_filewatcher_2.data = {
-            "data": {"type": "simple"},
-        }
-        with self.assertRaises(CorruptSecretError):
-            self.store.get_simple("secret2")
-
-        # test bad base64
-        self.fake_filewatcher_3.data = {
-            "data": {"type": "simple", "value": "aHVudGVyMg", "encoding": "base64"},
-        }
-        with self.assertRaises(CorruptSecretError):
-            self.store.get_simple("secret3")
-
-    def test_versioned_secrets(self):
-        # simple test
-        self.fake_filewatcher_1.data = {
-            "data": {"type": "versioned", "current": "easy"},
-        }
-        simple = self.store.get_versioned("secret1")
-        self.assertEqual(simple.current, b"easy")
-        self.assertEqual(list(simple.all_versions), [b"easy"])
-
-        # test base64
-        self.fake_filewatcher_2.data = {
-            "data": {
-                "type": "versioned",
-                "previous": "aHVudGVyMQ==",
-                "current": "aHVudGVyMg==",
-                "next": "aHVudGVyMw==",
-                "encoding": "base64",
-            },
-        }
-        encoded = self.store.get_versioned("secret2")
-        self.assertEqual(encoded.previous, b"hunter1")
-        self.assertEqual(encoded.current, b"hunter2")
-        self.assertEqual(encoded.next, b"hunter3")
-        self.assertEqual(list(encoded.all_versions), [b"hunter2", b"hunter1", b"hunter3"])
-
-        # test unknown encoding
-        self.fake_filewatcher_3.data = {
-            "data": {"type": "versioned", "current": "sdlfkj", "encoding": "mystery"},
-        }
-        with self.assertRaises(CorruptSecretError):
-            self.store.get_versioned("secret3")
-
-        # test not versioned
-        self.fake_filewatcher_1.data = {
-            "data": {"something": "else"},
-        }
-        with self.assertRaises(CorruptSecretError):
-            self.store.get_versioned("secret1")
-
-        # test no value
-        self.fake_filewatcher_2.data = {
-            "data": {"type": "versioned"},
-        }
-        with self.assertRaises(CorruptSecretError):
-            self.store.get_versioned("secret2")
-
-        # test bad base64
-        self.fake_filewatcher_3.data = {
-            "data": {"type": "simple", "value": "aHVudGVyMg", "encoding": "base64"},
-        }
-        with self.assertRaises(CorruptSecretError):
-            self.store.get_versioned("secret3")
-
-    def test_credential_secrets(self):
-        # simple test
-        self.fake_filewatcher_1.data = {
-            "data": {"type": "credential", "username": "user", "password": "password"},
-        }
-        self.assertEqual(
-            self.store.get_credentials("secret1"), CredentialSecret("user", "password")
-        )
-
-        # test identiy
-        self.fake_filewatcher_2.data = {
-            "data": {
-                "type": "credential",
-                "username": "spez",
-                "password": "hunter2",
-                "encoding": "identity",
-            },
-        }
-        self.assertEqual(self.store.get_credentials("secret2"), CredentialSecret("spez", "hunter2"))
-
-        # test base64
-        self.fake_filewatcher_2.data = {
-            "data": {
-                "type": "credential",
-                "username": "foo",
-                "password": "aHVudGVyMg==",
-                "encoding": "base64",
-            },
-        }
-        with self.assertRaises(CorruptSecretError):
-            self.store.get_credentials("secret2")
-
-        # test unknkown encoding
-        self.fake_filewatcher_3.data = {
-            "data": {
-                "type": "credential",
-                "username": "fizz",
-                "password": "buzz",
-                "encoding": "something",
-            },
-        }
-        with self.assertRaises(CorruptSecretError):
-            self.store.get_credentials("secret3")
-
-        # test not credentials
-        self.fake_filewatcher_1.data = {
-            "data": {"type": "versioned", "current": "easy"},
-        }
-        with self.assertRaises(CorruptSecretError):
-            self.store.get_credentials("secret1")
-
-        # test no values
-        self.fake_filewatcher_2.data = {
-            "data": {"type": "credential"},
-        }
-        with self.assertRaises(CorruptSecretError):
-            self.store.get_credentials("secret2")
-
-        # test no username
-        self.fake_filewatcher_3.data = {
-            "data": {"type": "credential", "password": "password"},
-        }
-        with self.assertRaises(CorruptSecretError):
-            self.store.get_credentials("secret3")
-
-        # test no password
-        self.fake_filewatcher_1.data = {
-            "data": {"type": "credential", "username": "user"},
-        }
-        with self.assertRaises(CorruptSecretError):
-            self.store.get_credentials("secret1")
-
-        # test bad type
-        self.fake_filewatcher_2.data = {
-            "data": {"type": "credential", "username": "user", "password": 100},
-        }
-        with self.assertRaises(CorruptSecretError):
-            self.store.get_credentials("secret2")