File state.apply-don-t-check-for-cached-pillar-error... of Package salt

Overview Repositories Revisions Requests Users Attributes Meta

File state.apply-don-t-check-for-cached-pillar-errors.patch of Package salt (Revision 77de3216393dfaf8b54e7a07a1ac4abf)

Currently displaying revision 77de3216393dfaf8b54e7a07a1ac4abf , Show latest

From 63b7c251214f41932be7f8f04d998af0ed89cb3d Mon Sep 17 00:00:00 2001
From: Alexander Graul <agraul@suse.com>
Date: Fri, 5 Nov 2021 17:52:12 +0100
Subject: [PATCH] state.apply: don't check for cached pillar errors

state.apply request new pillar data from the server. This done to always
have the most up-to-date pillar to work with. Previously, checking for
pillar errors looked at both the new pillar and the in-memory pillar.
The latter might contain pillar rendering errors even if the former does
not.

For this reason, only the new pillar should be checked, not both.
---
 changelog/52354.fixed                         |   1 +
 changelog/57180.fixed                         |   1 +
 changelog/59339.fixed                         |   1 +
 salt/modules/state.py                         |  17 ++-
 .../modules/state/test_state_pillar_errors.py | 131 ++++++++++++++++++
 .../pytests/unit/modules/state/test_state.py  |  34 +++++
 tests/unit/modules/test_state.py              |  83 -----------
 7 files changed, 176 insertions(+), 92 deletions(-)
 create mode 100644 changelog/52354.fixed
 create mode 100644 changelog/57180.fixed
 create mode 100644 changelog/59339.fixed
 create mode 100644 tests/pytests/integration/modules/state/test_state_pillar_errors.py
 create mode 100644 tests/pytests/unit/modules/state/test_state.py

diff --git a/changelog/52354.fixed b/changelog/52354.fixed
new file mode 100644
index 00000000000..af885d77fa4
--- /dev/null
+++ b/changelog/52354.fixed
@@ -0,0 +1 @@
+Don't check for cached pillar errors on state.apply
diff --git a/changelog/57180.fixed b/changelog/57180.fixed
new file mode 100644
index 00000000000..af885d77fa4
--- /dev/null
+++ b/changelog/57180.fixed
@@ -0,0 +1 @@
+Don't check for cached pillar errors on state.apply
diff --git a/changelog/59339.fixed b/changelog/59339.fixed
new file mode 100644
index 00000000000..af885d77fa4
--- /dev/null
+++ b/changelog/59339.fixed
@@ -0,0 +1 @@
+Don't check for cached pillar errors on state.apply
diff --git a/salt/modules/state.py b/salt/modules/state.py
index 6d8333d3b4f..a453cf1b706 100644
--- a/salt/modules/state.py
+++ b/salt/modules/state.py
@@ -108,18 +108,17 @@ def _set_retcode(ret, highstate=None):
 
 def _get_pillar_errors(kwargs, pillar=None):
     """
-    Checks all pillars (external and internal) for errors.
-    Return an error message, if anywhere or None.
+    Check pillar for errors.
+
+    If a pillar is passed, it will be checked. Otherwise, the in-memory pillar
+    will checked instead. Passing kwargs['force'] = True short cuts the check
+    and always returns None, indicating no errors.
 
     :param kwargs: dictionary of options
-    :param pillar: external pillar
-    :return: None or an error message
+    :param pillar: pillar
+    :return: None or a list of error messages
     """
-    return (
-        None
-        if kwargs.get("force")
-        else (pillar or {}).get("_errors", __pillar__.get("_errors")) or None
-    )
+    return None if kwargs.get("force") else (pillar or __pillar__).get("_errors")
 
 
 def _wait(jid):
diff --git a/tests/pytests/integration/modules/state/test_state_pillar_errors.py b/tests/pytests/integration/modules/state/test_state_pillar_errors.py
new file mode 100644
index 00000000000..af65a059452
--- /dev/null
+++ b/tests/pytests/integration/modules/state/test_state_pillar_errors.py
@@ -0,0 +1,131 @@
+#!/usr/bin/python3
+
+import textwrap
+
+import pytest
+from saltfactories.utils.functional import StateResult
+
+pytestmark = [
+    pytest.mark.slow_test,
+]
+
+
+@pytest.fixture(scope="module")
+def reset_pillar(salt_call_cli):
+    try:
+        # Run tests
+        yield
+    finally:
+        # Refresh pillar once all tests are done.
+        ret = salt_call_cli.run("saltutil.refresh_pillar", wait=True)
+        assert ret.exitcode == 0
+        assert ret.json is True
+
+
+@pytest.fixture
+def testfile_path(tmp_path, base_env_state_tree_root_dir):
+    testfile = tmp_path / "testfile"
+    sls_contents = textwrap.dedent(
+        """
+        {}:
+          file:
+            - managed
+            - source: salt://testfile
+            - makedirs: true
+        """.format(testfile)
+    )
+    with pytest.helpers.temp_file(
+        "sls-id-test.sls", sls_contents, base_env_state_tree_root_dir
+    ):
+        yield testfile
+
+
+@pytest.mark.usefixtures("testfile_path", "reset_pillar")
+def test_state_apply_aborts_on_pillar_error(
+    salt_cli,
+    salt_minion,
+    base_env_pillar_tree_root_dir,
+):
+    """
+    Test state.apply with error in pillar.
+    """
+    pillar_top_file = textwrap.dedent(
+        """
+        base:
+          '{}':
+            - basic
+        """
+    ).format(salt_minion.id)
+    basic_pillar_file = textwrap.dedent(
+        """
+        syntax_error
+        """
+    )
+
+    with pytest.helpers.temp_file(
+        "top.sls", pillar_top_file, base_env_pillar_tree_root_dir
+    ), pytest.helpers.temp_file(
+        "basic.sls", basic_pillar_file, base_env_pillar_tree_root_dir
+    ):
+        expected_comment = [
+            "Pillar failed to render with the following messages:",
+            "SLS 'basic' does not render to a dictionary",
+        ]
+        shell_result = salt_cli.run(
+            "state.apply", "sls-id-test", minion_tgt=salt_minion.id
+        )
+        assert shell_result.exitcode == 1
+        assert shell_result.json == expected_comment
+
+
+@pytest.mark.usefixtures("testfile_path", "reset_pillar")
+def test_state_apply_continues_after_pillar_error_is_fixed(
+    salt_cli,
+    salt_minion,
+    base_env_pillar_tree_root_dir,
+):
+    """
+    Test state.apply with error in pillar.
+    """
+    pillar_top_file = textwrap.dedent(
+        """
+        base:
+          '{}':
+            - basic
+        """.format(salt_minion.id)
+    )
+    basic_pillar_file_error = textwrap.dedent(
+        """
+        syntax_error
+        """
+    )
+    basic_pillar_file = textwrap.dedent(
+        """
+        syntax_error: Fixed!
+        """
+    )
+
+    # save pillar render error in minion's in-memory pillar
+    with pytest.helpers.temp_file(
+        "top.sls", pillar_top_file, base_env_pillar_tree_root_dir
+    ), pytest.helpers.temp_file(
+        "basic.sls", basic_pillar_file_error, base_env_pillar_tree_root_dir
+    ):
+        shell_result = salt_cli.run(
+            "saltutil.refresh_pillar", minion_tgt=salt_minion.id
+        )
+        assert shell_result.exitcode == 0
+
+    # run state.apply with fixed pillar render error
+    with pytest.helpers.temp_file(
+        "top.sls", pillar_top_file, base_env_pillar_tree_root_dir
+    ), pytest.helpers.temp_file(
+        "basic.sls", basic_pillar_file, base_env_pillar_tree_root_dir
+    ):
+        shell_result = salt_cli.run(
+            "state.apply", "sls-id-test", minion_tgt=salt_minion.id
+        )
+        assert shell_result.exitcode == 0
+        state_result = StateResult(shell_result.json)
+        assert state_result.result is True
+        assert state_result.changes == {"diff": "New file", "mode": "0644"}
diff --git a/tests/pytests/unit/modules/state/test_state.py b/tests/pytests/unit/modules/state/test_state.py
new file mode 100644
index 00000000000..edd030a6826
--- /dev/null
+++ b/tests/pytests/unit/modules/state/test_state.py
@@ -0,0 +1,34 @@
+from collections import namedtuple
+
+import pytest
+import salt.modules.state as state
+from tests.support.mock import patch
+
+
+PillarPair = namedtuple("PillarPair", ["in_memory", "fresh"])
+pillar_combinations = [
+    (PillarPair({"foo": "bar"}, {"fred": "baz"}), None),
+    (PillarPair({"foo": "bar"}, {"fred": "baz", "_errors": ["Failure"]}), ["Failure"]),
+    (PillarPair({"foo": "bar"}, None), None),
+    (PillarPair({"foo": "bar", "_errors": ["Failure"]}, None), ["Failure"]),
+]
+
+
+@pytest.mark.parametrize("pillar,expected_errors", pillar_combinations)
+def test_get_pillar_errors(pillar: PillarPair, expected_errors):
+    """
+    test _get_pillar_errors function
+
+    There are three cases to consider:
+    1. kwargs['force'] is True -> None, no matter what's in pillar/__pillar__
+    2. pillar kwarg is available -> only check pillar, no matter what's in __pillar__
+    3. pillar kwarg is not available -> check __pillar__
+    """
+    with patch("salt.modules.state.__pillar__", pillar.in_memory):
+        assert (
+            state._get_pillar_errors(kwargs={"force": True}, pillar=pillar.fresh)
+            is None
+        )
+        assert (
+            state._get_pillar_errors(kwargs={}, pillar=pillar.fresh) == expected_errors
+        )
diff --git a/tests/unit/modules/test_state.py b/tests/unit/modules/test_state.py
index dda48d8c1f2..eb6bf4e659d 100644
--- a/tests/unit/modules/test_state.py
+++ b/tests/unit/modules/test_state.py
@@ -1256,89 +1256,6 @@ class StateTestCase(TestCase, LoaderModuleMockMixin):
                     saltenv="base",
                 )
 
-    def test_get_pillar_errors_CC(self):
-        """
-        Test _get_pillar_errors function.
-        CC: External clean, Internal clean
-        :return:
-        """
-        for int_pillar, ext_pillar in [
-            ({"foo": "bar"}, {"fred": "baz"}),
-            ({"foo": "bar"}, None),
-            ({}, {"fred": "baz"}),
-        ]:
-            with patch("salt.modules.state.__pillar__", int_pillar):
-                for opts, res in [
-                    ({"force": True}, None),
-                    ({"force": False}, None),
-                    ({}, None),
-                ]:
-                    assert res == state._get_pillar_errors(
-                        kwargs=opts, pillar=ext_pillar
-                    )
-
-    def test_get_pillar_errors_EC(self):
-        """
-        Test _get_pillar_errors function.
-        EC: External erroneous, Internal clean
-        :return:
-        """
-        errors = ["failure", "everywhere"]
-        for int_pillar, ext_pillar in [
-            ({"foo": "bar"}, {"fred": "baz", "_errors": errors}),
-            ({}, {"fred": "baz", "_errors": errors}),
-        ]:
-            with patch("salt.modules.state.__pillar__", int_pillar):
-                for opts, res in [
-                    ({"force": True}, None),
-                    ({"force": False}, errors),
-                    ({}, errors),
-                ]:
-                    assert res == state._get_pillar_errors(
-                        kwargs=opts, pillar=ext_pillar
-                    )
-
-    def test_get_pillar_errors_EE(self):
-        """
-        Test _get_pillar_errors function.
-        CC: External erroneous, Internal erroneous
-        :return:
-        """
-        errors = ["failure", "everywhere"]
-        for int_pillar, ext_pillar in [
-            ({"foo": "bar", "_errors": errors}, {"fred": "baz", "_errors": errors})
-        ]:
-            with patch("salt.modules.state.__pillar__", int_pillar):
-                for opts, res in [
-                    ({"force": True}, None),
-                    ({"force": False}, errors),
-                    ({}, errors),
-                ]:
-                    assert res == state._get_pillar_errors(
-                        kwargs=opts, pillar=ext_pillar
-                    )
-
-    def test_get_pillar_errors_CE(self):
-        """
-        Test _get_pillar_errors function.
-        CC: External clean, Internal erroneous
-        :return:
-        """
-        errors = ["failure", "everywhere"]
-        for int_pillar, ext_pillar in [
-            ({"foo": "bar", "_errors": errors}, {"fred": "baz"}),
-            ({"foo": "bar", "_errors": errors}, None),
-        ]:
-            with patch("salt.modules.state.__pillar__", int_pillar):
-                for opts, res in [
-                    ({"force": True}, None),
-                    ({"force": False}, errors),
-                    ({}, errors),
-                ]:
-                    assert res == state._get_pillar_errors(
-                        kwargs=opts, pillar=ext_pillar
-                    )
-
 
 class TopFileMergingCase(TestCase, LoaderModuleMockMixin):
     def setup_loader_modules(self):
-- 
2.34.1

Places

File state.apply-don-t-check-for-cached-pillar-errors.patch of Package salt (Revision 77de3216393dfaf8b54e7a07a1ac4abf)

Places