templates: move the globals up to the Environment (Jinja2 3.0.0) #60811

aplanas · 2021-08-30T14:49:55Z

What does this PR do?

When creating a Jinja2 environment, we populate the globals in the
Template object that we generate from the environment. This cause a
problem when there is a {% include "./file.sls" %} in the template, as
cannot find in the environment globals information like the "tpldir",
for example, making the relative path to be unresolved.

Seems that in Jinja2 2.X this behaviour is not present, so attaching the
globals to the Template will make the include to work, but since Jinja2
3.0.0 this is not the case. Maybe related with the re-architecture from
pallets/jinja#295

This patch populate the globals in the Environment level, making this
and other variables reachable by the Jinja templates.

Also fix a wrong call to TemplateNotFound

What issues does this PR fix or reference?

Fixes: #55159

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

The TemplateNotFound exception requires a parameter, name, that is missing in one of the calls. File "/usr/lib/python3.8/site-packages/salt/utils/jinja.py", line 158, in get_source raise TemplateNotFound TypeError: __init__() missing 1 required positional argument: 'name' This patch add the missing parameter in the raise call. Signed-off-by: Alberto Planas <[email protected]>

When creating a Jinja2 environment, we populate the globals in the Template object that we generate from the environment. This cause a problem when there is a {% include "./file.sls" %} in the template, as cannot find in the environment globals information like the "tpldir", for example, making the relative path to be unresolved. Seems that in Jinja2 2.X this behaviour is not present, so attaching the globals to the Template will make the include to work, but since Jinja2 3.0.0 this is not the case. Maybe related with the re-architecture from pallets/jinja#295 This patch populate the globals in the Environment level, making this and other variables reachable by the Jinja templates. Fix saltstack#55159 Signed-off-by: Alberto Planas <[email protected]>

garethgreenaway · 2021-08-31T21:02:02Z

@aplanas Thanks for the fix, this also resolves another issue that was reported related to the changes that went into Jinja2 3.0.x. We'll want to add some additional tests for this scenario and move these existing tests over to Pytest. If you don't have time to migrate these tests, a member of the Salt core team can help out.

salt/utils/templates.py

baby-gnu · 2021-09-02T13:38:36Z

Is it planned to backport this fix or is there a strong dependency between Jinja2 3.0 and salt?

i.e. will it be required to upgrade salt version when a distribution starts using newer Jinja2?

aplanas · 2021-09-02T13:43:56Z

In my test the patch is safe to backport. I tested master with the patch against Jinja2 2.10.X, 2.11.X and 3.0.1. More tests are welcome (but note that the CI was green, and none is using 3.0.1)

myii · 2021-09-02T14:52:52Z

I've done a before and after comparison using the Formula team's pre-salted images. We've been downgrading Jinja2 until now, so all our instances have been working. There are a few issues with installing on Gentoo with the salt-bootstrap so we'll ignore Gentoo 3000.9 failures below.

Before:

All instances are passing.

After:

The following instances are now failing (where git or stable is shown in the table):

Distro	`3003.2`	`3003.1`	`3002.6`	`3001.7`	`3000.9`
Arch Linux	`git`	`stable`	`git`	`git`
Fedora 33			`git`	`git`
Fedora 34			`git`	`git`
Gentoo (latest)	`git`	`stable`	`git`	`git`
Gentoo (systemd)	`git`	`stable`	`git`	`git`
openSUSE Leap 15.2	`git`			`git`	`git`
openSUSE Leap 15.3	`git`			`git`	`git`
openSUSE Tumbleweed	`git`		`stable`	`git`

Notes:
- Showing git or stable to clarify the Salt installation method that was used with the bootstrap script.
- stable on openSUSE for 3002.X is actually 3002.2.
- I wouldn't be surprised to find that stable for 3003.2 on both Arch Linux and Gentoo will also fail, once they are actually released (not available at the time of writing this comment).
- Also had to downgrade Jinja2 on our FreeBSD Vagrant boxes -- will report back these findings when I get a chance to work with those.

Fix has been released: * saltstack/salt#60811 Maintain Jinja2 downgrades where necessary, as outlined here: * saltstack/salt#60811 (comment)

# [1.50.0](https://gitlab.com/saltstack-formulas/infrastructure/salt-image-builder/compare/v1.49.0...v1.50.0) (2021-09-02) ### Continuous Integration * **gitlab-ci:** deprecate `3000.9` ([3529c9b](https://gitlab.com/saltstack-formulas/infrastructure/salt-image-builder/commit/3529c9b8b8ddd37643d372bc8316599ebaa3d8a4)) ### Features * **dockerfile.*:** remove Jinja2 downgrade workaround where possible ([07a0d03](https://gitlab.com/saltstack-formulas/infrastructure/salt-image-builder/commit/07a0d0393b91b4cc4ac76c137178d7794151a890)), closes [/github.com/saltstack/salt/pull/60811#issuecomment-911772632](https://gitlab.com//github.com/saltstack/salt/pull/60811/issues/issuecomment-911772632)

garethgreenaway · 2021-09-02T18:45:55Z

We're not planning to back port any fixes at this time unless it's considered a CVE issue, which this one is not. This fix will be available in the Silicon release.

damon-atkins · 2021-09-03T02:43:24Z

I assume old OS may not have access to the new Jinja2. Might consider running test case against both versions, to ensure salt works with both? In house code or other software already installed may not support newer version, if salt supplies a new version and it overrides the existing version.

baby-gnu · 2021-09-03T06:28:43Z

We're not planning to back port any fixes at this time unless it's considered a CVE issue, which this one is not. This fix will be available in the Silicon release.

This seems a reasonable policy, I think some special notes should be in the documentation for users to easily spot possible troubles, something like this release of salt does not work with Jinja2 >= 3.0.

Or maybe in the dependencies where the Jinja2 should have < 3.0 mentioned.

Regards.

…ltstack#60811) * jinja: fix TemplateNotFound missing name The TemplateNotFound exception requires a parameter, name, that is missing in one of the calls. File "/usr/lib/python3.8/site-packages/salt/utils/jinja.py", line 158, in get_source raise TemplateNotFound TypeError: __init__() missing 1 required positional argument: 'name' This patch add the missing parameter in the raise call. Signed-off-by: Alberto Planas <[email protected]> * templates: move the globals up to the Environment When creating a Jinja2 environment, we populate the globals in the Template object that we generate from the environment. This cause a problem when there is a {% include "./file.sls" %} in the template, as cannot find in the environment globals information like the "tpldir", for example, making the relative path to be unresolved. Seems that in Jinja2 2.X this behaviour is not present, so attaching the globals to the Template will make the include to work, but since Jinja2 3.0.0 this is not the case. Maybe related with the re-architecture from pallets/jinja#295 This patch populate the globals in the Environment level, making this and other variables reachable by the Jinja templates. Fix saltstack#55159 Signed-off-by: Alberto Planas <[email protected]>

…ltstack#60811) * jinja: fix TemplateNotFound missing name The TemplateNotFound exception requires a parameter, name, that is missing in one of the calls. File "/usr/lib/python3.8/site-packages/salt/utils/jinja.py", line 158, in get_source raise TemplateNotFound TypeError: __init__() missing 1 required positional argument: 'name' This patch add the missing parameter in the raise call. Signed-off-by: Alberto Planas <[email protected]> * templates: move the globals up to the Environment When creating a Jinja2 environment, we populate the globals in the Template object that we generate from the environment. This cause a problem when there is a {% include "./file.sls" %} in the template, as cannot find in the environment globals information like the "tpldir", for example, making the relative path to be unresolved. Seems that in Jinja2 2.X this behaviour is not present, so attaching the globals to the Template will make the include to work, but since Jinja2 3.0.0 this is not the case. Maybe related with the re-architecture from pallets/jinja#295 This patch populate the globals in the Environment level, making this and other variables reachable by the Jinja templates. Fix saltstack#55159 Bug-Debian: https://bugs.debian.org/996271 Origin: upstream, saltstack#60811 Signed-off-by: Alberto Planas <[email protected]>

With default version 3003.3, you run into saltstack/salt#55159, which was fixed by saltstack/salt#60811 If you want to install from git master, it fails because py2-pip is unavailable - Alpine 3.11 was the last version to include py2-pip This PR changes the git master process to python3 and adds all requirements for a successful build

Check for problems associated with this bug: * saltstack/salt#60188 - Affects use of `grains`, `opts`, `pillar` & `salt` in Jinja. Fix was first available in `3004`: * saltstack/salt#60811 Use tests to apply the Jinja2 downgrade only where necessary (and to ensure that it is actually working).

…ltstack#60811) * jinja: fix TemplateNotFound missing name The TemplateNotFound exception requires a parameter, name, that is missing in one of the calls. File "/usr/lib/python3.8/site-packages/salt/utils/jinja.py", line 158, in get_source raise TemplateNotFound TypeError: __init__() missing 1 required positional argument: 'name' This patch add the missing parameter in the raise call. Signed-off-by: Alberto Planas <[email protected]> * templates: move the globals up to the Environment When creating a Jinja2 environment, we populate the globals in the Template object that we generate from the environment. This cause a problem when there is a {% include "./file.sls" %} in the template, as cannot find in the environment globals information like the "tpldir", for example, making the relative path to be unresolved. Seems that in Jinja2 2.X this behaviour is not present, so attaching the globals to the Template will make the include to work, but since Jinja2 3.0.0 this is not the case. Maybe related with the re-architecture from pallets/jinja#295 This patch populate the globals in the Environment level, making this and other variables reachable by the Jinja templates. Fix saltstack#55159 Signed-off-by: Alberto Planas <[email protected]>

aplanas requested a review from a team as a code owner August 30, 2021 14:49

aplanas requested review from waynew and removed request for a team August 30, 2021 14:49

aplanas force-pushed the fix_include branch from e0164ec to 9752457 Compare August 30, 2021 15:23

aplanas added 2 commits August 31, 2021 09:43

aplanas force-pushed the fix_include branch from 9752457 to c53365c Compare August 31, 2021 09:08

garethgreenaway mentioned this pull request Aug 31, 2021

[BUG] Jinja2 3.0 support breaking changes #60188

Closed

garethgreenaway linked an issue Aug 31, 2021 that may be closed by this pull request

[BUG] Jinja2 3.0 support breaking changes #60188

Closed

baby-gnu mentioned this pull request Sep 1, 2021

fix(map): workaround salt bug with Jinja2 3.0 saltstack-formulas/template-formula#239

Closed

19 tasks

garethgreenaway added the Silicon v3004.0 Release code name label Sep 1, 2021

garethgreenaway added this to the Silicon milestone Sep 1, 2021

waynew suggested changes Sep 1, 2021

View reviewed changes

salt/utils/templates.py Show resolved Hide resolved

aplanas requested a review from waynew September 1, 2021 15:01

twangboy approved these changes Sep 1, 2021

View reviewed changes

garethgreenaway approved these changes Sep 1, 2021

View reviewed changes

dwoz approved these changes Sep 1, 2021

View reviewed changes

garethgreenaway merged commit 7275ecf into saltstack:master Sep 1, 2021

Nascire mentioned this pull request Oct 6, 2021

Fix git master install on alpine 3.12+ saltstack/salt-bootstrap#1604

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

templates: move the globals up to the Environment (Jinja2 3.0.0) #60811

templates: move the globals up to the Environment (Jinja2 3.0.0) #60811

aplanas commented Aug 30, 2021

garethgreenaway commented Aug 31, 2021 •

edited

Loading

baby-gnu commented Sep 2, 2021

aplanas commented Sep 2, 2021 •

edited

Loading

myii commented Sep 2, 2021 •

edited

Loading

garethgreenaway commented Sep 2, 2021

damon-atkins commented Sep 3, 2021 •

edited

Loading

baby-gnu commented Sep 3, 2021

templates: move the globals up to the Environment (Jinja2 3.0.0) #60811

templates: move the globals up to the Environment (Jinja2 3.0.0) #60811

Conversation

aplanas commented Aug 30, 2021

What does this PR do?

What issues does this PR fix or reference?

Merge requirements satisfied?

Commits signed with GPG?

garethgreenaway commented Aug 31, 2021 • edited Loading

baby-gnu commented Sep 2, 2021

aplanas commented Sep 2, 2021 • edited Loading

myii commented Sep 2, 2021 • edited Loading

garethgreenaway commented Sep 2, 2021

damon-atkins commented Sep 3, 2021 • edited Loading

baby-gnu commented Sep 3, 2021

garethgreenaway commented Aug 31, 2021 •

edited

Loading

aplanas commented Sep 2, 2021 •

edited

Loading

myii commented Sep 2, 2021 •

edited

Loading

damon-atkins commented Sep 3, 2021 •

edited

Loading