From a649fec423f6de8751825673f9f2cdf4a686bf17 Mon Sep 17 00:00:00 2001 From: Tyler Levy Conde Date: Tue, 2 Apr 2024 15:57:58 -0600 Subject: [PATCH 01/13] file.managed correctly handles paths containing a '#' --- salt/fileclient.py | 2 ++ tests/pytests/unit/fileclient/test_fileclient.py | 14 ++++++++++++++ 2 files changed, 16 insertions(+) diff --git a/salt/fileclient.py b/salt/fileclient.py index fad1da72ece..90b5ee47cc9 100644 --- a/salt/fileclient.py +++ b/salt/fileclient.py @@ -481,9 +481,11 @@ class Client: """ Get a single file from a URL. """ + url = urllib.parse.quote(url, safe=":/") url_data = urllib.parse.urlparse(url) url_scheme = url_data.scheme url_path = os.path.join(url_data.netloc, url_data.path).rstrip(os.sep) + url_path = urllib.parse.unquote(url_path) # If dest is a directory, rewrite dest with filename if dest is not None and (os.path.isdir(dest) or dest.endswith(("/", "\\"))): diff --git a/tests/pytests/unit/fileclient/test_fileclient.py b/tests/pytests/unit/fileclient/test_fileclient.py index 94512e50633..a423e075e0d 100644 --- a/tests/pytests/unit/fileclient/test_fileclient.py +++ b/tests/pytests/unit/fileclient/test_fileclient.py @@ -223,3 +223,17 @@ def test_get_file_client(file_client): with patch("salt.fileclient.RemoteClient", MagicMock(return_value="remote_client")): ret = fileclient.get_file_client(minion_opts) assert "remote_client" == ret +def test_get_url_with_hash(client_opts): + """ + Test get_url function with a URL containing a hash character. + """ + with patch("os.path.isfile", return_value=True): + with patch("os.makedirs", return_value=None): + with patch("urllib.request.urlretrieve", return_value=None): + client = fileclient.Client(client_opts) + url = "file:///path/to/file#with#hash" + dest = "/mocked/destination" + + result = client.get_url(url, dest) + + assert result == "/path/to/file#with#hash" From e9c86090ae7c760ca8cb5f1894972f01b450a0b0 Mon Sep 17 00:00:00 2001 From: Tyler Levy Conde Date: Tue, 2 Apr 2024 16:19:58 -0600 Subject: [PATCH 02/13] Black and changelog added for 63060 --- changelog/63060.fixed.md | 1 + tests/pytests/unit/fileclient/test_fileclient.py | 2 ++ 2 files changed, 3 insertions(+) create mode 100644 changelog/63060.fixed.md diff --git a/changelog/63060.fixed.md b/changelog/63060.fixed.md new file mode 100644 index 00000000000..e0290447aca --- /dev/null +++ b/changelog/63060.fixed.md @@ -0,0 +1 @@ +file.managed correctly handles file path with '#' diff --git a/tests/pytests/unit/fileclient/test_fileclient.py b/tests/pytests/unit/fileclient/test_fileclient.py index a423e075e0d..0892c6b35f6 100644 --- a/tests/pytests/unit/fileclient/test_fileclient.py +++ b/tests/pytests/unit/fileclient/test_fileclient.py @@ -223,6 +223,8 @@ def test_get_file_client(file_client): with patch("salt.fileclient.RemoteClient", MagicMock(return_value="remote_client")): ret = fileclient.get_file_client(minion_opts) assert "remote_client" == ret + + def test_get_url_with_hash(client_opts): """ Test get_url function with a URL containing a hash character. From 73d6ca8914cde47cda1eb67b379743e93f1ad975 Mon Sep 17 00:00:00 2001 From: Tyler Levy Conde Date: Thu, 4 Apr 2024 11:13:18 -0600 Subject: [PATCH 03/13] Passing pre-commit --- tests/pytests/unit/fileclient/test_fileclient.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/tests/pytests/unit/fileclient/test_fileclient.py b/tests/pytests/unit/fileclient/test_fileclient.py index 0892c6b35f6..b9222dbe688 100644 --- a/tests/pytests/unit/fileclient/test_fileclient.py +++ b/tests/pytests/unit/fileclient/test_fileclient.py @@ -8,9 +8,12 @@ import os import pytest -import salt.utils.files -from salt import fileclient -from tests.support.mock import AsyncMock, MagicMock, Mock, patch +try: + import salt.utils.files + from salt import fileclient + from tests.support.mock import AsyncMock, MagicMock, Mock, patch +except ImportError: + ... log = logging.getLogger(__name__) From 2868ac1f89a94f8a1d232cdceef1eda8818fc168 Mon Sep 17 00:00:00 2001 From: Tyler Levy Conde Date: Fri, 5 Apr 2024 10:33:05 -0600 Subject: [PATCH 04/13] revert test changes --- tests/pytests/unit/fileclient/test_fileclient.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/tests/pytests/unit/fileclient/test_fileclient.py b/tests/pytests/unit/fileclient/test_fileclient.py index b9222dbe688..0892c6b35f6 100644 --- a/tests/pytests/unit/fileclient/test_fileclient.py +++ b/tests/pytests/unit/fileclient/test_fileclient.py @@ -8,12 +8,9 @@ import os import pytest -try: - import salt.utils.files - from salt import fileclient - from tests.support.mock import AsyncMock, MagicMock, Mock, patch -except ImportError: - ... +import salt.utils.files +from salt import fileclient +from tests.support.mock import AsyncMock, MagicMock, Mock, patch log = logging.getLogger(__name__) From e8c3bdb4825a53d82ffe6339af6f2af2524b84e6 Mon Sep 17 00:00:00 2001 From: Tyler Levy Conde Date: Mon, 8 Apr 2024 13:01:54 -0600 Subject: [PATCH 05/13] set allow_fragments to False --- salt/fileclient.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/salt/fileclient.py b/salt/fileclient.py index 90b5ee47cc9..32c5cd0d948 100644 --- a/salt/fileclient.py +++ b/salt/fileclient.py @@ -481,11 +481,9 @@ class Client: """ Get a single file from a URL. """ - url = urllib.parse.quote(url, safe=":/") - url_data = urllib.parse.urlparse(url) + url_data = urllib.parse.urlparse(url, allow_fragments=False) url_scheme = url_data.scheme url_path = os.path.join(url_data.netloc, url_data.path).rstrip(os.sep) - url_path = urllib.parse.unquote(url_path) # If dest is a directory, rewrite dest with filename if dest is not None and (os.path.isdir(dest) or dest.endswith(("/", "\\"))): From 9d4d730d59fb1b0b46b8655f222d07ffb9a02ab3 Mon Sep 17 00:00:00 2001 From: Pedro Algarvio Date: Tue, 9 Apr 2024 09:44:24 +0100 Subject: [PATCH 06/13] One more tests that needs the host keys ignored --- tests/pytests/integration/netapi/test_ssh_client.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/pytests/integration/netapi/test_ssh_client.py b/tests/pytests/integration/netapi/test_ssh_client.py index c1eb93f303e..79a98de2742 100644 --- a/tests/pytests/integration/netapi/test_ssh_client.py +++ b/tests/pytests/integration/netapi/test_ssh_client.py @@ -191,6 +191,7 @@ def test_shell_inject_tgt(client, salt_ssh_roster_file, tmp_path, salt_auto_acco "eauth": "auto", "username": salt_auto_account.username, "password": salt_auto_account.password, + "ignore_host_keys": True, } ret = client.run(low) assert path.exists() is False From 0340fcea03d0e6731ab160a4a463f775af4d2a4c Mon Sep 17 00:00:00 2001 From: Shane Lee Date: Wed, 27 Mar 2024 07:46:25 -0600 Subject: [PATCH 07/13] Remove _git from winrepo dir name --- salt/runners/winrepo.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/salt/runners/winrepo.py b/salt/runners/winrepo.py index 856115d6642..0c81864a43c 100644 --- a/salt/runners/winrepo.py +++ b/salt/runners/winrepo.py @@ -174,7 +174,7 @@ def update_git_repos(opts=None, clean=False, masterless=False): rev, remote_url = remote_info.strip().split() except ValueError: remote_url = remote_info - gittarget = os.path.join(base_dir, targetname).replace(".", "_") + gittarget = os.path.join(base_dir, targetname).replace(".git", "") if masterless: result = __salt__["state.single"]( "git.latest", From e5d1e4db9dfffc2c91719ae7b72b4c48b97ad449 Mon Sep 17 00:00:00 2001 From: Shane Lee Date: Wed, 27 Mar 2024 12:09:19 -0600 Subject: [PATCH 08/13] Keep directory structure the same on Master and Masterless Fixes winrepo.update_git_repos so that the directory structure is the same whether run on the master or on a minion using salt-call. --- salt/utils/gitfs.py | 2 +- .../functional/runners/test_winrepo.py | 41 ++++++++++++------- tests/pytests/unit/utils/test_gitfs.py | 2 +- tests/unit/utils/test_gitfs.py | 4 +- 4 files changed, 30 insertions(+), 19 deletions(-) diff --git a/salt/utils/gitfs.py b/salt/utils/gitfs.py index aa9190b6ee3..e6924ae6f70 100644 --- a/salt/utils/gitfs.py +++ b/salt/utils/gitfs.py @@ -472,7 +472,7 @@ class GitProvider: "/", "_" ) # replace "/" with "_" to not cause trouble with file system self._cache_hash = salt.utils.path.join(cache_root, self._cache_basehash) - self._cache_basename = "_" + self._cache_basename = "" if self.id.startswith("__env__"): try: self._cache_basename = self.get_checkout_target() diff --git a/tests/pytests/functional/runners/test_winrepo.py b/tests/pytests/functional/runners/test_winrepo.py index b22764fbe4d..65d54df62ec 100644 --- a/tests/pytests/functional/runners/test_winrepo.py +++ b/tests/pytests/functional/runners/test_winrepo.py @@ -1,3 +1,5 @@ +import os + import pytest from salt.runners import winrepo @@ -10,27 +12,21 @@ pytestmark = [ @pytest.fixture def configure_loader_modules(minion_opts, tmp_path): - opts = minion_opts.copy() winrepo_dir = tmp_path / "winrepo" winrepo_dir.mkdir() winrepo_dir_ng = tmp_path / "winrepo_ng" winrepo_dir_ng.mkdir() - opts["winrepo_dir"] = str(winrepo_dir) - opts["winrepo_dir_ng"] = str(winrepo_dir_ng) - - return { - winrepo: { - "__opts__": opts, - } - } + minion_opts["winrepo_dir"] = str(winrepo_dir) + minion_opts["winrepo_dir_ng"] = str(winrepo_dir_ng) + return {winrepo: {"__opts__": minion_opts}} @pytest.fixture def winrepo_remotes(minion_opts): - winrepo_remotes = minion_opts.get("winrepo_remotes", []) - winrepo_remotes_ng = minion_opts.get("winrepo_remotes_ng", []) - winrepo_remotes.extend(winrepo_remotes_ng) - return winrepo_remotes + remotes = set() + remotes.update(minion_opts.get("winrepo_remotes", [])) + remotes.update(minion_opts.get("winrepo_remotes_ng", [])) + return remotes def test_update_git_repos(winrepo_remotes): @@ -38,15 +34,18 @@ def test_update_git_repos(winrepo_remotes): Ensure update git repos works as intended. """ res = winrepo.update_git_repos() - assert res for remote in winrepo_remotes: assert remote in res assert res[remote] + # Make sure there are package definitions in the root + pkg_def = os.path.join(res[remote], "7zip.sls") + assert os.path.exists(pkg_def) -def test_legacy_update_git_repos(winrepo_remotes): + +def test_legacy_update_git_repos(winrepo_remotes, minion_opts): """ Ensure update git repos works as intended with legacy (non-gitfs) code. """ @@ -58,3 +57,15 @@ def test_legacy_update_git_repos(winrepo_remotes): for remote in winrepo_remotes: assert remote in res assert res[remote] + + # Make sure there are package definitions in the root + # We have to look up the actual repo dir here because the legacy + # update only returns True or False, not a path + if "-ng" in remote: + path = minion_opts["winrepo_dir_ng"] + pkg_def = os.path.join(path, "salt-winrepo-ng", "7zip.sls") + else: + path = minion_opts["winrepo_dir"] + pkg_def = os.path.join(path, "salt-winrepo", "7zip.sls") + + assert os.path.exists(pkg_def) diff --git a/tests/pytests/unit/utils/test_gitfs.py b/tests/pytests/unit/utils/test_gitfs.py index 4b0c11afd60..91d5a392feb 100644 --- a/tests/pytests/unit/utils/test_gitfs.py +++ b/tests/pytests/unit/utils/test_gitfs.py @@ -248,4 +248,4 @@ def test_checkout_pygit2(_prepare_provider): reason="Skip Pygit2 on windows, due to pygit2 access error on windows" ) def test_get_cachedir_basename_pygit2(_prepare_provider): - assert "_" == _prepare_provider.get_cache_basename() + assert "" == _prepare_provider.get_cache_basename() diff --git a/tests/unit/utils/test_gitfs.py b/tests/unit/utils/test_gitfs.py index e72f94050c8..95994d6e6b4 100644 --- a/tests/unit/utils/test_gitfs.py +++ b/tests/unit/utils/test_gitfs.py @@ -117,11 +117,11 @@ class TestGitBase(TestCase, AdaptedConfigurationTestCaseMixin): def test_get_cachedir_basename(self): self.assertEqual( self.main_class.remotes[0].get_cache_basename(), - "_", + "", ) self.assertEqual( self.main_class.remotes[1].get_cache_basename(), - "_", + "", ) def test_git_provider_mp_lock(self): From 0c05af434aa05c2862e34e459bb920e736d3af10 Mon Sep 17 00:00:00 2001 From: Shane Lee Date: Tue, 9 Apr 2024 14:38:35 -0600 Subject: [PATCH 09/13] Make the underscore directory standard --- salt/runners/winrepo.py | 6 +++++- salt/utils/gitfs.py | 2 +- tests/pytests/functional/runners/test_winrepo.py | 5 +++-- tests/pytests/functional/utils/gitfs/test_pillar.py | 1 + tests/pytests/unit/utils/test_gitfs.py | 2 +- tests/unit/utils/test_gitfs.py | 4 ++-- 6 files changed, 13 insertions(+), 7 deletions(-) diff --git a/salt/runners/winrepo.py b/salt/runners/winrepo.py index 0c81864a43c..fa58397a48c 100644 --- a/salt/runners/winrepo.py +++ b/salt/runners/winrepo.py @@ -174,7 +174,11 @@ def update_git_repos(opts=None, clean=False, masterless=False): rev, remote_url = remote_info.strip().split() except ValueError: remote_url = remote_info - gittarget = os.path.join(base_dir, targetname).replace(".git", "") + targetname = targetname.replace(".git", "") + # GitFS using pygit2 and gitpython place the repo in a + # subdirectory named `_`. We need to stay consistent when using + # the legacy method as well + gittarget = os.path.join(base_dir, targetname, "_") if masterless: result = __salt__["state.single"]( "git.latest", diff --git a/salt/utils/gitfs.py b/salt/utils/gitfs.py index e6924ae6f70..aa9190b6ee3 100644 --- a/salt/utils/gitfs.py +++ b/salt/utils/gitfs.py @@ -472,7 +472,7 @@ class GitProvider: "/", "_" ) # replace "/" with "_" to not cause trouble with file system self._cache_hash = salt.utils.path.join(cache_root, self._cache_basehash) - self._cache_basename = "" + self._cache_basename = "_" if self.id.startswith("__env__"): try: self._cache_basename = self.get_checkout_target() diff --git a/tests/pytests/functional/runners/test_winrepo.py b/tests/pytests/functional/runners/test_winrepo.py index 65d54df62ec..f744c2a98dd 100644 --- a/tests/pytests/functional/runners/test_winrepo.py +++ b/tests/pytests/functional/runners/test_winrepo.py @@ -41,6 +41,7 @@ def test_update_git_repos(winrepo_remotes): assert res[remote] # Make sure there are package definitions in the root + assert res[remote].endswith("_") pkg_def = os.path.join(res[remote], "7zip.sls") assert os.path.exists(pkg_def) @@ -63,9 +64,9 @@ def test_legacy_update_git_repos(winrepo_remotes, minion_opts): # update only returns True or False, not a path if "-ng" in remote: path = minion_opts["winrepo_dir_ng"] - pkg_def = os.path.join(path, "salt-winrepo-ng", "7zip.sls") + pkg_def = os.path.join(path, "salt-winrepo-ng", "_", "7zip.sls") else: path = minion_opts["winrepo_dir"] - pkg_def = os.path.join(path, "salt-winrepo", "7zip.sls") + pkg_def = os.path.join(path, "salt-winrepo", "_", "7zip.sls") assert os.path.exists(pkg_def) diff --git a/tests/pytests/functional/utils/gitfs/test_pillar.py b/tests/pytests/functional/utils/gitfs/test_pillar.py index 143edbf6ff5..5d6729dc5f7 100644 --- a/tests/pytests/functional/utils/gitfs/test_pillar.py +++ b/tests/pytests/functional/utils/gitfs/test_pillar.py @@ -7,6 +7,7 @@ from salt.utils.gitfs import GitPillar, GitPython, Pygit2 from salt.utils.immutabletypes import ImmutableDict, ImmutableList pytestmark = [ + pytest.mark.windows_whitelisted, pytest.mark.slow_test, ] diff --git a/tests/pytests/unit/utils/test_gitfs.py b/tests/pytests/unit/utils/test_gitfs.py index 91d5a392feb..4b0c11afd60 100644 --- a/tests/pytests/unit/utils/test_gitfs.py +++ b/tests/pytests/unit/utils/test_gitfs.py @@ -248,4 +248,4 @@ def test_checkout_pygit2(_prepare_provider): reason="Skip Pygit2 on windows, due to pygit2 access error on windows" ) def test_get_cachedir_basename_pygit2(_prepare_provider): - assert "" == _prepare_provider.get_cache_basename() + assert "_" == _prepare_provider.get_cache_basename() diff --git a/tests/unit/utils/test_gitfs.py b/tests/unit/utils/test_gitfs.py index 95994d6e6b4..e72f94050c8 100644 --- a/tests/unit/utils/test_gitfs.py +++ b/tests/unit/utils/test_gitfs.py @@ -117,11 +117,11 @@ class TestGitBase(TestCase, AdaptedConfigurationTestCaseMixin): def test_get_cachedir_basename(self): self.assertEqual( self.main_class.remotes[0].get_cache_basename(), - "", + "_", ) self.assertEqual( self.main_class.remotes[1].get_cache_basename(), - "", + "_", ) def test_git_provider_mp_lock(self): From d78245ce3596448e8aa01694234740d6c785f3ff Mon Sep 17 00:00:00 2001 From: Shane Lee Date: Wed, 10 Apr 2024 13:23:16 -0600 Subject: [PATCH 10/13] Make sure dict keys are lowercase --- changelog/66290.fixed.md | 2 ++ salt/modules/chocolatey.py | 8 ++++---- .../functional/states/test_chocolatey_1_2_1.py | 15 +++++++++++++++ .../functional/states/test_chocolatey_latest.py | 15 +++++++++++++++ tests/pytests/unit/modules/test_chocolatey.py | 14 ++++++++++++++ 5 files changed, 50 insertions(+), 4 deletions(-) create mode 100644 changelog/66290.fixed.md diff --git a/changelog/66290.fixed.md b/changelog/66290.fixed.md new file mode 100644 index 00000000000..c013043dd6b --- /dev/null +++ b/changelog/66290.fixed.md @@ -0,0 +1,2 @@ +Chocolatey: Make sure the return dictionary from ``chocolatey.version`` +contains lowercase keys diff --git a/salt/modules/chocolatey.py b/salt/modules/chocolatey.py index ade057da95c..da088665253 100644 --- a/salt/modules/chocolatey.py +++ b/salt/modules/chocolatey.py @@ -1157,8 +1157,8 @@ def version(name, check_remote=False, source=None, pre_versions=False): if installed: for pkg in installed: if lower_name == pkg.lower(): - packages.setdefault(pkg, {}) - packages[pkg]["installed"] = installed[pkg] + packages.setdefault(lower_name, {}) + packages[lower_name]["installed"] = installed[pkg] if check_remote: # If there's a remote package available, then also include that @@ -1169,8 +1169,8 @@ def version(name, check_remote=False, source=None, pre_versions=False): if available: for pkg in available: if lower_name == pkg.lower(): - packages.setdefault(pkg, {}) - packages[pkg]["available"] = available[pkg] + packages.setdefault(lower_name, {}) + packages[lower_name]["available"] = available[pkg] return packages diff --git a/tests/pytests/functional/states/test_chocolatey_1_2_1.py b/tests/pytests/functional/states/test_chocolatey_1_2_1.py index 0e9972df17e..5dd8deed543 100644 --- a/tests/pytests/functional/states/test_chocolatey_1_2_1.py +++ b/tests/pytests/functional/states/test_chocolatey_1_2_1.py @@ -109,6 +109,13 @@ def vim(chocolatey_mod): chocolatey_mod.uninstall(name="vim", force=True) +@pytest.fixture(scope="function") +def everything(chocolatey_mod): + chocolatey_mod.install(name="everything", version="1.4.1935") + yield + chocolatey_mod.uninstall(name="everything", force=True) + + def test_installed_latest(clean, chocolatey, chocolatey_mod): chocolatey.installed(name="vim") result = chocolatey_mod.version(name="vim") @@ -122,6 +129,14 @@ def test_installed_version(clean, chocolatey, chocolatey_mod): assert result["vim"]["installed"][0] == "9.0.1672" +def test_installed_version_existing_capitalization( + everything, chocolatey, chocolatey_mod +): + result = chocolatey.installed(name="everything", version="1.4.11024") + expected_changes = {"Everything": {"new": ["1.4.11024"], "old": ["1.4.1935"]}} + assert result["changes"] == expected_changes + + def test_uninstalled(vim, chocolatey, chocolatey_mod): chocolatey.uninstalled(name="vim") result = chocolatey_mod.version(name="vim") diff --git a/tests/pytests/functional/states/test_chocolatey_latest.py b/tests/pytests/functional/states/test_chocolatey_latest.py index 41ba0df5b38..c71df59e8e8 100644 --- a/tests/pytests/functional/states/test_chocolatey_latest.py +++ b/tests/pytests/functional/states/test_chocolatey_latest.py @@ -109,6 +109,13 @@ def vim(chocolatey_mod): chocolatey_mod.uninstall(name="vim", force=True) +@pytest.fixture(scope="function") +def everything(chocolatey_mod): + chocolatey_mod.install(name="everything", version="1.4.1935") + yield + chocolatey_mod.uninstall(name="everything", force=True) + + def test_installed_latest(clean, chocolatey, chocolatey_mod): chocolatey.installed(name="vim") result = chocolatey_mod.version(name="vim") @@ -122,6 +129,14 @@ def test_installed_version(clean, chocolatey, chocolatey_mod): assert result["vim"]["installed"][0] == "9.0.1672" +def test_installed_version_existing_capitalization( + everything, chocolatey, chocolatey_mod +): + result = chocolatey.installed(name="everything", version="1.4.11024") + expected_changes = {"Everything": {"new": ["1.4.11024"], "old": ["1.4.1935"]}} + assert result["changes"] == expected_changes + + def test_uninstalled(vim, chocolatey, chocolatey_mod): chocolatey.uninstalled(name="vim") result = chocolatey_mod.version(name="vim") diff --git a/tests/pytests/unit/modules/test_chocolatey.py b/tests/pytests/unit/modules/test_chocolatey.py index 8dd630793f1..6c83fa0c4b3 100644 --- a/tests/pytests/unit/modules/test_chocolatey.py +++ b/tests/pytests/unit/modules/test_chocolatey.py @@ -208,6 +208,20 @@ def test_version_check_remote_true(): assert result == expected +def test_version_check_remote_true_capitalization(): + """ + Test version when remote is True + """ + list_side_effect = [ + {"Ack": ["3.1.1"]}, + {"Ack": ["3.1.1"], "Wolfpack": ["3.0.17"], "blackbird": ["1.0.79.3"]}, + ] + with patch.object(chocolatey, "list_", side_effect=list_side_effect): + expected = {"ack": {"available": ["3.1.1"], "installed": ["3.1.1"]}} + result = chocolatey.version("ack", check_remote=True) + assert result == expected + + def test_version_check_remote_true_not_available(): """ Test version when remote is True but remote version is unavailable From 8b1e7c8427aad1238f4aad1b480993c7c48e5ce0 Mon Sep 17 00:00:00 2001 From: Thomas Phipps Date: Mon, 8 Apr 2024 18:34:54 +0000 Subject: [PATCH 11/13] add fix for pillar_override being saved in PillarCache --- salt/pillar/__init__.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/salt/pillar/__init__.py b/salt/pillar/__init__.py index 21df2b9afb7..c4653d8a558 100644 --- a/salt/pillar/__init__.py +++ b/salt/pillar/__init__.py @@ -460,12 +460,15 @@ class PillarCache: """ return os.path.join(self.opts["cachedir"], "pillar_cache", minion_id) - def fetch_pillar(self): + def fetch_pillar(self, save_override=True): """ In the event of a cache miss, we need to incur the overhead of caching a new pillar. """ log.debug("Pillar cache getting external pillar with ext: %s", self.ext) + override = None + if save_override: + override = self.pillar_override fresh_pillar = Pillar( self.opts, self.grains, @@ -473,7 +476,7 @@ class PillarCache: self.saltenv, ext=self.ext, functions=self.functions, - pillar_override=self.pillar_override, + pillar_override=override, pillarenv=self.pillarenv, extra_minion_data=self.extra_minion_data, ) @@ -528,7 +531,7 @@ class PillarCache: return fresh_pillar else: # We haven't seen this minion yet in the cache. Store it. - fresh_pillar = self.fetch_pillar() + fresh_pillar = self.fetch_pillar(save_override=False) self.cache[self.minion_id] = {self.pillarenv: fresh_pillar} log.debug("Pillar cache miss for minion %s", self.minion_id) log.debug("Current pillar cache: %s", cache_dict) # FIXME hack! From 4a8f72a15d96a457ce97b64fc4ced472429084d1 Mon Sep 17 00:00:00 2001 From: Thomas Phipps Date: Mon, 8 Apr 2024 19:00:52 +0000 Subject: [PATCH 12/13] add changelog and tests --- changelog/66292.fixed.md | 1 + tests/pytests/unit/pillar/test_pillar.py | 30 ++++++++++++++++++++++++ 2 files changed, 31 insertions(+) create mode 100644 changelog/66292.fixed.md diff --git a/changelog/66292.fixed.md b/changelog/66292.fixed.md new file mode 100644 index 00000000000..1055bb65797 --- /dev/null +++ b/changelog/66292.fixed.md @@ -0,0 +1 @@ +fix cacheing inline pillar, by not rendering inline pillar during cache save function. diff --git a/tests/pytests/unit/pillar/test_pillar.py b/tests/pytests/unit/pillar/test_pillar.py index 3b0aa2e1929..3ee5892d3b4 100644 --- a/tests/pytests/unit/pillar/test_pillar.py +++ b/tests/pytests/unit/pillar/test_pillar.py @@ -164,6 +164,36 @@ def test_pillar_get_cache_disk(temp_salt_minion, caplog): assert fresh_pillar == {} +def test_pillar_save_cache(temp_salt_minion, caplog): + with pytest.helpers.temp_directory() as temp_path: + tmp_cachedir = Path(str(temp_path) + "/pillar_cache/") + tmp_cachedir.mkdir(parents=True) + assert tmp_cachedir.exists() + tmp_cachefile = Path(str(temp_path) + "/pillar_cache/" + temp_salt_minion.id) + assert tmp_cachefile.exists() is False + + opts = temp_salt_minion.config.copy() + opts["pillarenv"] = None + opts["pillar_cache"] = True + opts["cachedir"] = str(temp_path) + + pillar_override = {"inline_pillar": True} + + caplog.at_level(logging.DEBUG) + pillar = salt.pillar.PillarCache( + opts=opts, + grains=salt.loader.grains(opts), + minion_id=temp_salt_minion.id, + saltenv="base", + pillar_override=pillar_override, + ) + + fresh_pillar = pillar.fetch_pillar(save_override=False) + assert fresh_pillar == {} + fresh_pillar2 = pillar.fetch_pillar() + assert fresh_pillar2 == pillar_override + + def test_remote_pillar_timeout(temp_salt_minion, tmp_path): opts = temp_salt_minion.config.copy() opts["master_uri"] = "tcp://127.0.0.1:12323" From 559825c92249da630e95c2034ed19d01854ce823 Mon Sep 17 00:00:00 2001 From: Thomas Phipps Date: Tue, 9 Apr 2024 16:48:17 +0000 Subject: [PATCH 13/13] remove backwords compatibility. internally salt doesn't need it. and if someone is using PillarCache incorrectly that is on them. --- salt/pillar/__init__.py | 9 +++------ tests/pytests/unit/pillar/test_pillar.py | 6 ++---- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/salt/pillar/__init__.py b/salt/pillar/__init__.py index c4653d8a558..02b1006b5ef 100644 --- a/salt/pillar/__init__.py +++ b/salt/pillar/__init__.py @@ -460,15 +460,12 @@ class PillarCache: """ return os.path.join(self.opts["cachedir"], "pillar_cache", minion_id) - def fetch_pillar(self, save_override=True): + def fetch_pillar(self): """ In the event of a cache miss, we need to incur the overhead of caching a new pillar. """ log.debug("Pillar cache getting external pillar with ext: %s", self.ext) - override = None - if save_override: - override = self.pillar_override fresh_pillar = Pillar( self.opts, self.grains, @@ -476,7 +473,7 @@ class PillarCache: self.saltenv, ext=self.ext, functions=self.functions, - pillar_override=override, + pillar_override=None, pillarenv=self.pillarenv, extra_minion_data=self.extra_minion_data, ) @@ -531,7 +528,7 @@ class PillarCache: return fresh_pillar else: # We haven't seen this minion yet in the cache. Store it. - fresh_pillar = self.fetch_pillar(save_override=False) + fresh_pillar = self.fetch_pillar() self.cache[self.minion_id] = {self.pillarenv: fresh_pillar} log.debug("Pillar cache miss for minion %s", self.minion_id) log.debug("Current pillar cache: %s", cache_dict) # FIXME hack! diff --git a/tests/pytests/unit/pillar/test_pillar.py b/tests/pytests/unit/pillar/test_pillar.py index 3ee5892d3b4..763739f9193 100644 --- a/tests/pytests/unit/pillar/test_pillar.py +++ b/tests/pytests/unit/pillar/test_pillar.py @@ -164,7 +164,7 @@ def test_pillar_get_cache_disk(temp_salt_minion, caplog): assert fresh_pillar == {} -def test_pillar_save_cache(temp_salt_minion, caplog): +def test_pillar_fetch_pillar_override_skipped(temp_salt_minion, caplog): with pytest.helpers.temp_directory() as temp_path: tmp_cachedir = Path(str(temp_path) + "/pillar_cache/") tmp_cachedir.mkdir(parents=True) @@ -188,10 +188,8 @@ def test_pillar_save_cache(temp_salt_minion, caplog): pillar_override=pillar_override, ) - fresh_pillar = pillar.fetch_pillar(save_override=False) + fresh_pillar = pillar.fetch_pillar() assert fresh_pillar == {} - fresh_pillar2 = pillar.fetch_pillar() - assert fresh_pillar2 == pillar_override def test_remote_pillar_timeout(temp_salt_minion, tmp_path):