File state.apply-don-t-check-for-cached-pillar-errors.patch of Package salt.26144
From 5880703551d82a68a0e2f3108878124d8ae98bf0 Mon Sep 17 00:00:00 2001
From: Alexander Graul <agraul@suse.com>
Date: Tue, 25 Jan 2022 17:20:55 +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 | 115 +++++----------
6 files changed, 177 insertions(+), 89 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
diff --git a/changelog/52354.fixed b/changelog/52354.fixed
new file mode 100644
index 0000000000..af885d77fa
--- /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 0000000000..af885d77fa
--- /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 0000000000..af885d77fa
--- /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 0c3dfc3317..0027744229 100644
--- a/salt/modules/state.py
+++ b/salt/modules/state.py
@@ -106,18 +106,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 0000000000..af65a05945
--- /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
index 02fd2dd307..30cda303cc 100644
--- a/tests/pytests/unit/modules/state/test_state.py
+++ b/tests/pytests/unit/modules/state/test_state.py
@@ -1,14 +1,16 @@
"""
:codeauthor: Rahul Handay <rahulha@saltstack.com>
"""
-
import datetime
import logging
import os
+from collections import namedtuple
import pytest
+
import salt.config
import salt.loader
+import salt.loader.context
import salt.modules.config as config
import salt.modules.state as state
import salt.state
@@ -1200,85 +1202,6 @@ def test_lock_saltenv():
)
-def test_get_pillar_errors_CC():
- """
- 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():
- """
- 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():
- """
- 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():
- """
- 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)
-
-
def test_event():
"""
test state.event runner
@@ -1318,3 +1241,35 @@ def test_event():
if _expected in x.args[0]:
found = True
assert found is True
+
+
+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"]),
+ (PillarPair({"foo": "bar", "_errors": ["Failure"]}, {"fred": "baz"}), None),
+]
+
+
+@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__
+ """
+ ctx = salt.loader.context.LoaderContext()
+ named_ctx = ctx.named_context("__pillar__", pillar.in_memory)
+ with patch("salt.modules.state.__pillar__", named_ctx, create=True):
+ assert (
+ state._get_pillar_errors(kwargs={"force": True}, pillar=pillar.fresh)
+ is None
+ )
+ assert (
+ state._get_pillar_errors(kwargs={}, pillar=pillar.fresh) == expected_errors
+ )
--
2.34.1
