Fix Jinja template cache loading #62043

nicholasmhughes · 2022-05-09T21:43:50Z

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.

Docs
Changelog - https://docs.saltproject.io/en/master/topics/development/changelog.html
Tests written/updated

Commits signed with GPG?

Yes

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

nicholasmhughes · 2022-05-11T20:32:03Z

re-run pr-amazon-2-x86_64-py3-pytest

nicholasmhughes · 2022-05-11T20:32:39Z

re-run pr-fedora-35-x86_64-py3-pytest

nicholasmhughes · 2022-05-11T20:32:46Z

re-run pr-centos-7-x86_64-py3-pytest

nicholasmhughes · 2022-05-11T22:01:37Z

re-run pr-freebsd-130-amd64-py3-pytest

nicholasmhughes · 2022-05-11T22:02:13Z

re-run pr-macosx-catalina-x86_64-py3-pytest

salt/utils/jinja.py

waynew · 2022-05-12T22:33:23Z

Well. That's fun. Looks like it's still failing for both freebsd & mac

nicholasmhughes · 2022-05-13T11:18:57Z

re-run pr-amazon-2-x86_64-py3-pytest

nicholasmhughes · 2022-05-14T11:39:13Z

@waynew I had to build a FreeBSD box... but I was able to clear those last two checks.

Ch3LL · 2022-05-18T19:58:55Z

@waynew can you re-review please

waynew

👍 Pretty sure that I poked at this locally last time I reviewed. LGTM

myii · 2022-05-24T14:05:11Z

Unfortunately, this PR appears to have resulted in a regression when working with Jinja relative imports, at least across all Linux platforms:

https://gitlab.com/myii/salt-image-builder/-/pipelines/546881711

local:
    Data failed to compile:
----------
    Rendering SLS 'base:test_jinja.grains' failed: Jinja error: ./grains.jinja
Traceback (most recent call last):
  File "/usr/local/lib/python3.7/dist-packages/salt/utils/templates.py", line 469, in render_jinja_tmpl
    output = template.render(**decoded_context)
  File "/usr/local/lib/python3.7/dist-packages/jinja2/environment.py", line 1301, in render
    self.environment.handle_exception()
  File "/usr/local/lib/python3.7/dist-packages/jinja2/environment.py", line 936, in handle_exception
    raise rewrite_traceback_stack(source=source)
  File "<template>", line 1, in top-level template code
  File "/usr/local/lib/python3.7/dist-packages/salt/utils/jinja.py", line 219, in get_source
    raise TemplateNotFound(template)
jinja2.exceptions.TemplateNotFound: ./grains.jinja
; line 1
---
{%- from "./grains.jinja" import grains with context %}    <======================
test_jinja/grains/cmd.run:
  cmd.run:
    - name: pwd
[...]

I'm a bit short of time at the moment but I'll try to debug further when I get a chance.

Ch3LL · 2022-05-24T14:07:20Z

@nicholasmhughes can you investigate ^

myii · 2022-05-24T14:20:19Z

OK, this seems to confirm that the regression has been introduced by the merge of this PR.

I've rerun the pipeline without this PR's commits and all passing fine again (all four states are using Jinja relative imports):

https://gitlab.com/myii/salt-image-builder/-/pipelines/546923757

local:
----------
          ID: test_jinja/grains/cmd.run
    Function: cmd.run
        Name: pwd
      Result: True
     Comment: Command "pwd" run
     Started: 14:14:49.481086
    Duration: 6.682 ms
     Changes:   
              ----------
              pid:
                  145
              retcode:
                  0
              stderr:
              stdout:
                  /root
Summary for local
------------
Succeeded: 1 (changed=1)
Failed:    0
------------
Total states run:     1
Total run time:   6.682 ms
local:
----------
          ID: test_jinja/opts/cmd.run
    Function: cmd.run
        Name: pwd
      Result: True
     Comment: Command "pwd" run
     Started: 14:14:51.407127
    Duration: 6.81 ms
     Changes:   
              ----------
              pid:
                  175
              retcode:
                  0
              stderr:
              stdout:
                  /root
Summary for local
------------
Succeeded: 1 (changed=1)
Failed:    0
------------
Total states run:     1
Total run time:   6.810 ms
local:
----------
          ID: test_jinja/pillar/cmd.run
    Function: cmd.run
        Name: pwd
      Result: True
     Comment: Command "pwd" run
     Started: 14:14:53.373170
    Duration: 6.791 ms
     Changes:   
              ----------
              pid:
                  205
              retcode:
                  0
              stderr:
              stdout:
                  /root
Summary for local
------------
Succeeded: 1 (changed=1)
Failed:    0
------------
Total states run:     1
Total run time:   6.791 ms
local:
----------
          ID: test_jinja/salt/cmd.run
    Function: cmd.run
        Name: pwd
      Result: True
     Comment: Command "pwd" run
     Started: 14:14:55.344258
    Duration: 6.793 ms
     Changes:   
              ----------
              pid:
                  235
              retcode:
                  0
              stderr:
              stdout:
                  /root
Summary for local
------------
Succeeded: 1 (changed=1)
Failed:    0
------------
Total states run:     1
Total run time:   6.793 ms

myii · 2022-05-24T16:55:31Z

The following bisect should help (b3dacf1):

b3dacf1188a6507dc01efa6ab7d480a82c11f6b6 is the first bad commit
commit b3dacf1188a6507dc01efa6ab7d480a82c11f6b6
...
...

    handle local paths properly

 salt/utils/jinja.py | 3 +++
 1 file changed, 3 insertions(+)

I.e.

salt/salt/utils/jinja.py

Lines 178 to 180 in 4e91f38

    
           # local file clients should pass the dot-expanded relative path 
        
           if environment.globals.get("opts", {}).get("file_client") == "local": 
        
               _template = os.path.relpath(_template, base_path)

nicholasmhughes · 2022-05-27T21:20:08Z

@myii , I was able to reproduce in an operational environment. I installed Salt from the master branch today on a fresh Debian 10 machine using the bootstrap script.

curl -L https://bootstrap.saltproject.io | sudo sh -s -- -M git master

Versions Report:

Salt Version:
          Salt: 3005+0na.b18281e
 
Dependency Versions:
          cffi: Not Installed
      cherrypy: Not Installed
      dateutil: Not Installed
     docker-py: Not Installed
         gitdb: Not Installed
     gitpython: Not Installed
        Jinja2: 3.1.2
       libgit2: Not Installed
      M2Crypto: Not Installed
          Mako: Not Installed
       msgpack: 1.0.3
  msgpack-pure: Not Installed
  mysql-python: Not Installed
     pycparser: Not Installed
      pycrypto: 2.6.1
  pycryptodome: 3.14.1
        pygit2: Not Installed
        Python: 3.7.3 (default, Jan 22 2021, 20:04:44)
  python-gnupg: Not Installed
        PyYAML: 6.0
         PyZMQ: 22.3.0
         smmap: Not Installed
       timelib: Not Installed
       Tornado: 4.5.3
           ZMQ: 4.3.4
 
System Versions:
          dist: debian 10 buster
        locale: UTF-8
       machine: x86_64
       release: 4.19.0-18-amd64
        system: Linux
       version: Debian GNU/Linux 10 buster

And then pulled down the test files the pipeline is running onto the master:

root@d10-saltgit-01:/srv# pwd
/srv

root@d10-saltgit-01:/srv# git clone https://gitlab.com/saltstack-formulas/infrastructure/salt-image-builder.git
Cloning into 'salt-image-builder'...
remote: Enumerating objects: 4012, done.
remote: Counting objects: 100% (714/714), done.
remote: Compressing objects: 100% (162/162), done.
remote: Total 4012 (delta 579), reused 662 (delta 544), pack-reused 3298
Receiving objects: 100% (4012/4012), 2.48 MiB | 6.01 MiB/s, done.
Resolving deltas: 100% (3144/3144), done.

root@d10-saltgit-01:/srv# mkdir salt

root@d10-saltgit-01:/srv# cp -r salt-image-builder/test/test_jinja /srv/salt/

root@d10-saltgit-01:/srv# ll salt/test_jinja
total 48
-rw-r--r-- 1 root root  45 May 27 20:37 grains.jinja
-rw-r--r-- 1 root root 111 May 27 20:37 grains.sls
-rw-r--r-- 1 root root  41 May 27 20:37 grains.yaml
-rw-r--r-- 1 root root  41 May 27 20:37 opts.jinja
-rw-r--r-- 1 root root 105 May 27 20:37 opts.sls
-rw-r--r-- 1 root root  40 May 27 20:37 opts.yaml
-rw-r--r-- 1 root root  45 May 27 20:37 pillar.jinja
-rw-r--r-- 1 root root 111 May 27 20:37 pillar.sls
-rw-r--r-- 1 root root  45 May 27 20:37 pillar.yaml
-rw-r--r-- 1 root root  41 May 27 20:37 salt.jinja
-rw-r--r-- 1 root root 105 May 27 20:37 salt.sls
-rw-r--r-- 1 root root  49 May 27 20:37 salt.yaml

"Normal" runs work, but local doesn't:

root@d10-saltgit-01:/srv# salt-call state.apply test_jinja.grains
local:
----------
          ID: test_jinja/grains/cmd.run
    Function: cmd.run
        Name: pwd
      Result: True
     Comment: Command "pwd" run
     Started: 21:13:51.801142
    Duration: 3.235 ms
     Changes:   
              ----------
              pid:
                  15193
              retcode:
                  0
              stderr:
              stdout:
                  /root

Summary for local
------------
Succeeded: 1 (changed=1)
Failed:    0
------------
Total states run:     1
Total run time:   3.235 ms

root@d10-saltgit-01:/srv# salt-call --local state.apply test_jinja.grains
[ERROR   ] Rendering exception occurred
Traceback (most recent call last):
  File "/usr/local/lib/python3.7/dist-packages/salt/utils/templates.py", line 469, in render_jinja_tmpl
    output = template.render(**decoded_context)
  File "/usr/local/lib/python3.7/dist-packages/jinja2/environment.py", line 1301, in render
    self.environment.handle_exception()
  File "/usr/local/lib/python3.7/dist-packages/jinja2/environment.py", line 936, in handle_exception
    raise rewrite_traceback_stack(source=source)
  File "<template>", line 1, in top-level template code
  File "/usr/local/lib/python3.7/dist-packages/salt/utils/jinja.py", line 219, in get_source
    raise TemplateNotFound(template)
jinja2.exceptions.TemplateNotFound: ./grains.jinja

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/usr/local/lib/python3.7/dist-packages/salt/utils/templates.py", line 216, in render_tmpl
    output = render_str(tmplstr, context, tmplpath)
  File "/usr/local/lib/python3.7/dist-packages/salt/utils/templates.py", line 514, in render_jinja_tmpl
    "Jinja error: {}{}".format(exc, out), line, tmplstr, trace=tracestr
salt.exceptions.SaltRenderError: Jinja error: ./grains.jinja
Traceback (most recent call last):
  File "/usr/local/lib/python3.7/dist-packages/salt/utils/templates.py", line 469, in render_jinja_tmpl
    output = template.render(**decoded_context)
  File "/usr/local/lib/python3.7/dist-packages/jinja2/environment.py", line 1301, in render
    self.environment.handle_exception()
  File "/usr/local/lib/python3.7/dist-packages/jinja2/environment.py", line 936, in handle_exception
    raise rewrite_traceback_stack(source=source)
  File "<template>", line 1, in top-level template code
  File "/usr/local/lib/python3.7/dist-packages/salt/utils/jinja.py", line 219, in get_source
    raise TemplateNotFound(template)
jinja2.exceptions.TemplateNotFound: ./grains.jinja

; line 1

---
{%- from "./grains.jinja" import grains with context %}    <======================

test_jinja/grains/cmd.run:
  cmd.run:
    - name: pwd

[...]
---
[CRITICAL] Rendering SLS 'base:test_jinja.grains' failed: Jinja error: ./grains.jinja
Traceback (most recent call last):
  File "/usr/local/lib/python3.7/dist-packages/salt/utils/templates.py", line 469, in render_jinja_tmpl
    output = template.render(**decoded_context)
  File "/usr/local/lib/python3.7/dist-packages/jinja2/environment.py", line 1301, in render
    self.environment.handle_exception()
  File "/usr/local/lib/python3.7/dist-packages/jinja2/environment.py", line 936, in handle_exception
    raise rewrite_traceback_stack(source=source)
  File "<template>", line 1, in top-level template code
  File "/usr/local/lib/python3.7/dist-packages/salt/utils/jinja.py", line 219, in get_source
    raise TemplateNotFound(template)
jinja2.exceptions.TemplateNotFound: ./grains.jinja

; line 1

---
{%- from "./grains.jinja" import grains with context %}    <======================

test_jinja/grains/cmd.run:
  cmd.run:
    - name: pwd

[...]
---
local:
    Data failed to compile:
----------
    Rendering SLS 'base:test_jinja.grains' failed: Jinja error: ./grains.jinja
Traceback (most recent call last):
  File "/usr/local/lib/python3.7/dist-packages/salt/utils/templates.py", line 469, in render_jinja_tmpl
    output = template.render(**decoded_context)
  File "/usr/local/lib/python3.7/dist-packages/jinja2/environment.py", line 1301, in render
    self.environment.handle_exception()
  File "/usr/local/lib/python3.7/dist-packages/jinja2/environment.py", line 936, in handle_exception
    raise rewrite_traceback_stack(source=source)
  File "<template>", line 1, in top-level template code
  File "/usr/local/lib/python3.7/dist-packages/salt/utils/jinja.py", line 219, in get_source
    raise TemplateNotFound(template)
jinja2.exceptions.TemplateNotFound: ./grains.jinja

; line 1

---
{%- from "./grains.jinja" import grains with context %}    <======================

test_jinja/grains/cmd.run:
  cmd.run:
    - name: pwd

[...]
---

The lines in question were actually added to satisfy an existing test... which seems to have absolute paths in local mode. Changing that logic to check for an absolute path seems to solve the problem for both cases. I'll put in a PR presently.

This makes it detect cases where remote/local paths gets confused (like in saltstack#60003 or saltstack#62043). Without using a container, remote paths works locally too, so tests do not detect an issue. Rename test_ssh_setup.py to test_ssh_setup_and_state.py to match its new content. pytest.mark.skip_on_windows is dropped as test_ssh_setup.py already has necessary mark (dockerd present).

fixes saltstack#62042 jinja template cache loading

04da627

nicholasmhughes requested a review from a team as a code owner May 9, 2022 21:43

nicholasmhughes requested review from dwoz and removed request for a team May 9, 2022 21:43

nicholasmhughes added 3 commits May 11, 2022 10:29

handle local paths properly

82b1857

prevent reuse of fileclient if options have changed

62bf576

fix jinja tests for opts logic

f52ad78

nicholasmhughes requested a review from waynew May 11, 2022 19:08

waynew suggested changes May 12, 2022

View reviewed changes

salt/utils/jinja.py Outdated Show resolved Hide resolved

use os.path.relpath for path comparison

c856dfe

nicholasmhughes requested a review from waynew May 12, 2022 19:43

protect against attributeerror

87218ff

prevent reuse of fileclient if file_roots have changed

e32bd08

nicholasmhughes added the Phosphorus v3005.0 Release code name and version label May 14, 2022

Merge branch 'master' into fix-jinja-include-cache

83e1007

saltstack deleted a comment from github-actions bot May 23, 2022

waynew approved these changes May 23, 2022

View reviewed changes

garethgreenaway merged commit 4e91f38 into saltstack:master May 24, 2022

nicholasmhughes deleted the fix-jinja-include-cache branch May 24, 2022 02:30

This was referenced May 27, 2022

[BUG] relative jinja includes broken in local mode #62117

Closed

Fix relative jinja includes broken in local mode #62118

Merged

sabedevops mentioned this pull request Sep 1, 2022

[BUG] Including Jinja template from absolute path broken #62611

Closed

6 tasks

This was referenced Sep 7, 2022

[BUG] salt-ssh support for extra_filerefs in Saltfile is broken #60003

Closed

Prevent reuse of file client if cachedir does not match #62631

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Jinja template cache loading #62043

Fix Jinja template cache loading #62043

nicholasmhughes commented May 9, 2022

nicholasmhughes commented May 11, 2022

nicholasmhughes commented May 11, 2022

nicholasmhughes commented May 11, 2022

nicholasmhughes commented May 11, 2022

nicholasmhughes commented May 11, 2022

waynew commented May 12, 2022

nicholasmhughes commented May 13, 2022

nicholasmhughes commented May 14, 2022

Ch3LL commented May 18, 2022

waynew left a comment

myii commented May 24, 2022

Ch3LL commented May 24, 2022

myii commented May 24, 2022

myii commented May 24, 2022 •

edited

Loading

nicholasmhughes commented May 27, 2022

Fix Jinja template cache loading #62043

Fix Jinja template cache loading #62043

Conversation

nicholasmhughes commented May 9, 2022

What does this PR do?

What issues does this PR fix or reference?

Previous Behavior

New Behavior

Merge requirements satisfied?

Commits signed with GPG?

nicholasmhughes commented May 11, 2022

nicholasmhughes commented May 11, 2022

nicholasmhughes commented May 11, 2022

nicholasmhughes commented May 11, 2022

nicholasmhughes commented May 11, 2022

waynew commented May 12, 2022

nicholasmhughes commented May 13, 2022

nicholasmhughes commented May 14, 2022

Ch3LL commented May 18, 2022

waynew left a comment

Choose a reason for hiding this comment

myii commented May 24, 2022

Ch3LL commented May 24, 2022

myii commented May 24, 2022

myii commented May 24, 2022 • edited Loading

nicholasmhughes commented May 27, 2022

myii commented May 24, 2022 •

edited

Loading