From 04da627fcbd8f60b40fad09ad7365da6c5be36bc Mon Sep 17 00:00:00 2001 From: nicholasmhughes Date: Mon, 9 May 2022 17:39:39 -0400 Subject: [PATCH 1/7] fixes saltstack/salt#62042 jinja template cache loading --- changelog/62042.fixed | 1 + salt/utils/jinja.py | 46 ++++++++-------- tests/pytests/functional/utils/test_jinja.py | 56 ++++++++++++++++++++ 3 files changed, 81 insertions(+), 22 deletions(-) create mode 100644 changelog/62042.fixed create mode 100644 tests/pytests/functional/utils/test_jinja.py diff --git a/changelog/62042.fixed b/changelog/62042.fixed new file mode 100644 index 000000000000..6107828cbe7a --- /dev/null +++ b/changelog/62042.fixed @@ -0,0 +1 @@ +Fix cache checking for Jinja templates diff --git a/salt/utils/jinja.py b/salt/utils/jinja.py index aa8ebe90546c..f34f03a8fbba 100644 --- a/salt/utils/jinja.py +++ b/salt/utils/jinja.py @@ -119,15 +119,16 @@ def cache_file(self, template): Cache a file from the salt master """ saltpath = salt.utils.url.create(template) - self.file_client().get_file(saltpath, "", True, self.saltenv) + return self.file_client().get_file(saltpath, "", True, self.saltenv) def check_cache(self, template): """ Cache a file only once """ if template not in self.cached: - self.cache_file(template) - self.cached.append(template) + ret = self.cache_file(template) + if ret is not False: + self.cached.append(template) def get_source(self, environment, template): """ @@ -181,25 +182,26 @@ def get_source(self, environment, template): } environment.globals.update(tpldata) - # pylint: disable=cell-var-from-loop - for spath in self.searchpath: - filepath = os.path.join(spath, _template) - try: - with salt.utils.files.fopen(filepath, "rb") as ifile: - contents = ifile.read().decode(self.encoding) - mtime = os.path.getmtime(filepath) - - def uptodate(): - try: - return os.path.getmtime(filepath) == mtime - except OSError: - return False - - return contents, filepath, uptodate - except OSError: - # there is no file under current path - continue - # pylint: enable=cell-var-from-loop + if _template in self.cached: + # pylint: disable=cell-var-from-loop + for spath in self.searchpath: + filepath = os.path.join(spath, _template) + try: + with salt.utils.files.fopen(filepath, "rb") as ifile: + contents = ifile.read().decode(self.encoding) + mtime = os.path.getmtime(filepath) + + def uptodate(): + try: + return os.path.getmtime(filepath) == mtime + except OSError: + return False + + return contents, filepath, uptodate + except OSError: + # there is no file under current path + continue + # pylint: enable=cell-var-from-loop # there is no template file within searchpaths raise TemplateNotFound(template) diff --git a/tests/pytests/functional/utils/test_jinja.py b/tests/pytests/functional/utils/test_jinja.py new file mode 100644 index 000000000000..e79f6e8ae37c --- /dev/null +++ b/tests/pytests/functional/utils/test_jinja.py @@ -0,0 +1,56 @@ +import pytest +import salt.utils.jinja +from jinja2.exceptions import TemplateNotFound +from salt.utils.odict import OrderedDict + + +def test_utils_jinja_cache_removed_file_from_root(temp_salt_minion, tmp_path): + """ + this tests for a condition where an included jinja template + is removed from the salt filesystem, but is still loaded from + the cache. + """ + opts = temp_salt_minion.config.copy() + file_root = tmp_path / "root" + file_root.mkdir(parents=True, exist_ok=True) + cache_root = tmp_path / "cache" + cache_root.mkdir(parents=True, exist_ok=True) + filename = "jinja_cache" + sls_file = file_root / "{}.sls".format(filename) + jinja_file = file_root / "{}.jinja".format(filename) + sls_file.write_text("{% include '" + filename + ".jinja' %}") + jinja_file.write_text("{% set this = 'that' %}") + + # Stop using OrderedDict once we drop Py3.5 support + opts["file_roots"] = OrderedDict() + opts["file_roots"]["base"] = [str(file_root)] + opts["cachedir"] = str(cache_root) + opts["master_type"] = "disable" + opts["file_client"] = "local" + + loader = salt.utils.jinja.SaltCacheLoader( + opts, + "base", + ) + # sls file is in the root + loader.get_source(None, sls_file.name) + # jinja file is in the root + loader.get_source(None, jinja_file.name) + # both files cached + assert len(loader.cached) == 2 + + # remove the jinja file and reset loader + jinja_file.unlink() + loader = salt.utils.jinja.SaltCacheLoader( + opts, + "base", + ) + # sls file is still in the root + loader.get_source(None, sls_file.name) + # jinja file is gone from the root + with pytest.raises(TemplateNotFound): + loader.get_source(None, jinja_file.name) + # only one was cached this run + assert len(loader.cached) == 1 + # the cached jinja file is still present, but not used + assert (cache_root / "files" / "base" / jinja_file.name).exists() From 82b185707d9d218f7bf452ea53c4e8fb7676e01b Mon Sep 17 00:00:00 2001 From: nicholasmhughes Date: Wed, 11 May 2022 10:29:31 -0400 Subject: [PATCH 2/7] handle local paths properly --- salt/utils/jinja.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/salt/utils/jinja.py b/salt/utils/jinja.py index f34f03a8fbba..72244f825e5d 100644 --- a/salt/utils/jinja.py +++ b/salt/utils/jinja.py @@ -166,6 +166,9 @@ def get_source(self, environment, template): template, ) raise TemplateNotFound(template) + # local file clients should pass the dot-expanded relative path + if environment.globals["opts"]["file_client"] == "local": + _template = _template[len(base_path) + 1 :] self.check_cache(_template) From 62bf576d69438bf0c145e1ef6ff2ed70c364ddbd Mon Sep 17 00:00:00 2001 From: nicholasmhughes Date: Wed, 11 May 2022 13:04:21 -0400 Subject: [PATCH 3/7] prevent reuse of fileclient if options have changed --- salt/utils/jinja.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/salt/utils/jinja.py b/salt/utils/jinja.py index 72244f825e5d..7e65cbf926d1 100644 --- a/salt/utils/jinja.py +++ b/salt/utils/jinja.py @@ -103,10 +103,10 @@ def file_client(self): # If there was no file_client passed to the class, create a cache_client # and use that. This avoids opening a new file_client every time this # class is instantiated - if self._file_client is None: + if self._file_client is None or self._file_client.opts != self.opts: attr = "_cached_pillar_client" if self.pillar_rend else "_cached_client" cached_client = getattr(self, attr, None) - if cached_client is None: + if cached_client is None or cached_client.opts != self.opts: cached_client = salt.fileclient.get_file_client( self.opts, self.pillar_rend ) @@ -119,7 +119,8 @@ def cache_file(self, template): Cache a file from the salt master """ saltpath = salt.utils.url.create(template) - return self.file_client().get_file(saltpath, "", True, self.saltenv) + fcl = self.file_client() + return fcl.get_file(saltpath, "", True, self.saltenv) def check_cache(self, template): """ From f52ad78342eac700392d8c9cce75bd1d8f772821 Mon Sep 17 00:00:00 2001 From: nicholasmhughes Date: Wed, 11 May 2022 14:20:11 -0400 Subject: [PATCH 4/7] fix jinja tests for opts logic --- salt/utils/jinja.py | 2 +- tests/pytests/unit/utils/jinja/conftest.py | 1 + tests/pytests/unit/utils/jinja/test_salt_cache_loader.py | 3 +++ 3 files changed, 5 insertions(+), 1 deletion(-) diff --git a/salt/utils/jinja.py b/salt/utils/jinja.py index 7e65cbf926d1..ba279c0f764b 100644 --- a/salt/utils/jinja.py +++ b/salt/utils/jinja.py @@ -168,7 +168,7 @@ def get_source(self, environment, template): ) raise TemplateNotFound(template) # local file clients should pass the dot-expanded relative path - if environment.globals["opts"]["file_client"] == "local": + if environment.globals.get("opts", {}).get("file_client") == "local": _template = _template[len(base_path) + 1 :] self.check_cache(_template) diff --git a/tests/pytests/unit/utils/jinja/conftest.py b/tests/pytests/unit/utils/jinja/conftest.py index b334daded17a..0b77a4fb2f73 100644 --- a/tests/pytests/unit/utils/jinja/conftest.py +++ b/tests/pytests/unit/utils/jinja/conftest.py @@ -13,6 +13,7 @@ def __init__(self, loader=None): if loader: loader._file_client = self self.requests = [] + self.opts = {} def get_file(self, template, dest="", makedirs=False, saltenv="base"): self.requests.append( diff --git a/tests/pytests/unit/utils/jinja/test_salt_cache_loader.py b/tests/pytests/unit/utils/jinja/test_salt_cache_loader.py index d7d989368294..505cc25f6381 100644 --- a/tests/pytests/unit/utils/jinja/test_salt_cache_loader.py +++ b/tests/pytests/unit/utils/jinja/test_salt_cache_loader.py @@ -110,6 +110,7 @@ def run_command(opts=None, saltenv="base", **kwargs): """ if opts is None: opts = minion_opts + mock_file_client.opts = opts loader = SaltCacheLoader(opts, saltenv, _file_client=mock_file_client) # Create a mock file client and attach it to the loader return loader @@ -221,6 +222,7 @@ def test_file_client_kwarg(minion_opts, mock_file_client): A file client can be passed to SaltCacheLoader overriding the any cached file client """ + mock_file_client.opts = minion_opts loader = SaltCacheLoader(minion_opts, _file_client=mock_file_client) assert loader._file_client is mock_file_client @@ -231,6 +233,7 @@ def test_cache_loader_shutdown(minion_opts, mock_file_client): file_client does not have a destroy method """ assert not hasattr(mock_file_client, "destroy") + mock_file_client.opts = minion_opts loader = SaltCacheLoader(minion_opts, _file_client=mock_file_client) assert loader._file_client is mock_file_client # Shutdown method should not raise any exceptions From c856dfe50f5286601c7c9ad9b49919004e5adfee Mon Sep 17 00:00:00 2001 From: nicholasmhughes Date: Thu, 12 May 2022 14:09:46 -0400 Subject: [PATCH 5/7] use os.path.relpath for path comparison --- salt/utils/jinja.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/salt/utils/jinja.py b/salt/utils/jinja.py index ba279c0f764b..c82e986494d9 100644 --- a/salt/utils/jinja.py +++ b/salt/utils/jinja.py @@ -169,7 +169,7 @@ def get_source(self, environment, template): raise TemplateNotFound(template) # local file clients should pass the dot-expanded relative path if environment.globals.get("opts", {}).get("file_client") == "local": - _template = _template[len(base_path) + 1 :] + _template = os.path.relpath(_template, base_path) self.check_cache(_template) From 87218ff8daf30fdfc4284dfdc3e595faa829466f Mon Sep 17 00:00:00 2001 From: nicholasmhughes Date: Thu, 12 May 2022 19:04:08 -0400 Subject: [PATCH 6/7] protect against attributeerror --- salt/utils/jinja.py | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/salt/utils/jinja.py b/salt/utils/jinja.py index c82e986494d9..ba5e3e15e33d 100644 --- a/salt/utils/jinja.py +++ b/salt/utils/jinja.py @@ -103,10 +103,18 @@ def file_client(self): # If there was no file_client passed to the class, create a cache_client # and use that. This avoids opening a new file_client every time this # class is instantiated - if self._file_client is None or self._file_client.opts != self.opts: + if ( + self._file_client is None + or not hasattr(self._file_client, "opts") + or self._file_client.opts != self.opts + ): attr = "_cached_pillar_client" if self.pillar_rend else "_cached_client" cached_client = getattr(self, attr, None) - if cached_client is None or cached_client.opts != self.opts: + if ( + cached_client is None + or not hasattr(cached_client, "opts") + or cached_client.opts != self.opts + ): cached_client = salt.fileclient.get_file_client( self.opts, self.pillar_rend ) From e32bd08cd50130c7d401e912eb8138995fe812a7 Mon Sep 17 00:00:00 2001 From: nicholasmhughes Date: Fri, 13 May 2022 16:55:00 -0400 Subject: [PATCH 7/7] prevent reuse of fileclient if file_roots have changed --- salt/utils/jinja.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/salt/utils/jinja.py b/salt/utils/jinja.py index ba5e3e15e33d..627697f5feed 100644 --- a/salt/utils/jinja.py +++ b/salt/utils/jinja.py @@ -106,14 +106,14 @@ def file_client(self): if ( self._file_client is None or not hasattr(self._file_client, "opts") - or self._file_client.opts != self.opts + or self._file_client.opts["file_roots"] != self.opts["file_roots"] ): attr = "_cached_pillar_client" if self.pillar_rend else "_cached_client" cached_client = getattr(self, attr, None) if ( cached_client is None or not hasattr(cached_client, "opts") - or cached_client.opts != self.opts + or cached_client.opts["file_roots"] != self.opts["file_roots"] ): cached_client = salt.fileclient.get_file_client( self.opts, self.pillar_rend