Repository: saltstack/salt
Pull request: Fix Jinja template cache loading
### What does this PR do?
This PR fixes an issue where Jinja templates can be loaded from the cache despite being removed from the Salt filesystem. See issue for more details.

### What issues does this PR fix or reference?
Fixes: #62042 

### Previous Behavior
Salt would load cached Jinja templates if the files were removed from the Salt filesystem.

### New Behavior
If a file is cached but no longer present on the Salt filesystem, it will not be loaded.

### Merge requirements satisfied?
**[NOTICE] Bug fixes or features added to Salt require tests.**
<!-- Please review the [test documentation](https://docs.saltproject.io/en/master/topics/tutorials/writing_tests.html) for details on how to implement tests into Salt's test suite. -->
- [x] Docs
- [x] Changelog - https://docs.saltproject.io/en/master/topics/development/changelog.html
- [x] Tests written/updated

### Commits signed with GPG?
Yes

Please review [Salt's Contributing Guide](https://docs.saltproject.io/en/master/topics/development/contributing.html) for best practices.

See GitHub's [page on GPG signing](https://help.github.com/articles/signing-commits-using-gpg/) for more information about signing commits with GPG.


Problem statement:
[BUG] removed Jinja templates can still be loaded from cache
**Description**
Found a bug where an included Jinja template can still be loaded from the cache despite being removed from the Salt filesystem.

**Setup**

Create two test files on the Salt filesystem:

```yaml
# /srv/salt/jinja_cache.sls
{% from "jinja_cache.jinja" import myvar -%}

test:
  test.show_notification:
    - text: {{ myvar }}
```

```yaml
# /srv/salt/jinja_cache.jinja
{% set myvar = "hello" -%}
```
  
**Steps to Reproduce the behavior**

Applying the state file as-is produces the expected "hello" notification:

```bash
# salt-call state.apply jinja_cache
local:
----------
          ID: test
    Function: test.show_notification
      Result: True
     Comment: hello
     Started: 21:19:26.078569
    Duration: 0.329 ms
     Changes:   

Summary for local
------------
Succeeded: 1
Failed:    0
------------
Total states run:     1
Total run time:   0.329 ms
```

...and changing the variable text like so:

```yaml
{% set myvar = "world" -%}
```

...results in a changed notification text to match "world" from the variable as expected:

```bash
# salt-call state.apply jinja_cache
local:
----------
          ID: test
    Function: test.show_notification
      Result: True
     Comment: world
     Started: 21:19:37.647817
    Duration: 0.894 ms
     Changes:   

Summary for local
------------
Succeeded: 1
Failed:    0
------------
Total states run:     1
Total run time:   0.894 ms
```

However, removing the included jinja file _after it has already been cached_ can result in the file being loaded from cache despite no longer being present on the Salt filesystem:

```bash
# mv jinja_cache.jinja{,.bak}
```

```bash
# salt-call state.apply jinja_cache
local:
----------
          ID: test
    Function: test.show_notification
      Result: True
     Comment: world
     Started: 21:22:55.004962
    Duration: 0.386 ms
     Changes:   

Summary for local
------------
Succeeded: 1
Failed:    0
------------
Total states run:     1
Total run time:   0.386 ms
```

**Expected behavior**
After the Jinja file is removed from the Salt filesystem, the state should failed to run with a `TemplateNotFound` error. Loading cached files can result in user surprise when complex map loading decisions are made.

**Versions Report**
<details><summary>salt --versions-report</summary>
(Provided by running salt --versions-report. Please also mention any differences in master/minion versions.) 

```yaml
Salt Version:
          Salt: 3004.1
 
Dependency Versions:
          cffi: Not Installed
      cherrypy: Not Installed
      dateutil: 2.7.3
     docker-py: Not Installed
         gitdb: 2.0.5
     gitpython: 2.1.11
        Jinja2: 2.10
       libgit2: Not Installed
      M2Crypto: Not Installed
          Mako: Not Installed
       msgpack: 0.5.6
  msgpack-pure: Not Installed
  mysql-python: Not Installed
     pycparser: Not Installed
      pycrypto: 2.6.1
  pycryptodome: 3.6.1
        pygit2: Not Installed
        Python: 3.7.3 (default, Jan 22 2021, 20:04:44)
  python-gnupg: Not Installed
        PyYAML: 3.13
         PyZMQ: 17.1.2
         smmap: 2.0.5
       timelib: Not Installed
       Tornado: 4.5.3
           ZMQ: 4.3.1
 
System Versions:
          dist: debian 10 buster
        locale: UTF-8
       machine: x86_64
       release: 4.19.0-20-amd64
        system: Linux
       version: Debian GNU/Linux 10 buster
```
</details>

Hints from issue discussion:

Commit message: fix jinja tests for opts logic

Patch to review:
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..ba279c0f764b 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,15 +119,17 @@ 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)
+        fcl = self.file_client()
+        return fcl.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):
         """
@@ -165,6 +167,9 @@ def get_source(self, environment, template):
                     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 :]
 
         self.check_cache(_template)
 
@@ -181,25 +186,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()
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


Code review comments:
diff_hunk: @@ -165,6 +167,9 @@ def get_source(self, environment, template):
                     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 :]
path: salt/utils/jinja.py
text: @user1:
**suggestion** this does not appear to get any test coverage - adding `assert False` before/after `_template = _template[...]` doesn't cause a test failure.

I *think* it also makes sense to just use `_template = os.path.basename(_template)`? Though if there are `/` in the `_template` then that wouldn't be accurate I think? It might make more sense to do `_template = os.path.relpath(_template, base_path)`

@author:
this code path gets hit in `tests/pytests/unit/utils/jinja/test_get_template.py::test_relative_include`