From 41061215e59997fb15744559d02dd404268ac126 Mon Sep 17 00:00:00 2001 From: Wayne Werner Date: Tue, 23 Feb 2021 15:55:28 -0600 Subject: [PATCH 1/4] Add tests for syncing extmods According to #52001 we need to add the mod_dir to sys.path if we have any `utils_dirs`. These tests ensure that happens. --- tests/pytests/unit/utils/test_extmods.py | 131 +++++++++++++++++++++++ 1 file changed, 131 insertions(+) create mode 100644 tests/pytests/unit/utils/test_extmods.py diff --git a/tests/pytests/unit/utils/test_extmods.py b/tests/pytests/unit/utils/test_extmods.py new file mode 100644 index 000000000000..a24318626ba6 --- /dev/null +++ b/tests/pytests/unit/utils/test_extmods.py @@ -0,0 +1,131 @@ +import os + +import pytest +import salt.utils.extmods as extmods +from tests.support.mock import patch + + +@pytest.mark.parametrize( + "utils_dirs", + [["blerp"], ["/bang/fnord/blerp", "d/bang/fnord/blerp", "bang/fnord/blerp"]], +) +def test_when_utils_dirs_in_opts_and_mod_dir_ends_with_any_dir_in_utils_and_mod_dir_not_on_sys_path_then_mod_dir_should_be_added_to_sys_path( + utils_dirs, +): + + dir_to_sync = "blerp" + extension_modules = "fnord/bang/fnord" + expected_path = [os.path.join(extension_modules, dir_to_sync)] + fake_path = [] + + with patch( + "salt.fileclient.get_file_client", autospec=True + ) as fake_fileclient, patch("shutil.copyfile", autospec=True), patch( + "sys.path", fake_path + ): + fake_fileclient.return_value.cache_dir.return_value = ["something_good"] + extmods.sync( + opts={ + "utils_dirs": utils_dirs, + "extension_modules": extension_modules, + "extmod_whitelist": None, + "extmod_blacklist": None, + "cachedir": "", + "clean_dynamic_modules": False, + }, + form=dir_to_sync, + ) + + assert fake_path == expected_path + + +def test_when_utils_dirs_is_empty_then_mod_dir_should_not_be_added_to_path(): + expected_path = [] + fake_path = [] + empty_utils_dirs = [] + + with patch( + "salt.fileclient.get_file_client", autospec=True + ) as fake_fileclient, patch("shutil.copyfile", autospec=True), patch( + "sys.path", fake_path + ): + fake_fileclient.return_value.cache_dir.return_value = ["something_good"] + extmods.sync( + opts={ + "utils_dirs": empty_utils_dirs, + "extmod_whitelist": None, + "extmod_blacklist": None, + "extension_modules": "fnord", + "cachedir": "", + "clean_dynamic_modules": False, + }, + form="blerp", + ) + + assert ( + fake_path == expected_path + ), "mod_dir was added to sys.path when it should not have been." + + +@pytest.mark.parametrize( + "utils_dirs", + [ + ["mid"], + ["start/mid"], + ["start/m/end", "id/end", "d/end", "/end", "/mid/end", "t/mid/end"], + ], +) +def test_when_utils_dirs_but_mod_dir_is_not_parent_of_any_util_dir_then_mod_dir_should_not_be_added_to_path( + utils_dirs, +): + dir_to_sync = "end" + extension_modules = "start/mid" + expected_path = [] + fake_path = [] + + with patch( + "salt.fileclient.get_file_client", autospec=True + ) as fake_fileclient, patch("shutil.copyfile", autospec=True), patch( + "sys.path", fake_path + ): + fake_fileclient.return_value.cache_dir.return_value = ["something_good"] + extmods.sync( + opts={ + "utils_dirs": utils_dirs, + "extension_modules": extension_modules, + "extmod_whitelist": None, + "extmod_blacklist": None, + "cachedir": "", + "clean_dynamic_modules": False, + }, + form=dir_to_sync, + ) + + assert fake_path == expected_path + + +def test_when_mod_dir_already_on_path_it_should_not_be_added_to_path(): + dir_to_sync = "blerp" + extension_modules = "fnord/bang/fnord" + expected_path = [os.path.join(extension_modules, dir_to_sync)] + fake_path = expected_path[:] + + with patch( + "salt.fileclient.get_file_client", autospec=True + ) as fake_fileclient, patch("shutil.copyfile", autospec=True), patch( + "sys.path", fake_path + ): + fake_fileclient.return_value.cache_dir.return_value = ["something_good"] + extmods.sync( + opts={ + "utils_dirs": ["blerp", "blerp", "blerp"], + "extension_modules": extension_modules, + "extmod_whitelist": None, + "extmod_blacklist": None, + "cachedir": "", + "clean_dynamic_modules": False, + }, + form=dir_to_sync, + ) + + assert fake_path == expected_path From f2d27b4d364f2f04a0d9fc607fc858b8a080dd5c Mon Sep 17 00:00:00 2001 From: Alberto Planas Date: Wed, 6 Mar 2019 14:12:10 +0100 Subject: [PATCH 2/4] extmods: add utils directories in sys.path When the minion start clean and there are not utils directory in the extmods cache, any import from a module to any code inside the utils directory will fail. A second run of the highstate will fix this issue, as the utils directories are added into sys.path inside config/__init__.py, as part of the master and minion startup. This commit add the utils directories during the synchronization of the custom modules. Fixes #51958 --- salt/utils/extmods.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/salt/utils/extmods.py b/salt/utils/extmods.py index 18de4ba08e02..121167c2d138 100644 --- a/salt/utils/extmods.py +++ b/salt/utils/extmods.py @@ -8,6 +8,7 @@ import logging import os import shutil +import sys # Import salt libs import salt.fileclient @@ -136,6 +137,12 @@ def sync(opts, form, saltenv=None, extmod_whitelist=None, extmod_blacklist=None) shutil.copyfile(fn_, dest) ret.append("{0}.{1}".format(form, relname)) + # If the synchronized module is an utils + # directory, we add it to sys.path + for util_dir in opts['utils_dirs']: + if mod_dir.endswith(util_dir) and mod_dir not in sys.path: + sys.path.append(mod_dir) + touched = bool(ret) if opts["clean_dynamic_modules"] is True: current = set(_listdir_recursively(mod_dir)) From 14d4b6458f9ff47569d5d14d591e219b310c3180 Mon Sep 17 00:00:00 2001 From: Wayne Werner Date: Tue, 23 Feb 2021 16:00:31 -0600 Subject: [PATCH 3/4] drop py2 code --- salt/utils/extmods.py | 25 +++++++++---------------- 1 file changed, 9 insertions(+), 16 deletions(-) diff --git a/salt/utils/extmods.py b/salt/utils/extmods.py index 121167c2d138..62474166843d 100644 --- a/salt/utils/extmods.py +++ b/salt/utils/extmods.py @@ -1,25 +1,18 @@ -# -*- coding: utf-8 -*- """ Functions used to sync external modules """ -from __future__ import absolute_import, print_function, unicode_literals -# Import Python libs import logging import os import shutil import sys -# Import salt libs import salt.fileclient import salt.utils.files import salt.utils.hashutils import salt.utils.path import salt.utils.url -# Import 3rd-party libs -from salt.ext import six - log = logging.getLogger(__name__) @@ -49,7 +42,7 @@ def sync(opts, form, saltenv=None, extmod_whitelist=None, extmod_blacklist=None) if extmod_whitelist is None: extmod_whitelist = opts["extmod_whitelist"] - elif isinstance(extmod_whitelist, six.string_types): + elif isinstance(extmod_whitelist, str): extmod_whitelist = {form: extmod_whitelist.split(",")} elif not isinstance(extmod_whitelist, dict): log.error( @@ -58,19 +51,19 @@ def sync(opts, form, saltenv=None, extmod_whitelist=None, extmod_blacklist=None) if extmod_blacklist is None: extmod_blacklist = opts["extmod_blacklist"] - elif isinstance(extmod_blacklist, six.string_types): + elif isinstance(extmod_blacklist, str): extmod_blacklist = {form: extmod_blacklist.split(",")} elif not isinstance(extmod_blacklist, dict): log.error( "extmod_blacklist must be a string or dictionary: %s", extmod_blacklist ) - if isinstance(saltenv, six.string_types): + if isinstance(saltenv, str): saltenv = saltenv.split(",") ret = [] remote = set() source = salt.utils.url.create("_" + form) - mod_dir = os.path.join(opts["extension_modules"], "{0}".format(form)) + mod_dir = os.path.join(opts["extension_modules"], "{}".format(form)) touched = False with salt.utils.files.set_umask(0o077): try: @@ -78,7 +71,7 @@ def sync(opts, form, saltenv=None, extmod_whitelist=None, extmod_blacklist=None) log.info("Creating module dir '%s'", mod_dir) try: os.makedirs(mod_dir) - except (IOError, OSError): + except OSError: log.error( "Cannot create cache module directory %s. Check " "permissions.", @@ -100,7 +93,7 @@ def sync(opts, form, saltenv=None, extmod_whitelist=None, extmod_blacklist=None) ) ) local_cache_dir = os.path.join( - opts["cachedir"], "files", sub_env, "_{0}".format(form) + opts["cachedir"], "files", sub_env, "_{}".format(form) ) log.debug("Local cache dir: '%s'", local_cache_dir) for fn_ in cache: @@ -129,17 +122,17 @@ def sync(opts, form, saltenv=None, extmod_whitelist=None, extmod_blacklist=None) if src_digest != dst_digest: # The downloaded file differs, replace! shutil.copyfile(fn_, dest) - ret.append("{0}.{1}".format(form, relname)) + ret.append("{}.{}".format(form, relname)) else: dest_dir = os.path.dirname(dest) if not os.path.isdir(dest_dir): os.makedirs(dest_dir) shutil.copyfile(fn_, dest) - ret.append("{0}.{1}".format(form, relname)) + ret.append("{}.{}".format(form, relname)) # If the synchronized module is an utils # directory, we add it to sys.path - for util_dir in opts['utils_dirs']: + for util_dir in opts["utils_dirs"]: if mod_dir.endswith(util_dir) and mod_dir not in sys.path: sys.path.append(mod_dir) From 5c9115ff8d94f417a4e4f820efa2d30b3409ebd2 Mon Sep 17 00:00:00 2001 From: Wayne Werner Date: Tue, 23 Feb 2021 16:02:47 -0600 Subject: [PATCH 4/4] Fix potential bug In the original PR it assumed paths would be well-formed. This fix uses pathlib to split out the paths, which should be much more robust, as well as ensuring that the mod_dir actually does end with the dir in utils_dirs. --- salt/utils/extmods.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/salt/utils/extmods.py b/salt/utils/extmods.py index 62474166843d..e1baaea31c26 100644 --- a/salt/utils/extmods.py +++ b/salt/utils/extmods.py @@ -4,6 +4,7 @@ import logging import os +import pathlib import shutil import sys @@ -129,11 +130,15 @@ def sync(opts, form, saltenv=None, extmod_whitelist=None, extmod_blacklist=None) os.makedirs(dest_dir) shutil.copyfile(fn_, dest) ret.append("{}.{}".format(form, relname)) - # If the synchronized module is an utils # directory, we add it to sys.path for util_dir in opts["utils_dirs"]: - if mod_dir.endswith(util_dir) and mod_dir not in sys.path: + util_parts = pathlib.Path(util_dir).parts + mod_parts = pathlib.Path(mod_dir).parts + if ( + util_parts == mod_parts[-len(util_parts) :] + and mod_dir not in sys.path + ): sys.path.append(mod_dir) touched = bool(ret)