diff --git a/changelog/69734.fixed.md b/changelog/69734.fixed.md new file mode 100644 index 00000000000..9ef0eb2fc9c --- /dev/null +++ b/changelog/69734.fixed.md @@ -0,0 +1 @@ +Fixed ``cp._client`` raising ``LoaderError`` (surfaced as ``KeyError: '__file_client__'``) when the executing loader has not packed a ``__file_client__`` context. It now falls back to building a file client from ``__opts__``, so ``cp.cache_file`` and other ``salt://`` fetches work under loaders that do not pack a file client. diff --git a/salt/modules/cp.py b/salt/modules/cp.py index 2d878a308b6..913c4214570 100644 --- a/salt/modules/cp.py +++ b/salt/modules/cp.py @@ -20,7 +20,7 @@ import salt.utils.path import salt.utils.templates import salt.utils.url -from salt.exceptions import CommandExecutionError +from salt.exceptions import CommandExecutionError, LoaderError from salt.loader.context import NamedLoaderContext from salt.loader.dunder import ( __context__, @@ -173,8 +173,20 @@ def _client(): If the __file_client__ context is set return it, otherwize create a new file client using __opts__. """ - if __file_client__: - return __file_client__.value() + # ``__file_client__`` is a NamedLoaderContext. When the loader executing + # this module has not packed a file client (e.g. loaders other than + # minion_mods, such as the resource module loader), evaluating the context + # raises ``LoaderError`` instead of yielding a falsey value -- so the + # ``__opts__`` fallback below was never reached and callers such as + # ``cp.cache_file`` (and therefore every ``salt://`` fetch) failed with + # ``KeyError: '__file_client__'``. Guard the lookup so a missing/None + # context falls back to building a client from ``__opts__``. + try: + file_client = __file_client__.value() + except LoaderError: + file_client = None + if file_client: + return file_client return salt.fileclient.get_file_client(__opts__.value()) diff --git a/tests/pytests/unit/modules/test_cp.py b/tests/pytests/unit/modules/test_cp.py index b198381c9a7..3b905357db8 100644 --- a/tests/pytests/unit/modules/test_cp.py +++ b/tests/pytests/unit/modules/test_cp.py @@ -9,7 +9,7 @@ import salt.utils.files import salt.utils.platform import salt.utils.templates as templates -from salt.exceptions import CommandExecutionError +from salt.exceptions import CommandExecutionError, LoaderError from tests.support.mock import MagicMock, Mock, mock_open, patch @@ -18,6 +18,41 @@ def configure_loader_modules(): return {cp: {"__opts__": {"saltenv": None}}} +def test__client_returns_packed_file_client(): + """ + _client() returns the file client from the __file_client__ context when + one is packed. + """ + packed_client = Mock() + ctx = MagicMock() + ctx.value.return_value = packed_client + with patch.object(cp, "__file_client__", ctx, create=True): + with patch("salt.fileclient.get_file_client") as get_file_client: + assert cp._client() is packed_client + get_file_client.assert_not_called() + + +def test__client_falls_back_when_file_client_not_packed(): + """ + When the executing loader has not packed __file_client__, evaluating the + context raises LoaderError. _client() must fall back to building a client + from __opts__ instead of propagating the error. + """ + opts = {"saltenv": None} + opts_ctx = MagicMock() + opts_ctx.value.return_value = opts + file_client_ctx = MagicMock() + file_client_ctx.value.side_effect = LoaderError("__file_client__ not packed") + built_client = Mock() + with patch.object(cp, "__file_client__", file_client_ctx, create=True): + with patch.object(cp, "__opts__", opts_ctx, create=True): + with patch( + "salt.fileclient.get_file_client", return_value=built_client + ) as get_file_client: + assert cp._client() is built_client + get_file_client.assert_called_once_with(opts) + + def test__render_filenames_undefined_template(): """ Test if _render_filenames fails upon getting a template not in