From 82f90e2f15cb93fd7094a04b1493f74450b39d0c Mon Sep 17 00:00:00 2001 From: jeanluc Date: Mon, 6 Nov 2023 12:37:23 +0100 Subject: [PATCH 01/13] Add state.test to SSH wrapper --- changelog/61100.fixed.md | 1 + salt/client/ssh/wrapper/state.py | 15 ++++ tests/pytests/integration/ssh/conftest.py | 69 +++++++++++++++++++ .../integration/ssh/state/test_state.py | 10 +++ 4 files changed, 95 insertions(+) create mode 100644 changelog/61100.fixed.md create mode 100644 tests/pytests/integration/ssh/conftest.py diff --git a/changelog/61100.fixed.md b/changelog/61100.fixed.md new file mode 100644 index 000000000000..d7ac2b6bc3f6 --- /dev/null +++ b/changelog/61100.fixed.md @@ -0,0 +1 @@ +Fixed state.test does not work with salt-ssh diff --git a/salt/client/ssh/wrapper/state.py b/salt/client/ssh/wrapper/state.py index 8e059f2532dc..b2cfdaf67e45 100644 --- a/salt/client/ssh/wrapper/state.py +++ b/salt/client/ssh/wrapper/state.py @@ -1242,3 +1242,18 @@ def single(fun, name, test=None, **kwargs): pass return {"local": salt.client.ssh.wrapper.parse_ret(stdout, stderr, retcode)} + + +def test(*args, **kwargs): + """ + .. versionadded:: 3001 + + Alias for `state.apply` with the kwarg `test` forced to `True`. + + This is a nicety to avoid the need to type out `test=True` and the possibility of + a typo causing changes you do not intend. + """ + kwargs["test"] = True + ret = apply_(*args, **kwargs) + + return ret diff --git a/tests/pytests/integration/ssh/conftest.py b/tests/pytests/integration/ssh/conftest.py new file mode 100644 index 000000000000..0ecc58a6d759 --- /dev/null +++ b/tests/pytests/integration/ssh/conftest.py @@ -0,0 +1,69 @@ +import pytest + + +@pytest.fixture(scope="module") +def state_tree(base_env_state_tree_root_dir): + top_file = """ + {%- from "map.jinja" import abc with context %} + base: + 'localhost': + - basic + '127.0.0.1': + - basic + """ + map_file = """ + {%- set abc = "def" %} + """ + state_file = """ + {%- from "map.jinja" import abc with context %} + Ok with {{ abc }}: + test.succeed_with_changes + """ + top_tempfile = pytest.helpers.temp_file( + "top.sls", top_file, base_env_state_tree_root_dir + ) + map_tempfile = pytest.helpers.temp_file( + "map.jinja", map_file, base_env_state_tree_root_dir + ) + state_tempfile = pytest.helpers.temp_file( + "test.sls", state_file, base_env_state_tree_root_dir + ) + with top_tempfile, map_tempfile, state_tempfile: + yield + + +@pytest.fixture(scope="module") +def state_tree_dir(base_env_state_tree_root_dir): + """ + State tree with files to test salt-ssh + when the map.jinja file is in another directory + """ + top_file = """ + {%- from "test/map.jinja" import abc with context %} + base: + 'localhost': + - test + '127.0.0.1': + - test + """ + map_file = """ + {%- set abc = "def" %} + """ + state_file = """ + {%- from "test/map.jinja" import abc with context %} + + Ok with {{ abc }}: + test.succeed_without_changes + """ + top_tempfile = pytest.helpers.temp_file( + "top.sls", top_file, base_env_state_tree_root_dir + ) + map_tempfile = pytest.helpers.temp_file( + "test/map.jinja", map_file, base_env_state_tree_root_dir + ) + state_tempfile = pytest.helpers.temp_file( + "test.sls", state_file, base_env_state_tree_root_dir + ) + + with top_tempfile, map_tempfile, state_tempfile: + yield diff --git a/tests/pytests/integration/ssh/state/test_state.py b/tests/pytests/integration/ssh/state/test_state.py index 62e8cbf513b1..a7ebb22a601b 100644 --- a/tests/pytests/integration/ssh/state/test_state.py +++ b/tests/pytests/integration/ssh/state/test_state.py @@ -101,3 +101,13 @@ def test_state_high(salt_ssh_cli): """ ret = salt_ssh_cli.run("state.high", '{"echo blah": {"cmd": ["run"]}}') assert ret.data["cmd_|-echo blah_|-echo blah_|-run"]["changes"]["stdout"] == "blah" + + +def test_state_test(salt_ssh_cli, state_tree): + ret = salt_ssh_cli.run("state.test", "test") + assert ret.returncode == 0 + assert ret.data + assert ( + ret.data["test_|-Ok with def_|-Ok with def_|-succeed_with_changes"]["result"] + is None + ) From 8356be888bf32e2f4b081c54a6a56b21d5cf833c Mon Sep 17 00:00:00 2001 From: jeanluc Date: Mon, 6 Nov 2023 12:42:55 +0100 Subject: [PATCH 02/13] Sync config SSH wrapper with execution module The wrapper has diverged significantly from the module. * `option` did not check grains * `option` did not have `omit_all` and `wildcard` parameters * `get` missed several parameters: `delimiter`, `merge` and all `omit_*` * There was no wrapping function for `items`. --- changelog/56441.fixed.md | 1 + salt/client/ssh/wrapper/config.py | 377 +++++++++++++++--- tests/pytests/integration/ssh/test_config.py | 66 +++ .../unit/client/ssh/wrapper/test_config.py | 219 ++++++++++ 4 files changed, 614 insertions(+), 49 deletions(-) create mode 100644 changelog/56441.fixed.md create mode 100644 tests/pytests/integration/ssh/test_config.py create mode 100644 tests/pytests/unit/client/ssh/wrapper/test_config.py diff --git a/changelog/56441.fixed.md b/changelog/56441.fixed.md new file mode 100644 index 000000000000..489ad80f7709 --- /dev/null +++ b/changelog/56441.fixed.md @@ -0,0 +1 @@ +Fixed config.get does not support merge option with salt-ssh diff --git a/salt/client/ssh/wrapper/config.py b/salt/client/ssh/wrapper/config.py index eaf55d998ae6..7dd3a7aa463f 100644 --- a/salt/client/ssh/wrapper/config.py +++ b/salt/client/ssh/wrapper/config.py @@ -2,17 +2,22 @@ Return config information """ +import copy +import fnmatch +import logging import os -import re +import urllib.parse import salt.syspaths as syspaths import salt.utils.data import salt.utils.files +import salt.utils.sdb as sdb + +log = logging.getLogger(__name__) # Set up the default values for all systems DEFAULTS = { "mongo.db": "salt", - "mongo.host": "salt", "mongo.password": "", "mongo.port": 27017, "mongo.user": "", @@ -38,9 +43,12 @@ "solr.num_backups": 1, "poudriere.config": "/usr/local/etc/poudriere.conf", "poudriere.config_dir": "/usr/local/etc/poudriere.d", + "ldap.uri": "", "ldap.server": "localhost", "ldap.port": "389", "ldap.tls": False, + "ldap.no_verify": False, + "ldap.anonymous": True, "ldap.scope": 2, "ldap.attrs": None, "ldap.binddn": "", @@ -51,6 +59,11 @@ "tunnel": False, "images": os.path.join(syspaths.SRV_ROOT_DIR, "salt-images"), }, + "docker.exec_driver": "docker-exec", + "docker.compare_container_networks": { + "static": ["Aliases", "Links", "IPAMConfig"], + "automatic": ["IPAddress", "Gateway", "GlobalIPv6Address", "IPv6Gateway"], + }, } @@ -96,15 +109,66 @@ def valid_fileproto(uri): salt '*' config.valid_fileproto salt://path/to/file """ - try: - return bool(re.match("^(?:salt|https?|ftp)://", uri)) - except Exception: # pylint: disable=broad-except - return False + return urllib.parse.urlparse(uri).scheme in salt.utils.files.VALID_PROTOS + + +def option( + value, + default=None, + omit_opts=False, + omit_grains=False, + omit_pillar=False, + omit_master=False, + omit_all=False, + wildcard=False, +): + """ + Returns the setting for the specified config value. The priority for + matches is the same as in :py:func:`config.get `, + only this function does not recurse into nested data structures. Another + difference between this function and :py:func:`config.get + ` is that it comes with a set of "sane defaults". + To view these, you can run the following command: + .. code-block:: bash -def option(value, default="", omit_opts=False, omit_master=False, omit_pillar=False): - """ - Pass in a generic option and receive the value that will be assigned + salt '*' config.option '*' omit_all=True wildcard=True + + default + The default value if no match is found. If not specified, then the + fallback default will be an empty string, unless ``wildcard=True``, in + which case the return will be an empty dictionary. + + omit_opts : False + Pass as ``True`` to exclude matches from the minion configuration file + + omit_grains : False + Pass as ``True`` to exclude matches from the grains + + omit_pillar : False + Pass as ``True`` to exclude matches from the pillar data + + omit_master : False + Pass as ``True`` to exclude matches from the master configuration file + + omit_all : True + Shorthand to omit all of the above and return matches only from the + "sane defaults". + + .. versionadded:: 3000 + + wildcard : False + If used, this will perform pattern matching on keys. Note that this + will also significantly change the return data. Instead of only a value + being returned, a dictionary mapping the matched keys to their values + is returned. For example, using ``wildcard=True`` with a ``key`` of + ``'foo.ba*`` could return a dictionary like so: + + .. code-block:: python + + {'foo.bar': True, 'foo.baz': False} + + .. versionadded:: 3000 CLI Example: @@ -112,18 +176,48 @@ def option(value, default="", omit_opts=False, omit_master=False, omit_pillar=Fa salt '*' config.option redis.host """ - if not omit_opts: - if value in __opts__: - return __opts__[value] - if not omit_master: - if value in __pillar__.get("master", {}): - return __pillar__["master"][value] - if not omit_pillar: - if value in __pillar__: - return __pillar__[value] - if value in DEFAULTS: - return DEFAULTS[value] - return default + if omit_all: + omit_opts = omit_grains = omit_pillar = omit_master = True + + if default is None: + default = "" if not wildcard else {} + + if not wildcard: + if not omit_opts: + if value in __opts__: + return __opts__[value] + if not omit_grains: + if value in __grains__: + return __grains__[value] + if not omit_pillar: + if value in __pillar__: + return __pillar__[value] + if not omit_master: + if value in __pillar__.get("master", {}): + return __pillar__["master"][value] + if value in DEFAULTS: + return DEFAULTS[value] + + # No match + return default + else: + # We need to do the checks in the reverse order so that minion opts + # takes precedence + ret = {} + for omit, data in ( + (omit_master, __pillar__.get("master", {})), + (omit_pillar, __pillar__), + (omit_grains, __grains__), + (omit_opts, __opts__), + ): + if not omit: + ret.update({x: data[x] for x in fnmatch.filter(data, value)}) + # Check the DEFAULTS as well to see if the pattern matches it + for item in (x for x in fnmatch.filter(DEFAULTS, value) if x not in ret): + ret[item] = DEFAULTS[item] + + # If no matches, return the default + return ret or default def merge(value, default="", omit_opts=False, omit_master=False, omit_pillar=False): @@ -171,54 +265,223 @@ def merge(value, default="", omit_opts=False, omit_master=False, omit_pillar=Fal ret = list(ret) + list(tmp) if ret is None and value in DEFAULTS: return DEFAULTS[value] - return ret or default + if ret is None: + return default + return ret -def get(key, default=""): +def get( + key, + default="", + delimiter=":", + merge=None, + omit_opts=False, + omit_pillar=False, + omit_master=False, + omit_grains=False, +): """ .. versionadded:: 0.14.0 - Attempt to retrieve the named value from opts, pillar, grains of the master - config, if the named value is not available return the passed default. - The default return is an empty string. + Attempt to retrieve the named value from the minion config file, pillar, + grains or the master config. If the named value is not available, return + the value specified by the ``default`` argument. If this argument is not + specified, ``default`` falls back to an empty string. - The value can also represent a value in a nested dict using a ":" delimiter - for the dict. This means that if a dict looks like this:: + Values can also be retrieved from nested dictionaries. Assume the below + data structure: + + .. code-block:: python {'pkg': {'apache': 'httpd'}} - To retrieve the value associated with the apache key in the pkg dict this - key can be passed:: + To retrieve the value associated with the ``apache`` key, in the + sub-dictionary corresponding to the ``pkg`` key, the following command can + be used: + + .. code-block:: bash + + salt myminion config.get pkg:apache + + The ``:`` (colon) is used to represent a nested dictionary level. - pkg:apache + .. versionchanged:: 2015.5.0 + The ``delimiter`` argument was added, to allow delimiters other than + ``:`` to be used. - This routine traverses these data stores in this order: + This function traverses these data stores in this order, returning the + first match found: - - Local minion config (opts) + - Minion configuration - Minion's grains - - Minion's pillar - - Master config + - Minion's pillar data + - Master configuration (requires :conf_minion:`pillar_opts` to be set to + ``True`` in Minion config file in order to work) + + This means that if there is a value that is going to be the same for the + majority of minions, it can be configured in the Master config file, and + then overridden using the grains, pillar, or Minion config file. + + Adding config options to the Master or Minion configuration file is easy: + + .. code-block:: yaml + + my-config-option: value + cafe-menu: + - egg and bacon + - egg sausage and bacon + - egg and spam + - egg bacon and spam + - egg bacon sausage and spam + - spam bacon sausage and spam + - spam egg spam spam bacon and spam + - spam sausage spam spam bacon spam tomato and spam + + .. note:: + Minion configuration options built into Salt (like those defined + :ref:`here `) will *always* be defined in + the Minion configuration and thus *cannot be overridden by grains or + pillar data*. However, additional (user-defined) configuration options + (as in the above example) will not be in the Minion configuration by + default and thus can be overridden using grains/pillar data by leaving + the option out of the minion config file. + + **Arguments** + + delimiter + .. versionadded:: 2015.5.0 + + Override the delimiter used to separate nested levels of a data + structure. + + merge + .. versionadded:: 2015.5.0 + + If passed, this parameter will change the behavior of the function so + that, instead of traversing each data store above in order and + returning the first match, the data stores are first merged together + and then searched. The pillar data is merged into the master config + data, then the grains are merged, followed by the Minion config data. + The resulting data structure is then searched for a match. This allows + for configurations to be more flexible. + + .. note:: + + The merging described above does not mean that grain data will end + up in the Minion's pillar data, or pillar data will end up in the + master config data, etc. The data is just combined for the purposes + of searching an amalgam of the different data stores. + + The supported merge strategies are as follows: + + - **recurse** - If a key exists in both dictionaries, and the new value + is not a dictionary, it is replaced. Otherwise, the sub-dictionaries + are merged together into a single dictionary, recursively on down, + following the same criteria. For example: + + .. code-block:: python + + >>> dict1 = {'foo': {'bar': 1, 'qux': True}, + 'hosts': ['a', 'b', 'c'], + 'only_x': None} + >>> dict2 = {'foo': {'baz': 2, 'qux': False}, + 'hosts': ['d', 'e', 'f'], + 'only_y': None} + >>> merged + {'foo': {'bar': 1, 'baz': 2, 'qux': False}, + 'hosts': ['d', 'e', 'f'], + 'only_dict1': None, + 'only_dict2': None} + + - **overwrite** - If a key exists in the top level of both + dictionaries, the new value completely overwrites the old. For + example: + + .. code-block:: python + + >>> dict1 = {'foo': {'bar': 1, 'qux': True}, + 'hosts': ['a', 'b', 'c'], + 'only_x': None} + >>> dict2 = {'foo': {'baz': 2, 'qux': False}, + 'hosts': ['d', 'e', 'f'], + 'only_y': None} + >>> merged + {'foo': {'baz': 2, 'qux': False}, + 'hosts': ['d', 'e', 'f'], + 'only_dict1': None, + 'only_dict2': None} CLI Example: .. code-block:: bash salt '*' config.get pkg:apache + salt '*' config.get lxc.container_profile:centos merge=recurse """ - ret = salt.utils.data.traverse_dict_and_list(__opts__, key, "_|-") - if ret != "_|-": - return ret - ret = salt.utils.data.traverse_dict_and_list(__grains__, key, "_|-") - if ret != "_|-": - return ret - ret = salt.utils.data.traverse_dict_and_list(__pillar__, key, "_|-") - if ret != "_|-": - return ret - ret = salt.utils.data.traverse_dict_and_list( - __pillar__.get("master", {}), key, "_|-" - ) - if ret != "_|-": - return ret + if merge is None: + if not omit_opts: + ret = salt.utils.data.traverse_dict_and_list( + __opts__, key, "_|-", delimiter=delimiter + ) + if ret != "_|-": + return sdb.sdb_get(ret, __opts__) + + if not omit_grains: + ret = salt.utils.data.traverse_dict_and_list( + __grains__, key, "_|-", delimiter + ) + if ret != "_|-": + return sdb.sdb_get(ret, __opts__) + + if not omit_pillar: + ret = salt.utils.data.traverse_dict_and_list( + __pillar__, key, "_|-", delimiter=delimiter + ) + if ret != "_|-": + return sdb.sdb_get(ret, __opts__) + + if not omit_master: + ret = salt.utils.data.traverse_dict_and_list( + __pillar__.get("master", {}), key, "_|-", delimiter=delimiter + ) + if ret != "_|-": + return sdb.sdb_get(ret, __opts__) + + ret = salt.utils.data.traverse_dict_and_list( + DEFAULTS, key, "_|-", delimiter=delimiter + ) + if ret != "_|-": + return sdb.sdb_get(ret, __opts__) + else: + if merge not in ("recurse", "overwrite"): + log.warning( + "Unsupported merge strategy '%s'. Falling back to 'recurse'.", merge + ) + merge = "recurse" + + merge_lists = salt.config.master_config("/etc/salt/master").get( + "pillar_merge_lists" + ) + + data = copy.copy(DEFAULTS) + data = salt.utils.dictupdate.merge( + data, __pillar__.get("master", {}), strategy=merge, merge_lists=merge_lists + ) + data = salt.utils.dictupdate.merge( + data, __pillar__, strategy=merge, merge_lists=merge_lists + ) + data = salt.utils.dictupdate.merge( + data, __grains__, strategy=merge, merge_lists=merge_lists + ) + data = salt.utils.dictupdate.merge( + data, __opts__, strategy=merge, merge_lists=merge_lists + ) + ret = salt.utils.data.traverse_dict_and_list( + data, key, "_|-", delimiter=delimiter + ) + if ret != "_|-": + return sdb.sdb_get(ret, __opts__) + return default @@ -241,3 +504,19 @@ def dot_vals(value): if key.startswith("{}.".format(value)): ret[key] = val return ret + + +def items(): + """ + Return the complete config from the currently running minion process. + This includes defaults for values not set in the config file. + + CLI Example: + + .. code-block:: bash + + salt '*' config.items + """ + # This would otherwise be parsed as just the value of "local" in opts. + # In case the wfunc parsing is improved, this can be removed. + return {"local": {"return": __opts__.copy()}} diff --git a/tests/pytests/integration/ssh/test_config.py b/tests/pytests/integration/ssh/test_config.py new file mode 100644 index 000000000000..d3ae2b03a3e4 --- /dev/null +++ b/tests/pytests/integration/ssh/test_config.py @@ -0,0 +1,66 @@ +import pytest + +pytestmark = [pytest.mark.slow_test] + + +def test_items(salt_ssh_cli): + ret = salt_ssh_cli.run("config.items") + assert ret.returncode == 0 + assert isinstance(ret.data, dict) + assert "id" in ret.data + assert "grains" in ret.data + assert "__master_opts__" in ret.data + assert "cachedir" in ret.data + + +@pytest.mark.parametrize("omit", (False, True)) +def test_option_minion_opt(salt_ssh_cli, omit): + # Minion opt + ret = salt_ssh_cli.run("config.option", "id", omit_opts=omit, omit_grains=True) + assert ret.returncode == 0 + assert (ret.data != salt_ssh_cli.get_minion_tgt()) is omit + assert (ret.data == "") is omit + + +@pytest.mark.parametrize("omit", (False, True)) +def test_option_pillar(salt_ssh_cli, omit): + ret = salt_ssh_cli.run("config.option", "ext_spam", omit_pillar=omit) + assert ret.returncode == 0 + assert (ret.data != "eggs") is omit + assert (ret.data == "") is omit + + +@pytest.mark.parametrize("omit", (False, True)) +def test_option_grain(salt_ssh_cli, omit): + ret = salt_ssh_cli.run("config.option", "kernel", omit_grains=omit) + assert ret.returncode == 0 + assert ( + ret.data not in ("Darwin", "Linux", "FreeBSD", "OpenBSD", "Windows") + ) is omit + assert (ret.data == "") is omit + + +@pytest.mark.parametrize("omit", (False, True)) +def test_get_minion_opt(salt_ssh_cli, omit): + ret = salt_ssh_cli.run("config.get", "cachedir", omit_master=True, omit_opts=omit) + assert ret.returncode == 0 + assert (ret.data == "") is omit + assert ("minion" not in ret.data) is omit + + +@pytest.mark.parametrize("omit", (False, True)) +def test_get_pillar(salt_ssh_cli, omit): + ret = salt_ssh_cli.run("config.get", "ext_spam", omit_pillar=omit) + assert ret.returncode == 0 + assert (ret.data != "eggs") is omit + assert (ret.data == "") is omit + + +@pytest.mark.parametrize("omit", (False, True)) +def test_get_grain(salt_ssh_cli, omit): + ret = salt_ssh_cli.run("config.get", "kernel", omit_grains=omit) + assert ret.returncode == 0 + assert ( + ret.data not in ("Darwin", "Linux", "FreeBSD", "OpenBSD", "Windows") + ) is omit + assert (ret.data == "") is omit diff --git a/tests/pytests/unit/client/ssh/wrapper/test_config.py b/tests/pytests/unit/client/ssh/wrapper/test_config.py new file mode 100644 index 000000000000..64e89c762ad2 --- /dev/null +++ b/tests/pytests/unit/client/ssh/wrapper/test_config.py @@ -0,0 +1,219 @@ +""" + Taken 1:1 from test cases for salt.modules.config + This tests the SSH wrapper module. +""" + + +import fnmatch + +import pytest + +import salt.client.ssh.wrapper.config as config +from tests.support.mock import patch + + +@pytest.fixture +def defaults(): + return { + "test.option.foo": "value of test.option.foo in defaults", + "test.option.bar": "value of test.option.bar in defaults", + "test.option.baz": "value of test.option.baz in defaults", + "test.option": "value of test.option in defaults", + } + + +@pytest.fixture +def no_match(): + return "test.option.nope" + + +@pytest.fixture +def opt_name(): + return "test.option.foo" + + +@pytest.fixture +def wildcard_opt_name(): + return "test.option.b*" + + +@pytest.fixture +def configure_loader_modules(): + return { + config: { + "__opts__": { + "test.option.foo": "value of test.option.foo in __opts__", + "test.option.bar": "value of test.option.bar in __opts__", + "test.option.baz": "value of test.option.baz in __opts__", + }, + "__pillar__": { + "test.option.foo": "value of test.option.foo in __pillar__", + "test.option.bar": "value of test.option.bar in __pillar__", + "test.option.baz": "value of test.option.baz in __pillar__", + "master": { + "test.option.foo": "value of test.option.foo in master", + "test.option.bar": "value of test.option.bar in master", + "test.option.baz": "value of test.option.baz in master", + }, + }, + "__grains__": { + "test.option.foo": "value of test.option.foo in __grains__", + "test.option.bar": "value of test.option.bar in __grains__", + "test.option.baz": "value of test.option.baz in __grains__", + }, + } + } + + +def _wildcard_match(data, wildcard_opt_name): + return {x: data[x] for x in fnmatch.filter(data, wildcard_opt_name)} + + +def test_defaults_only_name(defaults): + with patch.dict(config.DEFAULTS, defaults): + opt_name = "test.option" + opt = config.option(opt_name) + assert opt == config.DEFAULTS[opt_name] + + +def test_no_match(defaults, no_match, wildcard_opt_name): + """ + Make sure that the defa + """ + with patch.dict(config.DEFAULTS, defaults): + ret = config.option(no_match) + assert ret == "", ret + + default = "wat" + ret = config.option(no_match, default=default) + assert ret == default, ret + + ret = config.option(no_match, wildcard=True) + assert ret == {}, ret + + default = {"foo": "bar"} + ret = config.option(no_match, default=default, wildcard=True) + assert ret == default, ret + + # Should be no match since wildcard=False + ret = config.option(wildcard_opt_name) + assert ret == "", ret + + +def test_omits(defaults, opt_name, wildcard_opt_name): + with patch.dict(config.DEFAULTS, defaults): + + # ********** OMIT NOTHING ********** + + # Match should be in __opts__ dict + ret = config.option(opt_name) + assert ret == config.__opts__[opt_name], ret + + # Wildcard match + ret = config.option(wildcard_opt_name, wildcard=True) + assert ret == _wildcard_match(config.__opts__, wildcard_opt_name), ret + + # ********** OMIT __opts__ ********** + + # Match should be in __grains__ dict + ret = config.option(opt_name, omit_opts=True) + assert ret == config.__grains__[opt_name], ret + + # Wildcard match + ret = config.option(wildcard_opt_name, omit_opts=True, wildcard=True) + assert ret == _wildcard_match(config.__grains__, wildcard_opt_name), ret + + # ********** OMIT __opts__, __grains__ ********** + + # Match should be in __pillar__ dict + ret = config.option(opt_name, omit_opts=True, omit_grains=True) + assert ret == config.__pillar__[opt_name], ret + + # Wildcard match + ret = config.option( + wildcard_opt_name, omit_opts=True, omit_grains=True, wildcard=True + ) + assert ret == _wildcard_match(config.__pillar__, wildcard_opt_name), ret + + # ********** OMIT __opts__, __grains__, __pillar__ ********** + + # Match should be in master opts + ret = config.option( + opt_name, omit_opts=True, omit_grains=True, omit_pillar=True + ) + assert ret == config.__pillar__["master"][opt_name], ret + + # Wildcard match + ret = config.option( + wildcard_opt_name, + omit_opts=True, + omit_grains=True, + omit_pillar=True, + wildcard=True, + ) + assert ret == _wildcard_match( + config.__pillar__["master"], wildcard_opt_name + ), ret + + # ********** OMIT ALL THE THINGS ********** + + # Match should be in master opts + ret = config.option( + opt_name, + omit_opts=True, + omit_grains=True, + omit_pillar=True, + omit_master=True, + ) + assert ret == config.DEFAULTS[opt_name], ret + + # Wildcard match + ret = config.option( + wildcard_opt_name, + omit_opts=True, + omit_grains=True, + omit_pillar=True, + omit_master=True, + wildcard=True, + ) + assert ret == _wildcard_match(config.DEFAULTS, wildcard_opt_name), ret + + # Match should be in master opts + ret = config.option(opt_name, omit_all=True) + assert ret == config.DEFAULTS[opt_name], ret + + # Wildcard match + ret = config.option(wildcard_opt_name, omit_all=True, wildcard=True) + assert ret == _wildcard_match(config.DEFAULTS, wildcard_opt_name), ret + + +# --- Additional tests not found in the execution module tests + + +@pytest.mark.parametrize("backup", ("", "minion", "master", "both")) +def test_backup_mode(backup): + res = config.backup_mode(backup) + assert res == backup or "minion" + + +@pytest.mark.parametrize( + "uri,expected", + (("salt://my/foo.txt", True), ("mysql://foo:bar@foo.bar/baz", False)), +) +def test_valid_fileproto(uri, expected): + res = config.valid_fileproto(uri) + assert res is expected + + +def test_dot_vals(): + extra_master_opt = ("test.option.baah", "value of test.option.baah in master") + with patch.dict(config.__pillar__, {"master": dict((extra_master_opt,))}): + res = config.dot_vals("test") + assert isinstance(res, dict) + assert res + for var in ("foo", "bar", "baz"): + key = f"test.option.{var}" + assert key in res + assert res[key] == f"value of test.option.{var} in __opts__" + assert extra_master_opt[0] in res + assert res[extra_master_opt[0]] == extra_master_opt[1] From ddc119764707a355069ff93350ab8e06f2a688ea Mon Sep 17 00:00:00 2001 From: jeanluc Date: Mon, 6 Nov 2023 12:53:50 +0100 Subject: [PATCH 03/13] Add `slsutil` SSH wrapper --- changelog/50196.fixed.md | 1 + changelog/61143.fixed.md | 1 + changelog/65067.fixed.md | 1 + salt/client/ssh/wrapper/slsutil.py | 450 ++++++++++++++++++ tests/pytests/integration/ssh/test_slsutil.py | 94 ++++ .../unit/client/ssh/wrapper/test_slsutil.py | 166 +++++++ 6 files changed, 713 insertions(+) create mode 100644 changelog/50196.fixed.md create mode 100644 changelog/61143.fixed.md create mode 100644 changelog/65067.fixed.md create mode 100644 salt/client/ssh/wrapper/slsutil.py create mode 100644 tests/pytests/integration/ssh/test_slsutil.py create mode 100644 tests/pytests/unit/client/ssh/wrapper/test_slsutil.py diff --git a/changelog/50196.fixed.md b/changelog/50196.fixed.md new file mode 100644 index 000000000000..979411a640da --- /dev/null +++ b/changelog/50196.fixed.md @@ -0,0 +1 @@ +Made slsutil.renderer work with salt-ssh diff --git a/changelog/61143.fixed.md b/changelog/61143.fixed.md new file mode 100644 index 000000000000..08a62c9d8b10 --- /dev/null +++ b/changelog/61143.fixed.md @@ -0,0 +1 @@ +Made slsutil.findup work with salt-ssh diff --git a/changelog/65067.fixed.md b/changelog/65067.fixed.md new file mode 100644 index 000000000000..d6de87b5bc15 --- /dev/null +++ b/changelog/65067.fixed.md @@ -0,0 +1 @@ +Fixed slsutil.update with salt-ssh during template rendering diff --git a/salt/client/ssh/wrapper/slsutil.py b/salt/client/ssh/wrapper/slsutil.py new file mode 100644 index 000000000000..e09ca1c29845 --- /dev/null +++ b/salt/client/ssh/wrapper/slsutil.py @@ -0,0 +1,450 @@ +import os.path +import posixpath + +import salt.exceptions +import salt.loader +import salt.template +import salt.utils.args +import salt.utils.dictupdate +import salt.utils.stringio + +CONTEXT_BASE = "slsutil" + + +def update(dest, upd, recursive_update=True, merge_lists=False): + """ + Merge ``upd`` recursively into ``dest`` + + If ``merge_lists=True``, will aggregate list object types instead of + replacing. This behavior is only activated when ``recursive_update=True``. + + CLI Example: + + .. code-block:: shell + + salt '*' slsutil.update '{foo: Foo}' '{bar: Bar}' + + """ + return salt.utils.dictupdate.update(dest, upd, recursive_update, merge_lists) + + +def merge(obj_a, obj_b, strategy="smart", renderer="yaml", merge_lists=False): + """ + Merge a data structure into another by choosing a merge strategy + + Strategies: + + * aggregate + * list + * overwrite + * recurse + * smart + + CLI Example: + + .. code-block:: shell + + salt '*' slsutil.merge '{foo: Foo}' '{bar: Bar}' + """ + return salt.utils.dictupdate.merge(obj_a, obj_b, strategy, renderer, merge_lists) + + +def merge_all(lst, strategy="smart", renderer="yaml", merge_lists=False): + """ + .. versionadded:: 2019.2.0 + + Merge a list of objects into each other in order + + :type lst: Iterable + :param lst: List of objects to be merged. + + :type strategy: String + :param strategy: Merge strategy. See utils.dictupdate. + + :type renderer: String + :param renderer: + Renderer type. Used to determine strategy when strategy is 'smart'. + + :type merge_lists: Bool + :param merge_lists: Defines whether to merge embedded object lists. + + CLI Example: + + .. code-block:: shell + + $ salt-call --output=txt slsutil.merge_all '[{foo: Foo}, {foo: Bar}]' + local: {u'foo': u'Bar'} + """ + + ret = {} + for obj in lst: + ret = salt.utils.dictupdate.merge(ret, obj, strategy, renderer, merge_lists) + + return ret + + +def renderer(path=None, string=None, default_renderer="jinja|yaml", **kwargs): + """ + Parse a string or file through Salt's renderer system + + .. versionchanged:: 2018.3.0 + Add support for Salt fileserver URIs. + + This is an open-ended function and can be used for a variety of tasks. It + makes use of Salt's "renderer pipes" system to run a string or file through + a pipe of any of the loaded renderer modules. + + :param path: The path to a file on Salt's fileserver (any URIs supported by + :py:func:`cp.get_url `) or on the local file + system. + :param string: An inline string to be used as the file to send through the + renderer system. Note, not all renderer modules can work with strings; + the 'py' renderer requires a file, for example. + :param default_renderer: The renderer pipe to send the file through; this + is overridden by a "she-bang" at the top of the file. + :param kwargs: Keyword args to pass to Salt's compile_template() function. + + Keep in mind the goal of each renderer when choosing a render-pipe; for + example, the Jinja renderer processes a text file and produces a string, + however the YAML renderer processes a text file and produces a data + structure. + + One possible use is to allow writing "map files", as are commonly seen in + Salt formulas, but without tying the renderer of the map file to the + renderer used in the other sls files. In other words, a map file could use + the Python renderer and still be included and used by an sls file that uses + the default 'jinja|yaml' renderer. + + For example, the two following map files produce identical results but one + is written using the normal 'jinja|yaml' and the other is using 'py': + + .. code-block:: jinja + + #!jinja|yaml + {% set apache = salt.grains.filter_by({ + ...normal jinja map file here... + }, merge=salt.pillar.get('apache:lookup')) %} + {{ apache | yaml() }} + + .. code-block:: python + + #!py + def run(): + apache = __salt__.grains.filter_by({ + ...normal map here but as a python dict... + }, merge=__salt__.pillar.get('apache:lookup')) + return apache + + Regardless of which of the above map files is used, it can be accessed from + any other sls file by calling this function. The following is a usage + example in Jinja: + + .. code-block:: jinja + + {% set apache = salt.slsutil.renderer('map.sls') %} + + CLI Example: + + .. code-block:: bash + + salt '*' slsutil.renderer salt://path/to/file + salt '*' slsutil.renderer /path/to/file + salt '*' slsutil.renderer /path/to/file.jinja default_renderer='jinja' + salt '*' slsutil.renderer /path/to/file.sls default_renderer='jinja|yaml' + salt '*' slsutil.renderer string='Inline template! {{ saltenv }}' + salt '*' slsutil.renderer string='Hello, {{ name }}.' name='world' + """ + if not path and not string: + raise salt.exceptions.SaltInvocationError("Must pass either path or string") + + renderers = salt.loader.render(__opts__, __salt__) + + if path: + path_or_string = __context__["fileclient"].get_url( + path, "", saltenv=kwargs.get("saltenv", "base") + ) + elif string: + path_or_string = ":string:" + kwargs["input_data"] = string + + ret = salt.template.compile_template( + path_or_string, + renderers, + default_renderer, + __opts__["renderer_blacklist"], + __opts__["renderer_whitelist"], + **kwargs + ) + return ret.read() if salt.utils.stringio.is_readable(ret) else ret + + +def _get_serialize_fn(serializer, fn_name): + serializers = salt.loader.serializers(__opts__) + fns = getattr(serializers, serializer, None) + fn = getattr(fns, fn_name, None) + + if not fns: + raise salt.exceptions.CommandExecutionError( + "Serializer '{}' not found.".format(serializer) + ) + + if not fn: + raise salt.exceptions.CommandExecutionError( + "Serializer '{}' does not implement {}.".format(serializer, fn_name) + ) + + return fn + + +def serialize(serializer, obj, **mod_kwargs): + """ + Serialize a Python object using one of the available + :ref:`all-salt.serializers`. + + CLI Example: + + .. code-block:: bash + + salt '*' --no-parse=obj slsutil.serialize 'json' obj="{'foo': 'Foo!'} + + Jinja Example: + + .. code-block:: jinja + + {% set json_string = salt.slsutil.serialize('json', + {'foo': 'Foo!'}) %} + """ + kwargs = salt.utils.args.clean_kwargs(**mod_kwargs) + return _get_serialize_fn(serializer, "serialize")(obj, **kwargs) + + +def deserialize(serializer, stream_or_string, **mod_kwargs): + """ + Deserialize a Python object using one of the available + :ref:`all-salt.serializers`. + + CLI Example: + + .. code-block:: bash + + salt '*' slsutil.deserialize 'json' '{"foo": "Foo!"}' + salt '*' --no-parse=stream_or_string slsutil.deserialize 'json' \\ + stream_or_string='{"foo": "Foo!"}' + + Jinja Example: + + .. code-block:: jinja + + {% set python_object = salt.slsutil.deserialize('json', + '{"foo": "Foo!"}') %} + """ + kwargs = salt.utils.args.clean_kwargs(**mod_kwargs) + return _get_serialize_fn(serializer, "deserialize")(stream_or_string, **kwargs) + + +def boolstr(value, true="true", false="false"): + """ + Convert a boolean value into a string. This function is + intended to be used from within file templates to provide + an easy way to take boolean values stored in Pillars or + Grains, and write them out in the appropriate syntax for + a particular file template. + + :param value: The boolean value to be converted + :param true: The value to return if ``value`` is ``True`` + :param false: The value to return if ``value`` is ``False`` + + In this example, a pillar named ``smtp:encrypted`` stores a boolean + value, but the template that uses that value needs ``yes`` or ``no`` + to be written, based on the boolean value. + + *Note: this is written on two lines for clarity. The same result + could be achieved in one line.* + + .. code-block:: jinja + + {% set encrypted = salt[pillar.get]('smtp:encrypted', false) %} + use_tls: {{ salt['slsutil.boolstr'](encrypted, 'yes', 'no') }} + + Result (assuming the value is ``True``): + + .. code-block:: none + + use_tls: yes + + """ + + if value: + return true + + return false + + +def _set_context(keys, function, fun_args=None, fun_kwargs=None, force=False): + """ + Convenience function to set a value in the ``__context__`` dictionary. + + .. versionadded:: 3004 + + :param keys: The list of keys specifying the dictionary path to set. This + list can be of arbitrary length and the path will be created + in the dictionary if it does not exist. + + :param function: A python function to be called if the specified path does + not exist, if the force parameter is ``True``. + + :param fun_args: A list of positional arguments to the function. + + :param fun_kwargs: A dictionary of keyword arguments to the function. + + :param force: If ``True``, force the ```__context__`` path to be updated. + Otherwise, only create it if it does not exist. + """ + + target = __context__ + + # Build each level of the dictionary as needed + for key in keys[:-1]: + if key not in target: + target[key] = {} + target = target[key] + + # Call the supplied function to populate the dictionary + if force or keys[-1] not in target: + if not fun_args: + fun_args = [] + + if not fun_kwargs: + fun_kwargs = {} + + target[keys[-1]] = function(*fun_args, *fun_kwargs) + + +def file_exists(path, saltenv="base"): + """ + Return ``True`` if a file exists in the state tree, ``False`` otherwise. + + .. versionadded:: 3004 + + :param str path: The fully qualified path to a file in the state tree. + :param str saltenv: The fileserver environment to search. Default: ``base`` + + CLI Example: + + .. code-block:: bash + + salt '*' slsutil.file_exists nginx/defaults.yaml + """ + + _set_context( + [CONTEXT_BASE, saltenv, "file_list"], __salt__["cp.list_master"], [saltenv] + ) + return path in __context__[CONTEXT_BASE][saltenv]["file_list"] + + +def dir_exists(path, saltenv="base"): + """ + Return ``True`` if a directory exists in the state tree, ``False`` otherwise. + + :param str path: The fully qualified path to a directory in the state tree. + :param str saltenv: The fileserver environment to search. Default: ``base`` + + .. versionadded:: 3004 + + CLI Example: + + .. code-block:: bash + + salt '*' slsutil.dir_exists nginx/files + """ + + _set_context( + [CONTEXT_BASE, saltenv, "dir_list"], __salt__["cp.list_master_dirs"], [saltenv] + ) + return path in __context__[CONTEXT_BASE][saltenv]["dir_list"] + + +def path_exists(path, saltenv="base"): + """ + Return ``True`` if a path exists in the state tree, ``False`` otherwise. The path + could refer to a file or directory. + + .. versionadded:: 3004 + + :param str path: The fully qualified path to a file or directory in the state tree. + :param str saltenv: The fileserver environment to search. Default: ``base`` + + CLI Example: + + .. code-block:: bash + + salt '*' slsutil.path_exists nginx/defaults.yaml + """ + + return file_exists(path, saltenv) or dir_exists(path, saltenv) + + +def findup(startpath, filenames, saltenv="base"): + """ + Find the first path matching a filename or list of filenames in a specified + directory or the nearest ancestor directory. Returns the full path to the + first file found. + + .. versionadded:: 3004 + + :param str startpath: The fileserver path from which to begin the search. + An empty string refers to the state tree root. + :param filenames: A filename or list of filenames to search for. Searching for + directory names is also supported. + :param str saltenv: The fileserver environment to search. Default: ``base`` + + Example: return the path to ``defaults.yaml``, walking up the tree from the + state file currently being processed. + + .. code-block:: jinja + + {{ salt["slsutil.findup"](tplfile, "defaults.yaml") }} + + CLI Example: + + .. code-block:: bash + + salt '*' slsutil.findup formulas/shared/nginx map.jinja + """ + + # Normalize the path + if startpath: + startpath = posixpath.normpath(startpath) + + # Verify the cwd is a valid path in the state tree + if startpath and not path_exists(startpath, saltenv): + raise salt.exceptions.SaltInvocationError( + "Starting path not found in the state tree: {}".format(startpath) + ) + + # Ensure that patterns is a string or list of strings + if isinstance(filenames, str): + filenames = [filenames] + if not isinstance(filenames, list): + raise salt.exceptions.SaltInvocationError( + "Filenames argument must be a string or list of strings" + ) + + while True: + + # Loop over filenames, looking for one at the current path level + for filename in filenames: + fullname = salt.utils.path.join( + startpath or "", filename, use_posixpath=True + ) + if path_exists(fullname, saltenv): + return fullname + + # If the root path was just checked, raise an error + if not startpath: + raise salt.exceptions.CommandExecutionError( + "File pattern(s) not found in path ancestry" + ) + + # Move up one level in the ancestry + startpath = os.path.dirname(startpath) diff --git a/tests/pytests/integration/ssh/test_slsutil.py b/tests/pytests/integration/ssh/test_slsutil.py new file mode 100644 index 000000000000..4ac9ed59a825 --- /dev/null +++ b/tests/pytests/integration/ssh/test_slsutil.py @@ -0,0 +1,94 @@ +import json + +import pytest + + +@pytest.mark.usefixtures("state_tree") +def test_renderer_file(salt_ssh_cli): + ret = salt_ssh_cli.run("slsutil.renderer", "salt://test.sls") + assert ret.returncode == 0 + assert isinstance(ret.data, dict) + assert "Ok with def" in ret.data + + +def test_renderer_string(salt_ssh_cli): + rend = "{{ salt['test.echo']('foo') }}: {{ pillar['ext_spam'] }}" + ret = salt_ssh_cli.run("slsutil.renderer", string=rend) + assert ret.returncode == 0 + assert isinstance(ret.data, dict) + assert ret.data == {"foo": "eggs"} + + +def test_serialize(salt_ssh_cli): + obj = {"foo": "bar"} + ret = salt_ssh_cli.run("slsutil.serialize", "json", obj) + assert ret.returncode == 0 + assert isinstance(ret.data, str) + assert ret.data == json.dumps(obj) + + +def test_deserialize(salt_ssh_cli): + obj = {"foo": "bar"} + data = json.dumps(obj) + # Need to quote it, otherwise it's deserialized by the + # test wrapper + ret = salt_ssh_cli.run("slsutil.deserialize", "json", f"'{data}'") + assert ret.returncode == 0 + assert isinstance(ret.data, type(obj)) + assert ret.data == obj + + +@pytest.mark.parametrize( + "path,expected", + [ + ("test_deep", True), + ("test_deep/test.sls", False), + ("test_deep/b/2", True), + ("does_not/ex/ist", False), + ], +) +def test_dir_exists(salt_ssh_cli, path, expected): + ret = salt_ssh_cli.run("slsutil.dir_exists", path) + assert ret.returncode == 0 + assert isinstance(ret.data, bool) + assert ret.data is expected + + +@pytest.mark.parametrize( + "path,expected", [("test_deep", False), ("test_deep/test.sls", True)] +) +def test_file_exists(salt_ssh_cli, path, expected): + ret = salt_ssh_cli.run("slsutil.file_exists", path) + assert ret.returncode == 0 + assert isinstance(ret.data, bool) + assert ret.data is expected + + +@pytest.mark.parametrize( + "start,name,expected", + [ + ("test_deep/b/2", "test.sls", "test_deep/b/2/test.sls"), + ("test_deep/b/2", "cheese", "cheese"), + ], +) +def test_findup(salt_ssh_cli, start, name, expected): + ret = salt_ssh_cli.run("slsutil.findup", start, name) + assert ret.returncode == 0 + assert isinstance(ret.data, str) + assert ret.data == expected + + +@pytest.mark.parametrize( + "path,expected", + [ + ("test_deep", True), + ("test_deep/test.sls", True), + ("test_deep/b/2", True), + ("does_not/ex/ist", False), + ], +) +def test_path_exists(salt_ssh_cli, path, expected): + ret = salt_ssh_cli.run("slsutil.path_exists", path) + assert ret.returncode == 0 + assert isinstance(ret.data, bool) + assert ret.data is expected diff --git a/tests/pytests/unit/client/ssh/wrapper/test_slsutil.py b/tests/pytests/unit/client/ssh/wrapper/test_slsutil.py new file mode 100644 index 000000000000..558d5ee74dcf --- /dev/null +++ b/tests/pytests/unit/client/ssh/wrapper/test_slsutil.py @@ -0,0 +1,166 @@ +import contextlib +import logging + +import pytest + +import salt.client.ssh.wrapper.slsutil as slsutil +from salt.exceptions import CommandExecutionError, SaltInvocationError +from tests.support.mock import MagicMock + +log = logging.getLogger(__name__) + + +# --- These tests are adapted from tests.pytests.unit.utils.slsutil + + +@pytest.fixture +def configure_loader_modules(master_dirs, master_files): + return { + slsutil: { + "__salt__": { + "cp.list_master": MagicMock(return_value=master_files), + "cp.list_master_dirs": MagicMock(return_value=master_dirs), + }, + "__opts__": { + "renderer": "jinja|yaml", + "renderer_blacklist": [], + "renderer_whitelist": [], + }, + } + } + + +@pytest.fixture +def master_dirs(): + return ["red", "red/files", "blue", "blue/files"] + + +@pytest.fixture +def master_files(): + return [ + "top.sls", + "red/init.sls", + "red/files/default.conf", + "blue/init.sls", + "blue/files/default.conf", + ] + + +@pytest.mark.parametrize("inpt,expected", ((True, "yes"), (False, "no"))) +def test_boolstr(inpt, expected): + assert slsutil.boolstr(inpt, true="yes", false="no") == expected + + +@pytest.mark.parametrize( + "inpt,expected", (("red/init.sls", True), ("green/init.sls", False)) +) +def test_file_exists(inpt, expected): + assert slsutil.file_exists(inpt) is expected + + +@pytest.mark.parametrize("inpt,expected", (("red", True), ("green", False))) +def test_dir_exists(inpt, expected): + assert slsutil.dir_exists(inpt) is expected + + +@pytest.mark.parametrize( + "inpt,expected", + ( + ("red", True), + ("green", False), + ("red/init.sls", True), + ("green/init.sls", False), + ), +) +def test_path_exists(inpt, expected): + assert slsutil.path_exists(inpt) is expected + + +@pytest.mark.parametrize( + "inpt,expected,raises", + [ + (("red/files", "init.sls"), "red/init.sls", None), + (("red/files", ["top.sls"]), "top.sls", None), + (("", "top.sls"), "top.sls", None), + ((None, "top.sls"), "top.sls", None), + (("red/files", ["top.sls", "init.sls"]), "red/init.sls", None), + ( + ("red/files", "notfound"), + None, + pytest.raises( + CommandExecutionError, match=r"File pattern\(s\) not found.*" + ), + ), + ( + ("red", "default.conf"), + None, + pytest.raises( + CommandExecutionError, match=r"File pattern\(s\) not found.*" + ), + ), + ( + ("green", "notfound"), + None, + pytest.raises(SaltInvocationError, match="Starting path not found.*"), + ), + ( + ("red", 1234), + None, + pytest.raises( + SaltInvocationError, match=".*must be a string or list of strings.*" + ), + ), + ], +) +def test_findup(inpt, expected, raises): + if raises is None: + raises = contextlib.nullcontext() + with raises: + res = slsutil.findup(*inpt) + assert res == expected + + +@pytest.mark.parametrize( + "a,b,merge_lists,expected", + [ + ( + {"foo": {"bar": "baz", "hi": "there", "some": ["list"]}}, + {"foo": {"baz": "quux", "bar": "hi", "some": ["other_list"]}}, + False, + { + "foo": { + "baz": "quux", + "bar": "hi", + "hi": "there", + "some": ["other_list"], + } + }, + ), + ( + {"foo": {"bar": "baz", "hi": "there", "some": ["list"]}}, + {"foo": {"baz": "quux", "bar": "hi", "some": ["other_list"]}}, + True, + { + "foo": { + "baz": "quux", + "bar": "hi", + "hi": "there", + "some": ["list", "other_list"], + } + }, + ), + ], +) +@pytest.mark.parametrize("func", ("update", "merge", "merge_all")) +def test_update_merge(a, b, merge_lists, expected, func): + arg = (a, b) + if func == "merge_all": + arg = ([a, b],) + res = getattr(slsutil, func)(*arg, merge_lists=merge_lists) + assert res == expected + assert (a is res) is (func == "update") + + +def test_renderer_requires_either_path_or_string(): + with pytest.raises(SaltInvocationError, match=".*either path or string.*"): + slsutil.renderer() From 0b1d6dff5f929e8f6f90d08db89fb0ee03bad845 Mon Sep 17 00:00:00 2001 From: jeanluc Date: Mon, 6 Nov 2023 15:21:36 +0100 Subject: [PATCH 04/13] Add missing functions to ``cp`` SSH wrapper --- salt/client/ssh/shell.py | 4 +- salt/client/ssh/wrapper/cp.py | 1139 +++++++++++++++- salt/client/ssh/wrapper/state.py | 28 +- salt/modules/cp.py | 11 +- tests/pytests/integration/ssh/test_cp.py | 896 +++++++++++++ tests/pytests/unit/client/ssh/test_shell.py | 13 + .../unit/client/ssh/wrapper/test_cp.py | 1156 +++++++++++++++++ tests/pytests/unit/modules/test_cp.py | 2 - 8 files changed, 3202 insertions(+), 47 deletions(-) create mode 100644 tests/pytests/integration/ssh/test_cp.py create mode 100644 tests/pytests/unit/client/ssh/wrapper/test_cp.py diff --git a/salt/client/ssh/shell.py b/salt/client/ssh/shell.py index 5d93cdeb801c..5ea044f7efe2 100644 --- a/salt/client/ssh/shell.py +++ b/salt/client/ssh/shell.py @@ -339,7 +339,9 @@ def send(self, local, remote, makedirs=False): scp a file or files to a remote system """ if makedirs: - self.exec_cmd(f"mkdir -p {os.path.dirname(remote)}") + ret = self.exec_cmd(f"mkdir -p {os.path.dirname(remote)}") + if ret[2]: + return ret # scp needs [/``. + + saltenv + Salt fileserver environment from which to retrieve the file. + Defaults to ``base``. + + makedirs + Whether to create the parent directories for ``dest`` as needed. + Defaults to false. + + template + If ``path`` and ``dest`` parameters should be interpreted as templates, + the name of the renderer to use. + + Template rendering can be enabled on both ``path`` and + ``dest`` file paths like so: + + .. code-block:: bash + + salt-ssh '*' cp.get_file "salt://{{grains.os}}/vimrc" /etc/vimrc template=jinja + + Additional keyword arguments are passed through to the renderer, otherwise discarded. + + .. note:: + + It may be necessary to quote the URL when using the querystring method, + depending on the shell being used to run the command. .. note:: gzip compression is not supported in the salt-ssh version of - cp.get_file. The argument is only accepted for interface compatibility. + ``cp.get_file``. """ + gzip = kwargs.pop("gzip", None) if gzip is not None: log.warning("The gzip argument to cp.get_file in salt-ssh is unsupported") - if template is not None: - (path, dest) = _render_filenames(path, dest, saltenv, template) + (path, dest) = _render_filenames(path, dest, saltenv, template, **kwargs) - src = __context__["fileclient"].cache_file( - path, saltenv, cachedir=os.path.join("salt-ssh", __salt__.kwargs["id_"]) - ) - single = salt.client.ssh.Single(__opts__, "", **__salt__.kwargs) - ret = single.shell.send(src, dest, makedirs) - return not ret[2] + path, senv = salt.utils.url.split_env(path) + if senv: + saltenv = senv + if not hash_file(path, saltenv): + return "" + else: + with _client() as client: + ret = client.get_file(path, dest, makedirs, saltenv) + if not ret: + return ret + # Return the cache path on the minion, not the local one + return client.target_map[ret] -def get_dir(path, dest, saltenv="base"): + +def envs(): """ - Transfer a directory down + List available fileserver environments. + + CLI Example: + + .. code-block:: bash + + salt-ssh '*' cp.envs """ - src = __context__["fileclient"].cache_dir( - path, saltenv, cachedir=os.path.join("salt-ssh", __salt__.kwargs["id_"]) - ) - src = " ".join(src) - single = salt.client.ssh.Single(__opts__, "", **__salt__.kwargs) - ret = single.shell.send(src, dest) - return not ret[2] + return __context__["fileclient"].envs() -def get_url(path, dest, saltenv="base"): +def get_template( + path, dest, template="jinja", saltenv="base", makedirs=False, **kwargs +): """ - retrieve a URL + Render a file as a template before writing it. + + CLI Example: + + .. code-block:: bash + + salt '*' cp.get_template salt://path/to/template /minion/dest + + path + The path on the fileserver, like ``salt://foo/bar.conf``. It is possible + to specify the ``saltenv`` using the querystring syntax: + ``salt://foo/bar.conf?saltenv=config`` + + dest + The absolute path to transfer the file to on the minion. If empty, + the rendered template will be cached in the minion's cache dir, + under ``extrn_files//``. + + template + The renderer to use for rendering the template. Defaults to ``jinja``. + + saltenv + The saltenv the template should be pulled from. Defaults to ``base``. + + makedirs + Whether to create the parent directories for ``dest`` as needed. + Defaults to false. + + Additional keyword arguments are passed verbatim to the renderer. """ - src = __context__["fileclient"].cache_file( - path, saltenv, cachedir=os.path.join("salt-ssh", __salt__.kwargs["id_"]) - ) - single = salt.client.ssh.Single(__opts__, "", **__salt__.kwargs) - ret = single.shell.send(src, dest) - return not ret[2] + if "salt" not in kwargs: + kwargs["salt"] = __salt__.value() + if "pillar" not in kwargs: + kwargs["pillar"] = __pillar__.value() + if "grains" not in kwargs: + kwargs["grains"] = __grains__.value() + if "opts" not in kwargs: + kwargs["opts"] = __opts__ + + with _client() as client: + ret = client.get_template(path, dest, template, makedirs, saltenv, **kwargs) + if not ret: + return ret + # Return the cache path on the minion, not the local one + return client.target_map[ret] + + +def get_dir(path, dest, saltenv="base", template=None, **kwargs): + """ + Recursively transfer a directory from the fileserver to the minion. + + .. note:: + + This can take a long time since each file is transferred separately + currently. + + CLI Example: + + .. code-block:: bash + + salt-ssh '*' cp.get_dir salt://path/to/dir/ /minion/dest + + path + The path on the fileserver, like ``salt://foo/bar/``. It is possible + to specify the ``saltenv`` using the querystring syntax: + ``salt://foo/bar?saltenv=config`` + + dest + The absolute path to transfer the directory to on the minion. If empty, + the directory will be cached in the minion's cache dir, under + ``files//``. Note that parent directories will + be created as required automatically. + + saltenv + Salt fileserver environment from which to retrieve the directory. + Defaults to ``base``. + + template + If ``path`` and ``dest`` parameters should be interpreted as templates, + the name of the renderer to use. + + .. note:: + + gzip compression is not supported in the salt-ssh version of + cp.get_dir. The argument is only accepted for interface compatibility. + """ + # FIXME: transfer dirs using tar + gzip = kwargs.pop("gzip", None) + if gzip is not None: + log.warning("The gzip argument to cp.get_dir in salt-ssh is unsupported") + (path, dest) = _render_filenames(path, dest, saltenv, template, **kwargs) + + with _client() as client: + ret = client.get_dir(path, dest, saltenv, gzip) + if not ret: + return ret + # Return the cache path on the minion, not the local one + return [client.target_map[x] for x in ret] + + +def get_url(path, dest="", saltenv="base", makedirs=False, source_hash=None): + """ + Retrieve a single file from a URL. + + path + A URL to download a file from. Supported URL schemes are: ``salt://``, + ``http://``, ``https://``, ``ftp://``, ``s3://``, ``swift://`` and + ``file://`` (local filesystem). If no scheme was specified, this is + equivalent of using ``file://``. + If a ``file://`` URL is given, the function just returns absolute path + to that file on a local filesystem. + The function returns ``False`` if Salt was unable to fetch a file from + a ``salt://`` URL. + + .. note:: + + The file:// scheme is currently only partially supported in salt-ssh. + It behaves the same as the unwrapped ``cp.get_url`` if dest is not + ``None``, but returning its contents will fail. Use ``get_file_str`` + as a workaround for text files. + + dest + The destination to write the cached file to. If empty, will cache the file + in the minion's cache dir under ``extrn_files///``. + Defaults to empty (i.e. caching the file). + + .. note:: + + To simply return the file contents instead, set destination to + ``None``. This works with ``salt://``, ``http://`` and ``https://`` + URLs. The files fetched by ``http://`` and ``https://`` will not + be cached. + + saltenv + Salt fileserver environment from which to retrieve the file. Ignored if + ``path`` is not a ``salt://`` URL. Defaults to ``base``. + + makedirs + Whether to create the parent directories for ``dest`` as needed. + Defaults to false. + + source_hash + If ``path`` is an http(s) or ftp URL and the file exists in the + minion's file cache, this option can be passed to keep the minion from + re-downloading the file if the cached copy matches the specified hash. + """ + with _client() as client: + if isinstance(dest, str): + result = client.get_url( + path, dest, makedirs, saltenv, source_hash=source_hash + ) + else: + result = client.get_url( + path, None, makedirs, saltenv, no_cache=True, source_hash=source_hash + ) + if not result: + log.error( + "Unable to fetch file %s from saltenv %s.", + salt.utils.url.redact_http_basic_auth(path), + saltenv, + ) + return result + if isinstance(dest, str): + # Return the cache path on the minion, not the local one + result = client.target_map[result] + return salt.utils.stringutils.to_unicode(result) + + +def get_file_str(path, saltenv="base"): + """ + Download a file from a URL to the Minion cache directory and return the + contents of that file. + + Returns ``False`` if Salt was unable to cache a file from a URL. + + CLI Example: + + .. code-block:: bash + + salt-ssh '*' cp.get_file_str salt://my/file + + path + The path on the fileserver, like ``salt://foo/bar/``. It is possible + to specify the ``saltenv`` using the querystring syntax: + ``salt://foo/bar?saltenv=config`` + + saltenv + Salt fileserver environment from which to retrieve the file. + """ + fn_ = cache_file(path, saltenv) + if isinstance(fn_, str): + try: + with salt.utils.files.fopen(fn_, "r") as fp_: + return salt.utils.stringutils.to_unicode(fp_.read()) + except OSError: + return False + return fn_ + + +def cache_file(path, saltenv="base", source_hash=None, verify_ssl=True, use_etag=False): + """ + Cache a single file on the Minion. + Returns the location of the new cached file on the Minion. + If the path being cached is a ``salt://`` URI, and the path does not exist, + then ``False`` will be returned. + + If the path refers to a fileserver path (``salt://`` URI) and this is a state run, + the file will also be added to the package of files that's sent to the minion + for executing the state run (this behaves like ``extra_filerefs``). + + CLI Example: + + .. code-block:: bash + + salt-ssh '*' cp.cache_file salt://path/to/file + + path + The path on the fileserver, like ``salt://foo/bar/``. It is possible + to specify the ``saltenv`` using the querystring syntax: + ``salt://foo/bar?saltenv=config`` + + saltenv + Salt fileserver environment from which to retrieve the file. Ignored if + ``path`` is not a ``salt://`` URL. Defaults to ``base``. + + source_hash + If ``name`` is an http(s) or ftp URL and the file exists in the + minion's file cache, this option can be passed to keep the minion from + re-downloading the file if the cached copy matches the specified hash. + + .. versionadded:: 2018.3.0 + + verify_ssl + If ``False``, remote https file sources (``https://``) and source_hash + will not attempt to validate the servers certificate. Default is True. + + .. versionadded:: 3002 + + use_etag + If ``True``, remote http/https file sources will attempt to use the + ETag header to determine if the remote file needs to be downloaded. + This provides a lightweight mechanism for promptly refreshing files + changed on a web server without requiring a full hash comparison via + the ``source_hash`` parameter. + + .. versionadded:: 3005 + + .. note:: + You can instrumentalize this function in your ``sls`` files to workaround a + limitation in how ``salt-ssh`` handles Jinja imports: + + Imports in templates that will be rendered on the minion (usually during + ``file.managed`` calls) will fail since the corresponding file is not + sent to the minion by default. + + By caching it explicitly in your states, you can ensure it will be included + in the filerefs that will be sent to the minion. + + .. code-block:: jinja + + # /srv/salt/my/config.sls + {%- do salt["cp.cache_file"]("salt://my/map.jinja") %} + + Serialize config: + file.managed: + - name: /etc/my/config.conf + - source: salt://my/files/config.conf.j2 + - template: jinja + + # /srv/salt/my/config.conf.j2 + {%- from "my/map.jinja" import mapdata with context %} + myconf = {{ mapdata["foo"] }} + + # /srv/salt/my/map.jinja + {%- set mapdata = {"foo": "bar"} %} + """ + path = salt.utils.data.decode(path) + saltenv = salt.utils.data.decode(saltenv) + + url_data = urllib.parse.urlparse(path) + if url_data.scheme in ("file", ""): + return __salt__["cp.cache_file_ssh"]( + path, + saltenv=saltenv, + source_hash=source_hash, + verify_ssl=verify_ssl, + use_etag=use_etag, + ) + + contextkey = "{}_|-{}_|-{}".format("cp.cache_file", path, saltenv) + filerefs_ckey = "_cp_extra_filerefs" + url_data = urllib.parse.urlparse(path) + path_is_remote = url_data.scheme in salt.utils.files.REMOTE_PROTOS + + def _check_return(result): + if result and url_data.scheme == "salt": + if filerefs_ckey not in __context__: + __context__[filerefs_ckey] = [] + if path not in __context__[filerefs_ckey]: + __context__[filerefs_ckey].append(path) + return result + + with _client() as client: + try: + if path_is_remote and contextkey in __context__: + # Prevent multiple caches in the same salt run. Affects remote URLs + # since the master won't know their hash, so the fileclient + # wouldn't be able to prevent multiple caches if we try to cache + # the remote URL more than once. + if client._path_exists(__context__[contextkey]): + return _check_return(__context__[contextkey]) + else: + # File is in __context__ but no longer exists in the minion + # cache, get rid of the context key and re-cache below. + # Accounts for corner case where file is removed from minion + # cache between cp.cache_file calls in the same salt-run. + __context__.pop(contextkey) + except AttributeError: + pass + + # saltenv split from path is performed in client + result = client.cache_file( + path, + saltenv, + source_hash=source_hash, + verify_ssl=verify_ssl, + use_etag=use_etag, + ) + if not result and not use_etag: + log.error("Unable to cache file '%s' from saltenv '%s'.", path, saltenv) + if result: + # Return the cache path on the minion, not the local one + result = client.target_map[result] + if path_is_remote: + # Cache was successful, store the result in __context__ to prevent + # multiple caches (see above). + __context__[contextkey] = result + return _check_return(result) + + +def cache_files(paths, saltenv="base"): + """ + Used to gather many files from the Master, the gathered files will be + saved in the minion cachedir reflective to the paths retrieved from the + Master. + + .. note:: + This can take a long time since each file is transferred separately. + + CLI Example: + + .. code-block:: bash + + salt-ssh '*' cp.cache_files salt://pathto/file1,salt://pathto/file1 + + There are two ways of defining the fileserver environment (a.k.a. + ``saltenv``) from which to cache the files. One is to use the ``saltenv`` + parameter, and the other is to use a querystring syntax in the ``salt://`` + URL. The below two examples are equivalent: + + .. code-block:: bash + + salt '*' cp.cache_files salt://foo/bar.conf,salt://foo/baz.conf saltenv=config + salt '*' cp.cache_files salt://foo/bar.conf?saltenv=config,salt://foo/baz.conf?saltenv=config + + The querystring method is less useful when all files are being cached from + the same environment, but is a good way of caching files from multiple + different environments in the same command. For example, the below command + will cache the first file from the ``config1`` environment, and the second + one from the ``config2`` environment. + + .. code-block:: bash + + salt '*' cp.cache_files salt://foo/bar.conf?saltenv=config1,salt://foo/bar.conf?saltenv=config2 + + .. note:: + It may be necessary to quote the URL when using the querystring method, + depending on the shell being used to run the command. + """ + # FIXME: transfer using tar + ret = [] + if isinstance(paths, str): + paths = paths.split(",") + for path in paths: + ret.append(cache_file(path, saltenv)) + return ret + + +def cache_dir( + path, saltenv="base", include_empty=False, include_pat=None, exclude_pat=None +): + """ + Download and cache everything under a directory from the master. + + .. note:: + This can take a long time since each file is transferred separately. + + CLI Example: + + .. code-block:: bash + + salt '*' cp.cache_dir salt://path/to/dir + salt '*' cp.cache_dir salt://path/to/dir include_pat='E@*.py$' + + path + The path on the fileserver, like ``salt://foo/bar/``. It is possible + to specify the ``saltenv`` using the querystring syntax: + ``salt://foo/bar?saltenv=config`` + + saltenv + Salt fileserver environment from which to retrieve the directory. + Defaults to ``base``. + + include_empty + Whether to cache empty directories as well. Defaults to false. + + include_pat : None + Glob or regex to narrow down the files cached from the given path. If + matching with a regex, the regex must be prefixed with ``E@``, + otherwise the expression will be interpreted as a glob. + + .. versionadded:: 2014.7.0 + + exclude_pat : None + Glob or regex to exclude certain files from being cached from the given + path. If matching with a regex, the regex must be prefixed with ``E@``, + otherwise the expression will be interpreted as a glob. + + .. note:: + + If used with ``include_pat``, files matching this pattern will be + excluded from the subset of files defined by ``include_pat``. + + .. versionadded:: 2014.7.0 + """ + # FIXME: transfer using tar + with _client() as client: + ret = client.cache_dir(path, saltenv, include_empty, include_pat, exclude_pat) + if not ret: + return ret + return [client.target_map[x] for x in ret] + + +def cache_master(saltenv="base"): + """ + Retrieve all of the files on the master and cache them locally. + + .. note:: + This can take a long time since each file is transferred separately. + + CLI Example: + + .. code-block:: bash + + salt-ssh '*' cp.cache_master + """ + # FIXME: transfer using tar + with _client() as client: + ret = client.cache_master(saltenv) + if not ret: + return ret + parsed = [] + for file in ret: + try: + parsed.append(client.target_map[file]) + except KeyError: + # Usually because file is False. We can't easily know which one though + log.error("Failed transferring a file") + return parsed def list_states(saltenv="base"): """ - List all the available state modules in an environment + List all of the available state files in an environment. + + CLI Example: + + .. code-block:: bash + + salt-ssh '*' cp.list_states + + saltenv + Salt fileserver environment from which to list states. + Defaults to ``base``. """ return __context__["fileclient"].list_states(saltenv) def list_master(saltenv="base", prefix=""): """ - List all of the files stored on the master + List all of the files stored on the master. + + CLI Example: + + .. code-block:: bash + + salt-ssh '*' cp.list_master + + saltenv + Salt fileserver environment from which to list files. + Defaults to ``base``. + + prefix + Only list files under this prefix. Defaults to empty. """ return __context__["fileclient"].file_list(saltenv, prefix) def list_master_dirs(saltenv="base", prefix=""): """ - List all of the directories stored on the master + List all of the directories stored on the master. + + CLI Example: + + .. code-block:: bash + + salt-ssh '*' cp.list_master_dirs + + saltenv + Salt fileserver environment from which to list directories. + Defaults to ``base``. + + prefix + Only list directories under this prefix. Defaults to empty. """ return __context__["fileclient"].dir_list(saltenv, prefix) def list_master_symlinks(saltenv="base", prefix=""): """ - List all of the symlinks stored on the master + List all of the symlinks stored on the master. + Will return a mapping of symlink names to absolute paths. + + CLI Example: + + .. code-block:: bash + + salt-ssh '*' cp.list_master_symlinks + + saltenv + Salt fileserver environment from which to list symlinks. + Defaults to ``base``. + + prefix + Only list symlinks under this prefix. Defaults to empty. """ return __context__["fileclient"].symlink_list(saltenv, prefix) -def _render_filenames(path, dest, saltenv, template): +def is_cached(path, saltenv="base"): + """ + Returns the full path to a file if it is cached locally on the minion + as well as the SSH master-minion, otherwise returns a blank string. + + CLI Example: + + .. code-block:: bash + + salt-ssh '*' cp.is_cached salt://path/to/file + + path + The path to check. + + saltenv + Salt fileserver environment the file was cached from. + Defaults to ``base``. + """ + # saltenv split is done in client + with _client() as client: + ret = client.is_cached(path, saltenv) + if not ret: + return ret + return str(client.convert_path(ret)) + + +def hash_file(path, saltenv="base"): + """ + Return the hash of a file. Supports ``salt://`` URIs and local files. + Local files should be specified with their absolute paths, without the + ``file://`` scheme. + + CLI Example: + + .. code-block:: bash + + salt-ssh '*' cp.hash_file salt://path/to/file + salt-ssh '*' cp.hash_file /path/to/file + + path + The path to return the hash for. + + saltenv + Salt fileserver environment from which the file should be hashed. + Defaults to ``base``. + """ + path, senv = salt.utils.url.split_env(path) + if senv: + saltenv = senv + url_data = urllib.parse.urlparse(path) + if url_data.scheme in ("file", ""): + return __salt__["cp.hash_file_ssh"](path, saltenv) + with _client() as client: + return client.hash_file(path, saltenv) + + +def convert_cache_path(path, cachedir=None, master=True): + """ + It converts a path received by caching a file to the minion cache to the + corresponding one in the local master cache (or the other way around). + + .. note:: + + This function is exclusive to the SSH wrapper module and is mostly + intended for other wrapper modules to use, not on the CLI. + + CLI Example: + + .. code-block:: bash + + salt-ssh '*' cp.convert_cache_path /var/tmp/.root_abc123_salt/running_data/var/cache/salt/minion/files/base/foo.txt + + path + The path to convert. It has to be in one of the (remote or SSH master-minion) + cachedirs to be converted, otherwise will be returned verbatim. + + cachedir + An optional cachedir override that was used when caching the file. + + master + Whether to convert the path to the master-side path. Defaults + to true (since this module returns the minion paths otherwise). + """ + with _client() as client: + return str(client.convert_path(path, cachedir, master)) + + +def _gather_pillar(pillarenv, pillar_override): + """ + The opts used during pillar rendering should contain the master + opts in the root namespace. self.opts is the modified minion opts, + containing the original master opts in __master_opts__. + """ + popts = {} + # Pillar compilation needs the master opts primarily, + # same as during regular operation. + popts.update(__opts__) + popts.update(__opts__.get("__master_opts__", {})) + pillar = salt.pillar.get_pillar( + popts, + __grains__.value(), + __salt__.kwargs["id_"], + __opts__["saltenv"] or "base", + pillar_override=pillar_override, + pillarenv=pillarenv, + ) + return pillar.compile_pillar() + + +def _render_filenames(path, dest, saltenv, template, **kw): """ Process markup in the :param:`path` and :param:`dest` variables (NOT the files under the paths they ultimately point to) according to the markup @@ -107,7 +821,11 @@ def _render_filenames(path, dest, saltenv, template): kwargs = {} kwargs["salt"] = __salt__.value() - kwargs["pillar"] = __pillar__.value() + if "pillarenv" in kw or "pillar" in kw: + pillarenv = kw.get("pillarenv", __opts__.get("pillarenv")) + kwargs["pillar"] = _gather_pillar(pillarenv, kw.get("pillar")) + else: + kwargs["pillar"] = __pillar__.value() kwargs["grains"] = __grains__.value() kwargs["opts"] = __opts__ kwargs["saltenv"] = saltenv @@ -136,3 +854,354 @@ def _render(contents): path = _render(path) dest = _render(dest) return (path, dest) + + +class SSHCpClient(salt.fileclient.FSClient): + """ + A FileClient that replicates between SSH master-minion and remote minion caches + """ + + def __init__(self, opts, shell, tgt): # pylint: disable=W0231 + salt.fileclient.FSClient.__init__(self, opts) # pylint: disable=W0233 + self.shell = shell + self.tgt = tgt + # Internally, we need to return master paths, but in the wrapper functions, + # we usually want to return the effective path on the minion. + # This client is used for a single execution, thus we can easily save + # all affected file paths for a lookup later. + self.target_map = {} + + def _local_path_exists(self, path): + file = self.convert_path(path, master=True) + return file.exists() + + def _remote_path_exists(self, path): + # ensure it's the minion path + path = self.convert_path(path) + _, _, retcode = self.shell.exec_cmd("test -e " + shlex.quote(str(path))) + return not retcode + + def _path_exists(self, path): + return self._local_path_exists(path) and self._remote_path_exists(path) + + def cache_local_file(self, path, **kwargs): + raise CommandExecutionError("Cannot cache local files via salt-ssh") + + def is_cached(self, path, saltenv="base", cachedir=None): + """ + Returns the full path to a file if it is cached both locally on the + SSH master-minion and the minion, otherwise returns a blank string + """ + if path.startswith("salt://"): + path, senv = salt.utils.url.parse(path) + if senv: + saltenv = senv + + escaped = True if salt.utils.url.is_escaped(path) else False + + # also strip escape character '|' + localsfilesdest = os.path.join( + self.opts["cachedir"], "localfiles", path.lstrip("|/") + ) + filesdest = os.path.join( + self.opts["cachedir"], "files", saltenv, path.lstrip("|/") + ) + extrndest = self._extrn_path(path, saltenv, cachedir=cachedir) + + if self._path_exists(filesdest): + return salt.utils.url.escape(filesdest) if escaped else filesdest + # While we do not cache minion-local files back on the master, + # we can inspect the minion cache dir remotely + if self._remote_path_exists(localsfilesdest): + return ( + salt.utils.url.escape(localsfilesdest) if escaped else localsfilesdest + ) + if self._path_exists(extrndest): + return extrndest + + return "" + + def get_cachedir( + self, cachedir=None, master=True + ): # pylint: disable=arguments-differ + prefix = [] + if master: + prefix = ["salt-ssh", self.tgt] + if cachedir is None: + cachedir = os.path.join(self.opts["cachedir"], *prefix) + elif not os.path.isabs(cachedir): + cachedir = os.path.join(self.opts["cachedir"], *prefix, cachedir) + elif master: + # The root cachedir on the master-side should not be overridden + cachedir = os.path.join( + self.opts["cachedir"], *prefix, "absolute_root", cachedir[1:] + ) + return cachedir + + def convert_path(self, path, cachedir=None, master=False): + """ + Convert a cache path from master/minion to the other. + + Both use the same cachedir in salt-ssh, but our fileclient + here caches to a subdir on the master. Remove/add it from/to + the path. + """ + path = Path(path) + master_cachedir = Path(self.get_cachedir(cachedir, master=True)) + minion_cachedir = Path(self.get_cachedir(cachedir, master=False)) + if master: + # This check could be path.is_relative_to(curr_prefix), + # but that requires Python 3.9 + if master_cachedir in path.parents: + return path + return master_cachedir / path.relative_to(minion_cachedir) + if master_cachedir not in path.parents: + return path + return minion_cachedir / path.relative_to(master_cachedir) + + def _send_file(self, src, dest, makedirs, cachedir): + def _error(stdout, stderr): + log.error(f"Failed sending file: {stderr or stdout}") + if Path(self.get_cachedir(cachedir)) in Path(src).parents: + # remove the cached file if the transfer fails + Path(src).unlink(missing_ok=True) + return False + + for path in (src, dest): + if not Path(path).is_absolute(): + raise ValueError( + f"Paths must be absolute, got '{path}' as {'src' if path == src else 'dest'}" + ) + src, dest = str(src), str(dest) # ensure we're using strings + stdout, stderr, retcode = self.shell.send(src, dest, makedirs) + if retcode and makedirs and "Not a directory" in stderr: + # This means the file path contains a parent that is currently a file + # Remove it, but only if it's in our cache dir + minion_cachedir = Path(self.get_cachedir(cachedir, master=False)) + dest = cur = Path(dest) + while minion_cachedir in cur.parents: + if self._isfile(cur): + if not self._rmpath(cur): + return _error(stdout, stderr) + dest = str(dest) + break + cur = cur.parent + else: + return _error(stdout, stderr) + # The offending file was removed, retry + stdout, stderr, retcode = self.shell.send(src, dest, makedirs) + if retcode: + return _error(stdout, stderr) + self.target_map[src] = dest + # we need to return the local source for internal functionality + return src + + def _isdir(self, path): + _, _, retcode = self.shell.exec_cmd("test -d " + shlex.quote(str(path))) + return not retcode + + def _isfile(self, path): + _, _, retcode = self.shell.exec_cmd("test -f " + shlex.quote(str(path))) + return not retcode + + def _rmpath(self, path, cachedir=None): + path = Path(path) + if not path or not path.is_absolute() or str(path) == "/": + raise ValueError( + f"Not deleting unspecified, relative or root path: '{path}'" + ) + minion_cachedir = Path(self.get_cachedir(cachedir, master=False)) + if minion_cachedir not in path.parents and path != minion_cachedir: + raise ValueError( + f"Not recursively deleting a path outside of the cachedir. Path: '{path}'" + ) + stdout, stderr, retcode = self.shell.exec_cmd( + "rm -rf " + shlex.quote(str(path)) + ) + if retcode: + log.error(f"Failed deleting path '{path}': {stderr or stdout}") + return not retcode + + def get_url( + self, + url, + dest, + makedirs=False, + saltenv="base", + no_cache=False, + cachedir=None, + source_hash=None, + verify_ssl=True, + use_etag=False, + ): + url_data = urllib.parse.urlparse(url) + if url_data.scheme in ("file", ""): + # This should be executed on the minion (unwrapped) + log.error("The file:// scheme is not supported via the salt-ssh cp wrapper") + return False + # Ensure we don't send the file twice + if url_data.scheme == "salt": + result = self.get_file(url, dest, makedirs, saltenv, cachedir=cachedir) + if result and dest is None: + with salt.utils.files.fopen(result, "rb") as fp_: + data = fp_.read() + return data + return result + cached = super().get_url( + url, + "", + makedirs=True, + saltenv=saltenv, + no_cache=no_cache, + cachedir=cachedir, + source_hash=source_hash, + verify_ssl=verify_ssl, + use_etag=use_etag, + ) + if not cached: + return cached + if not isinstance(dest, str) and no_cache: + # Only if dest is None and no_cache is True, the contents + # will be found in cached, otherwise the regular fileclient + # behaves the same as with dest == "" + return cached + strict = False + if not dest: + # The file needs to be cached to the minion cache. + # We're using the same cachedir on the ssh master and the minion, + # but for the master cache, we appended a subdir. Remove it. + makedirs = True + dest = str(self.convert_path(cached, cachedir)) + strict = True + # This is not completely foolproof, but should do the job most + # of the time and is mostly how the regular client handles it. + if dest.endswith("/") or self._isdir(dest): + if not dest.endswith("/"): + if ( + strict + or self.get_cachedir(cachedir, master=False) in Path(dest).parents + ): + strict = True + if not self._rmpath(dest): + Path(cached).unlink(missing_ok=True) + return False + if not strict: + if ( + url_data.query + or len(url_data.path) > 1 + and not url_data.path.endswith("/") + ): + strpath = url.split("/")[-1] + else: + strpath = "index.html" + dest = os.path.join(dest, strpath) + return self._send_file(cached, dest, makedirs, cachedir) + + def get_file( + self, path, dest="", makedirs=False, saltenv="base", gzip=None, cachedir=None + ): + """ + Get a single file from the salt-master + path must be a salt server location, aka, salt://path/to/file, if + dest is omitted, then the downloaded file will be placed in the minion + cache + """ + src = super().get_file( + path, + "", + makedirs=True, + saltenv=saltenv, + cachedir=cachedir, + ) + if not src: + return src + strict = False + # Passing None evokes the same behavior as an empty string + # in the parent class as well, which we want to replicate. + if not dest: + # The file needs to be cached to the minion cache. + # We're using the same cachedir on the ssh master and the minion, + # but for the master cache, we appended a subdir. Remove it. + makedirs = True + dest = str(self.convert_path(src, cachedir)) + strict = True + + # This is not completely foolproof, but should do the job most + # of the time and is mostly how the regular client handles it. + if dest.endswith("/") or self._isdir(dest): + if not dest.endswith("/"): + if ( + strict + or self.get_cachedir(cachedir, master=False) in Path(dest).parents + ): + strict = True + if not self._rmpath(dest): + Path(src).unlink(missing_ok=True) + return "" + if not strict: + dest = os.path.join(dest, os.path.basename(src)) + # TODO replicate hash checks to avoid unnecessary transfers, + # possibly in _send_file to also account for other sources + return self._send_file(src, dest, makedirs, cachedir) + + def get_template( + self, + url, + dest, + template="jinja", + makedirs=False, + saltenv="base", + cachedir=None, + **kwargs, + ): + """ + Cache a file then process it as a template + """ + res = super().get_template( + url, + "", + template=template, + makedirs=makedirs, + saltenv=saltenv, + cachedir=cachedir, + **kwargs, + ) + if not res: + return res + strict = False + if not dest: + # The file needs to be cached to the minion cache. + # We're using the same cachedir on the ssh master and the minion, + # but for the master cache, we appended a subdir. Remove it. + makedirs = True + dest = str(self.convert_path(res, cachedir)) + strict = True + if dest.endswith("/") or self._isdir(dest): + if not dest.endswith("/"): + if ( + strict + or self.get_cachedir(cachedir, master=False) in Path(dest).parents + ): + strict = True + if not self._rmpath(dest): + Path(res).unlink(missing_ok=True) + return "" + if not strict: + dest = os.path.join(dest, os.path.basename(res)) + return self._send_file(res, dest, makedirs, cachedir) + + def _extrn_path(self, url, saltenv, cachedir=None): + # _extrn_path accesses self.opts["cachedir"] directly, + # so we have to wrap it here to ensure our master prefix works + res = super()._extrn_path(url, saltenv, cachedir=cachedir) + return str(self.convert_path(res, cachedir, master=True)) + + def cache_dest(self, url, saltenv="base", cachedir=None): + """ + Return the expected cache location for the specified URL and + environment. + """ + # cache_dest accesses self.opts["cachedir"] directly, + # so we have to wrap it here to ensure our master prefix works + res = super().cache_dest(url, saltenv=saltenv, cachedir=cachedir) + return str(self.convert_path(res, cachedir, master=True)) diff --git a/salt/client/ssh/wrapper/state.py b/salt/client/ssh/wrapper/state.py index b2cfdaf67e45..4f29c92bc176 100644 --- a/salt/client/ssh/wrapper/state.py +++ b/salt/client/ssh/wrapper/state.py @@ -35,7 +35,9 @@ def _ssh_state(chunks, st_kwargs, kwargs, pillar, test=False): file_refs = salt.client.ssh.state.lowstate_file_refs( chunks, _merge_extra_filerefs( - kwargs.get("extra_filerefs", ""), __opts__.get("extra_filerefs", "") + kwargs.get("extra_filerefs", ""), + __opts__.get("extra_filerefs", ""), + __context__.get("_cp_extra_filerefs", ""), ), ) # Create the tar containing the state pkg and relevant files. @@ -201,7 +203,9 @@ def sls(mods, saltenv="base", test=None, exclude=None, **kwargs): file_refs = salt.client.ssh.state.lowstate_file_refs( chunks, _merge_extra_filerefs( - kwargs.get("extra_filerefs", ""), opts.get("extra_filerefs", "") + kwargs.get("extra_filerefs", ""), + opts.get("extra_filerefs", ""), + __context__.get("_cp_extra_filerefs", ""), ), ) @@ -339,7 +343,9 @@ def low(data, **kwargs): file_refs = salt.client.ssh.state.lowstate_file_refs( chunks, _merge_extra_filerefs( - kwargs.get("extra_filerefs", ""), __opts__.get("extra_filerefs", "") + kwargs.get("extra_filerefs", ""), + __opts__.get("extra_filerefs", ""), + __context__.get("_cp_extra_filerefs", ""), ), ) roster = salt.roster.Roster(__opts__, __opts__.get("roster", "flat")) @@ -428,7 +434,9 @@ def high(data, **kwargs): file_refs = salt.client.ssh.state.lowstate_file_refs( chunks, _merge_extra_filerefs( - kwargs.get("extra_filerefs", ""), opts.get("extra_filerefs", "") + kwargs.get("extra_filerefs", ""), + opts.get("extra_filerefs", ""), + __context__.get("_cp_extra_filerefs", ""), ), ) @@ -672,7 +680,9 @@ def highstate(test=None, **kwargs): file_refs = salt.client.ssh.state.lowstate_file_refs( chunks, _merge_extra_filerefs( - kwargs.get("extra_filerefs", ""), opts.get("extra_filerefs", "") + kwargs.get("extra_filerefs", ""), + opts.get("extra_filerefs", ""), + __context__.get("_cp_extra_filerefs", ""), ), ) # Check for errors @@ -766,7 +776,9 @@ def top(topfn, test=None, **kwargs): file_refs = salt.client.ssh.state.lowstate_file_refs( chunks, _merge_extra_filerefs( - kwargs.get("extra_filerefs", ""), opts.get("extra_filerefs", "") + kwargs.get("extra_filerefs", ""), + opts.get("extra_filerefs", ""), + __context__.get("_cp_extra_filerefs", ""), ), ) @@ -1195,7 +1207,9 @@ def single(fun, name, test=None, **kwargs): file_refs = salt.client.ssh.state.lowstate_file_refs( chunks, _merge_extra_filerefs( - kwargs.get("extra_filerefs", ""), opts.get("extra_filerefs", "") + kwargs.get("extra_filerefs", ""), + opts.get("extra_filerefs", ""), + __context__.get("_cp_extra_filerefs", ""), ), ) diff --git a/salt/modules/cp.py b/salt/modules/cp.py index ee4d19c3b7f6..346de51b1e37 100644 --- a/salt/modules/cp.py +++ b/salt/modules/cp.py @@ -15,6 +15,7 @@ import salt.minion import salt.utils.data import salt.utils.files +import salt.utils.functools import salt.utils.gzip_util import salt.utils.path import salt.utils.templates @@ -148,7 +149,7 @@ def _error(msg): log.debug("Setting mode for %s to %s", dest, mode) try: os.chmod(dest, mode) - except OSError: + except OSError as exc: return _error(exc.__str__()) return True finally: @@ -565,6 +566,9 @@ def cache_file(path, saltenv=None, source_hash=None, verify_ssl=True, use_etag=F return result +cache_file_ssh = salt.utils.functools.alias_function(cache_file, "cache_file_ssh") + + def cache_dest(url, saltenv=None): """ .. versionadded:: 3000 @@ -733,7 +737,7 @@ def list_states(saltenv=None): .. versionchanged:: 3005 ``saltenv`` will use value from config if not explicitly set - List all of the available state modules in an environment + List all of the available state files in an environment CLI Example: @@ -874,6 +878,9 @@ def hash_file(path, saltenv=None): return client.hash_file(path, saltenv) +hash_file_ssh = salt.utils.functools.alias_function(hash_file, "hash_file_ssh") + + def stat_file(path, saltenv=None, octal=True): """ .. versionchanged:: 3005 diff --git a/tests/pytests/integration/ssh/test_cp.py b/tests/pytests/integration/ssh/test_cp.py new file mode 100644 index 000000000000..051b07138776 --- /dev/null +++ b/tests/pytests/integration/ssh/test_cp.py @@ -0,0 +1,896 @@ +import hashlib +import os +import time +from pathlib import Path + +import pytest + +from tests.support.runtests import RUNTIME_VARS + +pytestmark = [ + pytest.mark.slow_test, + pytest.mark.skip_on_windows(reason="salt-ssh not available on Windows"), +] + + +@pytest.fixture(scope="module", autouse=True) +def pillar_tree(base_env_pillar_tree_root_dir): + top_file = """ + base: + 'localhost': + - basic + '127.0.0.1': + - basic + """ + basic_pillar_file = """ + test_pillar: cheese + alot: many + script: grail + """ + top_tempfile = pytest.helpers.temp_file( + "top.sls", top_file, base_env_pillar_tree_root_dir + ) + basic_tempfile = pytest.helpers.temp_file( + "basic.sls", basic_pillar_file, base_env_pillar_tree_root_dir + ) + + with top_tempfile, basic_tempfile: + yield + + +@pytest.fixture(scope="module") +def cachedir(salt_ssh_cli): + """ + The current minion cache dir + """ + # The salt-ssh cache dir in the minion context is different than + # the one available in the salt_ssh_cli opts. Any other way to get this? TODO + res = salt_ssh_cli.run("cp.cache_dest", "salt://file") + assert res.returncode == 0 + assert isinstance(res.data, str) + # This will return /files/base/file + return Path(res.data).parent.parent.parent + + +def _convert(cli, cachedir, path, master=False): + curr_prefix = cachedir / "salt-ssh" / cli.get_minion_tgt() + if not isinstance(path, Path): + path = Path(path) + if master: + if curr_prefix in path.parents: + return path + return curr_prefix / path.relative_to(cachedir) + if curr_prefix not in path.parents: + return path + return cachedir / Path(path).relative_to(curr_prefix) + + +@pytest.mark.parametrize("template", (None, "jinja")) +@pytest.mark.parametrize("dst_is_dir", (False, True)) +def test_get_file(salt_ssh_cli, tmp_path, template, dst_is_dir, cachedir): + src = "salt://" + ("cheese" if not template else "{{pillar.test_pillar}}") + if dst_is_dir: + tgt = tmp_path + else: + tgt = tmp_path / ("cheese" if not template else "{{pillar.test_pillar}}") + res = salt_ssh_cli.run("cp.get_file", src, str(tgt), template=template) + assert res.returncode == 0 + assert res.data + tgt = tmp_path / "cheese" + assert res.data == str(tgt) + master_path = ( + cachedir + / "salt-ssh" + / salt_ssh_cli.get_minion_tgt() + / "files" + / "base" + / "cheese" + ) + for path in (tgt, master_path): + assert path.exists() + data = path.read_text() + assert "Gromit" in data + assert "bacon" not in data + + +def test_get_file_gzipped(salt_ssh_cli, caplog, tmp_path): + tgt = tmp_path / "foo" + res = salt_ssh_cli.run("cp.get_file", "salt://grail/scene33", str(tgt), gzip=5) + assert res.returncode == 0 + assert res.data + assert res.data == str(tgt) + assert "The gzip argument to cp.get_file in salt-ssh is unsupported" in caplog.text + assert tgt.exists() + data = tgt.read_text() + assert "KNIGHT: They're nervous, sire." in data + assert "bacon" not in data + + +def test_get_file_makedirs(salt_ssh_cli, tmp_path, cachedir): + tgt = tmp_path / "make" / "dirs" / "scene33" + res = salt_ssh_cli.run( + "cp.get_file", "salt://grail/scene33", str(tgt), makedirs=True + ) + assert res.returncode == 0 + assert res.data + assert res.data == str(tgt) + master_path = ( + cachedir + / "salt-ssh" + / salt_ssh_cli.get_minion_tgt() + / "files" + / "base" + / "grail" + / "scene33" + ) + for path in (tgt, master_path): + assert path.exists() + data = path.read_text() + assert "KNIGHT: They're nervous, sire." in data + assert "bacon" not in data + + +@pytest.mark.parametrize("suffix", ("", "?saltenv=prod")) +def test_get_file_from_env(salt_ssh_cli, tmp_path, suffix): + tgt = tmp_path / "cheese" + ret = salt_ssh_cli.run("cp.get_file", "salt://cheese" + suffix, str(tgt)) + assert ret.returncode == 0 + assert ret.data + assert ret.data == str(tgt) + data = tgt.read_text() + assert "Gromit" in data + assert ("Comte" in data) is bool(suffix) + + +def test_get_file_nonexistent_source(salt_ssh_cli): + res = salt_ssh_cli.run("cp.get_file", "salt://grail/nonexistent_scene", "") + assert res.returncode == 0 # not a fan of this + assert res.data == "" + + +def test_envs(salt_ssh_cli): + ret = salt_ssh_cli.run("cp.envs") + assert ret.returncode == 0 + assert ret.data + assert isinstance(ret.data, list) + assert sorted(ret.data) == sorted(["base", "prod"]) + + +def test_get_template(salt_ssh_cli, tmp_path, cachedir): + tgt = tmp_path / "scene33" + res = salt_ssh_cli.run( + "cp.get_template", "salt://grail/scene33", str(tgt), spam="bacon" + ) + assert res.returncode == 0 + assert res.data + assert res.data == str(tgt) + master_path = ( + cachedir + / "salt-ssh" + / salt_ssh_cli.get_minion_tgt() + / "extrn_files" + / "base" + / "grail" + / "scene33" + ) + for path in (tgt, master_path): + assert tgt.exists() + data = tgt.read_text() + assert "bacon" in data + assert "spam" not in data + + +def test_get_template_dest_empty(salt_ssh_cli, cachedir): + res = salt_ssh_cli.run("cp.get_template", "salt://grail/scene33", "", spam="bacon") + assert res.returncode == 0 + assert res.data + assert isinstance(res.data, str) + master_path = ( + cachedir + / "salt-ssh" + / salt_ssh_cli.get_minion_tgt() + / "extrn_files" + / "base" + / "grail" + / "scene33" + ) + tgt = _convert(salt_ssh_cli, cachedir, master_path) + assert res.data == str(tgt) + for file in (tgt, master_path): + assert file.exists() + data = file.read_text() + assert "bacon" in data + assert "spam" not in data + + +def test_get_template_nonexistent_source(salt_ssh_cli, tmp_path): + res = salt_ssh_cli.run("cp.get_template", "salt://grail/nonexistent_scene", "") + assert res.returncode == 0 # not a fan of this + assert res.data == "" + # The regular module only logs "unable to fetch" with get_url + + +@pytest.mark.parametrize("template", (None, "jinja")) +@pytest.mark.parametrize("suffix", ("", "/")) +def test_get_dir(salt_ssh_cli, tmp_path, template, suffix, cachedir): + tgt = tmp_path / ("many" if not template else "{{pillar.alot}}") + res = salt_ssh_cli.run( + "cp.get_dir", + "salt://" + ("grail" if not template else "{{pillar.script}}"), + str(tgt) + suffix, + template=template, + ) + assert res.returncode == 0 + assert res.data + assert isinstance(res.data, list) + tgt = tmp_path / "many" + master_path = ( + cachedir / "salt-ssh" / salt_ssh_cli.get_minion_tgt() / "files" / "base" + ) + for path in (tgt, master_path): + assert path.exists() + assert "grail" in os.listdir(path) + assert "36" in os.listdir(path / "grail") + assert "empty" in os.listdir(path / "grail") + assert "scene" in os.listdir(path / "grail" / "36") + if path == master_path: + # otherwise we would include other cached files + path = path / "grail" + files = {str(master_path / Path(x).relative_to(tgt)) for x in res.data} + else: + files = set(res.data) + # The regular cp.get_dir keeps superfluous path separators as well + filelist = { + str(x).replace(str(tgt), str(tgt) + suffix) + for x in path.rglob("*") + if not x.is_dir() + } + assert files == filelist + + +def test_get_dir_gzipped(salt_ssh_cli, caplog, tmp_path): + tgt = tmp_path / "many" + res = salt_ssh_cli.run("cp.get_dir", "salt://grail", tgt, gzip=5) + assert "The gzip argument to cp.get_dir in salt-ssh is unsupported" in caplog.text + assert res.returncode == 0 + assert res.data + tgt = tmp_path / "many" + assert isinstance(res.data, list) + assert tgt.exists() + assert "grail" in os.listdir(tgt) + assert "36" in os.listdir(tgt / "grail") + assert "empty" in os.listdir(tgt / "grail") + assert "scene" in os.listdir(tgt / "grail" / "36") + + +def test_get_dir_nonexistent_source(salt_ssh_cli, caplog): + res = salt_ssh_cli.run("cp.get_dir", "salt://grail/non/ex/is/tent", "") + assert res.returncode == 0 # not a fan of this + assert isinstance(res.data, list) + assert not res.data + + +@pytest.mark.parametrize("dst_is_dir", (False, True)) +def test_get_url(salt_ssh_cli, tmp_path, dst_is_dir, cachedir): + src = "salt://grail/scene33" + if dst_is_dir: + tgt = tmp_path + else: + tgt = tmp_path / "scene33" + res = salt_ssh_cli.run("cp.get_url", src, str(tgt)) + assert res.returncode == 0 + assert res.data + tgt = tmp_path / "scene33" + assert res.data == str(tgt) + master_path = ( + cachedir + / "salt-ssh" + / salt_ssh_cli.get_minion_tgt() + / "files" + / "base" + / "grail" + / "scene33" + ) + for file in (tgt, master_path): + assert file.exists() + data = file.read_text() + assert "KNIGHT: They're nervous, sire." in data + assert "bacon" not in data + + +def test_get_url_makedirs(salt_ssh_cli, tmp_path, cachedir): + tgt = tmp_path / "make" / "dirs" / "scene33" + res = salt_ssh_cli.run( + "cp.get_url", "salt://grail/scene33", str(tgt), makedirs=True + ) + assert res.returncode == 0 + assert res.data + assert res.data == str(tgt) + master_path = ( + cachedir + / "salt-ssh" + / salt_ssh_cli.get_minion_tgt() + / "files" + / "base" + / "grail" + / "scene33" + ) + for file in (tgt, master_path): + assert file.exists() + data = file.read_text() + assert "KNIGHT: They're nervous, sire." in data + assert "bacon" not in data + + +def test_get_url_dest_empty(salt_ssh_cli, cachedir): + """ + salt:// source and destination omitted, should still cache the file + """ + res = salt_ssh_cli.run("cp.get_url", "salt://grail/scene33") + assert res.returncode == 0 + assert res.data + assert isinstance(res.data, str) + master_path = ( + cachedir + / "salt-ssh" + / salt_ssh_cli.get_minion_tgt() + / "files" + / "base" + / "grail" + / "scene33" + ) + tgt = _convert(salt_ssh_cli, cachedir, master_path) + assert res.data == str(tgt) + for file in (tgt, master_path): + assert file.exists() + data = file.read_text() + assert "KNIGHT: They're nervous, sire." in data + assert "bacon" not in data + + +def test_get_url_no_dest(salt_ssh_cli): + """ + salt:// source given and destination set as None, should return the data + """ + res = salt_ssh_cli.run("cp.get_url", "salt://grail/scene33", None) + assert res.returncode == 0 + assert res.data + assert isinstance(res.data, str) + assert "KNIGHT: They're nervous, sire." in res.data + assert "bacon" not in res.data + + +def test_get_url_nonexistent_source(salt_ssh_cli, caplog): + res = salt_ssh_cli.run("cp.get_url", "salt://grail/nonexistent_scene", None) + assert res.returncode == 0 # not a fan of this + assert res.data is False + assert ( + "Unable to fetch file salt://grail/nonexistent_scene from saltenv base." + in caplog.text + ) + + +def test_get_url_https(salt_ssh_cli, tmp_path, cachedir): + tgt = tmp_path / "index.html" + res = salt_ssh_cli.run("cp.get_url", "https://repo.saltproject.io/index.html", tgt) + assert res.returncode == 0 + assert res.data + assert res.data == str(tgt) + master_path = ( + cachedir + / "salt-ssh" + / salt_ssh_cli.get_minion_tgt() + / "extrn_files" + / "base" + / "repo.saltproject.io" + / "index.html" + ) + for path in (tgt, master_path): + assert path.exists() + data = path.read_text() + assert "Salt Project" in data + assert "Package" in data + assert "Repo" in data + assert "AYBABTU" not in data + + +def test_get_url_https_dest_empty(salt_ssh_cli, tmp_path, cachedir): + """ + https:// source given and destination omitted, should still cache the file + """ + res = salt_ssh_cli.run("cp.get_url", "https://repo.saltproject.io/index.html") + assert res.returncode == 0 + assert res.data + master_path = ( + cachedir + / "salt-ssh" + / salt_ssh_cli.get_minion_tgt() + / "extrn_files" + / "base" + / "repo.saltproject.io" + / "index.html" + ) + tgt = _convert(salt_ssh_cli, cachedir, master_path) + assert res.data == str(tgt) + for path in (tgt, master_path): + assert path.exists() + data = path.read_text() + assert "Salt Project" in data + assert "Package" in data + assert "Repo" in data + assert "AYBABTU" not in data + + +def test_get_url_https_no_dest(salt_ssh_cli): + """ + https:// source given and destination set as None, should return the data + """ + timeout = 500 + start = time.time() + sleep = 5 + while time.time() - start <= timeout: + res = salt_ssh_cli.run( + "cp.get_url", "https://repo.saltproject.io/index.html", None + ) + if isinstance(res.data, str) and res.data.find("HTTP 599") == -1: + break + time.sleep(sleep) + if isinstance(res.data, str) and res.data.find("HTTP 599") != -1: + raise Exception("https://repo.saltproject.io/index.html returned 599 error") + assert res.returncode == 0 + assert res.data + assert isinstance(res.data, str) + assert "Salt Project" in res.data + assert "Package" in res.data + assert "Repo" in res.data + assert "AYBABTU" not in res.data + + +@pytest.mark.parametrize("scheme", ("file://", "")) +@pytest.mark.parametrize( + "path,expected", + ( + (Path(RUNTIME_VARS.FILES) / "file" / "base" / "file.big", True), + (Path("_foo", "bar", "baz"), False), + ), +) +def test_get_url_file(salt_ssh_cli, path, expected, scheme): + """ + Ensure the file:// scheme is not supported + """ + res = salt_ssh_cli.run("cp.get_url", scheme + str(path)) + assert res.returncode == 0 + assert res.data is False + + +def test_get_url_file_contents(salt_ssh_cli, tmp_path, caplog): + """ + A file:// source is not supported since it would need to fetch + a file from the minion onto the master to be consistent + """ + src = Path(RUNTIME_VARS.FILES) / "file" / "base" / "file.big" + res = salt_ssh_cli.run("cp.get_url", "file://" + str(src), None) + assert res.returncode == 0 + assert res.data is False + assert ( + "The file:// scheme is not supported via the salt-ssh cp wrapper" in caplog.text + ) + + +def test_get_url_ftp(salt_ssh_cli, tmp_path, cachedir): + tgt = tmp_path / "README.TXT" + res = salt_ssh_cli.run( + "cp.get_url", "ftp://ftp.freebsd.org/pub/FreeBSD/releases/amd64/README.TXT", tgt + ) + assert res.returncode == 0 + assert res.data + assert res.data == str(tgt) + master_path = ( + cachedir + / "salt-ssh" + / salt_ssh_cli.get_minion_tgt() + / "extrn_files" + / "base" + / "ftp.freebsd.org" + / "pub" + / "FreeBSD" + / "releases" + / "amd64" + / "README.TXT" + ) + for path in (tgt, master_path): + assert path.exists() + data = path.read_text() + assert "The official FreeBSD" in data + + +def test_get_file_str_salt(salt_ssh_cli, cachedir): + src = "salt://grail/scene33" + res = salt_ssh_cli.run("cp.get_file_str", src) + assert res.returncode == 0 + assert res.data + assert isinstance(res.data, str) + assert "KNIGHT: They're nervous, sire." in res.data + tgt = cachedir / "files" / "base" / "grail" / "scene33" + master_path = _convert(salt_ssh_cli, cachedir, tgt, master=True) + for path in (tgt, master_path): + assert path.exists() + text = path.read_text() + assert "KNIGHT: They're nervous, sire." in text + + +def test_get_file_str_nonexistent_source(salt_ssh_cli, caplog): + src = "salt://grail/nonexistent_scene" + res = salt_ssh_cli.run("cp.get_file_str", src) + assert res.returncode == 0 # yup... + assert res.data is False + + +def test_get_file_str_https(salt_ssh_cli, cachedir): + src = "https://repo.saltproject.io/index.html" + res = salt_ssh_cli.run("cp.get_file_str", src) + assert res.returncode == 0 + assert res.data + assert isinstance(res.data, str) + assert "Salt Project" in res.data + assert "Package" in res.data + assert "Repo" in res.data + assert "AYBABTU" not in res.data + tgt = cachedir / "extrn_files" / "base" / "repo.saltproject.io" / "index.html" + master_path = _convert(salt_ssh_cli, cachedir, tgt, master=True) + for path in (tgt, master_path): + assert path.exists() + text = path.read_text() + assert "Salt Project" in text + assert "Package" in text + assert "Repo" in text + assert "AYBABTU" not in text + + +def test_get_file_str_local(salt_ssh_cli, cachedir, caplog): + src = Path(RUNTIME_VARS.FILES) / "file" / "base" / "cheese" + res = salt_ssh_cli.run("cp.get_file_str", "file://" + str(src)) + assert res.returncode == 0 + assert isinstance(res.data, str) + assert "Gromit" in res.data + assert ( + "The file:// scheme is not supported via the salt-ssh cp wrapper" + not in caplog.text + ) + + +@pytest.mark.parametrize("suffix", ("", "?saltenv=prod")) +def test_cache_file(salt_ssh_cli, suffix, cachedir): + res = salt_ssh_cli.run("cp.cache_file", "salt://cheese" + suffix) + assert res.returncode == 0 + assert res.data + tgt = ( + cachedir + / "files" + / ("base" if "saltenv" not in suffix else suffix.split("=")[1]) + / "cheese" + ) + master_path = _convert(salt_ssh_cli, cachedir, tgt, master=True) + for file in (tgt, master_path): + data = file.read_text() + assert "Gromit" in data + assert ("Comte" in data) is bool(suffix) + + +@pytest.fixture +def _cache_twice(base_env_state_tree_root_dir, request, salt_ssh_cli, cachedir): + # ensure the cache is clean + tgt = cachedir / "extrn_files" / "base" / "repo.saltproject.io" / "index.html" + tgt.unlink(missing_ok=True) + master_tgt = _convert(salt_ssh_cli, cachedir, tgt, master=True) + master_tgt.unlink(missing_ok=True) + + # create a template that will cause a file to get cached twice + # within the same context + name = "cp_cache" + src = "https://repo.saltproject.io/index.html" + remove = getattr(request, "param", False) + contents = f""" +{{%- set cache = salt["cp.cache_file"]("{src}") %}} +{{%- if not cache %}} +{{#- Stop rendering. It's one of the only ways to throw an exception + during master-side rendering currently (in order to fail it). +#}} +{{%- do salt["cp.get_file"]("foobar", template="thisthrowsanexception") %}} +{{%- endif %}} +{{%- set master_cache = salt["cp.convert_cache_path"](cache, master=true) %}} +{{%- do salt["file.append"](cache, "\nwasmodifiedhahaha") %}} +{{%- do salt["file.append"](master_cache, "\nwasmodifiedhahaha") %}} +""" + if remove: + contents += f""" +{{%- do salt["file.remove"]({'master_cache' if remove == 'master' else 'cache'}) %}}""" + contents += f""" +{{%- set res2 = salt["cp.cache_file"]("{src}") %}} +{{{{ res2 }}}} + """ + with pytest.helpers.temp_file(name, contents, base_env_state_tree_root_dir): + yield f"salt://{name}" + + +def test_cache_file_context_cache(salt_ssh_cli, cachedir, _cache_twice): + res = salt_ssh_cli.run("slsutil.renderer", _cache_twice, default_renderer="jinja") + assert res.returncode == 0 + tgt = res.data.strip() + assert tgt + tgt = Path(tgt) + for file in (tgt, _convert(salt_ssh_cli, cachedir, tgt, master=True)): + assert tgt.exists() + # If both files were present, they should not be re-fetched + assert "wasmodifiedhahaha" in tgt.read_text() + + +@pytest.mark.parametrize("_cache_twice", ("master", "minion"), indirect=True) +def test_cache_file_context_cache_requires_both_caches( + salt_ssh_cli, cachedir, _cache_twice +): + res = salt_ssh_cli.run("slsutil.renderer", _cache_twice, default_renderer="jinja") + assert res.returncode == 0 + tgt = res.data.strip() + assert tgt + tgt = Path(tgt) + for file in (tgt, _convert(salt_ssh_cli, cachedir, tgt, master=True)): + assert tgt.exists() + # If one of the files was removed, it should be re-fetched + assert "wasmodifiedhahaha" not in tgt.read_text() + + +def test_cache_file_nonexistent_source(salt_ssh_cli): + res = salt_ssh_cli.run("cp.get_template", "salt://grail/nonexistent_scene", "") + assert res.returncode == 0 # not a fan of this + assert res.data == "" + # The regular module only logs "unable to fetch" with get_url + + +@pytest.mark.parametrize( + "files", + ( + ["salt://grail/scene33", "salt://grail/36/scene"], + "salt://grail/scene33,salt://grail/36/scene", + ), +) +def test_cache_files(salt_ssh_cli, files): + res = salt_ssh_cli.run("cp.cache_files", files) + assert res.returncode == 0 + assert res.data + assert isinstance(res.data, list) + for path in res.data: + assert isinstance(path, str) + path = Path(path) + assert path.exists() + data = Path(path).read_text() + assert "ARTHUR:" in data + assert "bacon" not in data + + +def test_cache_dir(salt_ssh_cli, cachedir): + res = salt_ssh_cli.run("cp.cache_dir", "salt://grail") + assert res.returncode == 0 + assert res.data + assert isinstance(res.data, list) + tgt = cachedir / "files" / "base" / "grail" + master_path = _convert(salt_ssh_cli, cachedir, tgt, master=True) + for path in (tgt, master_path): + assert path.exists() + assert "36" in os.listdir(path) + assert "empty" in os.listdir(path) + assert "scene" in os.listdir(path / "36") + if path == master_path: + files = {str(master_path / Path(x).relative_to(tgt)) for x in res.data} + else: + files = set(res.data) + filelist = {str(x) for x in path.rglob("*") if not x.is_dir()} + assert files == filelist + + +def test_cache_dir_nonexistent_source(salt_ssh_cli, caplog): + res = salt_ssh_cli.run("cp.cache_dir", "salt://grail/non/ex/is/tent", "") + assert res.returncode == 0 # not a fan of this + assert isinstance(res.data, list) + assert not res.data + + +def test_list_states(salt_ssh_cli, tmp_path, base_env_state_tree_root_dir): + top_sls = """ + base: + '*': + - core + """ + tgt = tmp_path / "testfile" + + core_state = f""" + {tgt}/testfile: + file.managed: + - source: salt://testfile + - makedirs: true + """ + + with pytest.helpers.temp_file( + "top.sls", top_sls, base_env_state_tree_root_dir + ), pytest.helpers.temp_file("core.sls", core_state, base_env_state_tree_root_dir): + res = salt_ssh_cli.run( + "cp.list_states", + ) + assert res.returncode == 0 + assert res.data + assert isinstance(res.data, list) + assert "core" in res.data + assert "top" in res.data + assert "cheese" not in res.data + + +def test_list_master(salt_ssh_cli): + res = salt_ssh_cli.run("cp.list_master") + assert res.returncode == 0 + assert res.data + assert isinstance(res.data, list) + for file in [ + "cheese", + "grail/empty", + "grail/36/scene", + "_modules/salttest.py", + "running.sls", + "test_deep/a/test.sls", + ]: + assert file in res.data + assert "test_deep/a" not in res.data + + +def test_list_master_dirs(salt_ssh_cli): + res = salt_ssh_cli.run("cp.list_master_dirs") + assert res.returncode == 0 + assert res.data + assert isinstance(res.data, list) + for path in ["test_deep", "test_deep/a", "test_deep/b/2"]: + assert path in res.data + for path in [ + "test_deep/test.sls", + "test_deep/a/test.sls", + "test_deep/b/2/test.sls", + "cheese", + ]: + assert path not in res.data + + +def test_list_master_symlinks(salt_ssh_cli, base_env_state_tree_root_dir): + if salt_ssh_cli.config.get("fileserver_ignoresymlinks", False): + pytest.skip("Fileserver is configured to ignore symlinks") + with pytest.helpers.temp_file("foo", "", base_env_state_tree_root_dir) as tgt: + sym = tgt.parent / "test_list_master_symlinks" + try: + sym.symlink_to(tgt) + res = salt_ssh_cli.run("cp.list_master_symlinks") + assert res.returncode == 0 + assert res.data + assert isinstance(res.data, dict) + assert res.data + assert sym.name in res.data + assert res.data[sym.name] == str(tgt) + finally: + sym.unlink() + + +@pytest.fixture(params=(False, "cached", "render_cached")) +def _is_cached(salt_ssh_cli, suffix, request, cachedir): + remove = ["files", "extrn_files"] + if request.param == "cached": + ret = salt_ssh_cli.run("cp.cache_file", "salt://grail/scene33" + suffix) + assert ret.returncode == 0 + assert ret.data + remove.remove("files") + elif request.param == "render_cached": + ret = salt_ssh_cli.run( + "cp.get_template", "salt://grail/scene33" + suffix, "", spam="bacon" + ) + assert ret.returncode == 0 + assert ret.data + remove.remove("extrn_files") + for basedir in remove: + tgt = cachedir / basedir / "base" / "grail" / "scene33" + tgt.unlink(missing_ok=True) + master_tgt = _convert(salt_ssh_cli, cachedir, tgt, master=True) + master_tgt.unlink(missing_ok=True) + return request.param + + +@pytest.mark.parametrize("suffix", ("", "?saltenv=base")) +def test_is_cached(salt_ssh_cli, cachedir, _is_cached, suffix): + """ + is_cached should find both cached files from the fileserver as well + as cached rendered templates + """ + if _is_cached == "render_cached": + tgt = cachedir / "extrn_files" / "base" / "grail" / "scene33" + else: + tgt = cachedir / "files" / "base" / "grail" / "scene33" + res = salt_ssh_cli.run("cp.is_cached", "salt://grail/scene33" + suffix) + assert res.returncode == 0 + assert (res.data == str(tgt)) is bool(_is_cached) + assert (res.data != "") is bool(_is_cached) + + +def test_is_cached_nonexistent(salt_ssh_cli): + res2 = salt_ssh_cli.run("cp.is_cached", "salt://fasldkgj/poicxzbn") + assert res2.returncode == 0 + assert res2.data == "" + + +@pytest.mark.parametrize("suffix", ("", "?saltenv=base")) +def test_hash_file(salt_ssh_cli, cachedir, suffix): + res = salt_ssh_cli.run("cp.hash_file", "salt://grail/scene33" + suffix) + assert res.returncode == 0 + assert res.data + sha256_hash = res.data["hsum"] + res = salt_ssh_cli.run("cp.cache_file", "salt://grail/scene33") + assert res.returncode == 0 + assert res.data + master_path = _convert(salt_ssh_cli, cachedir, res.data, master=True) + assert master_path.exists() + data = master_path.read_bytes() + digest = hashlib.sha256(data).hexdigest() + assert digest == sha256_hash + + +def test_hash_file_local(salt_ssh_cli, caplog): + """ + Ensure that local files are run through ``salt-call`` on the target. + We have to trust that this would otherwise fail because the tests + run against localhost. + """ + path = Path(RUNTIME_VARS.FILES) / "file" / "base" / "cheese" + res = salt_ssh_cli.run("cp.hash_file", str(path)) + assert res.returncode == 0 + # This would be logged if SSHCpClient was used instead of + # performing a shimmed salt-call command + assert "Hashing local files is not supported via salt-ssh" not in caplog.text + assert isinstance(res.data, dict) + assert res.data + data = path.read_bytes() + digest = hashlib.sha256(data).hexdigest() + sha256_hash = res.data["hsum"] + assert digest == sha256_hash + + +@pytest.fixture +def state_tree_jinjaimport(base_env_state_tree_root_dir, tmp_path): + tgt = tmp_path / "config.conf" + base_path = base_env_state_tree_root_dir / "my" + map_contents = """{%- set mapdata = {"foo": "bar"} %}""" + managed_contents = """ + {%- from "my/map.jinja" import mapdata with context %} + {{- mapdata["foo"] -}} + """ + state_contents = f""" +{{%- do salt["cp.cache_file"]("salt://my/map.jinja") %}} + +Serialize config: + file.managed: + - name: {tgt} + - source: salt://my/files/config.conf.j2 + - template: jinja +""" + with pytest.helpers.temp_file( + "file_managed_import.sls", state_contents, base_path + ) as state: + with pytest.helpers.temp_file("map.jinja", map_contents, base_path): + with pytest.helpers.temp_file( + "config.conf.j2", managed_contents, base_path / "files" + ): + yield f"my.{state.stem}" + + +def test_cp_cache_file_as_workaround_for_missing_map_file( + salt_ssh_cli, state_tree_jinjaimport, tmp_path +): + tgt = tmp_path / "config.conf" + ret = salt_ssh_cli.run("state.sls", state_tree_jinjaimport) + assert ret.returncode == 0 + assert isinstance(ret.data, dict) + assert ret.data + assert tgt.exists() + assert tgt.read_text().strip() == "bar" diff --git a/tests/pytests/unit/client/ssh/test_shell.py b/tests/pytests/unit/client/ssh/test_shell.py index 0b87ec1082aa..b415e06ee9f2 100644 --- a/tests/pytests/unit/client/ssh/test_shell.py +++ b/tests/pytests/unit/client/ssh/test_shell.py @@ -98,3 +98,16 @@ def test_ssh_shell_exec_cmd_returns_status_code_with_highest_bit_set_if_process_ assert stdout == "" assert stderr == "leave me alone please" assert retcode == 137 + + +def test_ssh_shell_send_makedirs_failure_returns_immediately(): + def exec_cmd(cmd): + if cmd.startswith("mkdir -p"): + return "", "Not a directory", 1 + return "", "", 0 + + with patch("salt.client.ssh.shell.Shell.exec_cmd", side_effect=exec_cmd): + shl = shell.Shell({}, "localhost") + stdout, stderr, retcode = shl.send("/tmp/file", "/tmp/file", True) + assert retcode == 1 + assert "Not a directory" in stderr diff --git a/tests/pytests/unit/client/ssh/wrapper/test_cp.py b/tests/pytests/unit/client/ssh/wrapper/test_cp.py new file mode 100644 index 000000000000..698e72577ef3 --- /dev/null +++ b/tests/pytests/unit/client/ssh/wrapper/test_cp.py @@ -0,0 +1,1156 @@ +""" +Test the SSHCpClient. + +The following tests are adapted from +tests.pytests.unit.fileclient.test_fileclient_cache. + ++ additional ones below +""" +import errno +import logging +import os +import shlex +import shutil +from pathlib import Path + +import pytest + +import salt.client.ssh.shell +import salt.exceptions +import salt.utils.files +from salt.client.ssh.wrapper import cp +from tests.support.mock import Mock, call, patch + +log = logging.getLogger(__name__) + + +SUBDIR = "subdir" +TGT = "testtarget" + + +def _saltenvs(): + return ("base", "dev") + + +def _subdir_files(): + return ("foo.txt", "bar.txt", "baz.txt") + + +def _get_file_roots(fs_root): + return {x: [os.path.join(fs_root, x)] for x in _saltenvs()} + + +@pytest.fixture +def fs_root(tmp_path): + return os.path.join(tmp_path, "fileclient_fs_root") + + +@pytest.fixture +def cache_root(tmp_path): + return os.path.join(tmp_path, "fileclient_cache_root") + + +@pytest.fixture +def remote_list(cache_root): + cache_root = Path(cache_root) + return { + "files": { + x + for env in _saltenvs() + for x in ( + Path("/tmp/targetdir/existingfile"), + cache_root / f"files/{env}/filetodir", + cache_root / f"extrn_files/{env}/filetodir", + cache_root / f"files/{env}/foo.sls", + cache_root / f"files/{env}/nested/path/foo.sls", + cache_root / f"extrn_files/{env}/bar.sls", + cache_root / f"files/{env}/rmfail/rmfailfile", + cache_root / "localfiles/this/file/was/cached/locally", + ) + }, + "dirs": { + x + for env in _saltenvs() + for x in ( + (Path("/tmp/targetdir"),) + + tuple((cache_root / f"extrn_files/{env}/dummy").parents) + + tuple((cache_root / f"files/{env}/nested/path/dummy").parents) + + tuple((cache_root / f"files/{env}/dirtofile/dummy").parents) + + tuple((cache_root / f"extrn_files/{env}/dirtofile/dummy").parents) + + tuple((cache_root / f"files/{env}/rmfail/dummy").parents) + + tuple( + ( + cache_root / f"localfiles/{env}/this/file/was/cached/locally" + ).parents + ) + ) + }, + "send_fail": { + x + for env in _saltenvs() + for x in ( + Path("/tmp/targetdir/failfile"), + cache_root / f"files/{env}/{SUBDIR}/fail.sls", + ) + }, + "rm_fail": { + x + for env in _saltenvs() + for x in ( + cache_root / f"files/{env}/rmfail", + cache_root / f"files/{env}/rmfail/rmfailfile", + ) + }, + } + + +@pytest.fixture +def shell(request): + remote_list = getattr(request, "param", None) + removed = set() + if remote_list is None: + remote_list = request.getfixturevalue("remote_list") + + def exec_cmd(cmd): + cmd = shlex.split(cmd) + if cmd[0] == "rm": + path = Path(cmd[2]) + if path in remote_list["rm_fail"]: + return "", "you shall not pass", 1 + if cmd[1] == "-rf": + path = Path(cmd[2]) + if path in removed: + return "", "deleted a path twice", 1 + removed.add(path) + return "", "", 0 + if cmd[0] != "test": + return "", "", 0 + if cmd[1] == "-d": + return "", "", int(Path(cmd[2]) not in remote_list["dirs"]) + if cmd[1] == "-f": + return "", "", int(Path(cmd[2]) not in remote_list["files"]) + if cmd[1] == "-e": + return ( + "", + "", + int( + Path(cmd[2]) not in remote_list["files"] + and Path(cmd[2]) not in remote_list["dirs"] + ), + ) + return "", "", 0 + + def send(src, dest, makedirs=False): + dest = Path(dest) + if dest in remote_list["send_fail"] or str(dest).endswith("fail"): + return "", "sth went wrong", 1 + if any(x in remote_list["files"] and x not in removed for x in dest.parents): + return "", "path contains files which were not removed: Not a directory", 1 + if dest.parent not in remote_list["dirs"] and not makedirs: + return "", "tgt dir does not exist, no makedirs", 1 + if dest in remote_list["dirs"] and dest not in removed: + # send should always receive the full path of the file + return "", "just copied a file into a dir", 1 + return "", "", 0 + + shl = Mock(spec=salt.client.ssh.shell.Shell) + shl.exec_cmd.side_effect = exec_cmd + shl.send.side_effect = send + return shl + + +@pytest.fixture +def mocked_opts(tmp_path, fs_root, cache_root): + return { + "file_roots": _get_file_roots(fs_root), + "fileserver_backend": ["roots"], + "cachedir": cache_root, + "file_client": "local", + } + + +@pytest.fixture +def _setup(fs_root, cache_root): + """ + No need to add a dummy foo.txt to muddy up the github repo, just make + our own fileserver root on-the-fly. + """ + + def _new_dir(path): + """ + Add a new dir at ``path`` using os.makedirs. If the directory + already exists, remove it recursively and then try to create it + again. + """ + try: + os.makedirs(path) + except OSError as exc: + if exc.errno == errno.EEXIST: + # Just in case a previous test was interrupted, remove the + # directory and try adding it again. + shutil.rmtree(path) + os.makedirs(path) + else: + raise + + # Crete the FS_ROOT + for saltenv in _saltenvs(): + saltenv_root = os.path.join(fs_root, saltenv) + # Make sure we have a fresh root dir for this saltenv + _new_dir(saltenv_root) + + path = os.path.join(saltenv_root, "foo.txt") + with salt.utils.files.fopen(path, "w") as fp_: + fp_.write("This is a test file in the '{}' saltenv.\n".format(saltenv)) + (Path(saltenv_root) / "dirtofile").touch() + (Path(saltenv_root) / "filetodir").mkdir() + (Path(saltenv_root) / "filetodir" / "foo.sh").touch() + subdir_abspath = os.path.join(saltenv_root, SUBDIR) + os.makedirs(subdir_abspath) + for subdir_file in _subdir_files(): + path = os.path.join(subdir_abspath, subdir_file) + with salt.utils.files.fopen(path, "w") as fp_: + fp_.write( + "This is file '{}' in subdir '{} from saltenv '{}'".format( + subdir_file, SUBDIR, saltenv + ) + ) + (Path(subdir_abspath) / "fail.sls").touch() + + # Create the CACHE_ROOT + _new_dir(cache_root) + + +@pytest.fixture +def client(minion_opts, mocked_opts, shell): + patched_opts = minion_opts.copy() + patched_opts.update(mocked_opts) + return cp.SSHCpClient(patched_opts, shell, TGT) + + +@pytest.mark.usefixtures("_setup") +def test_cache_dir(client, cache_root): + """ + Ensure entire directory is cached to correct location + """ + for saltenv in _saltenvs(): + assert client.cache_dir("salt://{}".format(SUBDIR), saltenv, cachedir=None) + for subdir_file in _subdir_files(): + cache_loc = os.path.join( + cache_root, + "salt-ssh", + TGT, + "files", + saltenv, + SUBDIR, + subdir_file, + ) + # Double check that the content of the cached file + # identifies it as being from the correct saltenv. The + # setUp function creates the file with the name of the + # saltenv mentioned in the file, so a simple 'in' check is + # sufficient here. If opening the file raises an exception, + # this is a problem, so we are not catching the exception + # and letting it be raised so that the test fails. + with salt.utils.files.fopen(cache_loc) as fp_: + content = fp_.read() + log.debug("cache_loc = %s", cache_loc) + log.debug("content = %s", content) + assert subdir_file in content + assert SUBDIR in content + assert saltenv in content + minion_cache_loc = os.path.join( + cache_root, + "files", + saltenv, + SUBDIR, + subdir_file, + ) + client.shell.send.assert_any_call(cache_loc, minion_cache_loc, True) + + +@pytest.mark.usefixtures("_setup") +def test_cache_dir_with_alternate_cachedir_and_absolute_path( + client, tmp_path, cache_root +): + """ + Ensure entire directory is cached to the default location when an alternate + cachedir is specified and that cachedir is an absolute path - but then + sent to the correct cachedir on the minion. + """ + alt_cachedir = os.path.join(tmp_path, "abs_cachedir") + + for saltenv in _saltenvs(): + assert client.cache_dir( + "salt://{}".format(SUBDIR), saltenv, cachedir=alt_cachedir + ) + for subdir_file in _subdir_files(): + cache_loc = os.path.join( + cache_root, + "salt-ssh", + TGT, + "absolute_root", + alt_cachedir[1:], + "files", + saltenv, + SUBDIR, + subdir_file, + ) + # Double check that the content of the cached file + # identifies it as being from the correct saltenv. The + # setUp function creates the file with the name of the + # saltenv mentioned in the file, so a simple 'in' check is + # sufficient here. If opening the file raises an exception, + # this is a problem, so we are not catching the exception + # and letting it be raised so that the test fails. + with salt.utils.files.fopen(cache_loc) as fp_: + content = fp_.read() + log.debug("cache_loc = %s", cache_loc) + log.debug("content = %s", content) + assert subdir_file in content + assert SUBDIR in content + assert saltenv in content + minion_cache_loc = os.path.join( + alt_cachedir, "files", saltenv, SUBDIR, subdir_file + ) + client.shell.send.assert_any_call(cache_loc, minion_cache_loc, True) + + +@pytest.mark.usefixtures("_setup") +def test_cache_dir_with_alternate_cachedir_and_relative_path(client, cache_root): + """ + Ensure entire directory is cached to correct location when an alternate + cachedir is specified and that cachedir is a relative path + """ + alt_cachedir = "foo" + + for saltenv in _saltenvs(): + assert client.cache_dir( + "salt://{}".format(SUBDIR), saltenv, cachedir=alt_cachedir + ) + for subdir_file in _subdir_files(): + cache_loc = os.path.join( + cache_root, + "salt-ssh", + TGT, + alt_cachedir, + "files", + saltenv, + SUBDIR, + subdir_file, + ) + # Double check that the content of the cached file + # identifies it as being from the correct saltenv. The + # setUp function creates the file with the name of the + # saltenv mentioned in the file, so a simple 'in' check is + # sufficient here. If opening the file raises an exception, + # this is a problem, so we are not catching the exception + # and letting it be raised so that the test fails. + with salt.utils.files.fopen(cache_loc) as fp_: + content = fp_.read() + log.debug("cache_loc = %s", cache_loc) + log.debug("content = %s", content) + assert subdir_file in content + assert SUBDIR in content + assert saltenv in content + minion_cache_loc = os.path.join( + cache_root, + alt_cachedir, + "files", + saltenv, + SUBDIR, + subdir_file, + ) + client.shell.send.assert_any_call(cache_loc, minion_cache_loc, True) + + +@pytest.mark.usefixtures("_setup") +def test_cache_file(client, cache_root): + """ + Ensure file is cached to correct location + """ + for saltenv in _saltenvs(): + assert client.cache_file("salt://foo.txt", saltenv, cachedir=None) + cache_loc = os.path.join( + cache_root, "salt-ssh", TGT, "files", saltenv, "foo.txt" + ) + # Double check that the content of the cached file identifies + # it as being from the correct saltenv. The setUp function + # creates the file with the name of the saltenv mentioned in + # the file, so a simple 'in' check is sufficient here. If + # opening the file raises an exception, this is a problem, so + # we are not catching the exception and letting it be raised so + # that the test fails. + with salt.utils.files.fopen(cache_loc) as fp_: + content = fp_.read() + log.debug("cache_loc = %s", cache_loc) + log.debug("content = %s", content) + assert saltenv in content + minion_cache_loc = os.path.join(cache_root, "files", saltenv, "foo.txt") + client.shell.send.assert_any_call(cache_loc, minion_cache_loc, True) + + +@pytest.mark.usefixtures("_setup") +def test_cache_file_with_alternate_cachedir_and_absolute_path( + client, tmp_path, cache_root +): + """ + Ensure file is cached to the default location when an alternate cachedir is + specified and that cachedir is an absolute path, but then sent to + the correct path on the minion + """ + alt_cachedir = os.path.join(tmp_path, "abs_cachedir") + for saltenv in _saltenvs(): + assert client.cache_file("salt://foo.txt", saltenv, cachedir=alt_cachedir) + cache_loc = os.path.join( + cache_root, + "salt-ssh", + TGT, + "absolute_root", + alt_cachedir[1:], + "files", + saltenv, + "foo.txt", + ) + # Double check that the content of the cached file identifies + # it as being from the correct saltenv. The setUp function + # creates the file with the name of the saltenv mentioned in + # the file, so a simple 'in' check is sufficient here. If + # opening the file raises an exception, this is a problem, so + # we are not catching the exception and letting it be raised so + # that the test fails. + with salt.utils.files.fopen(cache_loc) as fp_: + content = fp_.read() + log.debug("cache_loc = %s", cache_loc) + log.debug("content = %s", content) + assert saltenv in content + minion_cache_loc = os.path.join(alt_cachedir, "files", saltenv, "foo.txt") + client.shell.send.assert_any_call(cache_loc, minion_cache_loc, True) + + +@pytest.mark.usefixtures("_setup") +def test_cache_file_with_alternate_cachedir_and_relative_path(client, cache_root): + """ + Ensure file is cached to correct location when an alternate cachedir is + specified and that cachedir is a relative path + """ + alt_cachedir = "foo" + + for saltenv in _saltenvs(): + assert client.cache_file("salt://foo.txt", saltenv, cachedir=alt_cachedir) + cache_loc = os.path.join( + cache_root, + "salt-ssh", + TGT, + alt_cachedir, + "files", + saltenv, + "foo.txt", + ) + # Double check that the content of the cached file identifies + # it as being from the correct saltenv. The setUp function + # creates the file with the name of the saltenv mentioned in + # the file, so a simple 'in' check is sufficient here. If + # opening the file raises an exception, this is a problem, so + # we are not catching the exception and letting it be raised so + # that the test fails. + with salt.utils.files.fopen(cache_loc) as fp_: + content = fp_.read() + log.debug("cache_loc = %s", cache_loc) + log.debug("content = %s", content) + assert saltenv in content + minion_cache_loc = os.path.join( + cache_root, + alt_cachedir, + "files", + saltenv, + "foo.txt", + ) + client.shell.send.assert_any_call(cache_loc, minion_cache_loc, True) + + +@pytest.mark.usefixtures("_setup") +def test_cache_dest(client, cache_root): + """ + Tests functionality for cache_dest + """ + relpath = "foo.com/bar.txt" + + def _external(saltenv="base"): + return salt.utils.path.join( + cache_root, "salt-ssh", TGT, "extrn_files", saltenv, relpath + ) + + def _salt(saltenv="base"): + return salt.utils.path.join( + cache_root, "salt-ssh", TGT, "files", saltenv, relpath + ) + + def _check(ret, expected): + assert ret == expected, "{} != {}".format(ret, expected) + + _check(client.cache_dest(f"https://{relpath}"), _external()) + + _check(client.cache_dest(f"https://{relpath}", "dev"), _external("dev")) + + _check(client.cache_dest(f"salt://{relpath}"), _salt()) + + _check(client.cache_dest(f"salt://{relpath}", "dev"), _salt("dev")) + + _check(client.cache_dest(f"salt://{relpath}?saltenv=dev"), _salt("dev")) + + _check("/foo/bar", "/foo/bar") + + +# ------ END of adaptation of existing fileclient tests ------ + +# The following tests are for the client specifically, the wrapper +# function tests are implemented as integration ones. + + +def test_cache_local_file(client): + """ + Caching local files would currently mean extracting files from the master + to the minion, something that is likely very unexpected. + Semantically, the other way around would be more expected, but receiving + files from the minion is currently not implemented and might not be + the best idea from a security perspective. + """ + with pytest.raises( + salt.exceptions.CommandExecutionError, + match="Cannot cache local files via salt-ssh", + ): + client.cache_local_file("/foo/bar") + + +@pytest.mark.parametrize("master", (False, True)) +@pytest.mark.parametrize("cachedir", (None, "", "relative/path", "/absolute/path")) +def test_get_cachedir(client, cachedir, master, cache_root): + """ + Ensure that the cachedirs are correctly returned and that + the master root cannot be overridden with an absolute path. + """ + subdir = cachedir + if cachedir is None: + base = cache_root + elif cachedir.startswith("/"): + if master: + base = cache_root + subdir = "absolute_root" + cachedir + else: + base = cachedir + else: + base = cache_root + if master: + expected = str(Path(base) / "salt-ssh" / TGT / (subdir or "")).rstrip("/") + else: + expected = str(Path(base) / (subdir or "")).rstrip("/") + if cachedir == "": + # The usual fileclient does it this way as well + expected += "/" + assert client.get_cachedir(cachedir, master=master) == expected + + +@pytest.mark.usefixtures("_setup") +@pytest.mark.parametrize("saltenv", _saltenvs()) +def test_cache_file_send_fail(client, saltenv, caplog, cache_root, fs_root): + """ + Ensure that when a file transfer fails, an error is logged and + the locally cached file is removed + """ + with caplog.at_level(logging.ERROR): + assert not client.cache_file(f"salt://{SUBDIR}/fail.sls", saltenv) + assert "Failed sending file: sth went wrong" in caplog.text + cache_loc = ( + Path(cache_root) / "salt-ssh" / TGT / "files" / saltenv / SUBDIR / "fail.sls" + ) + assert not cache_loc.exists() + + +@pytest.mark.usefixtures("_setup") +def test_cache_file_dir_to_file(client, cache_root, remote_list, caplog): + """ + Ensure that when a remote path is a directory, but should be a file, + cache_file will remove the directory. + """ + local_cache = Path(cache_root) / "salt-ssh" / TGT / "files" / "base" / "dirtofile" + expected = Path(cache_root) / "files" / "base" / "dirtofile" + res = client.cache_file("salt://dirtofile") + assert res + assert "just copied a file into a dir" not in caplog.text + assert res == str(local_cache) + assert local_cache.exists() + assert str(local_cache) in client.target_map + assert client.target_map[str(local_cache)] == str(expected) + client.shell.send.assert_called_once_with(str(local_cache), str(expected), True) + + +@pytest.mark.usefixtures("_setup") +def test_cache_file_file_to_dir(client, cache_root, caplog): + """ + Ensure that when a remote path is a file, but should be a directory, + cache_file will remove the file. + """ + local_cache = ( + Path(cache_root) / "salt-ssh" / TGT / "files" / "base" / "filetodir" / "foo.sh" + ) + expected = Path(cache_root) / "files" / "base" / "filetodir" / "foo.sh" + res = client.cache_file("salt://filetodir/foo.sh") + assert res + assert "path contains files which were not removed" not in caplog.text + assert res == str(local_cache) + assert local_cache.exists() + assert str(local_cache) in client.target_map + assert client.target_map[str(local_cache)] == str(expected) + client.shell.send.assert_called_with(str(local_cache), str(expected), True) + + +@pytest.mark.usefixtures("_setup") +def test_send_file_to_dir_rmfail(client, cache_root, caplog): + """ + Ensure that when a remote path is a file, but should be a directory, + cache_file will remove the file. + """ + res = client._send_file( + "/some/source/file", + Path(cache_root) / "files/base/rmfail/rmfailfile/child", + True, + None, + ) + assert res is False + assert "path contains files which were not removed" in caplog.text + assert "Failed deleting path" in caplog.text + assert "you shall not pass" in caplog.text + + +def test_send_file_file_to_dir_outside_cachedir(client, caplog): + """ + When sending a file to a path of which one parent is an existing file, + ensure that the failure is passed through instead of trying to delete + this file. + """ + res = client._send_file( + "/some/source/file", "/tmp/targetdir/existingfile/bar", True, None + ) + assert res is False + assert "path contains files which were not removed" in caplog.text + assert ( + call("rm -rf /tmp/targetdir/existingfile") + not in client.shell.exec_cmd.mock_calls + ) + + +def test_send_file_from_outside_cachedir_fail(client, caplog, tmp_path): + """ + Ensure failed transfers do not result in a deleted source file if it is outside + of the SSH master-minion cache + """ + src = tmp_path / "this_should_not_be_deleted" + src.touch() + with caplog.at_level(logging.ERROR): + res = client._send_file( + "/some/source/file", "/tmp/targetdir/failfile", False, None + ) + assert res is False + assert src.exists() + assert "sth went wrong" in caplog.text + + +@pytest.mark.usefixtures("_setup") +@pytest.mark.parametrize("saltenv", _saltenvs()) +def test_get_file_to_cache_send_fail(client, saltenv, caplog, cache_root, fs_root): + """ + Ensure that when a file transfer to the remote cache fails, an error is + logged and the locally cached file is removed + """ + with caplog.at_level(logging.ERROR): + assert not client.get_file(f"salt://{SUBDIR}/fail.sls", "", saltenv=saltenv) + assert "Failed sending file: sth went wrong" in caplog.text + cache_loc = ( + Path(cache_root) / "salt-ssh" / TGT / "files" / saltenv / SUBDIR / "fail.sls" + ) + assert not cache_loc.exists() + + +@pytest.mark.usefixtures("_setup") +@pytest.mark.parametrize("saltenv", _saltenvs()) +def test_get_file_send_fail_dst(client, saltenv, caplog, cache_root, fs_root, tmp_path): + """ + Ensure that when a file transfer to a remote path outside of the cachedir + fails, an error is logged and the locally cached file is removed + """ + tgt = tmp_path / "remote_target_fail" + with caplog.at_level(logging.ERROR): + assert not client.get_file(f"salt://{SUBDIR}/fail.sls", str(tgt), saltenv) + assert not tgt.exists() + assert "Failed sending file: sth went wrong" in caplog.text + cache_loc = ( + Path(cache_root) / "salt-ssh" / TGT / "files" / saltenv / SUBDIR / "fail.sls" + ) + assert not cache_loc.exists() + + +@pytest.mark.usefixtures("_setup") +@pytest.mark.parametrize("makedirs", (False, True)) +def test_get_file_remote_isdir(client, cache_root, makedirs): + """ + Ensure that if the remote path is a directory, the file will be + copied into it, _send will be called with the full file path + and that the correct path is present in the target map. + """ + tgt = "/tmp/targetdir" + expected = tgt + "/foo.txt" + local_cache = Path(cache_root) / "salt-ssh" / TGT / "files" / "base" / "foo.txt" + res = client.get_file("salt://foo.txt", tgt, makedirs=makedirs) + assert res + assert res == str(local_cache) + assert local_cache.exists() + assert str(local_cache) in client.target_map + assert client.target_map[str(local_cache)] == expected + client.shell.send.assert_called_once_with(str(local_cache), expected, makedirs) + + +@pytest.mark.usefixtures("_setup") +@pytest.mark.parametrize("makedirs", (False, True)) +def test_get_file_with_dest_slash(client, cache_root, makedirs): + """ + Ensure that if the destination is specified with a trailing slash, + it is assumed to be a directory and a check is not performed. + """ + tgt = "/tmp/targetdir/" + expected = tgt + "foo.txt" + local_cache = Path(cache_root) / "salt-ssh" / TGT / "files" / "base" / "foo.txt" + res = client.get_file("salt://foo.txt", tgt, makedirs=makedirs) + assert res + assert res == str(local_cache) + assert local_cache.exists() + assert str(local_cache) in client.target_map + assert client.target_map[str(local_cache)] == expected + client.shell.send.assert_called_once_with(str(local_cache), expected, makedirs) + # ensure we're not doing unnecessary remote executions + assert call(f"test -d {tgt}") not in client.shell.exec_cmd.mock_calls + + +@pytest.mark.usefixtures("_setup") +def test_get_file_with_different_name(client, cache_root): + """ + Ensure that if the target path is not a directory, the final + path component will be the name of the file. + """ + tgt = "/tmp/targetdir/hithere" + local_cache = Path(cache_root) / "salt-ssh" / TGT / "files" / "base" / "foo.txt" + res = client.get_file("salt://foo.txt", tgt) + assert res + assert res == str(local_cache) + assert local_cache.exists() + assert str(local_cache) in client.target_map + assert client.target_map[str(local_cache)] == tgt + client.shell.send.assert_called_once_with(str(local_cache), tgt, False) + + +@pytest.mark.usefixtures("_setup") +def test_get_url_salt_none_without_no_cache(client, cache_root): + """ + note: salt:// URIs do not respect no_cache + """ + tgt = Path(cache_root) / "files" / "base" / "foo.txt" + local_cache = Path(cache_root) / "salt-ssh" / TGT / "files" / "base" / "foo.txt" + res = client.get_url("salt://foo.txt", None) + assert res + assert local_cache.exists() + assert res == local_cache.read_bytes() + assert str(local_cache) in client.target_map + assert client.target_map[str(local_cache)] == str(tgt) + client.shell.send.assert_called_once_with(str(local_cache), str(tgt), True) + + +@pytest.fixture +def _http_patch(): + def query(*args, **kwargs): + kwargs["header_callback"]("HTTP/1.1 200 OK") + kwargs["streaming_callback"](b"hi there") + return {"handle": Mock(), "status": 200} + + with patch("salt.utils.http.query", side_effect=query): + yield + + +@pytest.mark.usefixtures("_setup", "_http_patch") +@pytest.mark.parametrize("dest", ("", None)) +def test_get_url_non_salt_dest_empty_without_no_cache(client, cache_root, dest): + """ + Even when dest is None, but no_cache is False, the file should be sent + to the minion cache. + """ + tgt = ( + Path(cache_root) / "extrn_files" / "base" / "repo.saltproject.io" / "index.html" + ) + local_cache = ( + Path(cache_root) + / "salt-ssh" + / TGT + / "extrn_files" + / "base" + / "repo.saltproject.io" + / "index.html" + ) + res = client.get_url("https://repo.saltproject.io/index.html", dest) + assert res + assert local_cache.exists() + assert local_cache.read_text() == "hi there" + assert res == str(local_cache) + assert str(local_cache) in client.target_map + assert client.target_map[str(local_cache)] == str(tgt) + client.shell.send.assert_called_once_with(str(local_cache), str(tgt), True) + + +@pytest.mark.usefixtures("_setup", "_http_patch") +def test_get_url_non_salt_dest_slash(client, cache_root, tmp_path): + expected = str(tmp_path) + "/foo.html" + local_cache = ( + Path(cache_root) + / "salt-ssh" + / TGT + / "extrn_files" + / "base" + / "repo.saltproject.io" + / "foo.html" + ) + res = client.get_url("https://repo.saltproject.io/foo.html", str(tmp_path) + "/") + assert res + assert local_cache.exists() + assert local_cache.read_text() == "hi there" + assert res == str(local_cache) + assert str(local_cache) in client.target_map + assert client.target_map[str(local_cache)] == expected + client.shell.send.assert_called_once_with(str(local_cache), expected, False) + assert call(f"test -d {tmp_path}") not in client.shell.exec_cmd.mock_calls + + +@pytest.mark.usefixtures("_setup", "_http_patch") +def test_get_url_non_salt_dest_isdir(client, cache_root): + dest = "/tmp/targetdir" + expected = dest + "/foo.html" + local_cache = ( + Path(cache_root) + / "salt-ssh" + / TGT + / "extrn_files" + / "base" + / "repo.saltproject.io" + / "foo.html" + ) + res = client.get_url("https://repo.saltproject.io/foo.html", dest) + assert res + assert local_cache.exists() + assert local_cache.read_text() == "hi there" + assert res == str(local_cache) + assert str(local_cache) in client.target_map + assert client.target_map[str(local_cache)] == expected + client.shell.send.assert_called_once_with(str(local_cache), expected, False) + client.shell.exec_cmd.assert_called_with(f"test -d {dest}") + + +@pytest.mark.usefixtures("_setup", "_http_patch") +def test_get_url_non_salt_dest_name_override(client, cache_root): + dest = "/tmp/targetdir/bar.html" + local_cache = ( + Path(cache_root) + / "salt-ssh" + / TGT + / "extrn_files" + / "base" + / "repo.saltproject.io" + / "foo.html" + ) + res = client.get_url("https://repo.saltproject.io/foo.html", dest) + assert res + assert local_cache.exists() + assert local_cache.read_text() == "hi there" + assert res == str(local_cache) + assert str(local_cache) in client.target_map + assert client.target_map[str(local_cache)] == dest + client.shell.send.assert_called_once_with(str(local_cache), dest, False) + client.shell.exec_cmd.assert_called_with(f"test -d {dest}") + + +@pytest.mark.usefixtures("_setup", "_http_patch") +def test_get_url_non_salt_dest_default_name(client, cache_root, tmp_path): + expected = str(tmp_path / "index.html") + # Yup, the regular fileclient will save this with the domain name only. + # If you then try to cache any other file from that domain, it will + # actually raise an exception because it attempts to create a dir with the same name + local_cache = ( + Path(cache_root) + / "salt-ssh" + / TGT + / "extrn_files" + / "base" + / "repo.saltproject.io" + ) + res = client.get_url("https://repo.saltproject.io", str(tmp_path) + "/") + assert res + assert local_cache.exists() + assert local_cache.read_text() == "hi there" + assert res == str(local_cache) + assert str(local_cache) in client.target_map + assert client.target_map[str(local_cache)] == expected + client.shell.send.assert_called_once_with(str(local_cache), expected, False) + + +def test_get_url_non_salt_fetch_fail(client): + with patch("salt.fileclient.Client.get_url", return_value=False): + res = client.get_url("https://fooba.rs", "") + assert res is False + + +@pytest.mark.usefixtures("_setup") +@pytest.mark.parametrize("dest", ("", None)) +def test_get_template_cache(client, dest, cache_root): + tgt = Path(cache_root) / "extrn_files" / "base" / "foo.txt" + local_cache = ( + Path(cache_root) / "salt-ssh" / TGT / "extrn_files" / "base" / "foo.txt" + ) + res = client.get_template("salt://foo.txt", dest, opts=client.opts) + assert res + assert local_cache.exists() + assert res == str(local_cache) + assert str(local_cache) in client.target_map + assert client.target_map[str(local_cache)] == str(tgt) + client.shell.send.assert_called_with(str(local_cache), str(tgt), True) + + +@pytest.mark.usefixtures("_setup") +def test_get_template_name_override(client, cache_root): + dest = "/tmp/targetdir/bar" + local_cache = ( + Path(cache_root) / "salt-ssh" / TGT / "extrn_files" / "base" / "foo.txt" + ) + res = client.get_template("salt://foo.txt", dest, opts=client.opts) + assert res + assert local_cache.exists() + assert res == str(local_cache) + assert str(local_cache) in client.target_map + assert client.target_map[str(local_cache)] == dest + client.shell.send.assert_called_with(str(local_cache), dest, False) + + +@pytest.mark.usefixtures("_setup", "_http_patch") +def test_get_template_dest_slash(client, cache_root, tmp_path): + expected = str(tmp_path) + "/foo.txt" + local_cache = ( + Path(cache_root) / "salt-ssh" / TGT / "extrn_files" / "base" / "foo.txt" + ) + res = client.get_template("salt://foo.txt", str(tmp_path) + "/", opts=client.opts) + assert res + assert local_cache.exists() + assert ( + local_cache.read_text().strip() == "This is a test file in the 'base' saltenv." + ) + assert res == str(local_cache) + assert str(local_cache) in client.target_map + assert client.target_map[str(local_cache)] == expected + client.shell.send.assert_called_with(str(local_cache), expected, False) + assert call(f"test -d {tmp_path}") not in client.shell.exec_cmd.mock_calls + + +@pytest.mark.usefixtures("_setup", "_http_patch") +def test_get_template_dest_isdir(client, cache_root): + dest = "/tmp/targetdir" + expected = dest + "/foo.txt" + local_cache = ( + Path(cache_root) / "salt-ssh" / TGT / "extrn_files" / "base" / "foo.txt" + ) + res = client.get_template("salt://foo.txt", dest, opts=client.opts) + assert res + assert local_cache.exists() + assert ( + local_cache.read_text().strip() == "This is a test file in the 'base' saltenv." + ) + assert res == str(local_cache) + assert str(local_cache) in client.target_map + assert client.target_map[str(local_cache)] == expected + client.shell.send.assert_called_with(str(local_cache), expected, False) + client.shell.exec_cmd.assert_called_with(f"test -d {dest}") + + +@pytest.mark.usefixtures("_setup", "_http_patch") +def test_get_template_dest_name_override(client, cache_root): + dest = "/tmp/targetdir/bar.html" + local_cache = ( + Path(cache_root) + / "salt-ssh" + / TGT + / "extrn_files" + / "base" + / "repo.saltproject.io" + / "foo.html" + ) + res = client.get_url("https://repo.saltproject.io/foo.html", dest) + assert res + assert local_cache.exists() + assert local_cache.read_text() == "hi there" + assert res == str(local_cache) + assert str(local_cache) in client.target_map + assert client.target_map[str(local_cache)] == dest + client.shell.send.assert_called_once_with(str(local_cache), dest, False) + client.shell.exec_cmd.assert_called_with(f"test -d {dest}") + + +@pytest.mark.usefixtures("_setup") +def test_get_template_dir_to_file(client, cache_root, remote_list, caplog): + """ + Ensure that when a remote path is a directory, but should be a file, + get_template will remove the directory. + """ + local_cache = ( + Path(cache_root) / "salt-ssh" / TGT / "extrn_files" / "base" / "dirtofile" + ) + expected = Path(cache_root) / "extrn_files" / "base" / "dirtofile" + res = client.get_template("salt://dirtofile", "", opts=client.opts) + assert res + assert "just copied a file into a dir" not in caplog.text + assert res == str(local_cache) + assert local_cache.exists() + assert str(local_cache) in client.target_map + assert client.target_map[str(local_cache)] == str(expected) + client.shell.send.assert_called_with(str(local_cache), str(expected), True) + + +@pytest.mark.usefixtures("_setup") +def test_get_template_file_to_dir(client, cache_root, caplog): + """ + Ensure that when a remote path is a file, but should be a directory, + get_template will remove the file. + """ + local_cache = ( + Path(cache_root) + / "salt-ssh" + / TGT + / "extrn_files" + / "base" + / "filetodir" + / "foo.sh" + ) + expected = Path(cache_root) / "extrn_files" / "base" / "filetodir" / "foo.sh" + res = client.get_template("salt://filetodir/foo.sh", "", opts=client.opts) + assert res + assert "path contains files which were not removed" not in caplog.text + assert res == str(local_cache) + assert local_cache.exists() + assert str(local_cache) in client.target_map + assert client.target_map[str(local_cache)] == str(expected) + client.shell.send.assert_called_with(str(local_cache), str(expected), True) + + +@pytest.mark.parametrize( + "arg,raises", + [ + ( + ("foo/bar", "/foo/bar"), + pytest.raises(ValueError, match=".*must be absolute.*as src"), + ), + ( + ("/foo/bar", "foo/bar"), + pytest.raises(ValueError, match=".*must be absolute.*as dest"), + ), + ], +) +def test_send_file_sanity_checks(client, raises, arg): + with raises: + client._send_file(*arg, True, None) + + +@pytest.mark.parametrize( + "path,raises", + [ + ( + "", + pytest.raises( + ValueError, match="Not deleting unspecified, relative or root path" + ), + ), + ( + "foo/bar", + pytest.raises( + ValueError, match="Not deleting unspecified, relative or root path" + ), + ), + ( + "/", + pytest.raises( + ValueError, match="Not deleting unspecified, relative or root path" + ), + ), + ( + "/some/path/not/in/cache/dir", + pytest.raises( + ValueError, + match="Not recursively deleting a path outside of the cachedir.*", + ), + ), + ], +) +def test_rmpath_sanity_checks(client, raises, path): + with raises: + client._rmpath(path) + client.shell.exec_cmd.assert_not_called() + + +def test_rmpath_can_delete_minion_cache_dir(client, cache_root): + client._rmpath(cache_root) + client.shell.exec_cmd.assert_called_once_with(f"rm -rf {cache_root}") + + +def test_rmpath_does_not_delete_minion_cache_dir_parent(client, cache_root): + with pytest.raises( + ValueError, match="Not recursively deleting a path outside of the cachedir.*" + ): + client._rmpath(Path(cache_root).parent) + client.shell.exec_cmd.assert_not_called() + + +@pytest.mark.parametrize( + "path,expected", [("files/base/dirtofile", True), ("files/base/rmfail", False)] +) +def test_rmpath_retcode(client, cache_root, path, expected, caplog): + with caplog.at_level(logging.ERROR): + res = client._rmpath(Path(cache_root) / path) + assert res is expected + if not expected: + assert "Failed deleting path" in caplog.text + assert "you shall not pass" in caplog.text + + +def test_is_cached_localfiles(client, cache_root): + """ + A check for having cached a local file on the minion should only check + if the file exists on the target since we're not syncing to the master + """ + tgt = "/this/file/was/cached/locally" + res = client.is_cached(tgt) + assert res == str(Path(cache_root) / "localfiles" / tgt[1:]) + + +@pytest.mark.parametrize("saltenv", _saltenvs()) +@pytest.mark.usefixtures("_setup") +def test_cache_master(client, cache_root, saltenv): + """ + Files are copied one by one currently, so this is done as a unit test + to save execution time and resources. + """ + with patch.object(cp, "_client", return_value=client): + res = cp.cache_master(saltenv) + assert isinstance(res, list) + assert res + minion_parent = Path(cache_root) / "files" / saltenv + master_parent = Path(cache_root) / "salt-ssh" / TGT / "files" / saltenv + for path in res: + assert path + path = Path(path) + assert minion_parent in path.parents + master_path = master_parent / path.relative_to(minion_parent) + assert master_path.exists() + if path.name == "foo.txt": + assert saltenv in master_path.read_text() + client.shell.send.assert_any_call(str(master_path), str(path), True) diff --git a/tests/pytests/unit/modules/test_cp.py b/tests/pytests/unit/modules/test_cp.py index 6caa9ef5938b..8fa0fd0d44bb 100644 --- a/tests/pytests/unit/modules/test_cp.py +++ b/tests/pytests/unit/modules/test_cp.py @@ -28,7 +28,6 @@ def test__render_filenames_undefined_template(): dest = "/srv/salt/cheese" saltenv = "base" template = "biscuits" - ret = (path, dest) pytest.raises( CommandExecutionError, cp._render_filenames, path, dest, saltenv, template ) @@ -94,7 +93,6 @@ def test_get_file_str_success(): path = "salt://saltines" dest = "/srv/salt/cheese/saltines" file_data = "Remember to keep your files well salted." - saltenv = "base" ret = file_data with patch("salt.utils.files.fopen", mock_open(read_data=file_data)): with patch("salt.modules.cp.cache_file", MagicMock(return_value=dest)): From c4fd97200bf91e5defb964984e8a8e7e9091449d Mon Sep 17 00:00:00 2001 From: jeanluc Date: Mon, 6 Nov 2023 15:25:01 +0100 Subject: [PATCH 05/13] Add SSH wrapper for `cmd.script` --- changelog/48067.fixed.md | 1 + salt/client/ssh/wrapper/cmdmod.py | 548 +++++++++++++++++++ tests/pytests/integration/ssh/test_cmdmod.py | 112 ++++ 3 files changed, 661 insertions(+) create mode 100644 changelog/48067.fixed.md create mode 100644 salt/client/ssh/wrapper/cmdmod.py create mode 100644 tests/pytests/integration/ssh/test_cmdmod.py diff --git a/changelog/48067.fixed.md b/changelog/48067.fixed.md new file mode 100644 index 000000000000..b060c44cde8c --- /dev/null +++ b/changelog/48067.fixed.md @@ -0,0 +1 @@ +Made cmd.script work with files from the fileserver via salt-ssh diff --git a/salt/client/ssh/wrapper/cmdmod.py b/salt/client/ssh/wrapper/cmdmod.py new file mode 100644 index 000000000000..701f21cf6d3e --- /dev/null +++ b/salt/client/ssh/wrapper/cmdmod.py @@ -0,0 +1,548 @@ +""" +SSH wrapper module for the ``cmdmod`` execution module. + +.. note:: + For consistency reasons, this wrapper currently does + not behave the same as the execution module regarding ``saltenv``. + The parameter defaults to ``base``, regardless of the current + value of the minion setting. + This is the same for the ``state`` and `cp`` wrappers. +""" +import logging +import os.path +import shlex + +import salt.utils.url +from salt.exceptions import CommandExecutionError, SaltInvocationError +from salt.modules.cmdmod import _python_shell_default + +log = logging.getLogger(__name__) + +__virtualname__ = "cmd" + + +def __virtual__(): + return __virtualname__ + + +def script( + source, + args=None, + cwd=None, + stdin=None, + runas=None, + group=None, + shell=None, + python_shell=None, + env=None, + template=None, + umask=None, + output_encoding=None, + output_loglevel="debug", + log_callback=None, + hide_output=False, + timeout=None, + reset_system_locale=True, + saltenv="base", + use_vt=False, + bg=False, + password=None, + success_retcodes=None, + success_stdout=None, + success_stderr=None, + **kwargs +): + """ + Download a script from a remote location and execute the script locally. + The script can be located on the salt master file server or on an HTTP/FTP + server. + + The script will be executed directly, so it can be written in any available + programming language. + + :param str source: The location of the script to download. If the file is + located on the master in the directory named spam, and is called eggs, + the source string is salt://spam/eggs + + :param str args: String of command line args to pass to the script. Only + used if no args are specified as part of the `name` argument. To pass a + string containing spaces in YAML, you will need to doubly-quote it: + + .. code-block:: bash + + salt myminion cmd.script salt://foo.sh "arg1 'arg two' arg3" + + :param str cwd: The directory from which to execute the command. Defaults + to the directory returned from Python's tempfile.mkstemp. + + :param str stdin: A string of standard input can be specified for the + command to be run using the ``stdin`` parameter. This can be useful in + cases where sensitive information must be read from standard input. + + :param str runas: Specify an alternate user to run the command. The default + behavior is to run as the user under which Salt is running. If running + on a Windows minion you must also use the ``password`` argument, and + the target user account must be in the Administrators group. + + .. note:: + + For Window's users, specifically Server users, it may be necessary + to specify your runas user using the User Logon Name instead of the + legacy logon name. Traditionally, logons would be in the following + format. + + ``Domain/user`` + + In the event this causes issues when executing scripts, use the UPN + format which looks like the following. + + ``user@domain.local`` + + More information + + :param str password: Windows only. Required when specifying ``runas``. This + parameter will be ignored on non-Windows platforms. + + .. versionadded:: 2016.3.0 + + :param str group: Group to run script as. Not currently supported + on Windows. + + :param str shell: Specify an alternate shell. Defaults to the system's + default shell. + + :param bool python_shell: If False, let python handle the positional + arguments. Set to True to use shell features, such as pipes or + redirection. + + :param bool bg: If True, run script in background and do not await or + deliver its results + + :param dict env: Environment variables to be set prior to execution. + + .. note:: + When passing environment variables on the CLI, they should be + passed as the string representation of a dictionary. + + .. code-block:: bash + + salt myminion cmd.script 'some command' env='{"FOO": "bar"}' + + .. note:: + When using environment variables on Window's, case-sensitivity + matters, i.e. Window's uses `Path` as opposed to `PATH` for other + systems. + + :param str template: If this setting is applied then the named templating + engine will be used to render the downloaded file. Currently jinja, + mako, and wempy are supported. + + :param str umask: The umask (in octal) to use when running the command. + + :param str output_encoding: Control the encoding used to decode the + command's output. + + .. note:: + This should not need to be used in most cases. By default, Salt + will try to use the encoding detected from the system locale, and + will fall back to UTF-8 if this fails. This should only need to be + used in cases where the output of the command is encoded in + something other than the system locale or UTF-8. + + To see the encoding Salt has detected from the system locale, check + the `locale` line in the output of :py:func:`test.versions_report + `. + + .. versionadded:: 2018.3.0 + + :param str output_loglevel: Control the loglevel at which the output from + the command is logged to the minion log. + + .. note:: + The command being run will still be logged at the ``debug`` + loglevel regardless, unless ``quiet`` is used for this value. + + :param bool ignore_retcode: If the exit code of the command is nonzero, + this is treated as an error condition, and the output from the command + will be logged to the minion log. However, there are some cases where + programs use the return code for signaling and a nonzero exit code + doesn't necessarily mean failure. Pass this argument as ``True`` to + skip logging the output if the command has a nonzero exit code. + + :param bool hide_output: If ``True``, suppress stdout and stderr in the + return data. + + .. note:: + This is separate from ``output_loglevel``, which only handles how + Salt logs to the minion log. + + .. versionadded:: 2018.3.0 + + :param int timeout: If the command has not terminated after timeout + seconds, send the subprocess sigterm, and if sigterm is ignored, follow + up with sigkill + + :param bool use_vt: Not supported via salt-ssh. + + :param list success_retcodes: This parameter will allow a list of + non-zero return codes that should be considered a success. If the + return code returned from the run matches any in the provided list, + the return code will be overridden with zero. + + .. versionadded:: 2019.2.0 + + :param list success_stdout: This parameter will allow a list of + strings that when found in standard out should be considered a success. + If stdout returned from the run matches any in the provided list, + the return code will be overridden with zero. + + .. versionadded:: 3004 + + :param list success_stderr: This parameter will allow a list of + strings that when found in standard error should be considered a success. + If stderr returned from the run matches any in the provided list, + the return code will be overridden with zero. + + .. versionadded:: 3004 + + :param bool stdin_raw_newlines: False + If ``True``, Salt will not automatically convert the characters ``\\n`` + present in the ``stdin`` value to newlines. + + .. versionadded:: 2019.2.0 + + CLI Example: + + .. code-block:: bash + + salt '*' cmd.script salt://scripts/runme.sh + salt '*' cmd.script salt://scripts/runme.sh 'arg1 arg2 "arg 3"' + salt '*' cmd.script salt://scripts/windows_task.ps1 args=' -Input c:\\tmp\\infile.txt' shell='powershell' + + + .. code-block:: bash + + salt '*' cmd.script salt://scripts/runme.sh stdin='one\\ntwo\\nthree\\nfour\\nfive\\n' + """ + + def _check_ret(ret): + # Failing unwrapped calls to the minion always return a result dict + # and do not throw exceptions currently. + if isinstance(ret, dict) and ret.get("stderr"): + raise CommandExecutionError(ret["stderr"]) + + def _cleanup_tempfile(path): + try: + _check_ret(__salt__["file.remove"](path)) + except (SaltInvocationError, CommandExecutionError) as exc: + log.error( + "cmd.script: Unable to clean tempfile '%s': %s", + path, + exc, + exc_info_on_loglevel=logging.DEBUG, + ) + + if shell is None: + shell = __grains__.get("shell", "/bin/sh") + python_shell = _python_shell_default(python_shell, kwargs.get("__pub_jid", "")) + + if "__env__" in kwargs: + # "env" is not supported; Use "saltenv". + kwargs.pop("__env__") + + path = __salt__["temp.file"]( + suffix=os.path.splitext(salt.utils.url.split_env(source)[0])[1], parent=cwd + ) + _check_ret(path) + try: + if template: + if "pillarenv" in kwargs or "pillar" in kwargs: + pillarenv = kwargs.get("pillarenv", __opts__.get("pillarenv")) + kwargs["pillar"] = _gather_pillar(pillarenv, kwargs.get("pillar")) + fn_ = __salt__["cp.get_template"](source, path, template, saltenv, **kwargs) + if not fn_: + _cleanup_tempfile(path) + return { + "pid": 0, + "retcode": 1, + "stdout": "", + "stderr": "", + "cache_error": True, + } + else: + fn_ = __salt__["cp.cache_file"](source, saltenv) + if not fn_: + _cleanup_tempfile(path) + return { + "pid": 0, + "retcode": 1, + "stdout": "", + "stderr": "", + "cache_error": True, + } + _check_ret(__salt__["file.copy"](fn_, path)) + _check_ret(__salt__["file.set_mode"](path, "0500")) + uid = __salt__["file.user_to_uid"](runas) + _check_ret(uid) + _check_ret(__salt__["file.chown"](path, runas, -1)) + + cmd_path = shlex.quote(path) + # We should remove the pillar from kwargs (cmd.run_all ignores it anyways) + # (it might also leak secrets in logs or break MAX_ARG) + kwargs.pop("pillar", None) + + return __salt__["cmd.run_all"]( + cmd_path + " " + str(args) if args else cmd_path, + cwd=cwd, + stdin=stdin, + output_encoding=output_encoding, + output_loglevel=output_loglevel, + log_callback=log_callback, + runas=runas, + group=group, + shell=shell, + python_shell=python_shell, + env=env, + umask=umask, + timeout=timeout, + reset_system_locale=reset_system_locale, + saltenv=saltenv, + use_vt=False, + bg=bg, + password=password, + success_retcodes=success_retcodes, + success_stdout=success_stdout, + success_stderr=success_stderr, + hide_output=hide_output, + **kwargs + ) + finally: + _cleanup_tempfile(path) + + +def script_retcode( + source, + args=None, + cwd=None, + stdin=None, + runas=None, + group=None, + shell=None, + python_shell=None, + env=None, + template="jinja", + umask=None, + timeout=None, + reset_system_locale=True, + saltenv="base", + output_encoding=None, + output_loglevel="debug", + log_callback=None, + use_vt=False, + password=None, + success_retcodes=None, + success_stdout=None, + success_stderr=None, + **kwargs +): + """ + Download a script from a remote location and execute the script locally. + The script can be located on the salt master file server or on an HTTP/FTP + server. + + The script will be executed directly, so it can be written in any available + programming language. + + The script can also be formatted as a template, the default is jinja. + + Only evaluate the script return code and do not block for terminal output + + :param str source: The location of the script to download. If the file is + located on the master in the directory named spam, and is called eggs, + the source string is salt://spam/eggs + + :param str args: String of command line args to pass to the script. Only + used if no args are specified as part of the `name` argument. To pass a + string containing spaces in YAML, you will need to doubly-quote it: + "arg1 'arg two' arg3" + + :param str cwd: The directory from which to execute the command. Defaults + to the home directory of the user specified by ``runas`` (or the user + under which Salt is running if ``runas`` is not specified). + + :param str stdin: A string of standard input can be specified for the + command to be run using the ``stdin`` parameter. This can be useful in + cases where sensitive information must be read from standard input. + + :param str runas: Specify an alternate user to run the command. The default + behavior is to run as the user under which Salt is running. If running + on a Windows minion you must also use the ``password`` argument, and + the target user account must be in the Administrators group. + + :param str password: Windows only. Required when specifying ``runas``. This + parameter will be ignored on non-Windows platforms. + + .. versionadded:: 2016.3.0 + + :param str group: Group to run script as. Not currently supported + on Windows. + + :param str shell: Specify an alternate shell. Defaults to the system's + default shell. + + :param bool python_shell: If False, let python handle the positional + arguments. Set to True to use shell features, such as pipes or + redirection. + + :param dict env: Environment variables to be set prior to execution. + + .. note:: + When passing environment variables on the CLI, they should be + passed as the string representation of a dictionary. + + .. code-block:: bash + + salt myminion cmd.script_retcode 'some command' env='{"FOO": "bar"}' + + .. note:: + When using environment variables on Window's, case-sensitivity + matters, i.e. Window's uses `Path` as opposed to `PATH` for other + systems. + + :param str template: If this setting is applied then the named templating + engine will be used to render the downloaded file. Currently jinja, + mako, and wempy are supported. + + :param str umask: The umask (in octal) to use when running the command. + + :param str output_encoding: Control the encoding used to decode the + command's output. + + .. note:: + This should not need to be used in most cases. By default, Salt + will try to use the encoding detected from the system locale, and + will fall back to UTF-8 if this fails. This should only need to be + used in cases where the output of the command is encoded in + something other than the system locale or UTF-8. + + To see the encoding Salt has detected from the system locale, check + the `locale` line in the output of :py:func:`test.versions_report + `. + + .. versionadded:: 2018.3.0 + + :param str output_loglevel: Control the loglevel at which the output from + the command is logged to the minion log. + + .. note:: + The command being run will still be logged at the ``debug`` + loglevel regardless, unless ``quiet`` is used for this value. + + :param bool ignore_retcode: If the exit code of the command is nonzero, + this is treated as an error condition, and the output from the command + will be logged to the minion log. However, there are some cases where + programs use the return code for signaling and a nonzero exit code + doesn't necessarily mean failure. Pass this argument as ``True`` to + skip logging the output if the command has a nonzero exit code. + + :param int timeout: If the command has not terminated after timeout + seconds, send the subprocess sigterm, and if sigterm is ignored, follow + up with sigkill + + :param bool use_vt: Use VT utils (saltstack) to stream the command output + more interactively to the console and the logs. This is experimental. + + :param list success_retcodes: This parameter will allow a list of + non-zero return codes that should be considered a success. If the + return code returned from the run matches any in the provided list, + the return code will be overridden with zero. + + .. versionadded:: 2019.2.0 + + :param list success_stdout: This parameter will allow a list of + strings that when found in standard out should be considered a success. + If stdout returned from the run matches any in the provided list, + the return code will be overridden with zero. + + .. versionadded:: 3004 + + :param list success_stderr: This parameter will allow a list of + strings that when found in standard error should be considered a success. + If stderr returned from the run matches any in the provided list, + the return code will be overridden with zero. + + .. versionadded:: 3004 + + :param bool stdin_raw_newlines: False + If ``True``, Salt will not automatically convert the characters ``\\n`` + present in the ``stdin`` value to newlines. + + .. versionadded:: 2019.2.0 + + CLI Example: + + .. code-block:: bash + + salt '*' cmd.script_retcode salt://scripts/runme.sh + salt '*' cmd.script_retcode salt://scripts/runme.sh 'arg1 arg2 "arg 3"' + salt '*' cmd.script_retcode salt://scripts/windows_task.ps1 args=' -Input c:\\tmp\\infile.txt' shell='powershell' + + A string of standard input can be specified for the command to be run using + the ``stdin`` parameter. This can be useful in cases where sensitive + information must be read from standard input. + + .. code-block:: bash + + salt '*' cmd.script_retcode salt://scripts/runme.sh stdin='one\\ntwo\\nthree\\nfour\\nfive\\n' + """ + if "__env__" in kwargs: + # "env" is not supported; Use "saltenv". + kwargs.pop("__env__") + + return script( + source=source, + args=args, + cwd=cwd, + stdin=stdin, + runas=runas, + group=group, + shell=shell, + python_shell=python_shell, + env=env, + template=template, + umask=umask, + timeout=timeout, + reset_system_locale=reset_system_locale, + saltenv=saltenv, + output_encoding=output_encoding, + output_loglevel=output_loglevel, + log_callback=log_callback, + use_vt=use_vt, + password=password, + success_retcodes=success_retcodes, + success_stdout=success_stdout, + success_stderr=success_stderr, + **kwargs + )["retcode"] + + +def _gather_pillar(pillarenv, pillar_override): + """ + The opts used during pillar rendering should contain the master + opts in the root namespace. self.opts is the modified minion opts, + containing the original master opts in __master_opts__. + """ + popts = {} + # Pillar compilation needs the master opts primarily, + # same as during regular operation. + popts.update(__opts__) + popts.update(__opts__.get("__master_opts__", {})) + pillar = salt.pillar.get_pillar( + popts, + __grains__.value(), + __salt__.kwargs["id_"], + __opts__["saltenv"] or "base", + pillar_override=pillar_override, + pillarenv=pillarenv, + ) + return pillar.compile_pillar() diff --git a/tests/pytests/integration/ssh/test_cmdmod.py b/tests/pytests/integration/ssh/test_cmdmod.py new file mode 100644 index 000000000000..577fa2eef298 --- /dev/null +++ b/tests/pytests/integration/ssh/test_cmdmod.py @@ -0,0 +1,112 @@ +import pytest + +pytestmark = [pytest.mark.slow_test] + + +@pytest.fixture(scope="module", autouse=True) +def pillar_tree(base_env_pillar_tree_root_dir): + top_file = """ + base: + 'localhost': + - basic + '127.0.0.1': + - basic + """ + basic_pillar_file = """ + alot: many + """ + top_tempfile = pytest.helpers.temp_file( + "top.sls", top_file, base_env_pillar_tree_root_dir + ) + basic_tempfile = pytest.helpers.temp_file( + "basic.sls", basic_pillar_file, base_env_pillar_tree_root_dir + ) + + with top_tempfile, basic_tempfile: + yield + + +@pytest.fixture(scope="module") +def script_templated(base_env_state_tree_root_dir): + contents = """ +#!/usr/bin/env bash + +echo {{ pillar["alot"] }} +""" + with pytest.helpers.temp_file( + "parrot.sh", contents, base_env_state_tree_root_dir + ) as script: + yield f"salt://{script.name}" + + +def test_script(salt_ssh_cli): + args = "saltines crackers biscuits=yes" + script = "salt://script.py" + ret = salt_ssh_cli.run("cmd.script", script, args) + assert ret.returncode == 0 + assert isinstance(ret.data, dict) + assert ret.data + assert ret.data["stdout"] == args + + +def test_script_query_string(salt_ssh_cli): + args = "saltines crackers biscuits=yes" + script = "salt://script.py?saltenv=base" + ret = salt_ssh_cli.run("cmd.script", script, args) + assert ret.returncode == 0 + assert isinstance(ret.data, dict) + assert ret.data + assert ret.data["stdout"] == args + + +def test_script_cwd(salt_ssh_cli, tmp_path): + args = "saltines crackers biscuits=yes" + script = "salt://script.py" + # can't pass cwd as kwarg + ret = salt_ssh_cli.run("cmd.script", script, args, tmp_path) + assert ret.returncode == 0 + assert isinstance(ret.data, dict) + assert ret.data + assert ret.data["stdout"] == args + + +def test_script_cwd_with_space(salt_ssh_cli, tmp_path): + tmp_cwd = tmp_path / "test 2" + tmp_cwd.mkdir() + args = "saltines crackers biscuits=yes" + script = "salt://script.py" + ret = salt_ssh_cli.run("cmd.script", script, args, cwd=tmp_cwd) + assert ret.returncode == 0 + assert isinstance(ret.data, dict) + assert ret.data + assert ret.data["stdout"] == args + + +@pytest.mark.parametrize("template", (None, "jinja")) +def test_script_nonexistent(salt_ssh_cli, template): + script = "salt://non/ex/is/tent.sh" + ret = salt_ssh_cli.run("cmd.script", script, "", template=template) + assert ret.returncode == 0 # meh + assert isinstance(ret.data, dict) + assert ret.data + assert "cache_error" in ret.data + assert "retcode" in ret.data + assert ret.data["retcode"] == 1 + + +@pytest.mark.parametrize("pillar", (None, {"alot": "meow"})) +def test_script_template(salt_ssh_cli, script_templated, pillar): + ret = salt_ssh_cli.run( + "cmd.script", script_templated, template="jinja", pillar=pillar + ) + assert ret.returncode == 0 + assert isinstance(ret.data, dict) + assert ret.data + assert ret.data["stdout"] == (pillar or {}).get("alot", "many") + + +def test_script_retcode(salt_ssh_cli): + script = "salt://script.py" + ret = salt_ssh_cli.run("cmd.script_retcode", script) + assert ret.returncode == 0 + assert ret.data == 0 From 5e16d8483458d4f58dee2d968a781c8e006633ee Mon Sep 17 00:00:00 2001 From: jeanluc Date: Mon, 6 Nov 2023 15:27:17 +0100 Subject: [PATCH 06/13] Fix unused var in `grains.get` wrapper `ordered=False` would not have worked before --- salt/client/ssh/wrapper/grains.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/salt/client/ssh/wrapper/grains.py b/salt/client/ssh/wrapper/grains.py index bbacdb4ea4f9..ff3789ff84c9 100644 --- a/salt/client/ssh/wrapper/grains.py +++ b/salt/client/ssh/wrapper/grains.py @@ -66,9 +66,7 @@ def get(key, default="", delimiter=DEFAULT_TARGET_DELIM, ordered=True): grains = __grains__.value() else: grains = salt.utils.json.loads(salt.utils.json.dumps(__grains__.value())) - return salt.utils.data.traverse_dict_and_list( - __grains__.value(), key, default, delimiter - ) + return salt.utils.data.traverse_dict_and_list(grains, key, default, delimiter) def has_value(key): From fa469ebc24e37455d5ad3250e3e30ec08eb3cf70 Mon Sep 17 00:00:00 2001 From: jeanluc Date: Mon, 6 Nov 2023 23:27:15 +0100 Subject: [PATCH 07/13] Don't `mkdir -p` on empty paths --- salt/client/ssh/__init__.py | 2 +- salt/client/ssh/shell.py | 16 +++++++---- tests/pytests/unit/client/ssh/test_shell.py | 30 +++++++++++++++------ 3 files changed, 34 insertions(+), 14 deletions(-) diff --git a/salt/client/ssh/__init__.py b/salt/client/ssh/__init__.py index c547cb1b136a..4eec0eadcf66 100644 --- a/salt/client/ssh/__init__.py +++ b/salt/client/ssh/__init__.py @@ -1517,7 +1517,7 @@ def shim_cmd(self, cmd_str, extension="py"): if self.winrm: target_shim_file = saltwinshell.get_target_shim_file(self, target_shim_file) stdout, stderr, retcode = self.shell.send( - shim_tmp_file.name, target_shim_file, makedirs=True + shim_tmp_file.name, target_shim_file, makedirs=self.winrm ) if retcode != 0: log.error("Could not copy the shim script to target") diff --git a/salt/client/ssh/shell.py b/salt/client/ssh/shell.py index 5ea044f7efe2..74d70f7f39b4 100644 --- a/salt/client/ssh/shell.py +++ b/salt/client/ssh/shell.py @@ -87,7 +87,7 @@ def __init__( ssh_options=None, ): self.opts = opts - # ssh , but scp [, but scp []:/path self.host = host.strip("[]") self.user = user self.port = port @@ -339,11 +339,17 @@ def send(self, local, remote, makedirs=False): scp a file or files to a remote system """ if makedirs: - ret = self.exec_cmd(f"mkdir -p {os.path.dirname(remote)}") - if ret[2]: - return ret + pardir = os.path.dirname(remote) + if not pardir: + log.warning( + f"Makedirs called on relative filename: '{remote}'. Skipping." + ) + else: + ret = self.exec_cmd("mkdir -p " + shlex.quote(pardir)) + if ret[2]: + return ret - # scp needs [] host = self.host if ":" in host: host = f"[{host}]" diff --git a/tests/pytests/unit/client/ssh/test_shell.py b/tests/pytests/unit/client/ssh/test_shell.py index b415e06ee9f2..a363406d4c22 100644 --- a/tests/pytests/unit/client/ssh/test_shell.py +++ b/tests/pytests/unit/client/ssh/test_shell.py @@ -1,10 +1,11 @@ +import logging import subprocess import types import pytest import salt.client.ssh.shell as shell -from tests.support.mock import MagicMock, PropertyMock, patch +from tests.support.mock import MagicMock, PropertyMock, call, patch @pytest.fixture @@ -100,14 +101,27 @@ def test_ssh_shell_exec_cmd_returns_status_code_with_highest_bit_set_if_process_ assert retcode == 137 -def test_ssh_shell_send_makedirs_failure_returns_immediately(): - def exec_cmd(cmd): - if cmd.startswith("mkdir -p"): - return "", "Not a directory", 1 - return "", "", 0 +def exec_cmd(cmd): + if cmd.startswith("mkdir -p"): + return "", "Not a directory", 1 + return "OK", "", 0 + +def test_ssh_shell_send_makedirs_failure_returns_immediately(): with patch("salt.client.ssh.shell.Shell.exec_cmd", side_effect=exec_cmd): shl = shell.Shell({}, "localhost") stdout, stderr, retcode = shl.send("/tmp/file", "/tmp/file", True) - assert retcode == 1 - assert "Not a directory" in stderr + assert retcode == 1 + assert "Not a directory" in stderr + + +def test_ssh_shell_send_makedirs_on_relative_filename_skips_exec(caplog): + with patch("salt.client.ssh.shell.Shell.exec_cmd", side_effect=exec_cmd) as cmd: + with patch("salt.client.ssh.shell.Shell._run_cmd", return_value=("", "", 0)): + shl = shell.Shell({}, "localhost") + with caplog.at_level(logging.WARNING): + stdout, stderr, retcode = shl.send("/tmp/file", "targetfile", True) + assert retcode == 0 + assert "Not a directory" not in stderr + assert call("mkdir -p ''") not in cmd.mock_calls + assert "Makedirs called on relative filename" in caplog.text From 4915107fc00c58e242cfac60636dd93542ee9965 Mon Sep 17 00:00:00 2001 From: jeanluc Date: Mon, 6 Nov 2023 23:28:30 +0100 Subject: [PATCH 08/13] Fix/skip tests on RHEL/Windows --- salt/client/ssh/wrapper/cp.py | 5 ++++- tests/integration/files/file/base/script.py | 2 +- tests/pytests/unit/client/ssh/wrapper/test_cp.py | 1 + 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/salt/client/ssh/wrapper/cp.py b/salt/client/ssh/wrapper/cp.py index bb73d71ce045..e888ce7203b1 100644 --- a/salt/client/ssh/wrapper/cp.py +++ b/salt/client/ssh/wrapper/cp.py @@ -934,7 +934,10 @@ def get_cachedir( elif master: # The root cachedir on the master-side should not be overridden cachedir = os.path.join( - self.opts["cachedir"], *prefix, "absolute_root", cachedir[1:] + self.opts["cachedir"], + *prefix, + "absolute_root", + str(Path(*cachedir.split(os.sep)[1:])), ) return cachedir diff --git a/tests/integration/files/file/base/script.py b/tests/integration/files/file/base/script.py index d1b98c720e49..fe1e9e73b5b6 100644 --- a/tests/integration/files/file/base/script.py +++ b/tests/integration/files/file/base/script.py @@ -1,4 +1,4 @@ -#!/usr/bin/env python +#!/usr/bin/env python3 import sys diff --git a/tests/pytests/unit/client/ssh/wrapper/test_cp.py b/tests/pytests/unit/client/ssh/wrapper/test_cp.py index 698e72577ef3..502df3acc90f 100644 --- a/tests/pytests/unit/client/ssh/wrapper/test_cp.py +++ b/tests/pytests/unit/client/ssh/wrapper/test_cp.py @@ -23,6 +23,7 @@ log = logging.getLogger(__name__) +pytestmark = [pytest.mark.skip_on_windows] SUBDIR = "subdir" TGT = "testtarget" From 47a609fab058105109159536d94b577d452155a0 Mon Sep 17 00:00:00 2001 From: jeanluc Date: Tue, 7 Nov 2023 00:02:30 +0100 Subject: [PATCH 09/13] Add `defaults` SSH wrapper module This is a 1:1 copy of the execution module, incl. tests... --- changelog/51605.fixed.md | 1 + salt/client/ssh/wrapper/defaults.py | 240 ++++++++++++++++++ .../unit/client/ssh/wrapper/test_defaults.py | 215 ++++++++++++++++ 3 files changed, 456 insertions(+) create mode 100644 changelog/51605.fixed.md create mode 100644 salt/client/ssh/wrapper/defaults.py create mode 100644 tests/pytests/unit/client/ssh/wrapper/test_defaults.py diff --git a/changelog/51605.fixed.md b/changelog/51605.fixed.md new file mode 100644 index 000000000000..990b34413d95 --- /dev/null +++ b/changelog/51605.fixed.md @@ -0,0 +1 @@ +Fixed defaults.merge is not available when using salt-ssh diff --git a/salt/client/ssh/wrapper/defaults.py b/salt/client/ssh/wrapper/defaults.py new file mode 100644 index 000000000000..d03990b87980 --- /dev/null +++ b/salt/client/ssh/wrapper/defaults.py @@ -0,0 +1,240 @@ +""" +SSH wrapper module to work with salt formula defaults files + +""" + +import copy +import logging +import os + +import salt.fileclient +import salt.utils.data +import salt.utils.dictupdate as dictupdate +import salt.utils.files +import salt.utils.json +import salt.utils.url +import salt.utils.yaml + +__virtualname__ = "defaults" + +log = logging.getLogger(__name__) + + +def _mk_client(): + """ + Create a file client and add it to the context + """ + return salt.fileclient.get_file_client(__opts__) + + +def _load(formula): + """ + Generates a list of salt:///defaults.(json|yaml) files + and fetches them from the Salt master. + + Returns first defaults file as python dict. + """ + + # Compute possibilities + paths = [] + for ext in ("yaml", "json"): + source_url = salt.utils.url.create(formula + "/defaults." + ext) + paths.append(source_url) + # Fetch files from master + with _mk_client() as client: + defaults_files = client.cache_files(paths) + + for file_ in defaults_files: + if not file_: + # Skip empty string returned by cp.fileclient.cache_files. + continue + + suffix = file_.rsplit(".", 1)[-1] + if suffix == "yaml": + loader = salt.utils.yaml.safe_load + elif suffix == "json": + loader = salt.utils.json.load + else: + log.debug("Failed to determine loader for %r", file_) + continue + + if os.path.exists(file_): + log.debug("Reading defaults from %r", file_) + with salt.utils.files.fopen(file_) as fhr: + defaults = loader(fhr) + log.debug("Read defaults %r", defaults) + + return defaults or {} + + +def get(key, default=""): + """ + defaults.get is used much like pillar.get except that it will read + a default value for a pillar from defaults.json or defaults.yaml + files that are stored in the root of a salt formula. + + CLI Example: + + .. code-block:: bash + + salt '*' defaults.get core:users:root + + The defaults is computed from pillar key. The first entry is considered as + the formula namespace. + + For example, querying ``core:users:root`` will try to load + ``salt://core/defaults.yaml`` and ``salt://core/defaults.json``. + """ + + # Determine formula namespace from query + if ":" in key: + namespace, key = key.split(":", 1) + else: + namespace, key = key, None + + # Fetch and load defaults formula files from states. + defaults = _load(namespace) + + # Fetch value + if key: + return salt.utils.data.traverse_dict_and_list(defaults, key, default) + else: + return defaults + + +def merge(dest, src, merge_lists=False, in_place=True, convert_none=True): + """ + defaults.merge + Allows deep merging of dicts in formulas. + + merge_lists : False + If True, it will also merge lists instead of replace their items. + + in_place : True + If True, it will merge into dest dict, + if not it will make a new copy from that dict and return it. + + convert_none : True + If True, it will convert src and dest to empty dicts if they are None. + If True and dest is None but in_place is True, raises TypeError. + If False it will make a new copy from that dict and return it. + + .. versionadded:: 3005 + + CLI Example: + + .. code-block:: bash + + salt '*' defaults.merge '{a: b}' '{d: e}' + + It is more typical to use this in a templating language in formulas, + instead of directly on the command-line. + """ + # Force empty dicts if applicable (useful for cleaner templating) + src = {} if (src is None and convert_none) else src + if dest is None and convert_none: + if in_place: + raise TypeError("Can't perform in-place merge into NoneType") + else: + dest = {} + + if in_place: + merged = dest + else: + merged = copy.deepcopy(dest) + return dictupdate.update(merged, src, merge_lists=merge_lists) + + +def deepcopy(source): + """ + defaults.deepcopy + Allows deep copy of objects in formulas. + + By default, Python does not copy objects, + it creates bindings between a target and an object. + + It is more typical to use this in a templating language in formulas, + instead of directly on the command-line. + """ + return copy.deepcopy(source) + + +def update(dest, defaults, merge_lists=True, in_place=True, convert_none=True): + """ + defaults.update + Allows setting defaults for group of data set e.g. group for nodes. + + This function is a combination of defaults.merge + and defaults.deepcopy to avoid redundant in jinja. + + Example: + + .. code-block:: yaml + + group01: + defaults: + enabled: True + extra: + - test + - stage + nodes: + host01: + index: foo + upstream: bar + host02: + index: foo2 + upstream: bar2 + + .. code-block:: jinja + + {% do salt['defaults.update'](group01.nodes, group01.defaults) %} + + Each node will look like the following: + + .. code-block:: yaml + + host01: + enabled: True + index: foo + upstream: bar + extra: + - test + - stage + + merge_lists : True + If True, it will also merge lists instead of replace their items. + + in_place : True + If True, it will merge into dest dict. + if not it will make a new copy from that dict and return it. + + convert_none : True + If True, it will convert src and dest to empty dicts if they are None. + If True and dest is None but in_place is True, raises TypeError. + If False it will make a new copy from that dict and return it. + + .. versionadded:: 3005 + + It is more typical to use this in a templating language in formulas, + instead of directly on the command-line. + """ + # Force empty dicts if applicable here + if in_place: + if dest is None: + raise TypeError("Can't perform in-place update into NoneType") + else: + nodes = dest + else: + dest = {} if (dest is None and convert_none) else dest + nodes = deepcopy(dest) + + defaults = {} if (defaults is None and convert_none) else defaults + + for node_name, node_vars in nodes.items(): + defaults_vars = deepcopy(defaults) + node_vars = merge( + defaults_vars, node_vars, merge_lists=merge_lists, convert_none=convert_none + ) + nodes[node_name] = node_vars + + return nodes diff --git a/tests/pytests/unit/client/ssh/wrapper/test_defaults.py b/tests/pytests/unit/client/ssh/wrapper/test_defaults.py new file mode 100644 index 000000000000..12d07bc2a854 --- /dev/null +++ b/tests/pytests/unit/client/ssh/wrapper/test_defaults.py @@ -0,0 +1,215 @@ +""" +Test cases for salt.client.ssh.wrapper.defaults + +This has been copied 1:1 from tests.pytests.unit.modules.test_defaults +""" + +import inspect + +import pytest + +import salt.client.ssh.wrapper.defaults as defaults +from tests.support.mock import MagicMock, patch + + +@pytest.fixture() +def configure_loader_modules(): + return {defaults: {}} + + +def test_get_mock(): + """ + Test if it execute a defaults client run and return a dict + """ + with patch.object(inspect, "stack", MagicMock(return_value=[])), patch( + "salt.client.ssh.wrapper.defaults.get", + MagicMock(return_value={"users": {"root": [0]}}), + ): + assert defaults.get("core:users:root") == {"users": {"root": [0]}} + + +def test_merge_with_list_merging(): + """ + Test deep merging of dicts with merge_lists enabled. + """ + + src_dict = { + "string_key": "string_val_src", + "list_key": ["list_val_src"], + "dict_key": {"dict_key_src": "dict_val_src"}, + } + + dest_dict = { + "string_key": "string_val_dest", + "list_key": ["list_val_dest"], + "dict_key": {"dict_key_dest": "dict_val_dest"}, + } + + merged_dict = { + "string_key": "string_val_src", + "list_key": ["list_val_dest", "list_val_src"], + "dict_key": { + "dict_key_dest": "dict_val_dest", + "dict_key_src": "dict_val_src", + }, + } + + defaults.merge(dest_dict, src_dict, merge_lists=True) + assert dest_dict == merged_dict + + +def test_merge_without_list_merging(): + """ + Test deep merging of dicts with merge_lists disabled. + """ + + src = { + "string_key": "string_val_src", + "list_key": ["list_val_src"], + "dict_key": {"dict_key_src": "dict_val_src"}, + } + + dest = { + "string_key": "string_val_dest", + "list_key": ["list_val_dest"], + "dict_key": {"dict_key_dest": "dict_val_dest"}, + } + + merged = { + "string_key": "string_val_src", + "list_key": ["list_val_src"], + "dict_key": { + "dict_key_dest": "dict_val_dest", + "dict_key_src": "dict_val_src", + }, + } + + defaults.merge(dest, src, merge_lists=False) + assert dest == merged + + +def test_merge_not_in_place(): + """ + Test deep merging of dicts not in place. + """ + + src = {"nested_dict": {"A": "A"}} + + dest = {"nested_dict": {"B": "B"}} + + dest_orig = {"nested_dict": {"B": "B"}} + + merged = {"nested_dict": {"A": "A", "B": "B"}} + + final = defaults.merge(dest, src, in_place=False) + assert dest == dest_orig + assert final == merged + + +def test_merge_src_is_none(): + """ + Test deep merging of dicts not in place. + """ + + dest = {"nested_dict": {"B": "B"}} + + dest_orig = {"nested_dict": {"B": "B"}} + + final = defaults.merge(dest, None, in_place=False) + assert dest == dest_orig + assert final == dest_orig + + +def test_merge_dest_is_none(): + """ + Test deep merging of dicts not in place. + """ + + src = {"nested_dict": {"B": "B"}} + + src_orig = {"nested_dict": {"B": "B"}} + + final = defaults.merge(None, src, in_place=False) + assert src == src_orig + assert final == src_orig + + +def test_merge_in_place_dest_is_none(): + """ + Test deep merging of dicts not in place. + """ + + src = {"nested_dict": {"B": "B"}} + + pytest.raises(TypeError, defaults.merge, None, src) + + +def test_deepcopy(): + """ + Test a deep copy of object. + """ + + src = {"A": "A", "B": "B"} + + dist = defaults.deepcopy(src) + dist.update({"C": "C"}) + + result = {"A": "A", "B": "B", "C": "C"} + + assert src != dist + assert dist == result + + +def test_update_in_place(): + """ + Test update with defaults values in place. + """ + + group01 = { + "defaults": {"enabled": True, "extra": ["test", "stage"]}, + "nodes": {"host01": {"index": "foo", "upstream": "bar"}}, + } + + host01 = { + "enabled": True, + "index": "foo", + "upstream": "bar", + "extra": ["test", "stage"], + } + + defaults.update(group01["nodes"], group01["defaults"]) + assert group01["nodes"]["host01"] == host01 + + +def test_update_with_defaults_none(): + group01 = { + "defaults": {"enabled": True, "extra": ["test", "stage"]}, + "nodes": {"host01": {"index": "foo", "upstream": "bar"}}, + } + + host01 = { + "index": "foo", + "upstream": "bar", + } + + defaults.update(group01["nodes"], None) + assert group01["nodes"]["host01"] == host01 + + +def test_update_with_dest_none(): + group01 = { + "defaults": {"enabled": True, "extra": ["test", "stage"]}, + "nodes": {"host01": {"index": "foo", "upstream": "bar"}}, + } + + ret = defaults.update(None, group01["defaults"], in_place=False) + assert ret == {} + + +def test_update_in_place_with_dest_none(): + group01 = { + "defaults": {"enabled": True, "extra": ["test", "stage"]}, + "nodes": {"host01": {"index": "foo", "upstream": "bar"}}, + } + + pytest.raises(TypeError, defaults.update, None, group01["defaults"]) From 4c15063de4aa4663bb984ca65a916adc8147a8b5 Mon Sep 17 00:00:00 2001 From: jeanluc Date: Fri, 17 Nov 2023 14:17:20 +0100 Subject: [PATCH 10/13] Run pre-commit after base change --- salt/client/ssh/wrapper/config.py | 4 ++-- salt/client/ssh/wrapper/cp.py | 2 +- salt/client/ssh/wrapper/slsutil.py | 8 ++++---- tests/pytests/unit/client/ssh/wrapper/test_cp.py | 14 +++++--------- 4 files changed, 12 insertions(+), 16 deletions(-) diff --git a/salt/client/ssh/wrapper/config.py b/salt/client/ssh/wrapper/config.py index 7dd3a7aa463f..a6db176453c2 100644 --- a/salt/client/ssh/wrapper/config.py +++ b/salt/client/ssh/wrapper/config.py @@ -498,10 +498,10 @@ def dot_vals(value): """ ret = {} for key, val in __pillar__.get("master", {}).items(): - if key.startswith("{}.".format(value)): + if key.startswith(f"{value}."): ret[key] = val for key, val in __opts__.items(): - if key.startswith("{}.".format(value)): + if key.startswith(f"{value}."): ret[key] = val return ret diff --git a/salt/client/ssh/wrapper/cp.py b/salt/client/ssh/wrapper/cp.py index e888ce7203b1..111eab1ae395 100644 --- a/salt/client/ssh/wrapper/cp.py +++ b/salt/client/ssh/wrapper/cp.py @@ -816,7 +816,7 @@ def _render_filenames(path, dest, saltenv, template, **kw): # render the path as a template using path_template_engine as the engine if template not in salt.utils.templates.TEMPLATE_REGISTRY: raise CommandExecutionError( - "Attempted to render file paths with unavailable engine {}".format(template) + f"Attempted to render file paths with unavailable engine {template}" ) kwargs = {} diff --git a/salt/client/ssh/wrapper/slsutil.py b/salt/client/ssh/wrapper/slsutil.py index e09ca1c29845..586d09ad2d61 100644 --- a/salt/client/ssh/wrapper/slsutil.py +++ b/salt/client/ssh/wrapper/slsutil.py @@ -173,7 +173,7 @@ def run(): default_renderer, __opts__["renderer_blacklist"], __opts__["renderer_whitelist"], - **kwargs + **kwargs, ) return ret.read() if salt.utils.stringio.is_readable(ret) else ret @@ -185,12 +185,12 @@ def _get_serialize_fn(serializer, fn_name): if not fns: raise salt.exceptions.CommandExecutionError( - "Serializer '{}' not found.".format(serializer) + f"Serializer '{serializer}' not found." ) if not fn: raise salt.exceptions.CommandExecutionError( - "Serializer '{}' does not implement {}.".format(serializer, fn_name) + f"Serializer '{serializer}' does not implement {fn_name}." ) return fn @@ -419,7 +419,7 @@ def findup(startpath, filenames, saltenv="base"): # Verify the cwd is a valid path in the state tree if startpath and not path_exists(startpath, saltenv): raise salt.exceptions.SaltInvocationError( - "Starting path not found in the state tree: {}".format(startpath) + f"Starting path not found in the state tree: {startpath}" ) # Ensure that patterns is a string or list of strings diff --git a/tests/pytests/unit/client/ssh/wrapper/test_cp.py b/tests/pytests/unit/client/ssh/wrapper/test_cp.py index 502df3acc90f..58f8334721c4 100644 --- a/tests/pytests/unit/client/ssh/wrapper/test_cp.py +++ b/tests/pytests/unit/client/ssh/wrapper/test_cp.py @@ -202,7 +202,7 @@ def _new_dir(path): path = os.path.join(saltenv_root, "foo.txt") with salt.utils.files.fopen(path, "w") as fp_: - fp_.write("This is a test file in the '{}' saltenv.\n".format(saltenv)) + fp_.write(f"This is a test file in the '{saltenv}' saltenv.\n") (Path(saltenv_root) / "dirtofile").touch() (Path(saltenv_root) / "filetodir").mkdir() (Path(saltenv_root) / "filetodir" / "foo.sh").touch() @@ -235,7 +235,7 @@ def test_cache_dir(client, cache_root): Ensure entire directory is cached to correct location """ for saltenv in _saltenvs(): - assert client.cache_dir("salt://{}".format(SUBDIR), saltenv, cachedir=None) + assert client.cache_dir(f"salt://{SUBDIR}", saltenv, cachedir=None) for subdir_file in _subdir_files(): cache_loc = os.path.join( cache_root, @@ -282,9 +282,7 @@ def test_cache_dir_with_alternate_cachedir_and_absolute_path( alt_cachedir = os.path.join(tmp_path, "abs_cachedir") for saltenv in _saltenvs(): - assert client.cache_dir( - "salt://{}".format(SUBDIR), saltenv, cachedir=alt_cachedir - ) + assert client.cache_dir(f"salt://{SUBDIR}", saltenv, cachedir=alt_cachedir) for subdir_file in _subdir_files(): cache_loc = os.path.join( cache_root, @@ -326,9 +324,7 @@ def test_cache_dir_with_alternate_cachedir_and_relative_path(client, cache_root) alt_cachedir = "foo" for saltenv in _saltenvs(): - assert client.cache_dir( - "salt://{}".format(SUBDIR), saltenv, cachedir=alt_cachedir - ) + assert client.cache_dir(f"salt://{SUBDIR}", saltenv, cachedir=alt_cachedir) for subdir_file in _subdir_files(): cache_loc = os.path.join( cache_root, @@ -488,7 +484,7 @@ def _salt(saltenv="base"): ) def _check(ret, expected): - assert ret == expected, "{} != {}".format(ret, expected) + assert ret == expected, f"{ret} != {expected}" _check(client.cache_dest(f"https://{relpath}"), _external()) From 18bc40c77af75c491d2009ad851e86287fc3f6dd Mon Sep 17 00:00:00 2001 From: jeanluc Date: Tue, 28 Nov 2023 21:46:14 +0100 Subject: [PATCH 11/13] Add logmod SSH wrapper --- changelog/65630.fixed.md | 1 + salt/client/ssh/wrapper/logmod.py | 79 +++++++++++++++++++++++++++++++ 2 files changed, 80 insertions(+) create mode 100644 changelog/65630.fixed.md create mode 100644 salt/client/ssh/wrapper/logmod.py diff --git a/changelog/65630.fixed.md b/changelog/65630.fixed.md new file mode 100644 index 000000000000..e8650abcdc1c --- /dev/null +++ b/changelog/65630.fixed.md @@ -0,0 +1 @@ +Added SSH wrapper for logmod diff --git a/salt/client/ssh/wrapper/logmod.py b/salt/client/ssh/wrapper/logmod.py new file mode 100644 index 000000000000..911fd7a1d4c7 --- /dev/null +++ b/salt/client/ssh/wrapper/logmod.py @@ -0,0 +1,79 @@ +""" +On-demand logging +================= + +.. versionadded:: 2017.7.0 + +The sole purpose of this module is logging messages in the minion. +It comes very handy when debugging complex Jinja templates, for example: + +.. code-block:: jinja + + {%- for var in range(10) %} + {%- do salt["log.info"](var) -%} + {%- endfor %} + +CLI Example: + +.. code-block:: bash + + salt '*' log.error "Please don't do that, this module is not for CLI use!" +""" + +import logging + +log = logging.getLogger(__name__) + +__virtualname__ = "log" + + +def __virtual__(): + return __virtualname__ + + +def debug(message): + """ + Log message at level DEBUG. + """ + log.debug(message) + return True + + +def info(message): + """ + Log message at level INFO. + """ + log.info(message) + return True + + +def warning(message): + """ + Log message at level WARNING. + """ + log.warning(message) + return True + + +def error(message): + """ + Log message at level ERROR. + """ + log.error(message) + return True + + +def critical(message): + """ + Log message at level CRITICAL. + """ + log.critical(message) + return True + + +def exception(message): + """ + Log message at level EXCEPTION. + """ + log.exception(message) + return True From 6a715107fa89b060175ef0c0ae0827b409411543 Mon Sep 17 00:00:00 2001 From: jeanluc Date: Fri, 8 Dec 2023 11:17:50 +0100 Subject: [PATCH 12/13] Fix tests after rebase --- .../pytests/integration/ssh/state/conftest.py | 2 +- .../integration/ssh/state/test_state.py | 59 ----------------- .../ssh/state/test_with_import_dir.py | 65 +++++++++++++++++++ tests/pytests/integration/ssh/test_config.py | 4 +- 4 files changed, 69 insertions(+), 61 deletions(-) create mode 100644 tests/pytests/integration/ssh/state/test_with_import_dir.py diff --git a/tests/pytests/integration/ssh/state/conftest.py b/tests/pytests/integration/ssh/state/conftest.py index 14d645ae8e8a..9de0d6bcad81 100644 --- a/tests/pytests/integration/ssh/state/conftest.py +++ b/tests/pytests/integration/ssh/state/conftest.py @@ -17,7 +17,7 @@ def state_tree(base_env_state_tree_root_dir): state_file = """ {%- from "map.jinja" import abc with context %} Ok with {{ abc }}: - test.succeed_without_changes + test.succeed_with_changes """ top_tempfile = pytest.helpers.temp_file( "top.sls", top_file, base_env_state_tree_root_dir diff --git a/tests/pytests/integration/ssh/state/test_state.py b/tests/pytests/integration/ssh/state/test_state.py index a7ebb22a601b..0a0cad152726 100644 --- a/tests/pytests/integration/ssh/state/test_state.py +++ b/tests/pytests/integration/ssh/state/test_state.py @@ -15,65 +15,6 @@ def test_state_with_import(salt_ssh_cli, state_tree): assert ret.data -@pytest.mark.parametrize( - "ssh_cmd", - [ - "state.sls", - "state.highstate", - "state.apply", - "state.show_top", - "state.show_highstate", - "state.show_low_sls", - "state.show_lowstate", - "state.sls_id", - "state.show_sls", - "state.top", - ], -) -def test_state_with_import_dir(salt_ssh_cli, state_tree_dir, ssh_cmd): - """ - verify salt-ssh can use imported map files in states - when the map files are in another directory outside of - sls files importing them. - """ - if ssh_cmd in ("state.sls", "state.show_low_sls", "state.show_sls"): - ret = salt_ssh_cli.run("-w", "-t", ssh_cmd, "test") - elif ssh_cmd == "state.top": - ret = salt_ssh_cli.run("-w", "-t", ssh_cmd, "top.sls") - elif ssh_cmd == "state.sls_id": - ret = salt_ssh_cli.run("-w", "-t", ssh_cmd, "Ok with def", "test") - else: - ret = salt_ssh_cli.run("-w", "-t", ssh_cmd) - assert ret.returncode == 0 - if ssh_cmd == "state.show_top": - assert ret.data == {"base": ["test", "master_tops_test"]} or {"base": ["test"]} - elif ssh_cmd in ("state.show_highstate", "state.show_sls"): - assert ret.data == { - "Ok with def": { - "__sls__": "test", - "__env__": "base", - "test": ["succeed_without_changes", {"order": 10000}], - } - } - elif ssh_cmd in ("state.show_low_sls", "state.show_lowstate", "state.show_sls"): - assert ret.data == [ - { - "state": "test", - "name": "Ok with def", - "__sls__": "test", - "__env__": "base", - "__id__": "Ok with def", - "order": 10000, - "fun": "succeed_without_changes", - } - ] - else: - assert ret.data["test_|-Ok with def_|-Ok with def_|-succeed_without_changes"][ - "result" - ] - assert ret.data - - def test_state_with_import_from_dir(salt_ssh_cli, nested_state_tree): """ verify salt-ssh can use imported map files in states diff --git a/tests/pytests/integration/ssh/state/test_with_import_dir.py b/tests/pytests/integration/ssh/state/test_with_import_dir.py new file mode 100644 index 000000000000..4048545bfc20 --- /dev/null +++ b/tests/pytests/integration/ssh/state/test_with_import_dir.py @@ -0,0 +1,65 @@ +""" +Verify salt-ssh can use imported map files in states +when the map files are in another directory outside of +sls files importing them. +""" +import pytest + +pytestmark = [ + pytest.mark.skip_on_windows(reason="salt-ssh not available on Windows"), + pytest.mark.slow_test, +] + + +@pytest.mark.parametrize( + "ssh_cmd", + [ + "state.sls", + "state.highstate", + "state.apply", + "state.show_top", + "state.show_highstate", + "state.show_low_sls", + "state.show_lowstate", + "state.sls_id", + "state.show_sls", + "state.top", + ], +) +def test_state_with_import_dir(salt_ssh_cli, state_tree_dir, ssh_cmd): + if ssh_cmd in ("state.sls", "state.show_low_sls", "state.show_sls"): + ret = salt_ssh_cli.run("-w", "-t", ssh_cmd, "test") + elif ssh_cmd == "state.top": + ret = salt_ssh_cli.run("-w", "-t", ssh_cmd, "top.sls") + elif ssh_cmd == "state.sls_id": + ret = salt_ssh_cli.run("-w", "-t", ssh_cmd, "Ok with def", "test") + else: + ret = salt_ssh_cli.run("-w", "-t", ssh_cmd) + assert ret.returncode == 0 + if ssh_cmd == "state.show_top": + assert ret.data == {"base": ["test", "master_tops_test"]} or {"base": ["test"]} + elif ssh_cmd in ("state.show_highstate", "state.show_sls"): + assert ret.data == { + "Ok with def": { + "__sls__": "test", + "__env__": "base", + "test": ["succeed_without_changes", {"order": 10000}], + } + } + elif ssh_cmd in ("state.show_low_sls", "state.show_lowstate", "state.show_sls"): + assert ret.data == [ + { + "state": "test", + "name": "Ok with def", + "__sls__": "test", + "__env__": "base", + "__id__": "Ok with def", + "order": 10000, + "fun": "succeed_without_changes", + } + ] + else: + assert ret.data["test_|-Ok with def_|-Ok with def_|-succeed_without_changes"][ + "result" + ] + assert ret.data diff --git a/tests/pytests/integration/ssh/test_config.py b/tests/pytests/integration/ssh/test_config.py index d3ae2b03a3e4..7f38ec5a0a87 100644 --- a/tests/pytests/integration/ssh/test_config.py +++ b/tests/pytests/integration/ssh/test_config.py @@ -16,7 +16,9 @@ def test_items(salt_ssh_cli): @pytest.mark.parametrize("omit", (False, True)) def test_option_minion_opt(salt_ssh_cli, omit): # Minion opt - ret = salt_ssh_cli.run("config.option", "id", omit_opts=omit, omit_grains=True) + ret = salt_ssh_cli.run( + "config.option", "id", omit_opts=omit, omit_grains=True, omit_master=True + ) assert ret.returncode == 0 assert (ret.data != salt_ssh_cli.get_minion_tgt()) is omit assert (ret.data == "") is omit From e8b4722b1ee43f4e78f5e00d56ef8a65c37ca66a Mon Sep 17 00:00:00 2001 From: jeanluc Date: Mon, 18 Dec 2023 18:42:56 +0100 Subject: [PATCH 13/13] Remove `_check_ret` calls since they're not needed anymore after #64542 was merged --- salt/client/ssh/wrapper/cmdmod.py | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/salt/client/ssh/wrapper/cmdmod.py b/salt/client/ssh/wrapper/cmdmod.py index 701f21cf6d3e..788c86b31447 100644 --- a/salt/client/ssh/wrapper/cmdmod.py +++ b/salt/client/ssh/wrapper/cmdmod.py @@ -225,15 +225,9 @@ def script( salt '*' cmd.script salt://scripts/runme.sh stdin='one\\ntwo\\nthree\\nfour\\nfive\\n' """ - def _check_ret(ret): - # Failing unwrapped calls to the minion always return a result dict - # and do not throw exceptions currently. - if isinstance(ret, dict) and ret.get("stderr"): - raise CommandExecutionError(ret["stderr"]) - def _cleanup_tempfile(path): try: - _check_ret(__salt__["file.remove"](path)) + __salt__["file.remove"](path) except (SaltInvocationError, CommandExecutionError) as exc: log.error( "cmd.script: Unable to clean tempfile '%s': %s", @@ -253,7 +247,6 @@ def _cleanup_tempfile(path): path = __salt__["temp.file"]( suffix=os.path.splitext(salt.utils.url.split_env(source)[0])[1], parent=cwd ) - _check_ret(path) try: if template: if "pillarenv" in kwargs or "pillar" in kwargs: @@ -280,11 +273,9 @@ def _cleanup_tempfile(path): "stderr": "", "cache_error": True, } - _check_ret(__salt__["file.copy"](fn_, path)) - _check_ret(__salt__["file.set_mode"](path, "0500")) - uid = __salt__["file.user_to_uid"](runas) - _check_ret(uid) - _check_ret(__salt__["file.chown"](path, runas, -1)) + __salt__["file.copy"](fn_, path) + __salt__["file.set_mode"](path, "0500") + __salt__["file.chown"](path, runas, -1) cmd_path = shlex.quote(path) # We should remove the pillar from kwargs (cmd.run_all ignores it anyways)