From 4a1a4cc579d2b7c860959f9a3f30538b3a04ee48 Mon Sep 17 00:00:00 2001 From: "Gary T. Giesen" Date: Wed, 27 May 2026 12:54:49 -0400 Subject: [PATCH 1/3] Add salt.cache.etcd3_cache: native etcd v3 minion data cache backend A from-scratch cache backend for the etcd v3 API. Each cache entry is a single etcd key holding salt.payload.dumps({"d": data, "t": }). expires= is honoured natively via etcd v3 leases (so salt.auth tokens are reaped by etcd rather than lingering until a manual flush). list/contains follow salt.cache.localfs semantics, and listing uses keys-only range scans so listing a bank does not transfer the stored values. Connection setup (profile, auth, TLS) reuses salt.utils.etcd_util.EtcdClientV3 and then operates on the etcd3-py client directly. Enable with cache: etcd3 (requires the etcd3-py library). --- changelog/61037.added.md | 1 + doc/ref/cache/all/index.rst | 1 + doc/ref/cache/all/salt.cache.etcd3_cache.rst | 5 + salt/cache/etcd3_cache.py | 401 ++++++++++++ tests/pytests/functional/cache/test_etcd3.py | 54 ++ .../integration/cache/test_etcd3_cache.py | 579 ++++++++++++++++++ tests/pytests/unit/cache/test_etcd3_cache.py | 459 ++++++++++++++ 7 files changed, 1500 insertions(+) create mode 100644 changelog/61037.added.md create mode 100644 doc/ref/cache/all/salt.cache.etcd3_cache.rst create mode 100644 salt/cache/etcd3_cache.py create mode 100644 tests/pytests/functional/cache/test_etcd3.py create mode 100644 tests/pytests/integration/cache/test_etcd3_cache.py create mode 100644 tests/pytests/unit/cache/test_etcd3_cache.py diff --git a/changelog/61037.added.md b/changelog/61037.added.md new file mode 100644 index 000000000000..b9dbf4171a8f --- /dev/null +++ b/changelog/61037.added.md @@ -0,0 +1 @@ +Added the ``etcd3`` minion data cache backend (:mod:`salt.cache.etcd3_cache `) for the etcd v3 API. Each entry is stored as a single etcd key, ``expires`` is honoured natively via etcd leases (so :mod:`salt.auth` tokens are reaped by etcd rather than lingering until a manual flush), and ``list``/``contains`` follow :mod:`salt.cache.localfs` semantics. Enable with ``cache: etcd3`` on the master; requires the ``etcd3-py`` library. diff --git a/doc/ref/cache/all/index.rst b/doc/ref/cache/all/index.rst index 8047be56a334..3465de85e85b 100644 --- a/doc/ref/cache/all/index.rst +++ b/doc/ref/cache/all/index.rst @@ -14,6 +14,7 @@ For understanding and usage of the cache modules see the :ref:`cache` topic. consul etcd_cache + etcd3_cache localfs localfs_key mmap_cache diff --git a/doc/ref/cache/all/salt.cache.etcd3_cache.rst b/doc/ref/cache/all/salt.cache.etcd3_cache.rst new file mode 100644 index 000000000000..cf669f42904a --- /dev/null +++ b/doc/ref/cache/all/salt.cache.etcd3_cache.rst @@ -0,0 +1,5 @@ +salt.cache.etcd3_cache +====================== + +.. automodule:: salt.cache.etcd3_cache + :members: diff --git a/salt/cache/etcd3_cache.py b/salt/cache/etcd3_cache.py new file mode 100644 index 000000000000..43bb63becd58 --- /dev/null +++ b/salt/cache/etcd3_cache.py @@ -0,0 +1,401 @@ +""" +Minion data cache plugin for the etcd key/value store, using API v3. + +.. versionadded:: 3009.0 + +A from-scratch cache backend built around etcd v3 semantics: a flat +keyspace, native byte values, single-PUT atomicity, and lease-based +expiry. It is not a port of +:mod:`salt.cache.etcd_cache `, which targets the +v2 HTTP API; the two use different etcd APIs and storage and do not share +data. + +Storage model +------------- + +Each cache entry is a single etcd key whose value is +``salt.payload.dumps({"d": , "t": })`` -- the data and its +modification timestamp wrapped together. A single etcd PUT is atomic, so +there is no sibling timestamp key to keep consistent. The ``bank/key`` +hierarchy is mapped to a key path by joining components with ``/``; prefix +operations always use a trailing slash so bank ``foo`` does not match keys +under bank ``foobar``. + +Concretely, with the default prefix, bank ``grains`` key ``minion-1`` is +stored at the etcd key ``/salt_cache/grains/minion-1`` and its value is the +msgpack-encoded ``{"d": , "t": }`` envelope as raw bytes (not +base64). To inspect the cache directly:: + + etcdctl get --prefix /salt_cache/ + +The v2 :mod:`salt.cache.etcd_cache ` driver shares +the default ``/salt_cache`` prefix, but the etcd v2 and v3 APIs use +independent keyspaces, so the two do not collide even on the same cluster. +No other Salt etcd integration (the ``etcd`` execution module, state, or +SDB) writes under this prefix. + +Setup +----- + +Install the etcd3-py client:: + + pip install etcd3-py + +Enable as the master's data cache:: + + cache: etcd3 + +Optional configuration (defaults shown):: + + etcd.host: 127.0.0.1 + etcd.port: 2379 + etcd.username: null + etcd.password: null + etcd.ca: null + etcd.client_cert: null + etcd.client_key: null + etcd.path_prefix: /salt_cache + +A profile may be used instead of top-level options by setting +``etcd.cache_profile: my_etcd_config`` on the master and placing the +``etcd.*`` keys under ``my_etcd_config``. + +Behaviour notes +--------------- + +- ``store(bank, key, data, expires=N)`` with a positive ``expires`` + attaches the key to an etcd v3 lease with that TTL in seconds; etcd + deletes the key when the lease expires. This is used by + :mod:`salt.auth` for token expiry, so expired tokens are reaped by etcd + rather than persisting until a manual flush. +- ``list`` and ``contains`` follow :mod:`salt.cache.localfs` semantics: + ``list(bank)`` returns the immediate children (direct keys and immediate + sub-bank names), which is what callers such as + :func:`salt.utils.master._get_cached_minion_data` expect from + ``cache.list('minions')``. +- ``flush`` returns ``True`` on success, matching + :mod:`salt.cache.redis_cache`. +- The driver refuses to initialize if ``etcd.path_prefix`` resolves to an + empty or root path, so a misconfiguration cannot turn a bank flush into + a range-delete at the cluster root. + +Deploying on a shared etcd cluster +---------------------------------- + +This cache can run against an etcd cluster shared with other tenants +(Patroni, Kubernetes, etc.). Recommended practice: + +- **Scope access with etcd RBAC.** The ``etcd.path_prefix`` check is only + a fat-finger guard; the real isolation boundary is a prefix-scoped etcd + role:: + + etcdctl role add salt-cache + etcdctl role grant-permission salt-cache --prefix=true readwrite /salt_cache/ + etcdctl user add salt-master + etcdctl user grant-role salt-master salt-cache + +- **Use TLS in production.** Set ``etcd.ca``, ``etcd.client_cert`` and + ``etcd.client_key`` so cache traffic and credentials are not in the + clear. +- **Point at the cluster, not a single node** (a TCP load balancer or DNS + round-robin), so the master survives a single etcd node failing. +- **Confirm auto-compaction is enabled** and **monitor the DB quota**. + Each ``store`` adds a revision; without compaction the steady-state + write rate (one revision per minion check-in) grows the DB without + bound. +- **Multi-master Salt** benefits from etcd's linearizable reads: multiple + masters sharing this cache see a consistent view without extra + coordination. + +Migrating from the v2 etcd cache +-------------------------------- + +The v2 API uses a store isolated from v3, so the v3 cache cannot read v2 +data (and cannot interfere with it). Because the master cache is +ephemeral and repopulates as minions check in, migration is just: install +``etcd3-py``, change ``cache: etcd`` to ``cache: etcd3``, and restart the +master. Old v2 keys are orphaned in the v2 store; remove them with +``ETCDCTL_API=2 etcdctl rm --recursive /salt_cache`` if desired. + +The one user-visible behaviour change is ``list``: the v2 driver returned +recursive leaf names, so ``cache.list('minions')`` produced +``['data', 'data', ...]``; this driver returns the immediate children +(``['minion-1', 'minion-2', ...]``). + +Value-size limit +---------------- + +etcd's default ``--max-request-bytes`` is 1.5 MiB per request. Grains, +mine returns, tokens and minion keys are well under this, but a large +pillar tree may exceed it, in which case ``store`` raises +``SaltCacheError`` wrapping ``etcdserver: request is too large``. Raise +the etcd flag (up to ~10 MiB) or use ``localfs``/``redis`` for very large +pillar. +""" + +import logging +import time + +import salt.payload +import salt.utils.etcd_util +from salt.exceptions import SaltCacheError + +# salt.utils.etcd_util imports cleanly without etcd3-py; HAS_ETCD_V3 reports +# whether the etcd3-py library is actually available. +HAS_ETCD = salt.utils.etcd_util.HAS_ETCD_V3 + +_DEFAULT_PATH_PREFIX = "/salt_cache" + +log = logging.getLogger(__name__) + +# Module-level singletons populated by _init_client() on first use. ``client`` +# is a configured etcd3-py client (etcd3.Client), not the EtcdClientV3 wrapper. +client = None +path_prefix = None + +__virtualname__ = "etcd3" +__func_alias__ = {"ls": "list"} + + +def __virtual__(): + """ + Only load if the etcd3-py library is available. + """ + if not HAS_ETCD: + return ( + False, + "Please install etcd3-py to use the etcd3 data cache driver", + ) + return __virtualname__ + + +def init_kwargs(kwargs): + """ + Cache-plugin hook; no per-instance state is needed, so this always + returns an empty dict (parity with :mod:`salt.cache.redis_cache`). + """ + return {} + + +def _init_client(): + """ + Build the etcd v3 client once and cache it at module level. + + Connection handling (profile resolution, auth, TLS) is reused from + :class:`salt.utils.etcd_util.EtcdClientV3`; we then operate on the + configured etcd3-py client directly. The wrapper's read/write layer + applies a msgpack codec and has neither lease nor keys-only support, + none of which suits a cache that serializes with :mod:`salt.payload` + and lists large banks. + """ + global client, path_prefix + if client is not None: + return + + raw_prefix = __opts__.get("etcd.path_prefix", _DEFAULT_PATH_PREFIX) + stripped = (raw_prefix or "").strip("/") + if not stripped: + raise SaltCacheError( + "etcd3 cache: etcd.path_prefix must resolve to a non-empty path " + f"(got {raw_prefix!r}). An empty or root prefix would let flush() " + "prefix-delete keys outside Salt's namespace; refusing to " + "initialize." + ) + path_prefix = "/" + stripped + + profile = __opts__.get("etcd.cache_profile") + # Resolve the (optionally profile-scoped) etcd config and force the v3 + # client regardless of the deprecated ``etcd.require_v2`` flag, so the + # user does not have to set a "require v2" option to use the v3 cache. + conf = dict( + salt.utils.etcd_util._get_etcd_opts( # pylint: disable=protected-access + __opts__, profile + ) + ) + conf["etcd.require_v2"] = False + log.info( + "etcd3 cache: initializing client (path_prefix=%s, profile=%s)", + path_prefix, + profile, + ) + client = salt.utils.etcd_util.EtcdClientV3(conf, has_etcd_opts=True).client + + +def _value_key(bank, key): + if bank: + return f"{path_prefix}/{bank}/{key}" + return f"{path_prefix}/{key}" + + +def _bank_prefix(bank): + # The trailing slash matters: prefix-scanning ``{prefix}/foo`` without it + # would also match ``{prefix}/foobar/...``. An empty bank is the + # root-listing case used by salt.runners.cache.migrate via + # ``cache.list("")``; scan from ``{path_prefix}/`` directly. + if bank: + return f"{path_prefix}/{bank}/" + return f"{path_prefix}/" + + +def _pack(data): + """Wrap ``data`` with its modification timestamp for storage.""" + return salt.payload.dumps({"d": data, "t": int(time.time())}) + + +def _unpack(raw): + """Inverse of :func:`_pack`; return ``(data, timestamp)``.""" + obj = salt.payload.loads(raw) + return obj["d"], obj["t"] + + +def _range(key, prefix=False, keys_only=False, limit=0): + """ + Issue an etcd range request and return its key/value pairs as a list + (etcd3-py returns ``None`` for an empty result). + """ + return client.range(key, prefix=prefix, keys_only=keys_only, limit=limit).kvs or [] + + +def store(bank, key, data, expires=None): + """ + Store ``data`` at ``bank/key`` as a single etcd key. + + :param bank: Bank path. May contain ``/`` for nested banks. + :param key: Leaf key name within the bank. + :param data: Anything serializable by :mod:`salt.payload`. + :param expires: If a positive integer, the key is attached to an etcd + v3 lease with that TTL in seconds and etcd deletes it on expiry. + ``None``, ``0`` or a negative value stores without a lease. + :returns: ``None`` on success. + :raises salt.exceptions.SaltCacheError: On any etcd or serialization + error. + """ + _init_client() + etcd_key = _value_key(bank, key) + try: + payload = _pack(data) + if expires and int(expires) > 0: + lease = client.Lease(ttl=int(expires)) + lease.grant() + client.put(etcd_key, payload, lease=lease.ID) + else: + client.put(etcd_key, payload) + except Exception as exc: # pylint: disable=broad-except + raise SaltCacheError( + f"etcd3 cache: error writing key {etcd_key}: {exc}" + ) from exc + + +def fetch(bank, key): + """ + Return the data stored at ``bank/key``, or ``{}`` on a miss (the cache + contract every Salt backend honours). + + :raises salt.exceptions.SaltCacheError: On any etcd or deserialization + error. + """ + _init_client() + etcd_key = _value_key(bank, key) + try: + kvs = _range(etcd_key) + if not kvs: + return {} + return _unpack(kvs[0].value)[0] + except Exception as exc: # pylint: disable=broad-except + raise SaltCacheError( + f"etcd3 cache: error reading key {etcd_key}: {exc}" + ) from exc + + +def flush(bank, key=None): + """ + Remove ``bank/key``, or the entire ``bank`` (and sub-banks) when + ``key`` is ``None``, via a single etcd delete. + + :returns: ``True`` on success, including the no-op case where nothing + was deleted (idempotent; matches :mod:`salt.cache.redis_cache`). + :raises salt.exceptions.SaltCacheError: On any etcd error. + """ + _init_client() + try: + if key is None: + client.delete_range(_bank_prefix(bank), prefix=True) + else: + client.delete_range(_value_key(bank, key)) + return True + except Exception as exc: # pylint: disable=broad-except + raise SaltCacheError( + f"etcd3 cache: error flushing bank={bank} key={key}: {exc}" + ) from exc + + +def ls(bank): + """ + Return the immediate children of ``bank``: direct keys plus immediate + sub-bank names, deduplicated. Matches :mod:`salt.cache.localfs`. + + Uses a keys-only range scan, so listing a large bank does not transfer + the stored values. + + :param bank: Bank path. The empty bank ``""`` lists the top-level bank + names (the :func:`salt.runners.cache.migrate` case). + :returns: A ``list`` of child names; ``[]`` for an empty or nonexistent + bank. + :raises salt.exceptions.SaltCacheError: On any etcd error. + """ + _init_client() + prefix = _bank_prefix(bank) + try: + seen = set() + result = [] + for kv in _range(prefix, prefix=True, keys_only=True): + # etcd returns keys as bytes; the first path component after the + # bank prefix is either a direct key name or an immediate + # sub-bank name (deeper nesting collapses to the same name). + first = kv.key.decode("utf-8")[len(prefix) :].split("/", 1)[0] + if first and first not in seen: + seen.add(first) + result.append(first) + return result + except Exception as exc: # pylint: disable=broad-except + raise SaltCacheError(f"etcd3 cache: error listing bank {bank}: {exc}") from exc + + +def contains(bank, key): + """ + Return whether ``bank/key`` exists, or -- when ``key`` is ``None`` -- + whether anything exists under the bank prefix (matching + :mod:`salt.cache.localfs`'s ``os.path.isdir(bank)`` semantic). + + :raises salt.exceptions.SaltCacheError: On any etcd error. + """ + _init_client() + try: + if key is None: + target = _bank_prefix(bank) + return bool(_range(target, prefix=True, keys_only=True, limit=1)) + return bool(_range(_value_key(bank, key), keys_only=True, limit=1)) + except Exception as exc: # pylint: disable=broad-except + raise SaltCacheError( + f"etcd3 cache: error checking bank={bank} key={key}: {exc}" + ) from exc + + +def updated(bank, key): + """ + Return the Unix-epoch timestamp at which ``bank/key`` was last stored, + or ``None`` on a miss. + + :raises salt.exceptions.SaltCacheError: On any etcd error. + """ + _init_client() + etcd_key = _value_key(bank, key) + try: + kvs = _range(etcd_key) + if not kvs: + return None + return _unpack(kvs[0].value)[1] + except Exception as exc: # pylint: disable=broad-except + raise SaltCacheError( + f"etcd3 cache: error reading timestamp for {etcd_key}: {exc}" + ) from exc diff --git a/tests/pytests/functional/cache/test_etcd3.py b/tests/pytests/functional/cache/test_etcd3.py new file mode 100644 index 000000000000..2b3a54fc1cd8 --- /dev/null +++ b/tests/pytests/functional/cache/test_etcd3.py @@ -0,0 +1,54 @@ +""" +Functional test for salt.cache.etcd3_cache. + +Runs the canonical cross-backend contract suite from +``tests/pytests/functional/cache/helpers.py:run_common_cache_tests`` so +this driver is validated against the same contract as localfs, redis, +mysql, consul, and the v2 etcd driver. + +A throwaway etcd v3 container is started by the shared +``tests.support.pytest.etcd`` fixtures, so this runs in CI wherever +docker is available. +""" + +import uuid + +import pytest + +import salt.cache +import salt.cache.etcd3_cache as etcd3_cache +from tests.pytests.functional.cache.helpers import run_common_cache_tests +from tests.support.pytest.etcd import * # pylint: disable=wildcard-import,unused-wildcard-import + +docker = pytest.importorskip("docker") + +pytestmark = [ + pytest.mark.slow_test, + pytest.mark.skip_if_binaries_missing("dockerd"), + pytest.mark.skipif( + not etcd3_cache.HAS_ETCD, + reason="etcd3-py is not installed", + ), +] + + +@pytest.fixture(scope="module", params=(EtcdVersion.v3,), ids=etcd_version_ids) +def etcd_version(request): # pylint: disable=function-redefined + # The etcd3 cache only speaks the v3 API. + if not HAS_ETCD_V3: + pytest.skip("No etcd3 library installed") + return request.param + + +@pytest.fixture +def cache(minion_opts, etcd_port): + opts = minion_opts.copy() + opts["cache"] = "etcd3" + opts["etcd.host"] = "127.0.0.1" + opts["etcd.port"] = etcd_port + opts["etcd.path_prefix"] = f"/salt_cache_functional_{uuid.uuid4().hex}" + return salt.cache.factory(opts) + + +def test_caching(subtests, cache): + run_common_cache_tests(subtests, cache) diff --git a/tests/pytests/integration/cache/test_etcd3_cache.py b/tests/pytests/integration/cache/test_etcd3_cache.py new file mode 100644 index 000000000000..0dfddd21e49e --- /dev/null +++ b/tests/pytests/integration/cache/test_etcd3_cache.py @@ -0,0 +1,579 @@ +""" +Integration tests for ``salt.cache.etcd3_cache`` against a real etcd v3 +server, covering behaviour the (mocked) unit tests cannot: real +prefix-range semantics, the trailing-slash bank boundary, the single-key +storage model, native lease expiry, and the bank/key patterns used by +in-tree cache callers (auth tokens, pillar, master keys, grains). + +A throwaway etcd v3 container is started by the shared +``tests.support.pytest.etcd`` fixtures, so this runs in CI wherever +docker is available. Each test uses a unique ``etcd.path_prefix``. +""" + +import time +import uuid + +import pytest + +import salt.cache.etcd3_cache as etcd3_cache +import salt.payload +from tests.support.pytest.etcd import * # pylint: disable=wildcard-import,unused-wildcard-import + +docker = pytest.importorskip("docker") + +pytestmark = [ + pytest.mark.slow_test, + pytest.mark.skip_if_binaries_missing("dockerd"), + pytest.mark.skipif( + not etcd3_cache.HAS_ETCD, + reason="etcd3-py is not installed", + ), +] + + +@pytest.fixture(scope="module", params=(EtcdVersion.v3,), ids=etcd_version_ids) +def etcd_version(request): # pylint: disable=function-redefined + # The etcd3 cache only speaks the v3 API. + if not HAS_ETCD_V3: + pytest.skip("No etcd3 library installed") + return request.param + + +@pytest.fixture +def opts(etcd_port): + return { + "etcd.host": "127.0.0.1", + "etcd.port": etcd_port, + "etcd.path_prefix": f"/salt_cache_test_{uuid.uuid4().hex}", + } + + +@pytest.fixture +def cache(opts, monkeypatch): + monkeypatch.setattr(etcd3_cache, "client", None) + monkeypatch.setattr(etcd3_cache, "path_prefix", None) + monkeypatch.setattr(etcd3_cache, "__opts__", opts, raising=False) + yield etcd3_cache + # Best-effort cleanup of this test's prefix. + try: + if etcd3_cache.client is not None and etcd3_cache.path_prefix: + etcd3_cache.client.delete_range(etcd3_cache.path_prefix + "/", prefix=True) + except Exception: # pylint: disable=broad-except + pass + + +# --- core round-trip --------------------------------------------------------- + + +def test_store_then_fetch_roundtrip(cache): + data = {"minion_id": "minion-1", "grains": {"os": "Ubuntu", "n": 42}} + cache.store("minions/minion-1", "data", data) + assert cache.fetch("minions/minion-1", "data") == data + + +def test_store_overwrites_previous_value(cache): + cache.store("b", "k", {"v": 1}) + cache.store("b", "k", {"v": 2}) + assert cache.fetch("b", "k") == {"v": 2} + + +def test_fetch_miss_returns_empty_dict(cache): + assert cache.fetch("does/not", "exist") == {} + + +@pytest.mark.parametrize( + "value", + [ + None, + "", + "string", + b"bytes", + 0, + -1, + 1.5, + True, + False, + [1, 2, 3], + {"nested": {"deeply": {"yes": True}}}, + [{"a": 1}, {"b": 2}], + {"unicode": "héllo wörld 🥗"}, + ], + ids=lambda v: type(v).__name__ + "=" + repr(v)[:32], +) +def test_store_handles_serializable_types(cache, value): + cache.store("types", "k", value) + assert cache.fetch("types", "k") == value + + +# --- single-key storage model ----------------------------------------------- + + +def test_store_writes_exactly_one_etcd_key_per_entry(cache): + """ + Confirms the single-key design: ``store`` writes one etcd key per + cached entry, not a value + sibling-tstamp pair. Regression guard + if anyone ever reintroduces a companion key. + """ + cache.store("b", "k", {"v": 1}) + + raw = etcd3_cache.client + # Prefix-scan the bank; only the single value key should exist. + bank_resp = raw.range(f"{etcd3_cache.path_prefix}/b/", prefix=True) + assert bank_resp.kvs and len(bank_resp.kvs) == 1 + assert bank_resp.kvs[0].key.decode() == f"{etcd3_cache.path_prefix}/b/k" + + +def test_stored_value_is_msgpack_wrapped_not_base64(cache): + """ + On the wire, the value is ``salt.payload.dumps({"d": ..., "t": ...})`` + -- raw bytes, no base64 (v3 takes binary natively). + """ + cache.store("b", "k", {"v": 1}) + + raw = etcd3_cache.client + kv = raw.range(f"{etcd3_cache.path_prefix}/b/k").kvs[0] + payload = salt.payload.loads(kv.value) + assert payload["d"] == {"v": 1} + assert isinstance(payload["t"], int) + + +def test_updated_matches_stored_timestamp(cache): + """ + The timestamp returned by updated() is the same one embedded in the + stored value -- there's no second source of truth. + """ + cache.store("b", "k", {"v": 1}) + via_updated = cache.updated("b", "k") + + raw = etcd3_cache.client + kv = raw.range(f"{etcd3_cache.path_prefix}/b/k").kvs[0] + via_payload = salt.payload.loads(kv.value)["t"] + assert via_updated == via_payload + + +# --- native expiry via etcd v3 leases ---------------------------------------- + + +def test_store_with_expires_attaches_lease(cache): + """ + ``store(..., expires=N)`` must attach an etcd v3 lease so the key + is auto-deleted when the TTL elapses. + """ + cache.store("tokens", "tok1", {"u": "alice"}, expires=3600) + + raw = etcd3_cache.client + kv = raw.range(f"{etcd3_cache.path_prefix}/tokens/tok1").kvs[0] + assert kv.lease != 0, "lease should be attached when expires> 0" + + +def test_store_without_expires_has_no_lease(cache): + cache.store("bank", "k", "v") + raw = etcd3_cache.client + kv = raw.range(f"{etcd3_cache.path_prefix}/bank/k").kvs[0] + assert kv.lease == 0, "no lease should be attached without expires=" + + +def test_store_with_expires_actually_expires_the_key(cache): + """ + End-to-end check: a key written with a short TTL is gone from etcd + after the TTL elapses (no manual flush required). + """ + import time as _time + + cache.store("ephemeral", "k", "v", expires=2) + assert cache.fetch("ephemeral", "k") == "v" + + # etcd lease minimum is 1 second; give ourselves slack to avoid + # flakiness on a loaded test machine. + _time.sleep(4) + + assert cache.fetch("ephemeral", "k") == {} + assert cache.contains("ephemeral", "k") is False + + +# --- listing and containment ------------------------------------------------ + + +def test_ls_returns_direct_keys(cache): + cache.store("bank1", "k1", "v1") + cache.store("bank1", "k2", "v2") + assert sorted(cache.ls("bank1")) == ["k1", "k2"] + + +def test_ls_returns_immediate_sub_bank_names(cache): + """ + The reason for the localfs-style semantic: ``ls('minions')`` must + return the minion IDs, which is what callers iterating + ``minions/``-style banks need. v2 etcd_cache returned the + recursive leaves (``['data', 'data', ...]``) and redis_cache + returned ``[]``; both are wrong for this use case. + """ + cache.store("minions/m1", "data", {"a": 1}) + cache.store("minions/m2", "data", {"a": 2}) + cache.store("minions/m3", "data", {"a": 3}) + + assert sorted(cache.ls("minions")) == ["m1", "m2", "m3"] + # And the sub-banks themselves still report their direct keys. + assert cache.ls("minions/m1") == ["data"] + assert cache.ls("minions/m2") == ["data"] + + +def test_ls_returns_direct_keys_alongside_sub_banks(cache): + cache.store("bank", "direct1", "v1") + cache.store("bank", "direct2", "v2") + cache.store("bank/sub", "nested", "v3") + + assert sorted(cache.ls("bank")) == ["direct1", "direct2", "sub"] + assert cache.ls("bank/sub") == ["nested"] + + +def test_ls_empty_bank_returns_empty_list(cache): + assert cache.ls("never_stored") == [] + + +def test_ls_trailing_slash_boundary(cache): + """ + The single most important correctness test for a flat-keyspace port: + bank "foo" must not pick up data from bank "foobar". + """ + cache.store("foo", "k", "in_foo") + cache.store("foobar", "k", "in_foobar") + + assert cache.ls("foo") == ["k"] + assert cache.ls("foobar") == ["k"] + assert cache.fetch("foo", "k") == "in_foo" + assert cache.fetch("foobar", "k") == "in_foobar" + + +def test_ls_empty_bank_returns_top_level_banks(cache): + """ + ``cache.list("")`` must enumerate top-level banks. This is the + semantic that ``salt.runners.cache.migrate`` relies on; if it + returns ``[]`` the migration silently does nothing. + """ + cache.store("grains", "minion-1", {"os": "Linux"}) + cache.store("grains", "minion-2", {"os": "Linux"}) + cache.store("mine", "minion-1", {}) + cache.store("tokens", "abcdef", {}) + + assert sorted(cache.ls("")) == ["grains", "mine", "tokens"] + + +def test_contains_specific_key(cache): + cache.store("bank1", "key1", {"x": 1}) + assert cache.contains("bank1", "key1") is True + assert cache.contains("bank1", "missing") is False + assert cache.contains("nonexistent_bank", "key1") is False + + +def test_contains_bank_with_direct_key(cache): + cache.store("bank1", "key1", {"x": 1}) + assert cache.contains("bank1", None) is True + assert cache.contains("nonexistent_bank", None) is False + + +def test_contains_bank_with_only_sub_banks(cache): + # localfs-style: a bank that exists only as a parent of sub-banks + # still counts as "containing" something for the existence check. + cache.store("minions/m1", "data", {"a": 1}) + assert cache.contains("minions", None) is True + assert cache.contains("minions/m1", None) is True + + +def test_contains_bank_boundary(cache): + cache.store("foobar", "k", "v") + assert cache.contains("foo", None) is False + assert cache.contains("foobar", None) is True + + +# --- timestamps -------------------------------------------------------------- + + +def test_updated_returns_recent_timestamp(cache): + before = int(time.time()) + cache.store("b", "k", {"d": 1}) + after = int(time.time()) + + ts = cache.updated("b", "k") + assert ts is not None + assert before <= ts <= after + + +def test_updated_returns_none_when_absent(cache): + assert cache.updated("b", "never_stored") is None + + +def test_overwrite_updates_timestamp(cache): + cache.store("b", "k", "v1") + first_ts = cache.updated("b", "k") + + time.sleep(1.1) + + cache.store("b", "k", "v2") + second_ts = cache.updated("b", "k") + + assert second_ts > first_ts + assert cache.fetch("b", "k") == "v2" + + +# --- flushing ---------------------------------------------------------------- + + +def test_flush_key_removes_entry(cache): + cache.store("b", "k", {"d": 1}) + assert cache.fetch("b", "k") == {"d": 1} + assert cache.updated("b", "k") is not None + + assert cache.flush("b", "k") is True + assert cache.fetch("b", "k") == {} + assert cache.updated("b", "k") is None + + +def test_flush_bank_removes_all_entries(cache): + cache.store("b", "k1", "v1") + cache.store("b", "k2", "v2") + assert sorted(cache.ls("b")) == ["k1", "k2"] + + assert cache.flush("b") is True + assert cache.ls("b") == [] + assert cache.contains("b", None) is False + + +def test_flush_bank_does_not_affect_sibling_banks(cache): + cache.store("foo", "k", "in_foo") + cache.store("foobar", "k", "in_foobar") + + cache.flush("foo") + + assert cache.fetch("foo", "k") == {} + assert cache.fetch("foobar", "k") == "in_foobar" + + +def test_flush_nonexistent_bank_returns_true(cache): + # A flush of a bank that never existed is a successful no-op. + # We return True to match redis_cache/localfs so callers like + # salt.auth.del_token surface a consistent value regardless of + # whether the entry was actually present. + assert cache.flush("never_existed") is True + + +def test_flush_nonexistent_key_returns_true(cache): + assert cache.flush("never_existed", "k") is True + + +# --- realistic Salt usage ---------------------------------------------------- + + +def test_minion_data_pattern(cache): + """ + Mirrors how ``salt.utils.minions`` uses the cache: + cbank=``minions/``, ckey=``data``. The key correctness check + here is ``ls('minions')`` returning the minion IDs -- this is what + callers iterating cached minions need, and what v2 etcd_cache fails + to deliver. + """ + minion_ids = [f"minion-{i}" for i in range(5)] + + for mid in minion_ids: + cache.store( + f"minions/{mid}", + "data", + {"grains": {"id": mid, "os": "Linux"}}, + ) + + # ls('minions') must return the minion IDs (immediate sub-banks). + assert sorted(cache.ls("minions")) == sorted(minion_ids) + + for mid in minion_ids: + got = cache.fetch(f"minions/{mid}", "data") + assert got["grains"]["id"] == mid + + for mid in minion_ids: + assert cache.contains(f"minions/{mid}", None) is True + # The parent bank exists as long as any sub-bank does. + assert cache.contains("minions", None) is True + + cache.flush(f"minions/{minion_ids[0]}") + assert cache.fetch(f"minions/{minion_ids[0]}", "data") == {} + # The flushed minion drops out of the listing. + assert sorted(cache.ls("minions")) == sorted(minion_ids[1:]) + for mid in minion_ids[1:]: + assert cache.fetch(f"minions/{mid}", "data")["grains"]["id"] == mid + + +# --- shared-cluster co-tenancy ----------------------------------------------- + + +def test_flush_does_not_touch_keys_outside_path_prefix(cache): + """ + Real-world deployment safety check: this cache is expected to be + pointed at etcd clusters shared with Patroni, Kubernetes, etc. + A flush() call must never delete keys outside the configured + ``etcd.path_prefix``, no matter what bank name is passed. + """ + # Plant some "foreign tenant" data alongside ours, simulating + # Patroni-style keys under a sibling prefix. + raw = etcd3_cache.client + foreign_prefix = "/service/test-patroni" + foreign_keys = [ + f"{foreign_prefix}/leader", + f"{foreign_prefix}/members/node-1", + f"{foreign_prefix}/config", + ] + try: + for k in foreign_keys: + raw.put(k, b"foreign-data") + + # Sanity: foreign keys are there. + for k in foreign_keys: + assert raw.range(k).kvs, f"foreign key {k} did not land" + + # Store something in our cache and then flush aggressively. + cache.store("bank1", "k1", "v1") + cache.store("bank2", "k2", "v2") + + cache.flush("bank1") + cache.flush("bank2") + # Even a flush with a bank name that overlaps the foreign + # tenant's path component must not reach the foreign tenant. + cache.flush("service") # not a real bank; would only hit our prefix + cache.flush("service/test-patroni") + + # Foreign keys must still be intact. + for k in foreign_keys: + resp = raw.range(k) + assert resp.kvs, f"flush() reached foreign key {k}" + assert resp.kvs[0].value == b"foreign-data" + finally: + # Clean up the foreign keys regardless of test outcome. + try: + raw.delete_range(foreign_prefix + "/", prefix=True) + except Exception: # pylint: disable=broad-except + pass + + +# --- direct cache users (auth, pillar, channel, key, etc.) ------------------- +# +# These tests exercise the actual bank/key patterns used by in-tree +# modules that call salt.cache.Cache directly, not via the minion data +# cache contract. The goal is to confirm the driver supports these +# patterns naturally, without ``etcd``-specific contortions. + + +def test_auth_token_pattern(cache): + """ + ``salt.auth`` stores tokens at ``tokens/``. The ``Cache`` + wrapper handles ``expires=`` automatically for drivers that don't + accept it natively, so we don't need to test that here; we only + confirm the bank/key shape works. + """ + cache.store("tokens", "deadbeef" * 8, {"name": "alice", "perms": ["*"]}) + cache.store("tokens", "cafebabe" * 8, {"name": "bob", "perms": ["test.*"]}) + + assert sorted(cache.ls("tokens")) == sorted(["deadbeef" * 8, "cafebabe" * 8]) + assert cache.fetch("tokens", "deadbeef" * 8)["name"] == "alice" + assert cache.contains("tokens", "cafebabe" * 8) is True + + +def test_pillar_pattern(cache): + """ + ``salt.pillar`` stores at ``pillar/``. Pillar data can + be large; this test uses a modest payload but exercises the shape. + """ + pillar_data = { + "users": {f"user{i}": {"uid": 1000 + i} for i in range(50)}, + "services": ["nginx", "postgres", "redis"], + } + cache.store("pillar", "minion-1:base", pillar_data) + assert cache.fetch("pillar", "minion-1:base") == pillar_data + + cache.flush("pillar", "minion-1:base") + assert cache.fetch("pillar", "minion-1:base") == {} + + +def test_master_keys_pattern(cache): + """ + ``salt.crypt`` and ``salt.key`` store cluster master keys under + ``master_keys/``, where filenames can contain dots + (e.g. ``master.pem``, ``master.pub``, ``.pem``). + """ + cache.store("master_keys", "master.pem", b"-----BEGIN PRIVATE KEY-----") + cache.store("master_keys", "master.pub", b"-----BEGIN PUBLIC KEY-----") + cache.store("master_keys", "peer-id.pub", b"-----BEGIN PUBLIC KEY-----") + + assert cache.contains("master_keys", "master.pem") is True + assert sorted(cache.ls("master_keys")) == sorted( + ["master.pem", "master.pub", "peer-id.pub"] + ) + + +def test_grains_iteration_pattern(cache): + """ + ``salt.utils.minions`` and ``salt.utils.master`` enumerate cached + minions via ``cache.list("grains")`` and then ``cache.fetch("grains", + minion_id)``. This is the dominant in-tree pattern -- flat keys + under a bank, not nested sub-banks. + """ + minion_ids = [f"node-{i}.example.com" for i in range(5)] + for mid in minion_ids: + cache.store("grains", mid, {"id": mid, "os": "Debian"}) + + listed = cache.ls("grains") + assert sorted(listed) == sorted(minion_ids) + for mid in listed: + grains = cache.fetch("grains", mid) + assert grains["id"] == mid + + +def test_spm_dot_bank_pattern(cache): + """ + ``salt.spm`` uses ``cache.store(".", repo, metadata)``. The bank + being a single ``.`` is unusual but legitimate; the driver must + treat it as an opaque path component. + """ + cache.store(".", "myrepo", {"packages": ["pkg1", "pkg2"]}) + cache.store(".", "otherrepo", {"packages": ["pkg3"]}) + + assert sorted(cache.ls(".")) == ["myrepo", "otherrepo"] + assert cache.fetch(".", "myrepo")["packages"] == ["pkg1", "pkg2"] + assert cache.updated(".", "myrepo") is not None + + +def test_migration_runner_walk_pattern(cache): + """ + ``salt.runners.cache.migrate`` walks the cache via: + + for bank in cache.list(""): + for name in cache.list(bank): + if cache.contains(bank, name): + # name is a value key -- fetch and copy + else: + # name is a sub-bank -- recurse + + Confirm the discriminator works on our driver: ``contains(bank, + name)`` is True for direct value keys and False for sub-bank names. + """ + # Mix of flat-bank and nested-bank patterns, like a real master. + cache.store("grains", "node-a", {"os": "Linux"}) + cache.store("grains", "node-b", {"os": "Linux"}) + cache.store("tokens", "tok1", {"u": "alice"}) + cache.store("minions/node-a", "data", {"mine": {}}) + + top_level = sorted(cache.ls("")) + assert top_level == ["grains", "minions", "tokens"] + + # Under "grains", every listed name is a value key. + for name in cache.ls("grains"): + assert ( + cache.contains("grains", name) is True + ), f"grains/{name} should be a value key" + + # Under "minions", the listed name ("node-a") is a sub-bank, not + # a value key. The migrator relies on this distinction to recurse. + for name in cache.ls("minions"): + assert ( + cache.contains("minions", name) is False + ), f"minions/{name} should be a sub-bank, not a value key" + # The sub-bank itself has the value key. + sub = f"minions/{name}" + assert cache.contains(sub, "data") is True diff --git a/tests/pytests/unit/cache/test_etcd3_cache.py b/tests/pytests/unit/cache/test_etcd3_cache.py new file mode 100644 index 000000000000..a87d01e12ddb --- /dev/null +++ b/tests/pytests/unit/cache/test_etcd3_cache.py @@ -0,0 +1,459 @@ +""" +Unit tests for salt.cache.etcd3_cache. + +These mock the underlying etcd3-py client (``etcd3_cache.client``) and +assert the module issues the right calls. The single-key storage model +(value and timestamp packed together as +``salt.payload.dumps({"d": data, "t": epoch})``) means a successful +``store`` is one ``client.put`` call, with an optional lease for +``expires=``. Listing uses keys-only range scans. + +For end-to-end correctness against a real etcd, see the functional and +integration tests. +""" + +import pytest + +import salt.cache.etcd3_cache as etcd3_cache +import salt.payload +from salt.exceptions import SaltCacheError +from tests.support.mock import MagicMock, patch + +pytestmark = [ + pytest.mark.skipif( + not etcd3_cache.HAS_ETCD, + reason="Install etcd3-py for this test", + ), +] + + +class FakeKv: + """ + Minimal stand-in for an etcd3 KeyValue. etcd returns keys and values + as bytes; keys-only scans leave ``value`` empty. + """ + + def __init__(self, key=b"", value=None): + self.key = key + self.value = value + + +def _range(kvs): + """A fake etcd3-py range response whose ``.kvs`` is ``kvs``.""" + return MagicMock(kvs=kvs) + + +@pytest.fixture +def configure_loader_modules(): + return { + etcd3_cache: { + "__opts__": { + "etcd.host": "127.0.0.1", + "etcd.port": 2379, + } + } + } + + +@pytest.fixture +def client(): + """ + Patch in a mocked, already-initialized etcd3-py client so the cache + functions skip ``_init_client()`` and operate against a known prefix. + """ + fake = MagicMock() + with patch.object(etcd3_cache, "client", fake), patch.object( + etcd3_cache, "path_prefix", "/salt/cache" + ): + yield fake + + +# --- __virtual__ ------------------------------------------------------------- + + +def test_virtual_returns_name_when_etcd_installed(): + with patch.object(etcd3_cache, "HAS_ETCD", True): + assert etcd3_cache.__virtual__() == "etcd3" + + +def test_virtual_returns_false_tuple_when_etcd_missing(): + with patch.object(etcd3_cache, "HAS_ETCD", False): + result = etcd3_cache.__virtual__() + assert result[0] is False + assert "etcd3-py" in result[1] + + +# --- init_kwargs ------------------------------------------------------------- + + +def test_init_kwargs_is_noop(): + assert etcd3_cache.init_kwargs({"anything": 1}) == {} + + +# --- _init_client ------------------------------------------------------------ + + +@pytest.mark.parametrize("bad_prefix", ["", "/", "//", "///", None]) +def test_init_client_rejects_empty_or_root_path_prefix(bad_prefix): + with patch.dict( + etcd3_cache.__opts__, {"etcd.path_prefix": bad_prefix}, clear=True + ), patch.object(etcd3_cache, "client", None), patch( + "salt.utils.etcd_util.EtcdClientV3" + ) as client_cls: + with pytest.raises(SaltCacheError, match="path_prefix"): + etcd3_cache._init_client() + client_cls.assert_not_called() + + +def test_init_client_normalises_path_prefix_and_strips_outer_slashes(): + with patch.dict( + etcd3_cache.__opts__, {"etcd.path_prefix": "/my/prefix/"}, clear=True + ), patch.object(etcd3_cache, "client", None), patch.object( + etcd3_cache, "path_prefix", None + ), patch( + "salt.utils.etcd_util._get_etcd_opts", return_value={} + ), patch( + "salt.utils.etcd_util.EtcdClientV3" + ): + etcd3_cache._init_client() + assert etcd3_cache.path_prefix == "/my/prefix" + + +def test_init_client_forces_require_v2_off_in_resolved_conf(): + """ + The cache is inherently v3; it must force ``etcd.require_v2`` off in + the resolved conf so EtcdClientV3 constructs (it raises otherwise) and + the user does not have to set a "require v2" option to use it. + """ + with patch.dict( + etcd3_cache.__opts__, {"etcd.path_prefix": "/test_cache"}, clear=True + ), patch.object(etcd3_cache, "client", None), patch.object( + etcd3_cache, "path_prefix", None + ), patch( + "salt.utils.etcd_util._get_etcd_opts", return_value={"etcd.host": "h"} + ), patch( + "salt.utils.etcd_util.EtcdClientV3" + ) as client_cls: + etcd3_cache._init_client() + conf = client_cls.call_args.args[0] + assert conf["etcd.require_v2"] is False + assert client_cls.call_args.kwargs.get("has_etcd_opts") is True + # The module operates on the raw etcd3-py client, not the wrapper. + assert etcd3_cache.client is client_cls.return_value.client + + +def test_init_client_resolves_cache_profile(): + with patch.dict( + etcd3_cache.__opts__, + {"etcd.path_prefix": "/test_cache", "etcd.cache_profile": "my_profile"}, + clear=True, + ), patch.object(etcd3_cache, "client", None), patch.object( + etcd3_cache, "path_prefix", None + ), patch( + "salt.utils.etcd_util._get_etcd_opts", return_value={} + ) as get_opts, patch( + "salt.utils.etcd_util.EtcdClientV3" + ): + etcd3_cache._init_client() + # Profile is passed through to the shared opts resolver. + assert get_opts.call_args.args[1] == "my_profile" + + +def test_init_client_is_idempotent(): + sentinel = MagicMock() + with patch.object(etcd3_cache, "client", sentinel): + etcd3_cache._init_client() + assert etcd3_cache.client is sentinel + + +# --- _pack / _unpack --------------------------------------------------------- + + +def test_pack_wraps_data_and_timestamp(): + obj = salt.payload.loads(etcd3_cache._pack({"a": 1})) + assert obj["d"] == {"a": 1} + assert isinstance(obj["t"], int) + assert obj["t"] > 0 + + +def test_unpack_returns_data_and_timestamp(): + data, ts = etcd3_cache._unpack(etcd3_cache._pack({"hello": "world"})) + assert data == {"hello": "world"} + assert isinstance(ts, int) + + +# --- store ------------------------------------------------------------------- + + +def test_store_writes_single_packed_key(client): + etcd3_cache.store("bank", "key", {"a": 1}) + + client.put.assert_called_once() + call = client.put.call_args + assert call.args[0] == "/salt/cache/bank/key" + payload = salt.payload.loads(call.args[1]) + assert payload["d"] == {"a": 1} + assert isinstance(payload["t"], int) + assert "lease" not in call.kwargs + + +def test_store_with_expires_attaches_lease(client): + fake_lease = MagicMock() + fake_lease.ID = 0xDEADBEEF + client.Lease.return_value = fake_lease + + etcd3_cache.store("tokens", "abc", {"u": "alice"}, expires=3600) + + client.Lease.assert_called_once_with(ttl=3600) + fake_lease.grant.assert_called_once() + put_call = client.put.call_args + assert put_call.args[0] == "/salt/cache/tokens/abc" + assert put_call.kwargs.get("lease") == 0xDEADBEEF + + +@pytest.mark.parametrize("falsy_expires", [None, 0, False, -1]) +def test_store_non_positive_expires_does_not_create_lease(client, falsy_expires): + etcd3_cache.store("bank", "key", "v", expires=falsy_expires) + client.Lease.assert_not_called() + assert "lease" not in client.put.call_args.kwargs + + +def test_store_handles_complex_serializable_types(client): + data = {"int": 42, "list": [1, 2, 3], "nested": {"a": "b"}, "bytes": b"\x00\xff"} + etcd3_cache.store("bank", "key", data) + payload = salt.payload.loads(client.put.call_args.args[1]) + assert payload["d"] == data + + +def test_store_nested_bank_path(client): + etcd3_cache.store("minions/node-a", "data", {"x": 1}) + assert client.put.call_args.args[0] == "/salt/cache/minions/node-a/data" + + +def test_store_error_wraps_as_saltcacheerror(client): + client.put.side_effect = Exception("boom") + with pytest.raises(SaltCacheError, match="error writing key"): + etcd3_cache.store("bank", "key", "v") + + +# --- fetch ------------------------------------------------------------------- + + +def test_fetch_returns_unwrapped_data(client): + payload = etcd3_cache._pack({"a": 1, "b": [1, 2]}) + client.range.return_value = _range([FakeKv(value=payload)]) + assert etcd3_cache.fetch("bank", "key") == {"a": 1, "b": [1, 2]} + + +def test_fetch_uses_value_key_path(client): + client.range.return_value = _range(None) + etcd3_cache.fetch("bank", "key") + assert client.range.call_args.args[0] == "/salt/cache/bank/key" + + +def test_fetch_miss_returns_empty_dict(client): + client.range.return_value = _range(None) + assert etcd3_cache.fetch("bank", "key") == {} + + +def test_fetch_error_wraps_as_saltcacheerror(client): + client.range.side_effect = Exception("boom") + with pytest.raises(SaltCacheError, match="error reading key"): + etcd3_cache.fetch("bank", "key") + + +# --- flush ------------------------------------------------------------------- + + +def test_flush_key_deletes_single_key(client): + assert etcd3_cache.flush("bank", "key") is True + client.delete_range.assert_called_once_with("/salt/cache/bank/key") + + +def test_flush_bank_uses_prefix_range_delete(client): + assert etcd3_cache.flush("bank", None) is True + client.delete_range.assert_called_once_with("/salt/cache/bank/", prefix=True) + + +def test_flush_bank_defaults_to_full_bank(client): + assert etcd3_cache.flush("bank") is True + client.delete_range.assert_called_once_with("/salt/cache/bank/", prefix=True) + + +def test_flush_returns_true_even_when_nothing_existed(client): + assert etcd3_cache.flush("nonexistent", "key") is True + assert etcd3_cache.flush("nonexistent", None) is True + + +def test_flush_error_wraps_as_saltcacheerror(client): + client.delete_range.side_effect = Exception("boom") + with pytest.raises(SaltCacheError, match="error flushing"): + etcd3_cache.flush("bank", "key") + + +# --- ls ---------------------------------------------------------------------- + + +def test_ls_returns_direct_keys(client): + client.range.return_value = _range( + [ + FakeKv(key=b"/salt/cache/bank/minion1"), + FakeKv(key=b"/salt/cache/bank/minion2"), + ] + ) + assert sorted(etcd3_cache.ls("bank")) == ["minion1", "minion2"] + + +def test_ls_returns_immediate_sub_bank_names(client): + """ + bank="minions" with data at minions/m1/data and minions/m2/data should + yield the minion IDs -- what salt.utils.master._get_cached_minion_data + and similar callers expect. + """ + client.range.return_value = _range( + [ + FakeKv(key=b"/salt/cache/minions/m1/data"), + FakeKv(key=b"/salt/cache/minions/m2/data"), + ] + ) + assert sorted(etcd3_cache.ls("minions")) == ["m1", "m2"] + + +def test_ls_dedupes_sub_bank_names_across_descendants(client): + client.range.return_value = _range( + [ + FakeKv(key=b"/salt/cache/bank/sub/k1"), + FakeKv(key=b"/salt/cache/bank/sub/k2"), + FakeKv(key=b"/salt/cache/bank/sub/deeper/k3"), + ] + ) + assert etcd3_cache.ls("bank") == ["sub"] + + +def test_ls_returns_direct_keys_alongside_sub_banks(client): + client.range.return_value = _range( + [ + FakeKv(key=b"/salt/cache/bank/direct1"), + FakeKv(key=b"/salt/cache/bank/direct2"), + FakeKv(key=b"/salt/cache/bank/sub/nested"), + ] + ) + assert sorted(etcd3_cache.ls("bank")) == ["direct1", "direct2", "sub"] + + +def test_ls_uses_keys_only_prefix_scan_with_trailing_slash(client): + client.range.return_value = _range(None) + etcd3_cache.ls("bank") + call = client.range.call_args + assert call.args[0] == "/salt/cache/bank/" + assert call.kwargs.get("prefix") is True + assert call.kwargs.get("keys_only") is True + + +def test_ls_empty_bank_returns_top_level_banks(client): + """ + ``cache.list("")`` enumerates top-level banks (salt.runners.cache.migrate). + """ + client.range.return_value = _range( + [ + FakeKv(key=b"/salt/cache/grains/minion-1"), + FakeKv(key=b"/salt/cache/grains/minion-2"), + FakeKv(key=b"/salt/cache/mine/minion-1"), + FakeKv(key=b"/salt/cache/tokens/abcdef"), + ] + ) + assert sorted(etcd3_cache.ls("")) == ["grains", "mine", "tokens"] + + +def test_ls_empty_bank_scans_root_prefix_without_double_slash(client): + client.range.return_value = _range(None) + etcd3_cache.ls("") + assert client.range.call_args.args[0] == "/salt/cache/" + + +def test_ls_missing_returns_empty(client): + client.range.return_value = _range(None) + assert etcd3_cache.ls("bank") == [] + + +def test_ls_error_wraps_as_saltcacheerror(client): + client.range.side_effect = Exception("boom") + with pytest.raises(SaltCacheError, match="error listing"): + etcd3_cache.ls("bank") + + +# --- contains ---------------------------------------------------------------- + + +def test_contains_existing_key(client): + client.range.return_value = _range([FakeKv(key=b"/salt/cache/bank/key")]) + assert etcd3_cache.contains("bank", "key") is True + + +def test_contains_missing_key(client): + client.range.return_value = _range(None) + assert etcd3_cache.contains("bank", "key") is False + + +def test_contains_key_uses_exact_keys_only_limited_read(client): + client.range.return_value = _range(None) + etcd3_cache.contains("bank", "key") + call = client.range.call_args + assert call.args[0] == "/salt/cache/bank/key" + assert call.kwargs.get("prefix") in (None, False) + assert call.kwargs.get("keys_only") is True + assert call.kwargs.get("limit") == 1 + + +def test_contains_bank_with_only_sub_bank_entries(client): + client.range.return_value = _range([FakeKv(key=b"/salt/cache/bank/sub/nested")]) + assert etcd3_cache.contains("bank", None) is True + + +def test_contains_missing_bank(client): + client.range.return_value = _range(None) + assert etcd3_cache.contains("bank", None) is False + + +def test_contains_bank_uses_keys_only_limited_prefix_scan(client): + client.range.return_value = _range(None) + etcd3_cache.contains("bank", None) + call = client.range.call_args + assert call.args[0] == "/salt/cache/bank/" + assert call.kwargs.get("prefix") is True + assert call.kwargs.get("keys_only") is True + assert call.kwargs.get("limit") == 1 + + +def test_contains_error_wraps_as_saltcacheerror(client): + client.range.side_effect = Exception("boom") + with pytest.raises(SaltCacheError, match="error checking"): + etcd3_cache.contains("bank", "key") + + +# --- updated ----------------------------------------------------------------- + + +def test_updated_returns_packed_timestamp(client): + payload = etcd3_cache._pack({"some": "data"}) + client.range.return_value = _range([FakeKv(value=payload)]) + ts = etcd3_cache.updated("bank", "key") + assert isinstance(ts, int) + assert ts > 0 + + +def test_updated_reads_value_key_not_sibling(client): + client.range.return_value = _range(None) + etcd3_cache.updated("bank", "key") + assert client.range.call_args.args[0] == "/salt/cache/bank/key" + + +def test_updated_returns_none_when_absent(client): + client.range.return_value = _range(None) + assert etcd3_cache.updated("bank", "key") is None + + +def test_updated_error_wraps_as_saltcacheerror(client): + client.range.side_effect = Exception("boom") + with pytest.raises(SaltCacheError, match="error reading timestamp"): + etcd3_cache.updated("bank", "key") From ec0e246c2ff7a31ce81356d6766aac5d2987f44b Mon Sep 17 00:00:00 2001 From: "Gary T. Giesen" Date: Mon, 8 Jun 2026 15:32:23 -0400 Subject: [PATCH 2/3] Sort cache autosummary alphabetically (etcd3_cache before etcd_cache) --- doc/ref/cache/all/index.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/ref/cache/all/index.rst b/doc/ref/cache/all/index.rst index 3465de85e85b..07a3799d659f 100644 --- a/doc/ref/cache/all/index.rst +++ b/doc/ref/cache/all/index.rst @@ -13,8 +13,8 @@ For understanding and usage of the cache modules see the :ref:`cache` topic. :template: autosummary.rst.tmpl consul - etcd_cache etcd3_cache + etcd_cache localfs localfs_key mmap_cache From 1e432154c316544335e415447996c34bd677368b Mon Sep 17 00:00:00 2001 From: "Gary T. Giesen" Date: Tue, 9 Jun 2026 09:07:58 -0400 Subject: [PATCH 3/3] Fix two etcd3_cache integration test bugs test_store_handles_serializable_types parametrized bytes round-trip: salt.payload.loads with default encoding=None applies decode_embedded_strs, which converts every bytes value to str. Every salt cache backend that goes through salt.payload (redis, mysql, localfs, ...) inherits this behaviour, and no real cache caller stores raw bytes, so the assertion was a stricter contract than the rest of salt.cache. Drop b'bytes' from the parametrize list and document why. test_flush_does_not_touch_keys_outside_path_prefix raw-client access: the test reaches into etcd3_cache.client directly to plant foreign data before any cache call has been issued. The module-level client is initialized lazily on the first store/fetch, so on an isolated run (e.g. 'pytest --lf') the test saw client is None and failed with AttributeError. Have the cache fixture call _init_client() up front so direct raw-client access is independent of test ordering. --- tests/pytests/integration/cache/test_etcd3_cache.py | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/tests/pytests/integration/cache/test_etcd3_cache.py b/tests/pytests/integration/cache/test_etcd3_cache.py index 0dfddd21e49e..35d2db545a6c 100644 --- a/tests/pytests/integration/cache/test_etcd3_cache.py +++ b/tests/pytests/integration/cache/test_etcd3_cache.py @@ -53,6 +53,11 @@ def cache(opts, monkeypatch): monkeypatch.setattr(etcd3_cache, "client", None) monkeypatch.setattr(etcd3_cache, "path_prefix", None) monkeypatch.setattr(etcd3_cache, "__opts__", opts, raising=False) + # Eagerly initialize so tests that poke ``etcd3_cache.client`` directly + # (rather than going through store/fetch first) do not depend on test + # ordering -- isolated runs via ``pytest --lf`` would otherwise see + # ``client is None``. + etcd3_cache._init_client() yield etcd3_cache # Best-effort cleanup of this test's prefix. try: @@ -81,13 +86,18 @@ def test_fetch_miss_returns_empty_dict(cache): assert cache.fetch("does/not", "exist") == {} +# ``bytes`` is deliberately excluded: ``salt.payload.loads`` calls +# ``decode_embedded_strs`` on the default code path, which converts every +# ``bytes`` value to ``str``. Every salt cache backend that goes through +# ``salt.payload`` (redis, mysql, localfs, ...) inherits this behavior, so +# real cache callers do not store raw bytes. Asserting bytes preservation +# here would be a stricter contract than the rest of ``salt.cache``. @pytest.mark.parametrize( "value", [ None, "", "string", - b"bytes", 0, -1, 1.5,