Cleanup calculation of template sls/tpl context (#58238)

* Cleanup calculation of template sls/tpl context

Fixes #56410

* fix string formatting

* Add unit test that works against old version (with bugs)

* Updated unit tests with non-buggy values and fix found bugs

* cleanup unit tests to test underlying function

* remove old unit test components no longer used

* Cleanup

* More Cleanup

* More cleanup. Add Mock to test.support.unit

* Add changelog entries

* Fix Mockery

* Import order fix

* Handle backslashes in sls names under *nix

* Cleanup

* Make sure we return a dictionary from jinja.load_map

* Fix scenario when sls is empty but present

* Touched another file - Cleanup to make pre-commit happy

* Adding variables to docs

* Fix expected tplpath value to be OS specific and note so in docs

* removing comments from imports as per pre-commit

* removing comments from imports as per pre-commit

* Put slsvars changes behind a feature flag

* Better documentation for enable_slsvars_fixes feature flag

* Fix test that should be skipped on windows

Co-authored-by: Michael "M3" Lasevich <Michael.Lasevich@bhnetwork.com>
Co-authored-by: Sage the Rage <36676171+sagetherage@users.noreply.github.com>
Co-authored-by: Daniel A. Wozniak <dwozniak@saltstack.com>
Co-authored-by: Shane Lee <slee@saltstack.com>

changelog/56410.fixed

@@ -0,0 +1 @@
+Proper calculation of tpldir and related context parameters

changelog/58249.fixed

@@ -0,0 +1 @@
+Fix blank tplfile context parameter when loading top level sls files

changelog/58250.fixed

@@ -0,0 +1 @@
+Do not include init directory in sls context parameters if explicitly specified

changelog/58652.added

@@ -0,0 +1,4 @@
+Added features config option for feature flags. Added a feature flag
+`enable_slsvars_fixes` to enable fixes to tpldir, tplfile and sls_path.
+This flag will be depricated in the Phosphorus release when this functionality
+becomes the default.

doc/ref/states/vars.rst

@@ -2,6 +2,21 @@
 SLS Template Variable Reference
 ===============================
 
+
+.. warning::
+   In the 3002 release sls_path, tplfile, and tpldir have had some significate
+   improvements which have the potential to break states that rely on old and
+   broken functionality. These fixes can be enabled by setting the
+   ``use_slsvars_fixes`` feature flag to ``True`` in your minion's config file.
+   This functionality will become the default in the 3005 release.
+
+   .. code-block:: yaml
+
+       features:
+         use_slsvars_fixes: True
+
+
+
 The template engines available to sls files and file templates come loaded
 with a number of context variables. These variables contain information and
 functions to assist in the generation of templates.  See each variable below
@@ -81,8 +96,13 @@ from an environment.
 
     {{ saltenv }}
 
+SLS Only Variables
+==================
+The following are only available when processing sls files. If you need these
+in other templates, you can usually pass them in as template context.
+
 sls
-====
+---
 
 The `sls` variable contains the sls reference value, and is only available in
 the actual SLS file (not in any files referenced in that SLS). The sls
@@ -94,14 +114,96 @@ include option.
     {{ sls }}
 
 slspath
-=======
+-------
 
 The `slspath` variable contains the path to the directory of the current sls
 file. The value of `slspath` in files referenced in the current sls depends on
 the reference method. For jinja includes `slspath` is the path to the current
 directory of the file. For salt includes `slspath` is the path to the directory
-of the included file.
+of the included file. If current sls file is in root of the file roots, this
+will return ""
 
 .. code-block:: jinja
 
     {{ slspath }}
+
+
+sls_path
+--------
+
+A version of `slspath` with underscores as path separators instead of slashes.
+So, if `slspath` is `path/to/state` then `sls_path` is `path_to_state`
+
+.. code-block:: jinja
+
+    {{ sls_path }}
+
+slsdotpath
+----------
+
+A version of `slspath` with dots as path separators instead of slashes. So, if
+`slspath` is `path/to/state` then `slsdotpath` is `path.to.state`. This is same
+as `sls` if `sls` points to a directory instead if a file.
+
+.. code-block:: jinja
+
+    {{ slsdotpath }}
+
+
+slscolonpath
+------------
+
+A version of `slspath` with colons (`:`) as path separators instead of slashes.
+So, if `slspath` is `path/to/state` then `slscolonpath` is `path:to:state`.
+
+.. code-block:: jinja
+
+    {{ slscolonpath }}
+
+tplpath
+-------
+
+Full path to sls template file being process on local disk. This is usually
+pointing to a copy of the sls file in a cache directory. This will be in OS
+specific format (windows vs posix). (It is probably best not to use this.)
+
+.. code-block:: jinja
+
+    {{ tplpath }}
+
+
+tplfile
+-------
+
+Relative path to exact sls template file being processed relative to file
+roots.
+
+.. code-block:: jinja
+
+    {{ tplfile }}
+
+tpldir
+------
+
+Directory, relative to file roots, of the current sls file. If current sls file
+is in root of the file roots, this will return ".". This is usually identical
+to `slspath` except in case of root-level sls, where this will return a "`.`".
+
+A Common use case for this variable is to generate relative salt urls like:
+.. code-block:: jinja
+
+    my-file:
+      file.managed:
+        source: salt://{{ tpldir }}/files/my-template
+
+
+tpldot
+------
+
+A version of `tpldir` with dots as path separators instead of slashes. So, if
+`tpldir` is `path/to/state` then `tpldot` is `path.to.state`. NOTE: if `tpldir`
+is `.`, this will be set to ""
+
+.. code-block:: jinja
+
+    {{ tpldot }}

salt/config/__init__.py

@@ -1,7 +1,6 @@
 """
 All salt configuration loading and defaults should be in this module
 """
-
 import codecs
 import glob
 import logging
@@ -944,6 +943,8 @@ VALID_OPTS = immutabletypes.freeze(
         # client via the Salt API
         "netapi_allow_raw_shell": bool,
         "disabled_requisites": (str, list),
+        # Feature flag config
+        "features": dict,
     }
 )
 

salt/features.py

@@ -0,0 +1,28 @@
+"""
+Feature flags
+"""
+import logging
+
+log = logging.getLogger(__name__)
+
+
+class Features:
+    def __init__(self, _features=None):
+        if _features is None:
+            self.features = {}
+        else:
+            self.features = _features
+        self.setup = False
+
+    def setup_features(self, opts):
+        if not self.setup:
+            self.features.update(opts.get("features", {}))
+        else:
+            log.warn("Features already setup")
+
+    def get(self, key, default=None):
+        return self.features.get(key, default)
+
+
+features = Features()
+setup_features = features.setup_features

salt/utils/templates.py

@@ -1,14 +1,13 @@
 """
 Template render systems
 """
-
-
 import codecs
 import logging
 import os
 import sys
 import tempfile
 import traceback
+from pathlib import Path
 
 import jinja2
 import jinja2.ext
@@ -25,6 +24,7 @@ import salt.utils.yamlencoding
 from salt import __path__ as saltpath
 from salt.exceptions import CommandExecutionError, SaltInvocationError, SaltRenderError
 from salt.ext import six
+from salt.features import features
 from salt.utils.decorators.jinja import JinjaFilter, JinjaGlobal, JinjaTest
 from salt.utils.odict import OrderedDict
 from salt.utils.versions import LooseVersion
@@ -92,6 +92,124 @@ class AliasedModule:
         return getattr(self.wrapped, name)
 
 
+def _generate_sls_context_legacy(tmplpath, sls):
+    """
+    Legacy version of generate_sls_context, this method should be remove in the
+    Phosphorus release.
+    """
+    salt.utils.versions.warn_until(
+        "Phosphorus",
+        "There have been significant improvement to template variables. "
+        "To enable these improvements set features.enable_slsvars_fixes "
+        "to True in your config file. This feature will become the default "
+        "in the Phoshorus release.",
+    )
+    context = {}
+    slspath = sls.replace(".", "/")
+    if tmplpath is not None:
+        context["tplpath"] = tmplpath
+        if not tmplpath.lower().replace("\\", "/").endswith("/init.sls"):
+            slspath = os.path.dirname(slspath)
+        template = tmplpath.replace("\\", "/")
+        i = template.rfind(slspath.replace(".", "/"))
+        if i != -1:
+            template = template[i:]
+        tpldir = os.path.dirname(template).replace("\\", "/")
+        tpldata = {
+            "tplfile": template,
+            "tpldir": "." if tpldir == "" else tpldir,
+            "tpldot": tpldir.replace("/", "."),
+        }
+        context.update(tpldata)
+    context["slsdotpath"] = slspath.replace("/", ".")
+    context["slscolonpath"] = slspath.replace("/", ":")
+    context["sls_path"] = slspath.replace("/", "_")
+    context["slspath"] = slspath
+    return context
+
+
+def _generate_sls_context(tmplpath, sls):
+    """
+    Generate SLS/Template Context Items
+
+    Return values:
+
+    tplpath - full path to template on filesystem including filename
+    tplfile - relative path to template -- relative to file roots
+    tpldir - directory of the template relative to file roots. If none, "."
+    tpldot - tpldir using dots instead of slashes, if none, ""
+    slspath - directory containing current sls - (same as tpldir), if none, ""
+    sls_path - slspath with underscores separating parts, if none, ""
+    slsdotpath - slspath with dots separating parts, if none, ""
+    slscolonpath- slspath with colons separating parts, if none, ""
+
+    """
+
+    sls_context = {}
+
+    # Normalize SLS as path.
+    slspath = sls.replace(".", "/")
+
+    if tmplpath:
+        # Normalize template path
+        template = str(Path(tmplpath).as_posix())
+
+        # Determine proper template name without root
+        if not sls:
+            template = template.rsplit("/", 1)[-1]
+        elif template.endswith("{}.sls".format(slspath)):
+            template = template[-(4 + len(slspath)) :]
+        elif template.endswith("{}/init.sls".format(slspath)):
+            template = template[-(9 + len(slspath)) :]
+        else:
+            # Something went wrong
+            log.warning("Failed to determine proper template path")
+
+        slspath = template.rsplit("/", 1)[0] if "/" in template else ""
+
+        sls_context.update(
+            dict(
+                tplpath=tmplpath,
+                tplfile=template,
+                tpldir=slspath if slspath else ".",
+                tpldot=slspath.replace("/", "."),
+            )
+        )
+
+    # Should this be normalized?
+    sls_context.update(
+        dict(
+            slspath=slspath,
+            slsdotpath=slspath.replace("/", "."),
+            slscolonpath=slspath.replace("/", ":"),
+            sls_path=slspath.replace("/", "_"),
+        )
+    )
+
+    return sls_context
+
+
+def generate_sls_context(tmplpath, sls):
+    """
+    Generate SLS/Template Context Items
+
+    Return values:
+
+    tplpath - full path to template on filesystem including filename
+    tplfile - relative path to template -- relative to file roots
+    tpldir - directory of the template relative to file roots. If none, "."
+    tpldot - tpldir using dots instead of slashes, if none, ""
+    slspath - directory containing current sls - (same as tpldir), if none, ""
+    sls_path - slspath with underscores separating parts, if none, ""
+    slsdotpath - slspath with dots separating parts, if none, ""
+    slscolonpath- slspath with colons separating parts, if none, ""
+
+    """
+    if not features.get("enable_slsvars_fixes", False):
+        return _generate_sls_context_legacy(tmplpath, sls)
+    _generate_sls_context(tmplpath, sls)
+
+
 def wrap_tmpl_func(render_str):
     def render_tmpl(
         tmplsrc, from_str=False, to_str=False, context=None, tmplpath=None, **kws
@@ -112,26 +230,8 @@ def wrap_tmpl_func(render_str):
         assert "saltenv" in context
 
         if "sls" in context:
-            slspath = context["sls"].replace(".", "/")
-            if tmplpath is not None:
-                context["tplpath"] = tmplpath
-                if not tmplpath.lower().replace("\\", "/").endswith("/init.sls"):
-                    slspath = os.path.dirname(slspath)
-                template = tmplpath.replace("\\", "/")
-                i = template.rfind(slspath.replace(".", "/"))
-                if i != -1:
-                    template = template[i:]
-                tpldir = os.path.dirname(template).replace("\\", "/")
-                tpldata = {
-                    "tplfile": template,
-                    "tpldir": "." if tpldir == "" else tpldir,
-                    "tpldot": tpldir.replace("/", "."),
-                }
-                context.update(tpldata)
-            context["slsdotpath"] = slspath.replace("/", ".")
-            context["slscolonpath"] = slspath.replace("/", ":")
-            context["sls_path"] = slspath.replace("/", "_")
-            context["slspath"] = slspath
+            sls_context = generate_sls_context(tmplpath, context["sls"])
+            context.update(sls_context)
 
         if isinstance(tmplsrc, str):
             if from_str:

tests/integration/modules/test_jinja.py

@@ -1,22 +1,14 @@
-# -*- coding: utf-8 -*-
 """
 Test the jinja module
 """
 
-# Import python libs
-from __future__ import absolute_import, print_function, unicode_literals
-
 import os
 
 import salt.utils.files
-
-# Import Salt libs
 import salt.utils.json
 import salt.utils.yaml
 from tests.support.case import ModuleCase
 from tests.support.helpers import requires_system_grains
-
-# Import Salt Testing libs
 from tests.support.runtests import RUNTIME_VARS
 
 
@@ -48,6 +40,10 @@ class TestModulesJinja(ModuleCase):
     def test_load_map(self, grains):
         ret = self.run_function("jinja.load_map", [self._path("map.jinja"), "template"])
 
+        assert isinstance(
+            ret, dict
+        ), "failed to return dictionary from jinja.load_map: {}".format(ret)
+
         with salt.utils.files.fopen(self._path("defaults.yaml", absolute=True)) as fh_:
             defaults = salt.utils.yaml.safe_load(fh_)
         with salt.utils.files.fopen(self._path("osarchmap.json", absolute=True)) as fh_:

tests/unit/test_config.py

@@ -364,6 +364,10 @@ class ConfigTestCase(TestCase, AdaptedConfigurationTestCaseMixin):
             config = salt.config.minion_config(fpath)
             self.assertEqual(config["log_file"], fpath)
 
+    @skipIf(
+        salt.utils.platform.is_windows(),
+        "You can't set an environment dynamically in Windows",
+    )
     @with_tempdir()
     def test_load_client_config_from_environ_var(self, tempdir):
         env_root_dir = os.path.join(tempdir, "foo", "env")

tests/unit/utils/test_templates.py

@@ -1,21 +1,15 @@
-# -*- coding: utf-8 -*-
 """
 Unit tests for salt.utils.templates.py
 """
 
-# Import python libs
-from __future__ import absolute_import, print_function, unicode_literals
-
 import logging
 import os
 import sys
+from pathlib import PurePath, PurePosixPath
 
 import salt.utils.files
-
-# Import Salt libs
 import salt.utils.templates
-
-# Import Salt Testing Libs
+from tests.support import mock
 from tests.support.helpers import with_tempdir
 from tests.support.unit import TestCase, skipIf
 
@@ -29,7 +23,6 @@ except ImportError:
 log = logging.getLogger(__name__)
 
 
-### Here we go!
 class RenderTestCase(TestCase):
     def setUp(self):
         # Default context for salt.utils.templates.render_*_tmpl to work
@@ -209,7 +202,7 @@ class RenderTestCase(TestCase):
         self.assertEqual(res.strip(), "OK")
 
 
-class MockRender(object):
+class MockRender:
     def __call__(self, tplstr, context, tmplpath=None):
         self.tplstr = tplstr
         self.context = context
@@ -218,26 +211,197 @@ class MockRender(object):
 
 
 class WrapRenderTestCase(TestCase):
-    @with_tempdir()
-    def test_wrap_issue_56119_a(self, tempdir):
-        slsfile = os.path.join(tempdir, "foo")
-        with salt.utils.files.fopen(slsfile, "w") as fp:
-            fp.write("{{ slspath }}")
-        context = {"opts": {}, "saltenv": "base", "sls": "foo.bar"}
-        render = MockRender()
-        wrapped = salt.utils.templates.wrap_tmpl_func(render)
-        res = wrapped(slsfile, context=context, tmplpath="/tmp/foo/bar/init.sls")
-        assert render.context["slspath"] == "foo/bar", render.context["slspath"]
-        assert render.context["tpldir"] == "foo/bar", render.context["tpldir"]
+    def assertDictContainsAll(self, actual, **expected):
+        """ Make sure dictionary contains at least all expected values"""
+        actual = {key: actual[key] for key in expected if key in actual}
+        self.assertEqual(expected, actual)
 
+    def _test_generated_sls_context(self, tmplpath, sls, **expected):
+        """ Generic SLS Context Test"""
+        # DeNormalize tmplpath
+        tmplpath = str(PurePath(PurePosixPath(tmplpath)))
+        if tmplpath.startswith("\\"):
+            tmplpath = "C:{}".format(tmplpath)
+        expected["tplpath"] = tmplpath
+        actual = salt.utils.templates._generate_sls_context(tmplpath, sls)
+        self.assertDictContainsAll(actual, **expected)
+
+    @mock.patch("salt.utils.templates.generate_sls_context")
     @with_tempdir()
-    def test_wrap_issue_56119_b(self, tempdir):
+    def test_sls_context_call(self, tempdir, generate_sls_context):
+        """ Check that generate_sls_context is called with proper parameters"""
+        sls = "foo.bar"
+        tmplpath = "/tmp/foo/bar.sls"
+
         slsfile = os.path.join(tempdir, "foo")
         with salt.utils.files.fopen(slsfile, "w") as fp:
             fp.write("{{ slspath }}")
-        context = {"opts": {}, "saltenv": "base", "sls": "foo.bar.bang"}
+        context = {"opts": {}, "saltenv": "base", "sls": sls}
         render = MockRender()
         wrapped = salt.utils.templates.wrap_tmpl_func(render)
-        res = wrapped(slsfile, context=context, tmplpath="/tmp/foo/bar/bang.sls")
-        assert render.context["slspath"] == "foo/bar", render.context["slspath"]
-        assert render.context["tpldir"] == "foo/bar", render.context["tpldir"]
+        res = wrapped(slsfile, context=context, tmplpath=tmplpath)
+        generate_sls_context.assert_called_with(tmplpath, sls)
+
+    @mock.patch("salt.utils.templates.generate_sls_context")
+    @with_tempdir()
+    def test_sls_context_no_call(self, tempdir, generate_sls_context):
+        """ Check that generate_sls_context is not called if sls is not set"""
+        sls = "foo.bar"
+        tmplpath = "/tmp/foo/bar.sls"
+
+        slsfile = os.path.join(tempdir, "foo")
+        with salt.utils.files.fopen(slsfile, "w") as fp:
+            fp.write("{{ slspath }}")
+        context = {"opts": {}, "saltenv": "base"}
+        render = MockRender()
+        wrapped = salt.utils.templates.wrap_tmpl_func(render)
+        res = wrapped(slsfile, context=context, tmplpath=tmplpath)
+        generate_sls_context.assert_not_called()
+
+    def test_generate_sls_context__top_level(self):
+        """ generate_sls_context - top_level Use case"""
+        self._test_generated_sls_context(
+            "/tmp/boo.sls",
+            "boo",
+            tplfile="boo.sls",
+            tpldir=".",
+            tpldot="",
+            slsdotpath="",
+            slscolonpath="",
+            sls_path="",
+            slspath="",
+        )
+
+    def test_generate_sls_context__one_level_init_implicit(self):
+        """ generate_sls_context - Basic one level with impliocit init.sls """
+        self._test_generated_sls_context(
+            "/tmp/foo/init.sls",
+            "foo",
+            tplfile="foo/init.sls",
+            tpldir="foo",
+            tpldot="foo",
+            slsdotpath="foo",
+            slscolonpath="foo",
+            sls_path="foo",
+            slspath="foo",
+        )
+
+    def test_generate_sls_context__one_level_init_explicit(self):
+        """ generate_sls_context - Basic one level with explicit init.sls """
+        self._test_generated_sls_context(
+            "/tmp/foo/init.sls",
+            "foo.init",
+            tplfile="foo/init.sls",
+            tpldir="foo",
+            tpldot="foo",
+            slsdotpath="foo",
+            slscolonpath="foo",
+            sls_path="foo",
+            slspath="foo",
+        )
+
+    def test_generate_sls_context__one_level(self):
+        """ generate_sls_context - Basic one level with name"""
+        self._test_generated_sls_context(
+            "/tmp/foo/boo.sls",
+            "foo.boo",
+            tplfile="foo/boo.sls",
+            tpldir="foo",
+            tpldot="foo",
+            slsdotpath="foo",
+            slscolonpath="foo",
+            sls_path="foo",
+            slspath="foo",
+        )
+
+    def test_generate_sls_context__one_level_repeating(self):
+        """ generate_sls_context - Basic one level with name same as dir
+
+        (Issue #56410)
+        """
+        self._test_generated_sls_context(
+            "/tmp/foo/foo.sls",
+            "foo.foo",
+            tplfile="foo/foo.sls",
+            tpldir="foo",
+            tpldot="foo",
+            slsdotpath="foo",
+            slscolonpath="foo",
+            sls_path="foo",
+            slspath="foo",
+        )
+
+    def test_generate_sls_context__two_level_init_implicit(self):
+        """ generate_sls_context - Basic two level with implicit init.sls """
+        self._test_generated_sls_context(
+            "/tmp/foo/bar/init.sls",
+            "foo.bar",
+            tplfile="foo/bar/init.sls",
+            tpldir="foo/bar",
+            tpldot="foo.bar",
+            slsdotpath="foo.bar",
+            slscolonpath="foo:bar",
+            sls_path="foo_bar",
+            slspath="foo/bar",
+        )
+
+    def test_generate_sls_context__two_level_init_explicit(self):
+        """ generate_sls_context - Basic two level with explicit init.sls """
+        self._test_generated_sls_context(
+            "/tmp/foo/bar/init.sls",
+            "foo.bar.init",
+            tplfile="foo/bar/init.sls",
+            tpldir="foo/bar",
+            tpldot="foo.bar",
+            slsdotpath="foo.bar",
+            slscolonpath="foo:bar",
+            sls_path="foo_bar",
+            slspath="foo/bar",
+        )
+
+    def test_generate_sls_context__two_level(self):
+        """ generate_sls_context - Basic two level with name"""
+        self._test_generated_sls_context(
+            "/tmp/foo/bar/boo.sls",
+            "foo.bar.boo",
+            tplfile="foo/bar/boo.sls",
+            tpldir="foo/bar",
+            tpldot="foo.bar",
+            slsdotpath="foo.bar",
+            slscolonpath="foo:bar",
+            sls_path="foo_bar",
+            slspath="foo/bar",
+        )
+
+    def test_generate_sls_context__two_level_repeating(self):
+        """ generate_sls_context - Basic two level with name same as dir
+
+        (Issue #56410)
+        """
+        self._test_generated_sls_context(
+            "/tmp/foo/foo/foo.sls",
+            "foo.foo.foo",
+            tplfile="foo/foo/foo.sls",
+            tpldir="foo/foo",
+            tpldot="foo.foo",
+            slsdotpath="foo.foo",
+            slscolonpath="foo:foo",
+            sls_path="foo_foo",
+            slspath="foo/foo",
+        )
+
+    @skipIf(sys.platform == "win32", "Backslash not possible under windows")
+    def test_generate_sls_context__backslash_in_path(self):
+        """ generate_sls_context - Handle backslash in path on non-windows
+        """
+        self._test_generated_sls_context(
+            "/tmp/foo/foo\\foo.sls",
+            "foo.foo\\foo",
+            tplfile="foo/foo\\foo.sls",
+            tpldir="foo",
+            tpldot="foo",
+            slsdotpath="foo",
+            slscolonpath="foo",
+            sls_path="foo",
+            slspath="foo",
+        )