From 7df8364939f431f050beb03db89e85267f2ee63e Mon Sep 17 00:00:00 2001 From: Tanmoy037 Date: Thu, 6 Apr 2023 16:53:06 +0530 Subject: [PATCH 01/37] [DOCS] service state and module docs do not mention that systemd daemon is reloaded #63601 --solved --- salt/states/service.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/salt/states/service.py b/salt/states/service.py index 93c7c4fb070..0bb04c34cc1 100644 --- a/salt/states/service.py +++ b/salt/states/service.py @@ -19,6 +19,13 @@ If your service states are running into trouble with init system detection, please see the :ref:`Overriding Virtual Module Providers ` section of Salt's module documentation to work around possible errors. +For services managed by systemd, the systemd_service module includes a built-in +feature to reload the daemon when unit files are changed or extended. This +feature is used automatically by the service state and the systemd_service +module when running on a systemd minion, so there is no need to set up your own +methods of reloading the daemon. If you need to manually reload the daemon for +some reason, you can use the :func:`systemd_service.systemctl_reload ` function provided by Salt. + .. note:: The current status of a service is determined by the return code of the init/rc script status command. A status return code of 0 it is considered running. Any From 31bd68ebb2c0324793152ec64dea71ca4562177e Mon Sep 17 00:00:00 2001 From: James Howe <675056+OrangeDog@users.noreply.github.com> Date: Mon, 6 Mar 2023 12:45:55 +0000 Subject: [PATCH 02/37] Change saltenv description I don't know what it's trying to say about "isolation" and it confuses a lot of people. (cherry picked from commit 52e795a0f52f9dd025c15a2fae1cf379a97f9d29) --- doc/ref/configuration/minion.rst | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/doc/ref/configuration/minion.rst b/doc/ref/configuration/minion.rst index 57af5ce4a3a..c81ec31c886 100644 --- a/doc/ref/configuration/minion.rst +++ b/doc/ref/configuration/minion.rst @@ -2438,10 +2438,7 @@ enabled and can be disabled by changing this value to ``False``. ``saltenv`` will take its value. If both are used, ``environment`` will be ignored and ``saltenv`` will be used. -Normally the minion is not isolated to any single environment on the master -when running states, but the environment can be isolated on the minion side -by statically setting it. Remember that the recommended way to manage -environments is to isolate via the top file. +The default fileserver environment to use when copy files and applying states. .. code-block:: yaml From fecf5b16b3877586c506a3dd50e4df9bd3c92583 Mon Sep 17 00:00:00 2001 From: Pedro Algarvio Date: Mon, 24 Apr 2023 19:46:54 +0100 Subject: [PATCH 03/37] Grammar fix Signed-off-by: Pedro Algarvio --- doc/ref/configuration/minion.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/ref/configuration/minion.rst b/doc/ref/configuration/minion.rst index c81ec31c886..69c7afbde84 100644 --- a/doc/ref/configuration/minion.rst +++ b/doc/ref/configuration/minion.rst @@ -2438,7 +2438,7 @@ enabled and can be disabled by changing this value to ``False``. ``saltenv`` will take its value. If both are used, ``environment`` will be ignored and ``saltenv`` will be used. -The default fileserver environment to use when copy files and applying states. +The default fileserver environment to use when copying files and applying states. .. code-block:: yaml From 005d0550f8e20df64e0c40c5ec5a09725cdf23e4 Mon Sep 17 00:00:00 2001 From: joshmcorreia <86431308+joshmcorreia@users.noreply.github.com> Date: Wed, 15 Feb 2023 09:40:15 -0800 Subject: [PATCH 04/37] Fix typo "occurence" -> "occurrence" (cherry picked from commit 5b152e5979f48b8fa2f9c158f8a5e23dd33dc0ea) --- salt/modules/ps.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/salt/modules/ps.py b/salt/modules/ps.py index ab307e98b53..7a3e247e2d6 100644 --- a/salt/modules/ps.py +++ b/salt/modules/ps.py @@ -768,7 +768,7 @@ def psaux(name): if not salt_exception_pattern.search(info): nb_lines += 1 found_infos.append(info) - pid_count = str(nb_lines) + " occurence(s)." + pid_count = str(nb_lines) + " occurrence(s)." ret = [] ret.extend([sanitize_name, found_infos, pid_count]) return ret From 2771e9aae2d2e5917654204b88fb68eb7c3f6d25 Mon Sep 17 00:00:00 2001 From: Timothy Warren Date: Fri, 10 Feb 2023 13:48:56 -0500 Subject: [PATCH 05/37] Fix typo (cherry picked from commit f34f21c517e2b4772e62196327778d264b602ed6) --- CONTRIBUTING.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CONTRIBUTING.rst b/CONTRIBUTING.rst index cd24612df49..42255742eb0 100644 --- a/CONTRIBUTING.rst +++ b/CONTRIBUTING.rst @@ -563,7 +563,7 @@ But that advice is backwards for the changelog. We follow the our changelog, and use towncrier to generate it for each release. As a contributor, all that means is that you need to add a file to the ``salt/changelog`` directory, using the ``.`` format. For -instanch, if you fixed issue 123, you would do: +instance, if you fixed issue 123, you would do: :: From 4112f05b771d52e271f6e7ba656ab1c3d27f72c9 Mon Sep 17 00:00:00 2001 From: Pedro Algarvio Date: Mon, 24 Apr 2023 15:50:46 +0100 Subject: [PATCH 06/37] Bump to `sqlparse>=0.4.4` due to https://github.com/advisories/GHSA-rrm6-wvj7-cwh2 Signed-off-by: Pedro Algarvio --- requirements/static/ci/common.in | 2 +- requirements/static/ci/py3.10/cloud.txt | 2 +- requirements/static/ci/py3.10/darwin.txt | 2 +- requirements/static/ci/py3.10/freebsd.txt | 2 +- requirements/static/ci/py3.10/lint.txt | 2 +- requirements/static/ci/py3.10/linux.txt | 2 +- requirements/static/ci/py3.10/windows.txt | 2 +- requirements/static/ci/py3.7/cloud.txt | 2 +- requirements/static/ci/py3.7/freebsd.txt | 2 +- requirements/static/ci/py3.7/lint.txt | 2 +- requirements/static/ci/py3.7/linux.txt | 2 +- requirements/static/ci/py3.7/windows.txt | 2 +- requirements/static/ci/py3.8/cloud.txt | 2 +- requirements/static/ci/py3.8/freebsd.txt | 2 +- requirements/static/ci/py3.8/lint.txt | 2 +- requirements/static/ci/py3.8/linux.txt | 2 +- requirements/static/ci/py3.8/windows.txt | 2 +- requirements/static/ci/py3.9/cloud.txt | 2 +- requirements/static/ci/py3.9/darwin.txt | 2 +- requirements/static/ci/py3.9/freebsd.txt | 2 +- requirements/static/ci/py3.9/lint.txt | 2 +- requirements/static/ci/py3.9/linux.txt | 2 +- requirements/static/ci/py3.9/windows.txt | 2 +- 23 files changed, 23 insertions(+), 23 deletions(-) diff --git a/requirements/static/ci/common.in b/requirements/static/ci/common.in index 590410cb3d0..addf9a7bafd 100644 --- a/requirements/static/ci/common.in +++ b/requirements/static/ci/common.in @@ -36,7 +36,7 @@ python-etcd>0.4.2 pyvmomi requests rfc3987 -sqlparse>=0.4.2 +sqlparse>=0.4.4 strict_rfc3339>=0.7 toml vcert~=0.7.0; sys_platform != 'win32' diff --git a/requirements/static/ci/py3.10/cloud.txt b/requirements/static/ci/py3.10/cloud.txt index 62633bf32e6..3fc7facf84f 100644 --- a/requirements/static/ci/py3.10/cloud.txt +++ b/requirements/static/ci/py3.10/cloud.txt @@ -838,7 +838,7 @@ smbprotocol==1.10.1 # pypsexec smmap==4.0.0 # via gitdb -sqlparse==0.4.2 +sqlparse==0.4.4 # via -r requirements/static/ci/common.in strict-rfc3339==0.7 # via -r requirements/static/ci/common.in diff --git a/requirements/static/ci/py3.10/darwin.txt b/requirements/static/ci/py3.10/darwin.txt index 3d845d2ebbc..25f965b961b 100644 --- a/requirements/static/ci/py3.10/darwin.txt +++ b/requirements/static/ci/py3.10/darwin.txt @@ -820,7 +820,7 @@ six==1.16.0 # websocket-client smmap==3.0.2 # via gitdb -sqlparse==0.4.2 +sqlparse==0.4.4 # via -r requirements/static/ci/common.in strict-rfc3339==0.7 # via -r requirements/static/ci/common.in diff --git a/requirements/static/ci/py3.10/freebsd.txt b/requirements/static/ci/py3.10/freebsd.txt index 1dc6b98462d..dabffdb3686 100644 --- a/requirements/static/ci/py3.10/freebsd.txt +++ b/requirements/static/ci/py3.10/freebsd.txt @@ -820,7 +820,7 @@ six==1.16.0 # websocket-client smmap==3.0.4 # via gitdb -sqlparse==0.4.2 +sqlparse==0.4.4 # via -r requirements/static/ci/common.in strict-rfc3339==0.7 # via -r requirements/static/ci/common.in diff --git a/requirements/static/ci/py3.10/lint.txt b/requirements/static/ci/py3.10/lint.txt index a3a1f55d398..b3552664df1 100644 --- a/requirements/static/ci/py3.10/lint.txt +++ b/requirements/static/ci/py3.10/lint.txt @@ -808,7 +808,7 @@ slack-sdk==3.19.5 # via slack-bolt smmap==4.0.0 # via gitdb -sqlparse==0.4.2 +sqlparse==0.4.4 # via -r requirements/static/ci/common.in strict-rfc3339==0.7 # via -r requirements/static/ci/common.in diff --git a/requirements/static/ci/py3.10/linux.txt b/requirements/static/ci/py3.10/linux.txt index 9bad6a4db40..ce83b819418 100644 --- a/requirements/static/ci/py3.10/linux.txt +++ b/requirements/static/ci/py3.10/linux.txt @@ -862,7 +862,7 @@ slack-sdk==3.19.5 # via slack-bolt smmap==3.0.4 # via gitdb -sqlparse==0.4.2 +sqlparse==0.4.4 # via -r requirements/static/ci/common.in strict-rfc3339==0.7 # via -r requirements/static/ci/common.in diff --git a/requirements/static/ci/py3.10/windows.txt b/requirements/static/ci/py3.10/windows.txt index 610ddbd8363..3f1db8377e8 100644 --- a/requirements/static/ci/py3.10/windows.txt +++ b/requirements/static/ci/py3.10/windows.txt @@ -369,7 +369,7 @@ six==1.15.0 # websocket-client smmap==4.0.0 # via gitdb -sqlparse==0.4.2 +sqlparse==0.4.4 # via -r requirements/static/ci/common.in strict-rfc3339==0.7 # via -r requirements/static/ci/common.in diff --git a/requirements/static/ci/py3.7/cloud.txt b/requirements/static/ci/py3.7/cloud.txt index 391c0d421a9..3a99b696d2b 100644 --- a/requirements/static/ci/py3.7/cloud.txt +++ b/requirements/static/ci/py3.7/cloud.txt @@ -888,7 +888,7 @@ smbprotocol==1.10.1 # pypsexec smmap==4.0.0 # via gitdb -sqlparse==0.4.2 +sqlparse==0.4.4 # via -r requirements/static/ci/common.in strict-rfc3339==0.7 # via -r requirements/static/ci/common.in diff --git a/requirements/static/ci/py3.7/freebsd.txt b/requirements/static/ci/py3.7/freebsd.txt index b09283b6b71..573134b8435 100644 --- a/requirements/static/ci/py3.7/freebsd.txt +++ b/requirements/static/ci/py3.7/freebsd.txt @@ -864,7 +864,7 @@ six==1.16.0 # websocket-client smmap==3.0.4 # via gitdb -sqlparse==0.4.2 +sqlparse==0.4.4 # via -r requirements/static/ci/common.in strict-rfc3339==0.7 # via -r requirements/static/ci/common.in diff --git a/requirements/static/ci/py3.7/lint.txt b/requirements/static/ci/py3.7/lint.txt index 56ded064f0b..ec6face4f99 100644 --- a/requirements/static/ci/py3.7/lint.txt +++ b/requirements/static/ci/py3.7/lint.txt @@ -859,7 +859,7 @@ slack-sdk==3.19.5 # via slack-bolt smmap==4.0.0 # via gitdb -sqlparse==0.4.2 +sqlparse==0.4.4 # via -r requirements/static/ci/common.in strict-rfc3339==0.7 # via -r requirements/static/ci/common.in diff --git a/requirements/static/ci/py3.7/linux.txt b/requirements/static/ci/py3.7/linux.txt index c2b36fdb36b..ff0ca6be36f 100644 --- a/requirements/static/ci/py3.7/linux.txt +++ b/requirements/static/ci/py3.7/linux.txt @@ -908,7 +908,7 @@ slack-sdk==3.19.5 # via slack-bolt smmap==3.0.4 # via gitdb -sqlparse==0.4.2 +sqlparse==0.4.4 # via -r requirements/static/ci/common.in strict-rfc3339==0.7 # via -r requirements/static/ci/common.in diff --git a/requirements/static/ci/py3.7/windows.txt b/requirements/static/ci/py3.7/windows.txt index 4596f2f2a17..b43b83d8678 100644 --- a/requirements/static/ci/py3.7/windows.txt +++ b/requirements/static/ci/py3.7/windows.txt @@ -384,7 +384,7 @@ six==1.15.0 # websocket-client smmap==4.0.0 # via gitdb -sqlparse==0.4.2 +sqlparse==0.4.4 # via -r requirements/static/ci/common.in strict-rfc3339==0.7 # via -r requirements/static/ci/common.in diff --git a/requirements/static/ci/py3.8/cloud.txt b/requirements/static/ci/py3.8/cloud.txt index 2d85b1fd4b3..7471be1d614 100644 --- a/requirements/static/ci/py3.8/cloud.txt +++ b/requirements/static/ci/py3.8/cloud.txt @@ -877,7 +877,7 @@ smbprotocol==1.10.1 # pypsexec smmap==4.0.0 # via gitdb -sqlparse==0.4.2 +sqlparse==0.4.4 # via -r requirements/static/ci/common.in strict-rfc3339==0.7 # via -r requirements/static/ci/common.in diff --git a/requirements/static/ci/py3.8/freebsd.txt b/requirements/static/ci/py3.8/freebsd.txt index d7db453ff39..71ded2f0ecb 100644 --- a/requirements/static/ci/py3.8/freebsd.txt +++ b/requirements/static/ci/py3.8/freebsd.txt @@ -854,7 +854,7 @@ six==1.16.0 # websocket-client smmap==3.0.4 # via gitdb -sqlparse==0.4.2 +sqlparse==0.4.4 # via -r requirements/static/ci/common.in strict-rfc3339==0.7 # via -r requirements/static/ci/common.in diff --git a/requirements/static/ci/py3.8/lint.txt b/requirements/static/ci/py3.8/lint.txt index b9112a90142..f4d6e66b243 100644 --- a/requirements/static/ci/py3.8/lint.txt +++ b/requirements/static/ci/py3.8/lint.txt @@ -850,7 +850,7 @@ slack-sdk==3.19.5 # via slack-bolt smmap==4.0.0 # via gitdb -sqlparse==0.4.2 +sqlparse==0.4.4 # via -r requirements/static/ci/common.in strict-rfc3339==0.7 # via -r requirements/static/ci/common.in diff --git a/requirements/static/ci/py3.8/linux.txt b/requirements/static/ci/py3.8/linux.txt index c63fbc126d7..dff8d779093 100644 --- a/requirements/static/ci/py3.8/linux.txt +++ b/requirements/static/ci/py3.8/linux.txt @@ -896,7 +896,7 @@ slack-sdk==3.19.5 # via slack-bolt smmap==3.0.4 # via gitdb -sqlparse==0.4.2 +sqlparse==0.4.4 # via -r requirements/static/ci/common.in strict-rfc3339==0.7 # via -r requirements/static/ci/common.in diff --git a/requirements/static/ci/py3.8/windows.txt b/requirements/static/ci/py3.8/windows.txt index 12c6e3dfe4d..4dca1d45370 100644 --- a/requirements/static/ci/py3.8/windows.txt +++ b/requirements/static/ci/py3.8/windows.txt @@ -372,7 +372,7 @@ six==1.15.0 # websocket-client smmap==4.0.0 # via gitdb -sqlparse==0.4.2 +sqlparse==0.4.4 # via -r requirements/static/ci/common.in strict-rfc3339==0.7 # via -r requirements/static/ci/common.in diff --git a/requirements/static/ci/py3.9/cloud.txt b/requirements/static/ci/py3.9/cloud.txt index c9f7786a00f..e0f1d9f3189 100644 --- a/requirements/static/ci/py3.9/cloud.txt +++ b/requirements/static/ci/py3.9/cloud.txt @@ -880,7 +880,7 @@ smbprotocol==1.10.1 # pypsexec smmap==4.0.0 # via gitdb -sqlparse==0.4.2 +sqlparse==0.4.4 # via -r requirements/static/ci/common.in strict-rfc3339==0.7 # via -r requirements/static/ci/common.in diff --git a/requirements/static/ci/py3.9/darwin.txt b/requirements/static/ci/py3.9/darwin.txt index daa71678906..59dc17fefc3 100644 --- a/requirements/static/ci/py3.9/darwin.txt +++ b/requirements/static/ci/py3.9/darwin.txt @@ -857,7 +857,7 @@ six==1.16.0 # websocket-client smmap==3.0.2 # via gitdb -sqlparse==0.4.2 +sqlparse==0.4.4 # via -r requirements/static/ci/common.in strict-rfc3339==0.7 # via -r requirements/static/ci/common.in diff --git a/requirements/static/ci/py3.9/freebsd.txt b/requirements/static/ci/py3.9/freebsd.txt index 5c01bd43804..950f0afd887 100644 --- a/requirements/static/ci/py3.9/freebsd.txt +++ b/requirements/static/ci/py3.9/freebsd.txt @@ -857,7 +857,7 @@ six==1.16.0 # websocket-client smmap==3.0.4 # via gitdb -sqlparse==0.4.2 +sqlparse==0.4.4 # via -r requirements/static/ci/common.in strict-rfc3339==0.7 # via -r requirements/static/ci/common.in diff --git a/requirements/static/ci/py3.9/lint.txt b/requirements/static/ci/py3.9/lint.txt index 5cc37808c9d..5509a1c5feb 100644 --- a/requirements/static/ci/py3.9/lint.txt +++ b/requirements/static/ci/py3.9/lint.txt @@ -851,7 +851,7 @@ slack-sdk==3.19.5 # via slack-bolt smmap==4.0.0 # via gitdb -sqlparse==0.4.2 +sqlparse==0.4.4 # via -r requirements/static/ci/common.in strict-rfc3339==0.7 # via -r requirements/static/ci/common.in diff --git a/requirements/static/ci/py3.9/linux.txt b/requirements/static/ci/py3.9/linux.txt index 050570b49dc..3461c0c3808 100644 --- a/requirements/static/ci/py3.9/linux.txt +++ b/requirements/static/ci/py3.9/linux.txt @@ -901,7 +901,7 @@ slack-sdk==3.19.5 # via slack-bolt smmap==3.0.4 # via gitdb -sqlparse==0.4.2 +sqlparse==0.4.4 # via -r requirements/static/ci/common.in strict-rfc3339==0.7 # via -r requirements/static/ci/common.in diff --git a/requirements/static/ci/py3.9/windows.txt b/requirements/static/ci/py3.9/windows.txt index 053437ca906..e70f94b0c9b 100644 --- a/requirements/static/ci/py3.9/windows.txt +++ b/requirements/static/ci/py3.9/windows.txt @@ -373,7 +373,7 @@ six==1.15.0 # websocket-client smmap==4.0.0 # via gitdb -sqlparse==0.4.2 +sqlparse==0.4.4 # via -r requirements/static/ci/common.in strict-rfc3339==0.7 # via -r requirements/static/ci/common.in From b420772dfd99dbc36ca45a9c07f7798aaa95eab8 Mon Sep 17 00:00:00 2001 From: joshmcorreia <86431308+joshmcorreia@users.noreply.github.com> Date: Fri, 10 Feb 2023 10:33:14 -0800 Subject: [PATCH 07/37] Fix incorrect reactor target The documentation has an incorrect reactor target The current documentation will cause the following error ``` jinja2.exceptions.UndefinedError: 'dict object' has no attribute 'data' ``` (cherry picked from commit 639ba5613e859c03ef4ab01515e8452bc551dc9e) --- doc/topics/beacons/index.rst | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/doc/topics/beacons/index.rst b/doc/topics/beacons/index.rst index 0c8cde04cf6..311c8549b46 100644 --- a/doc/topics/beacons/index.rst +++ b/doc/topics/beacons/index.rst @@ -246,7 +246,7 @@ Add the following to ``/srv/reactor/revert.sls``: revert-file: local.state.apply: - - tgt: {{ data['data']['id'] }} + - tgt: {{ data['id'] }} - arg: - maintain_important_file @@ -257,12 +257,6 @@ Add the following to ``/srv/reactor/revert.sls``: to modify the watched file, it is important to ensure the state applied is also :term:`idempotent `. -.. note:: - - The expression ``{{ data['data']['id'] }}`` :ref:`is correct - ` as it matches the event structure :ref:`shown above - `. - State SLS ````````` From 62e438a4b650367a03577d964cfd4768d3938242 Mon Sep 17 00:00:00 2001 From: nicholasmhughes Date: Mon, 24 Apr 2023 14:53:22 -0400 Subject: [PATCH 08/37] fixes saltstack/salt#64150 cmd.run doesn't output changes in test mode --- changelog/64150.fixed.md | 1 + salt/states/cmd.py | 1 + tests/pytests/unit/states/test_cmd.py | 4 +++- 3 files changed, 5 insertions(+), 1 deletion(-) create mode 100644 changelog/64150.fixed.md diff --git a/changelog/64150.fixed.md b/changelog/64150.fixed.md new file mode 100644 index 00000000000..a767e10bf8d --- /dev/null +++ b/changelog/64150.fixed.md @@ -0,0 +1 @@ +Fix cmd.run doesn't output changes in test mode diff --git a/salt/states/cmd.py b/salt/states/cmd.py index 2df04807ce3..5a859c8092c 100644 --- a/salt/states/cmd.py +++ b/salt/states/cmd.py @@ -849,6 +849,7 @@ def run( if __opts__["test"] and not test_name: ret["result"] = None ret["comment"] = 'Command "{}" would have been executed'.format(name) + ret["changes"] = {"cmd": name} return _reinterpreted_state(ret) if stateful else ret if cwd and not os.path.isdir(cwd): diff --git a/tests/pytests/unit/states/test_cmd.py b/tests/pytests/unit/states/test_cmd.py index 682ee621e7d..4a839c2f91a 100644 --- a/tests/pytests/unit/states/test_cmd.py +++ b/tests/pytests/unit/states/test_cmd.py @@ -77,7 +77,9 @@ def test_run(): with patch.dict(cmd.__opts__, {"test": True}): comt = 'Command "cmd.script" would have been executed' - ret.update({"comment": comt, "result": None, "changes": {}}) + ret.update( + {"comment": comt, "result": None, "changes": {"cmd": "cmd.script"}} + ) assert cmd.run(name) == ret From 196023a086f3e9c263f6e2b266e1b36e4bcc0c46 Mon Sep 17 00:00:00 2001 From: Thomas Phipps Date: Wed, 26 Apr 2023 00:21:49 +0000 Subject: [PATCH 09/37] fix #64082 --- changelog/64082.fixed.md | 1 + salt/modules/cryptdev.py | 2 +- tests/pytests/unit/modules/test_cryptdev.py | 38 +++++++++++++++++++++ 3 files changed, 40 insertions(+), 1 deletion(-) create mode 100644 changelog/64082.fixed.md create mode 100644 tests/pytests/unit/modules/test_cryptdev.py diff --git a/changelog/64082.fixed.md b/changelog/64082.fixed.md new file mode 100644 index 00000000000..c5bbc5a0ccb --- /dev/null +++ b/changelog/64082.fixed.md @@ -0,0 +1 @@ +Fix dmsetup device names with hyphen being picked up. diff --git a/salt/modules/cryptdev.py b/salt/modules/cryptdev.py index f4d24e5227f..40c28d17f10 100644 --- a/salt/modules/cryptdev.py +++ b/salt/modules/cryptdev.py @@ -113,7 +113,7 @@ def active(): ret = {} # TODO: This command should be extended to collect more information, such as UUID. devices = __salt__["cmd.run_stdout"]("dmsetup ls --target crypt") - out_regex = re.compile(r"(?P\w+)\W+\((?P\d+), (?P\d+)\)") + out_regex = re.compile(r"(?P\S+)\s+\((?P\d+), (?P\d+)\)") log.debug(devices) for line in devices.split("\n"): diff --git a/tests/pytests/unit/modules/test_cryptdev.py b/tests/pytests/unit/modules/test_cryptdev.py new file mode 100644 index 00000000000..c5932b5f369 --- /dev/null +++ b/tests/pytests/unit/modules/test_cryptdev.py @@ -0,0 +1,38 @@ +import pytest + +import salt.modules.cryptdev as cryptdev +from tests.support.mock import MagicMock, patch + + +@pytest.fixture +def configure_loader_modules(minion_opts): + return {cryptdev: {"__opts__": minion_opts}} + + +def test_active(caplog): + with patch.dict( + cryptdev.__salt__, + {"cmd.run_stdout": MagicMock(return_value="my-device (253, 1)\n")}, + ): + assert cryptdev.active() == { + "my-device": { + "devname": "my-device", + "major": "253", + "minor": "1", + } + } + + # debien output when no devices setup. + with patch.dict(cryptdev.__salt__, {"cmd.run_stdout": MagicMock(return_value="")}): + caplog.clear() + assert cryptdev.active() == {} + assert "dmsetup output does not match expected format" in caplog.text + + # centos output of dmsetup when no devices setup. + with patch.dict( + cryptdev.__salt__, + {"cmd.run_stdout": MagicMock(return_value="No devices found")}, + ): + caplog.clear() + assert cryptdev.active() == {} + assert "dmsetup output does not match expected format" in caplog.text From a1f54d9f2798716251bc338feb7c8484a54fcbe3 Mon Sep 17 00:00:00 2001 From: David Murphy < dmurphy@saltstack.com> Date: Tue, 11 Apr 2023 19:07:38 -0600 Subject: [PATCH 10/37] Check NamedLoaderContext obj is not None, before creating a dictionary --- changelog/62477.fixed.md | 1 + salt/payload.py | 5 +++-- 2 files changed, 4 insertions(+), 2 deletions(-) create mode 100644 changelog/62477.fixed.md diff --git a/changelog/62477.fixed.md b/changelog/62477.fixed.md new file mode 100644 index 00000000000..0a42b40725e --- /dev/null +++ b/changelog/62477.fixed.md @@ -0,0 +1 @@ +Issue #62477: Check NamedLoadedContext mapping is not None, before attempting to create a dictory with it diff --git a/salt/payload.py b/salt/payload.py index 790e4cf2f6f..989a3111802 100644 --- a/salt/payload.py +++ b/salt/payload.py @@ -4,7 +4,6 @@ encrypted keys to general payload dynamics and packaging, these happen in here """ -import collections.abc import datetime import gc import logging @@ -164,7 +163,9 @@ def dumps(msg, use_bin_type=False): return tuple(obj) elif isinstance(obj, CaseInsensitiveDict): return dict(obj) - elif isinstance(obj, collections.abc.MutableMapping): + elif isinstance(obj, salt.loader.context.NamedLoaderContext): + if obj.value() is None: + return {} return dict(obj) # Nothing known exceptions found. Let msgpack raise its own. return obj From 8c60ddb291ad891185b69886dfa9edb2230533f8 Mon Sep 17 00:00:00 2001 From: David Murphy < dmurphy@saltstack.com> Date: Wed, 12 Apr 2023 08:40:52 -0600 Subject: [PATCH 11/37] Updated as per reviewers comments --- changelog/62477.fixed.md | 2 +- salt/payload.py | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/changelog/62477.fixed.md b/changelog/62477.fixed.md index 0a42b40725e..118acea47aa 100644 --- a/changelog/62477.fixed.md +++ b/changelog/62477.fixed.md @@ -1 +1 @@ -Issue #62477: Check NamedLoadedContext mapping is not None, before attempting to create a dictory with it +Check NamedLoadedContext mapping is not None, before attempting to create a dictory with it diff --git a/salt/payload.py b/salt/payload.py index 989a3111802..c766e46c37f 100644 --- a/salt/payload.py +++ b/salt/payload.py @@ -4,6 +4,7 @@ encrypted keys to general payload dynamics and packaging, these happen in here """ +import collections.abc import datetime import gc import logging @@ -15,6 +16,7 @@ import salt.utils.msgpack import salt.utils.stringutils from salt.defaults import _Constant from salt.exceptions import SaltDeserializationError, SaltReqTimeoutError +from salt.loader.context import NamedLoaderContext from salt.utils.data import CaseInsensitiveDict try: @@ -163,10 +165,12 @@ def dumps(msg, use_bin_type=False): return tuple(obj) elif isinstance(obj, CaseInsensitiveDict): return dict(obj) - elif isinstance(obj, salt.loader.context.NamedLoaderContext): + elif isinstance(obj, NamedLoaderContext): if obj.value() is None: return {} return dict(obj) + elif isinstance(obj, collections.abc.MutableMapping): + return dict(obj) # Nothing known exceptions found. Let msgpack raise its own. return obj From 25e9a5c706b2c0e7228649a5f7e716beb22cfd54 Mon Sep 17 00:00:00 2001 From: David Murphy < dmurphy@saltstack.com> Date: Wed, 12 Apr 2023 18:59:33 -0600 Subject: [PATCH 12/37] Updated check and return if obj.value() if dict else empty dictionary --- changelog/62477.fixed.md | 2 +- salt/loader/context.py | 2 +- salt/payload.py | 5 +++-- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/changelog/62477.fixed.md b/changelog/62477.fixed.md index 118acea47aa..c50d97060ad 100644 --- a/changelog/62477.fixed.md +++ b/changelog/62477.fixed.md @@ -1 +1 @@ -Check NamedLoadedContext mapping is not None, before attempting to create a dictory with it +Check NamedLoadedContext mapping is not None, before attempting to create a dictionary with it diff --git a/salt/loader/context.py b/salt/loader/context.py index cc45441c96b..86f879cc592 100644 --- a/salt/loader/context.py +++ b/salt/loader/context.py @@ -32,7 +32,7 @@ def loader_context(loader): class NamedLoaderContext(collections.abc.MutableMapping): """ A NamedLoaderContext object is injected by the loader providing access to - Salt's 'magic dunders' (__salt__, __utils__, ect). + Salt's 'magic dunders' (__salt__, __utils__, etc). """ def __init__(self, name, loader_context, default=None): diff --git a/salt/payload.py b/salt/payload.py index c766e46c37f..59f2e821fde 100644 --- a/salt/payload.py +++ b/salt/payload.py @@ -166,9 +166,10 @@ def dumps(msg, use_bin_type=False): elif isinstance(obj, CaseInsensitiveDict): return dict(obj) elif isinstance(obj, NamedLoaderContext): - if obj.value() is None: + if isinstance(obj.value(), dict): + return obj.value() + else: return {} - return dict(obj) elif isinstance(obj, collections.abc.MutableMapping): return dict(obj) # Nothing known exceptions found. Let msgpack raise its own. From 44d5501388c04838c13d469a9c11846af601bd2a Mon Sep 17 00:00:00 2001 From: David Murphy < dmurphy@saltstack.com> Date: Tue, 18 Apr 2023 17:07:45 -0600 Subject: [PATCH 13/37] Updates to code to handle NamedLoaderContext with dunder globals, per reviewer comments --- salt/client/ssh/wrapper/state.py | 2 +- salt/modules/cmdmod.py | 2 +- salt/modules/cp.py | 2 +- salt/payload.py | 6 ------ 4 files changed, 3 insertions(+), 9 deletions(-) diff --git a/salt/client/ssh/wrapper/state.py b/salt/client/ssh/wrapper/state.py index 5bfc1ecd049..002853972ab 100644 --- a/salt/client/ssh/wrapper/state.py +++ b/salt/client/ssh/wrapper/state.py @@ -834,7 +834,7 @@ def show_highstate(**kwargs): opts = salt.utils.state.get_sls_opts(__opts__, **kwargs) with salt.client.ssh.state.SSHHighState( opts, - __pillar__, + __pillar__.value(), __salt__, __context__["fileclient"], context=__context__.value(), diff --git a/salt/modules/cmdmod.py b/salt/modules/cmdmod.py index 29d1c500782..4822971d552 100644 --- a/salt/modules/cmdmod.py +++ b/salt/modules/cmdmod.py @@ -217,7 +217,7 @@ def _gather_pillar(pillarenv, pillar_override): """ pillar = salt.pillar.get_pillar( __opts__, - __grains__, + __grains__.value(), __opts__["id"], __opts__["saltenv"], pillar_override=pillar_override, diff --git a/salt/modules/cp.py b/salt/modules/cp.py index e9ac77434e4..4c5396949fe 100644 --- a/salt/modules/cp.py +++ b/salt/modules/cp.py @@ -41,7 +41,7 @@ def _gather_pillar(pillarenv, pillar_override): """ pillar = salt.pillar.get_pillar( __opts__, - __grains__, + __grains__.value(), __opts__["id"], __opts__["saltenv"], pillar_override=pillar_override, diff --git a/salt/payload.py b/salt/payload.py index 59f2e821fde..790e4cf2f6f 100644 --- a/salt/payload.py +++ b/salt/payload.py @@ -16,7 +16,6 @@ import salt.utils.msgpack import salt.utils.stringutils from salt.defaults import _Constant from salt.exceptions import SaltDeserializationError, SaltReqTimeoutError -from salt.loader.context import NamedLoaderContext from salt.utils.data import CaseInsensitiveDict try: @@ -165,11 +164,6 @@ def dumps(msg, use_bin_type=False): return tuple(obj) elif isinstance(obj, CaseInsensitiveDict): return dict(obj) - elif isinstance(obj, NamedLoaderContext): - if isinstance(obj.value(), dict): - return obj.value() - else: - return {} elif isinstance(obj, collections.abc.MutableMapping): return dict(obj) # Nothing known exceptions found. Let msgpack raise its own. From f86e6a935f3036a34c689614a42bfb42c4a8a820 Mon Sep 17 00:00:00 2001 From: David Murphy < dmurphy@saltstack.com> Date: Tue, 18 Apr 2023 17:31:02 -0600 Subject: [PATCH 14/37] Further refinement of use of dunder globals --- salt/modules/cp.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/salt/modules/cp.py b/salt/modules/cp.py index 4c5396949fe..ec40643786b 100644 --- a/salt/modules/cp.py +++ b/salt/modules/cp.py @@ -185,8 +185,8 @@ def _render_filenames(path, dest, saltenv, template, **kw): pillarenv = kw.get("pillarenv", __opts__.get("pillarenv")) kwargs["pillar"] = _gather_pillar(pillarenv, kw.get("pillar")) else: - kwargs["pillar"] = __pillar__ - kwargs["grains"] = __grains__ + kwargs["pillar"] = __pillar__.value() + kwargs["grains"] = __grains__.value() kwargs["opts"] = __opts__ kwargs["saltenv"] = saltenv From fdf905da6b9f94f269fad82821d6bb59dfcb3f4c Mon Sep 17 00:00:00 2001 From: David Murphy < dmurphy@saltstack.com> Date: Tue, 18 Apr 2023 17:33:27 -0600 Subject: [PATCH 15/37] Updated changelog to reflect reviewer changes --- changelog/62477.fixed.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog/62477.fixed.md b/changelog/62477.fixed.md index c50d97060ad..88f47bdb4bd 100644 --- a/changelog/62477.fixed.md +++ b/changelog/62477.fixed.md @@ -1 +1 @@ -Check NamedLoadedContext mapping is not None, before attempting to create a dictionary with it +Ensure NamedLoaderContext's have their value() used if passing to other modules From c63920b2f1feb38b0e57766f4d507e26355f5bf3 Mon Sep 17 00:00:00 2001 From: David Murphy < dmurphy@saltstack.com> Date: Wed, 19 Apr 2023 19:50:36 -0600 Subject: [PATCH 16/37] Remove extra value() in cp.py, leaving it to pillar usage --- salt/modules/cp.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/salt/modules/cp.py b/salt/modules/cp.py index ec40643786b..4c5396949fe 100644 --- a/salt/modules/cp.py +++ b/salt/modules/cp.py @@ -185,8 +185,8 @@ def _render_filenames(path, dest, saltenv, template, **kw): pillarenv = kw.get("pillarenv", __opts__.get("pillarenv")) kwargs["pillar"] = _gather_pillar(pillarenv, kw.get("pillar")) else: - kwargs["pillar"] = __pillar__.value() - kwargs["grains"] = __grains__.value() + kwargs["pillar"] = __pillar__ + kwargs["grains"] = __grains__ kwargs["opts"] = __opts__ kwargs["saltenv"] = saltenv From b78d771454312901d534ddd05c978def6dc3347b Mon Sep 17 00:00:00 2001 From: David Murphy < dmurphy@saltstack.com> Date: Tue, 25 Apr 2023 18:11:17 -0600 Subject: [PATCH 17/37] Updated test --- .../integration/master/test_payload.py | 42 +++++++++++++++++++ 1 file changed, 42 insertions(+) create mode 100644 tests/pytests/integration/master/test_payload.py diff --git a/tests/pytests/integration/master/test_payload.py b/tests/pytests/integration/master/test_payload.py new file mode 100644 index 00000000000..e84e4278d8b --- /dev/null +++ b/tests/pytests/integration/master/test_payload.py @@ -0,0 +1,42 @@ +""" +Tests for payload +""" +import logging + +import pytest + +log = logging.getLogger(__name__) + + +@pytest.mark.slow_test +@pytest.mark.skip_if_not_root +@pytest.mark.skip_on_windows +@pytest.mark.skip_on_darwin +def test_payload_no_exception(salt_cli, salt_master, salt_minion): + """ + Test to confirm that no exception is thrown with the jinja file + when executed on the minion + """ + test_set_hostname = """ + {%- set host = pillar.get("hostname", "UNKNOWN") %} + {%- if host == 'UNKNOWN' %} + {{ raise("Unsupported UNKNOWN hostname") }} + {%- else %} + hostnamectl set-hostname {{ host }} + {%- endif %} + """ + with salt_master.state_tree.base.temp_file("set_hostname.j2", test_set_hostname): + + ret = salt_cli.run("test.ping", minion_tgt=salt_minion.id) + assert ret.returncode == 0 + assert ret.data is True + + ret = salt_cli.run( + "cmd.script", + "salt://set_hostname.j2", + "template=jinja", + pillar={"hostname": "test"}, + minion_tgt=salt_minion.id, + ) + log.warning(f"DGM output '{ret}'") + ## assert not ret.stdout.startswith("Authentication failure") From e25da80c7e988d7246953d1a7052bfefc0243976 Mon Sep 17 00:00:00 2001 From: David Murphy < dmurphy@saltstack.com> Date: Tue, 25 Apr 2023 18:25:03 -0600 Subject: [PATCH 18/37] Test to check for passing of dunder globals outside of execution modules --- tests/pytests/integration/master/test_payload.py | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/tests/pytests/integration/master/test_payload.py b/tests/pytests/integration/master/test_payload.py index e84e4278d8b..692005b5692 100644 --- a/tests/pytests/integration/master/test_payload.py +++ b/tests/pytests/integration/master/test_payload.py @@ -1,12 +1,8 @@ """ Tests for payload """ -import logging - import pytest -log = logging.getLogger(__name__) - @pytest.mark.slow_test @pytest.mark.skip_if_not_root @@ -38,5 +34,4 @@ def test_payload_no_exception(salt_cli, salt_master, salt_minion): pillar={"hostname": "test"}, minion_tgt=salt_minion.id, ) - log.warning(f"DGM output '{ret}'") - ## assert not ret.stdout.startswith("Authentication failure") + assert "AttributeError:" not in ret.stdout From ae7e0d1b1a4ceb2b2f109c04001f1e5ea0bb5207 Mon Sep 17 00:00:00 2001 From: cmcmarrow Date: Wed, 26 Apr 2023 15:06:51 -0500 Subject: [PATCH 19/37] flacky jail most have label --- .github/actions/get-pull-number/action.yml | 4 ++++ .github/workflows/test-action-macos.yml | 2 +- .github/workflows/test-action.yml | 2 +- 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/.github/actions/get-pull-number/action.yml b/.github/actions/get-pull-number/action.yml index 00fd0425aff..b4469e7b80a 100644 --- a/.github/actions/get-pull-number/action.yml +++ b/.github/actions/get-pull-number/action.yml @@ -28,6 +28,10 @@ runs: GITHUB_SHA: ${{ inputs.sha }} GITHUB_PULL_NUMBER: ${{ inputs.pull-number }} run: | + echo $GITHUB_OWNER + echo $GITHUB_REPO + echo $GITHUB_SHA + echo $GITHUB_PULL_NUMBER if [ -z "$GITHUB_PULL_NUMBER" ] then echo "Searching For Pull Number" diff --git a/.github/workflows/test-action-macos.yml b/.github/workflows/test-action-macos.yml index 971238966d1..81dcc99ba20 100644 --- a/.github/workflows/test-action-macos.yml +++ b/.github/workflows/test-action-macos.yml @@ -395,7 +395,7 @@ jobs: - name: Run Flaky Tests id: run-flaky-tests - if: ${{ fromJSON(inputs.testrun)['type'] != 'full' && steps.get-test-flags.outputs.flaky_jail_tests == 'false' }} + if: ${{ fromJSON(inputs.testrun)['type'] != 'full' && steps.get-test-flags.outputs.flaky_jail_tests == 'true' }} env: SKIP_REQUIREMENTS_INSTALL: "1" PRINT_TEST_SELECTION: "0" diff --git a/.github/workflows/test-action.yml b/.github/workflows/test-action.yml index b9ff40b3e3f..f7220b21d6e 100644 --- a/.github/workflows/test-action.yml +++ b/.github/workflows/test-action.yml @@ -381,7 +381,7 @@ jobs: - name: Run Flaky Tests id: run-flaky-tests - if: ${{ fromJSON(inputs.testrun)['type'] != 'full' && steps.get-test-flags.outputs.flaky_jail_tests == 'false' }} + if: ${{ fromJSON(inputs.testrun)['type'] != 'full' && steps.get-test-flags.outputs.flaky_jail_tests == 'true' }} run: | tools --timestamps --no-output-timeout-secs=1800 --timeout-secs=14400 vm test --skip-requirements-install \ --nox-session=${{ env.NOX_SESSION }} --rerun-failures ${{ inputs.distro-slug }} \ From 102b93a707467a9c59c9ef619e907e6c2dde07e8 Mon Sep 17 00:00:00 2001 From: cmcmarrow Date: Wed, 26 Apr 2023 15:22:20 -0500 Subject: [PATCH 20/37] red --- .github/actions/get-pull-number/action.yml | 4 ---- 1 file changed, 4 deletions(-) diff --git a/.github/actions/get-pull-number/action.yml b/.github/actions/get-pull-number/action.yml index b4469e7b80a..00fd0425aff 100644 --- a/.github/actions/get-pull-number/action.yml +++ b/.github/actions/get-pull-number/action.yml @@ -28,10 +28,6 @@ runs: GITHUB_SHA: ${{ inputs.sha }} GITHUB_PULL_NUMBER: ${{ inputs.pull-number }} run: | - echo $GITHUB_OWNER - echo $GITHUB_REPO - echo $GITHUB_SHA - echo $GITHUB_PULL_NUMBER if [ -z "$GITHUB_PULL_NUMBER" ] then echo "Searching For Pull Number" From 4d52af1336fca9f0323c817b77581f0ee60a23fc Mon Sep 17 00:00:00 2001 From: Twangboy Date: Wed, 26 Apr 2023 14:10:55 -0600 Subject: [PATCH 21/37] Join masters if it is a list --- changelog/64170.fixed.md | 2 + salt/utils/cloud.py | 27 +++++++++---- tests/pytests/unit/utils/test_cloud.py | 52 ++++++++++++++++++++++++++ 3 files changed, 73 insertions(+), 8 deletions(-) create mode 100644 changelog/64170.fixed.md diff --git a/changelog/64170.fixed.md b/changelog/64170.fixed.md new file mode 100644 index 00000000000..1d20355bf1e --- /dev/null +++ b/changelog/64170.fixed.md @@ -0,0 +1,2 @@ +Fixed issue in salt-cloud so that multiple masters specified in the cloud +are written to the minion config properly diff --git a/salt/utils/cloud.py b/salt/utils/cloud.py index 9edf006c299..841811dcdfe 100644 --- a/salt/utils/cloud.py +++ b/salt/utils/cloud.py @@ -1202,6 +1202,16 @@ def wait_for_passwd( time.sleep(trysleep) +def _format_master_param(master): + """ + If the master is a list, we need to convert it to a comma delimited string + Otherwise, we just return master + """ + if isinstance(master, list): + return ",".join(master) + return master + + def deploy_windows( host, port=445, @@ -1337,17 +1347,18 @@ def deploy_windows( conn=smb_conn, ) + cmd = "c:\\salttemp\\{}".format(installer) + args = [ + "/S", + "/master={}".format(_format_master_param(master)), + "/minion-name={}".format(name), + ] + if use_winrm: - winrm_cmd( - winrm_session, - "c:\\salttemp\\{}".format(installer), - ["/S", "/master={}".format(master), "/minion-name={}".format(name)], - ) + winrm_cmd(winrm_session, cmd, args) else: - cmd = "c:\\salttemp\\{}".format(installer) - args = "/S /master={} /minion-name={}".format(master, name) stdout, stderr, ret_code = run_psexec_command( - cmd, args, host, username, password + cmd, " ".join(args), host, username, password ) if ret_code != 0: diff --git a/tests/pytests/unit/utils/test_cloud.py b/tests/pytests/unit/utils/test_cloud.py index 550b63c9740..db9d258d399 100644 --- a/tests/pytests/unit/utils/test_cloud.py +++ b/tests/pytests/unit/utils/test_cloud.py @@ -605,3 +605,55 @@ def test_deploy_script_ssh_timeout(): ssh_kwargs = root_cmd.call_args.kwargs assert "ssh_timeout" in ssh_kwargs assert ssh_kwargs["ssh_timeout"] == 34 + + +@pytest.mark.parametrize( + "master,expected", + [ + (None, None), + ("single_master", "single_master"), + (["master1", "master2", "master3"], "master1,master2,master3"), + ], +) +def test__format_master_param(master, expected): + result = cloud._format_master_param(master) + assert result == expected + + +@pytest.mark.skip_unless_on_windows(reason="Only applicable for Windows.") +@pytest.mark.parametrize( + "master,expected", + [ + (None, None), + ("single_master", "single_master"), + (["master1", "master2", "master3"], "master1,master2,master3"), + ], +) +def test_deploy_windows_master(master, expected): + """ + Test deploy_windows with master parameter + """ + mock_true = MagicMock(return_value=True) + mock_tuple = MagicMock(return_value=(0, 0, 0)) + with patch("salt.utils.smb.get_conn", MagicMock()), patch( + "salt.utils.smb.mkdirs", MagicMock() + ), patch("salt.utils.smb.put_file", MagicMock()), patch( + "salt.utils.smb.delete_file", MagicMock() + ), patch( + "salt.utils.smb.delete_directory", MagicMock() + ), patch( + "time.sleep", MagicMock() + ), patch.object( + cloud, "wait_for_port", mock_true + ), patch.object( + cloud, "fire_event", MagicMock() + ), patch.object( + cloud, "wait_for_psexecsvc", mock_true + ), patch.object( + cloud, "run_psexec_command", mock_tuple + ) as mock: + cloud.deploy_windows(host="test", win_installer="install.exe", master=master) + expected_cmd = "c:\\salttemp\\install.exe" + expected_args = "/S /master={} /minion-name=None".format(expected) + assert mock.call_args_list[0].args[0] == expected_cmd + assert mock.call_args_list[0].args[1] == expected_args From 54be72064db1b1a9d40c6a8c2d550c32e2a14aef Mon Sep 17 00:00:00 2001 From: Twangboy Date: Wed, 26 Apr 2023 18:53:03 -0600 Subject: [PATCH 22/37] Don't fail when registry.pol missing --- changelog/64126.fixed.md | 1 + salt/modules/win_lgpo_reg.py | 34 ++++++------ salt/states/win_lgpo_reg.py | 54 +++++++++---------- salt/utils/win_lgpo_reg.py | 17 ++++-- .../pytests/unit/modules/test_win_lgpo_reg.py | 24 ++++++--- .../pytests/unit/states/test_win_lgpo_reg.py | 24 +-------- 6 files changed, 74 insertions(+), 80 deletions(-) create mode 100644 changelog/64126.fixed.md diff --git a/changelog/64126.fixed.md b/changelog/64126.fixed.md new file mode 100644 index 00000000000..fb6cf7c46b4 --- /dev/null +++ b/changelog/64126.fixed.md @@ -0,0 +1 @@ +lgpo_reg.set_value now returns ``True`` on success instead of ``None`` diff --git a/salt/modules/win_lgpo_reg.py b/salt/modules/win_lgpo_reg.py index cc678549ae4..4052de62bd3 100644 --- a/salt/modules/win_lgpo_reg.py +++ b/salt/modules/win_lgpo_reg.py @@ -137,9 +137,10 @@ def write_reg_pol(data, policy_class="Machine"): Raises: SaltInvocationError: Invalid policy class + CommandExecutionError: On failure Returns: - None + bool: True if successful CLI Example: @@ -175,7 +176,6 @@ def get_value(key, v_name, policy_class="Machine"): file. Args: - key (str): The registry key where the value name resides v_name (str): The value name to retrieve @@ -228,7 +228,6 @@ def get_key(key, policy_class="Machine"): Get all the values set in a key in the ``Registry.pol`` file. Args: - key (str): The registry key where the values reside policy_class (str): The registry class to read from. Can be one of the @@ -278,7 +277,6 @@ def set_value( style policies. This is the equivalent of setting a policy to ``Enabled`` Args: - key (str): The registry key path v_name (str): The registry value name within the key @@ -305,14 +303,14 @@ def set_value( Default is ``Machine`` - Returns: - bool: ``True`` if successful, otherwise ``False`` - Raises: SaltInvocationError: Invalid policy_class SaltInvocationError: Invalid v_type SaltInvocationError: v_data doesn't match v_type + Returns: + bool: ``True`` if successful, otherwise ``False`` + CLI Example: .. code-block:: bash @@ -385,7 +383,7 @@ def set_value( write_reg_pol(pol_data) - salt.utils.win_reg.set_value( + return salt.utils.win_reg.set_value( hive=hive, key=key, vname=v_name, @@ -401,7 +399,6 @@ def disable_value(key, v_name, policy_class="machine"): to ``Disabled`` in the Group Policy editor (``gpedit.msc``) Args: - key (str): The registry key path v_name (str): The registry value name within the key @@ -415,13 +412,14 @@ def disable_value(key, v_name, policy_class="machine"): Default is ``Machine`` + Raises: + SaltInvocationError: Invalid policy_class + CommandExecutionError: On failure + Returns: bool: ``True`` if successful, otherwise ``False`` None: If already disabled - Raises: - SaltInvocationError: Invalid policy_class - CLI Example: .. code-block:: bash @@ -468,7 +466,7 @@ def disable_value(key, v_name, policy_class="machine"): write_reg_pol(pol_data) - salt.utils.win_reg.delete_value(hive=hive, key=key, vname=v_name) + return salt.utils.win_reg.delete_value(hive=hive, key=key, vname=v_name) def delete_value(key, v_name, policy_class="Machine"): @@ -478,7 +476,6 @@ def delete_value(key, v_name, policy_class="Machine"): ``Not Configured``. Args: - key (str): The registry key path v_name (str): The registry value name within the key @@ -492,13 +489,14 @@ def delete_value(key, v_name, policy_class="Machine"): Default is ``Machine`` + Raises: + SaltInvocationError: Invalid policy_class + CommandExecutionError: On failure + Returns: bool: ``True`` if successful, otherwise ``False`` None: Key/value not present - Raises: - SaltInvocationError: Invalid policy_class - CLI Example: .. code-block:: bash @@ -538,7 +536,7 @@ def delete_value(key, v_name, policy_class="Machine"): write_reg_pol(pol_data) - salt.utils.win_reg.delete_value(hive=hive, key=key, vname=v_name) + return salt.utils.win_reg.delete_value(hive=hive, key=key, vname=v_name) # This is for testing different settings and verifying that we are writing the diff --git a/salt/states/win_lgpo_reg.py b/salt/states/win_lgpo_reg.py index 01b10e4e610..7a514068acb 100644 --- a/salt/states/win_lgpo_reg.py +++ b/salt/states/win_lgpo_reg.py @@ -72,23 +72,6 @@ def __virtual__(): return __virtualname__ -def _format_changes(changes, key, v_name): - """ - Reformat the changes dictionary to group new and old together. - """ - new_changes = {"new": {}, "old": {}} - for item in changes: - if changes[item]["new"]: - new_changes["new"][item] = changes[item]["new"] - new_changes["new"]["key"] = key - new_changes["new"]["name"] = v_name - if changes[item]["old"]: - new_changes["old"][item] = changes[item]["old"] - new_changes["old"]["key"] = key - new_changes["old"]["name"] = v_name - return new_changes - - def value_present(name, key, v_data, v_type="REG_DWORD", policy_class="Machine"): r""" Ensure a registry setting is present in the Registry.pol file. @@ -170,12 +153,16 @@ def value_present(name, key, v_data, v_type="REG_DWORD", policy_class="Machine") key=key, v_name=name, policy_class=policy_class ) - changes = salt.utils.data.compare_dicts(old, new) + if str(new["data"]) == v_data and new["type"] == v_type: + ret["comment"] = "Registry.pol value has been set" + ret["result"] = True + else: + ret["comment"] = "Failed to set Registry.pol value" + + changes = salt.utils.data.recursive_diff(old, new) if changes: - ret["comment"] = "Registry.pol value has been set" - ret["changes"] = _format_changes(changes, key, name) - ret["result"] = True + ret["changes"] = changes return ret @@ -238,12 +225,16 @@ def value_disabled(name, key, policy_class="Machine"): key=key, v_name=name, policy_class=policy_class ) - changes = salt.utils.data.compare_dicts(old, new) + if "**del." in str(new["data"]) and new["type"] == "REG_SZ": + ret["comment"] = "Registry.pol value disabled" + ret["result"] = True + else: + ret["comment"] = "Failed to disable Registry.pol value" + + changes = salt.utils.data.recursive_diff(old, new) if changes: - ret["comment"] = "Registry.pol value enabled" - ret["changes"] = _format_changes(changes, key, name) - ret["result"] = True + ret["changes"] = changes return ret @@ -306,14 +297,17 @@ def value_absent(name, key, policy_class="Machine"): key=key, v_name=name, policy_class=policy_class ) - if new is None: + if not new: + ret["comment"] = "Registry.pol value deleted" + ret["result"] = True + # We're setting this here in case new is None new = {} + else: + ret["comment"] = "Failed to delete Registry.pol value" - changes = salt.utils.data.compare_dicts(old, new) + changes = salt.utils.data.recursive_diff(old, new) if changes: - ret["comment"] = "Registry.pol value deleted" - ret["changes"] = _format_changes(changes, key, name) - ret["result"] = True + ret["changes"] = changes return ret diff --git a/salt/utils/win_lgpo_reg.py b/salt/utils/win_lgpo_reg.py index da4c4377631..0a96d584df8 100644 --- a/salt/utils/win_lgpo_reg.py +++ b/salt/utils/win_lgpo_reg.py @@ -67,13 +67,11 @@ def search_reg_pol(search_string, policy_data): gpt.ini Args: - search_string (str): The string to search for policy_data (str): The data to be searched Returns: - bool: ``True`` if the regex search_string is found, otherwise ``False`` """ if policy_data: @@ -91,7 +89,6 @@ def read_reg_pol_file(reg_pol_path): Helper function to read the content of the Registry.pol file Args: - reg_pol_path (str): The path to the Registry.pol file Returns: @@ -120,7 +117,6 @@ def write_reg_pol_data( to be processed Args: - data_to_write (bytes): Data to write into the user/machine registry.pol file @@ -132,6 +128,12 @@ def write_reg_pol_data( gpt_extension_guid (str): ADMX registry extension guid for the class gpt_ini_path (str): The path to the gpt.ini file + + Returns: + bool: True if successful + + Raises: + CommandExecutionError: On failure """ # Write Registry.pol file if not os.path.exists(policy_file_path): @@ -254,6 +256,7 @@ def write_reg_pol_data( ) log.exception(msg) raise CommandExecutionError(msg) + return True def reg_pol_to_dict(policy_data): @@ -273,6 +276,12 @@ def reg_pol_to_dict(policy_data): # https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-gpreg/5c092c22-bf6b-4e7f-b180-b20743d368f5 reg_pol_header = REG_POL_HEADER.encode("utf-16-le") + + # If policy_data is None, that means the Registry.pol file is missing + # So, we'll create it + if policy_data is None: + policy_data = reg_pol_header + if not policy_data.startswith(reg_pol_header): msg = "LGPO_REG Util: Invalid Header. Registry.pol may be corrupt" raise CommandExecutionError(msg) diff --git a/tests/pytests/unit/modules/test_win_lgpo_reg.py b/tests/pytests/unit/modules/test_win_lgpo_reg.py index df0b4624eae..6d4a824b308 100644 --- a/tests/pytests/unit/modules/test_win_lgpo_reg.py +++ b/tests/pytests/unit/modules/test_win_lgpo_reg.py @@ -152,12 +152,16 @@ def test_get_key_invalid_policy_class(): def test_set_value(empty_reg_pol): - expected = {"data": 1, "type": "REG_DWORD"} key = "SOFTWARE\\MyKey" v_name = "MyValue" - lgpo_reg.set_value(key=key, v_name=v_name, v_data="1") + # Test command return + result = lgpo_reg.set_value(key=key, v_name=v_name, v_data="1") + assert result is True + # Test value actually set in Registry.pol + expected = {"data": 1, "type": "REG_DWORD"} result = lgpo_reg.get_value(key=key, v_name=v_name) assert result == expected + # Test that the registry value has been set expected = { "hive": "HKLM", "key": key, @@ -249,14 +253,18 @@ def test_set_value_invalid_reg_dword(): def test_disable_value(reg_pol): + key = "SOFTWARE\\MyKey1" + # Test that the command completed successfully + result = lgpo_reg.disable_value(key=key, v_name="MyValue1") + assert result is True + # Test that the value was actually set in Registry.pol expected = { "**del.MyValue1": {"data": " ", "type": "REG_SZ"}, "**del.MyValue2": {"data": " ", "type": "REG_SZ"}, } - key = "SOFTWARE\\MyKey1" - lgpo_reg.disable_value(key=key, v_name="MyValue1") result = lgpo_reg.get_key(key=key) assert result == expected + # Test that the registry value has been removed result = salt.utils.win_reg.value_exists(hive="HKLM", key=key, vname="MyValue1") assert result is False @@ -283,16 +291,20 @@ def test_disable_value_invalid_policy_class(): def test_delete_value_existing(reg_pol): + key = "SOFTWARE\\MyKey1" + # Test that the command completes successfully + result = lgpo_reg.delete_value(key=key, v_name="MyValue1") + assert result is True + # Test that the value is actually removed from Registry.pol expected = { "**del.MyValue2": { "data": " ", "type": "REG_SZ", }, } - key = "SOFTWARE\\MyKey1" - lgpo_reg.delete_value(key=key, v_name="MyValue1") result = lgpo_reg.get_key(key=key) assert result == expected + # Test that the registry entry has been removed result = salt.utils.win_reg.value_exists(hive="HKLM", key=key, vname="MyValue2") assert result is False diff --git a/tests/pytests/unit/states/test_win_lgpo_reg.py b/tests/pytests/unit/states/test_win_lgpo_reg.py index f57262869f4..d2ca5cc7433 100644 --- a/tests/pytests/unit/states/test_win_lgpo_reg.py +++ b/tests/pytests/unit/states/test_win_lgpo_reg.py @@ -84,8 +84,6 @@ def test_value_present(empty_reg_pol): expected = { "changes": { "new": { - "name": "MyValue", - "key": "SOFTWARE\\MyKey", "data": 1, "type": "REG_DWORD", }, @@ -111,14 +109,10 @@ def test_value_present_existing_change(reg_pol): expected = { "changes": { "new": { - "name": "MyValue1", - "key": "SOFTWARE\\MyKey1", "data": 2, "type": "REG_DWORD", }, "old": { - "name": "MyValue1", - "key": "SOFTWARE\\MyKey1", "data": "squidward", "type": "REG_SZ", }, @@ -183,14 +177,10 @@ def test_value_present_existing_disabled(reg_pol): "changes": { "new": { "data": 2, - "key": "SOFTWARE\\MyKey1", - "name": "MyValue2", "type": "REG_DWORD", }, "old": { "data": "**del.MyValue2", - "key": "SOFTWARE\\MyKey1", - "name": "MyValue2", "type": "REG_SZ", }, }, @@ -213,13 +203,11 @@ def test_value_disabled(empty_reg_pol): "changes": { "new": { "data": "**del.MyValue1", - "key": "SOFTWARE\\MyKey1", - "name": "MyValue1", "type": "REG_SZ", }, "old": {}, }, - "comment": "Registry.pol value enabled", + "comment": "Registry.pol value disabled", "name": "MyValue1", "result": True, } @@ -238,16 +226,12 @@ def test_value_disabled_existing_change(reg_pol): "changes": { "new": { "data": "**del.MyValue1", - "key": "SOFTWARE\\MyKey1", - "name": "MyValue1", }, "old": { "data": "squidward", - "key": "SOFTWARE\\MyKey1", - "name": "MyValue1", }, }, - "comment": "Registry.pol value enabled", + "comment": "Registry.pol value disabled", "name": "MyValue1", "result": True, } @@ -299,8 +283,6 @@ def test_value_absent(reg_pol): "new": {}, "old": { "data": "squidward", - "key": "SOFTWARE\\MyKey1", - "name": "MyValue1", "type": "REG_SZ", }, }, @@ -335,8 +317,6 @@ def test_value_absent_disabled(reg_pol): "new": {}, "old": { "data": "**del.MyValue2", - "key": "SOFTWARE\\MyKey1", - "name": "MyValue2", "type": "REG_SZ", }, }, From a099e92bdcc2461d16a5a6fa019c9964b988366b Mon Sep 17 00:00:00 2001 From: Thomas Phipps Date: Thu, 27 Apr 2023 17:39:41 +0000 Subject: [PATCH 23/37] fix #63589 --- changelog/63589.fixed.md | 1 + doc/topics/reactor/index.rst | 8 ++++++++ 2 files changed, 9 insertions(+) create mode 100644 changelog/63589.fixed.md diff --git a/changelog/63589.fixed.md b/changelog/63589.fixed.md new file mode 100644 index 00000000000..1f63f9ee993 --- /dev/null +++ b/changelog/63589.fixed.md @@ -0,0 +1 @@ +add documentation note about reactor state ids. diff --git a/doc/topics/reactor/index.rst b/doc/topics/reactor/index.rst index 9372e76d024..53fbe680942 100644 --- a/doc/topics/reactor/index.rst +++ b/doc/topics/reactor/index.rst @@ -212,6 +212,10 @@ in :ref:`local reactions `, but as noted above this is not very user-friendly. Therefore, the new config schema is recommended if the master is running a supported release. +.. note:: + state ids of reactors for runners and wheel should all be unique. they can + overwrite each other when added to the async queue. causing lost reactions + The below two examples are equivalent: +-------------------------------------------------+-------------------------------------------------+ @@ -248,6 +252,10 @@ Like :ref:`runner reactions `, the old config schema called for wheel reactions to have arguments passed directly under the name of the :ref:`wheel function ` (or in ``arg`` or ``kwarg`` parameters). +.. note:: + state ids of reactors for runners and wheel should all be unique. they can + overwrite each other when added to the async queue. causing lost reactions + The below two examples are equivalent: +-----------------------------------+---------------------------------+ From 48e71b713a0539f546f1b72c1f39f96a36240e47 Mon Sep 17 00:00:00 2001 From: Thomas Phipps Date: Thu, 27 Apr 2023 17:58:43 +0000 Subject: [PATCH 24/37] corrections --- doc/topics/reactor/index.rst | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/doc/topics/reactor/index.rst b/doc/topics/reactor/index.rst index 53fbe680942..38a27b216a7 100644 --- a/doc/topics/reactor/index.rst +++ b/doc/topics/reactor/index.rst @@ -213,8 +213,8 @@ user-friendly. Therefore, the new config schema is recommended if the master is running a supported release. .. note:: - state ids of reactors for runners and wheel should all be unique. they can - overwrite each other when added to the async queue. causing lost reactions + State ids of reactors for runners and wheels should all be unique.Tthey can + overwrite each other when added to the async queue causing lost reactions The below two examples are equivalent: @@ -253,8 +253,8 @@ wheel reactions to have arguments passed directly under the name of the :ref:`wheel function ` (or in ``arg`` or ``kwarg`` parameters). .. note:: - state ids of reactors for runners and wheel should all be unique. they can - overwrite each other when added to the async queue. causing lost reactions + State ids of reactors for runners and wheels should all be unique. They can + overwrite each other when added to the async queue causing lost reactions The below two examples are equivalent: From 0f9ed5103df82bb2c6ad3c95e1945cbcc2a21497 Mon Sep 17 00:00:00 2001 From: Thomas Phipps Date: Thu, 27 Apr 2023 19:30:43 +0000 Subject: [PATCH 25/37] corrections number 2 --- doc/topics/reactor/index.rst | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/doc/topics/reactor/index.rst b/doc/topics/reactor/index.rst index 38a27b216a7..7cdadff29cd 100644 --- a/doc/topics/reactor/index.rst +++ b/doc/topics/reactor/index.rst @@ -213,8 +213,8 @@ user-friendly. Therefore, the new config schema is recommended if the master is running a supported release. .. note:: - State ids of reactors for runners and wheels should all be unique.Tthey can - overwrite each other when added to the async queue causing lost reactions + State ids of reactors for runners and wheels should all be unique. They can + overwrite each other when added to the async queue causing lost reactions. The below two examples are equivalent: @@ -254,7 +254,7 @@ wheel reactions to have arguments passed directly under the name of the .. note:: State ids of reactors for runners and wheels should all be unique. They can - overwrite each other when added to the async queue causing lost reactions + overwrite each other when added to the async queue causing lost reactions. The below two examples are equivalent: From 959ea4fc29ad2005d10965e300f85bbc4dc87e93 Mon Sep 17 00:00:00 2001 From: MKLeb Date: Tue, 25 Apr 2023 15:44:22 -0400 Subject: [PATCH 26/37] Move `/etc/salt/proxy` to the salt-minion debian package --- pkg/debian/salt-common.install | 1 - pkg/debian/salt-minion.install | 1 + 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/debian/salt-common.install b/pkg/debian/salt-common.install index 4b612bd3aa6..178b7c65ccc 100644 --- a/pkg/debian/salt-common.install +++ b/pkg/debian/salt-common.install @@ -1,7 +1,6 @@ pkg/common/salt-proxy@.service /lib/systemd/system conf/roster /etc/salt conf/cloud /etc/salt -conf/proxy /etc/salt pkg/common/fish-completions/salt-cp.fish /usr/share/fish/vendor_completions.d pkg/common/fish-completions/salt-call.fish /usr/share/fish/vendor_completions.d pkg/common/fish-completions/salt-syndic.fish /usr/share/fish/vendor_completions.d diff --git a/pkg/debian/salt-minion.install b/pkg/debian/salt-minion.install index 4fc4633bda8..2a7bcf82004 100644 --- a/pkg/debian/salt-minion.install +++ b/pkg/debian/salt-minion.install @@ -1,2 +1,3 @@ conf/minion /etc/salt +conf/proxy /etc/salt pkg/common/salt-minion.service /lib/systemd/system From 9c7f8198171165ff378ec1b62c0f25be856ecbc0 Mon Sep 17 00:00:00 2001 From: MKLeb Date: Wed, 26 Apr 2023 15:53:47 -0400 Subject: [PATCH 27/37] Move the systemd unit to salt-minion for salt-proxy --- pkg/debian/salt-common.install | 1 - pkg/debian/salt-minion.install | 1 + 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/debian/salt-common.install b/pkg/debian/salt-common.install index 178b7c65ccc..7e88a78ee3a 100644 --- a/pkg/debian/salt-common.install +++ b/pkg/debian/salt-common.install @@ -1,4 +1,3 @@ -pkg/common/salt-proxy@.service /lib/systemd/system conf/roster /etc/salt conf/cloud /etc/salt pkg/common/fish-completions/salt-cp.fish /usr/share/fish/vendor_completions.d diff --git a/pkg/debian/salt-minion.install b/pkg/debian/salt-minion.install index 2a7bcf82004..d7a23a423bd 100644 --- a/pkg/debian/salt-minion.install +++ b/pkg/debian/salt-minion.install @@ -1,3 +1,4 @@ conf/minion /etc/salt conf/proxy /etc/salt pkg/common/salt-minion.service /lib/systemd/system +pkg/common/salt-proxy@.service /lib/systemd/system From 89c858b8544267fa4c456cb3267e4152fde73ac4 Mon Sep 17 00:00:00 2001 From: MKLeb Date: Thu, 27 Apr 2023 11:52:39 -0400 Subject: [PATCH 28/37] changelog --- changelog/64117.fixed.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog/64117.fixed.md diff --git a/changelog/64117.fixed.md b/changelog/64117.fixed.md new file mode 100644 index 00000000000..0bca97e167d --- /dev/null +++ b/changelog/64117.fixed.md @@ -0,0 +1 @@ +Moved /etc/salt/proxy and /lib/systemd/system/salt-proxy@.service to the salt-minion DEB package From a747b034c32a66f998f690d886e42b6e12f1fe17 Mon Sep 17 00:00:00 2001 From: Massimiliano Torromeo Date: Wed, 19 Apr 2023 12:28:32 +0200 Subject: [PATCH 29/37] fix passing wrong keyword arguments to cp.cache_file in pkg.installed with sources --- salt/states/pkg.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/salt/states/pkg.py b/salt/states/pkg.py index 12fbc87a1a0..a605b231076 100644 --- a/salt/states/pkg.py +++ b/salt/states/pkg.py @@ -760,7 +760,9 @@ def _find_install_targets( err = "Unable to cache {0}: {1}" try: cached_path = __salt__["cp.cache_file"]( - version_string, saltenv=kwargs["saltenv"], **kwargs + version_string, + saltenv=kwargs["saltenv"], + verify_ssl=kwargs.get("verify_ssl", True), ) except CommandExecutionError as exc: problems.append(err.format(version_string, exc)) From 7f2650ae37fc7740d84b2ad581426045285dc00b Mon Sep 17 00:00:00 2001 From: Pedro Algarvio Date: Thu, 27 Apr 2023 15:43:42 +0100 Subject: [PATCH 30/37] Drop `**kwargs` usage and be explicit about the supported keyword arguments. Signed-off-by: Pedro Algarvio --- salt/modules/win_pkg.py | 25 ++++++++++++++-------- tests/pytests/unit/modules/test_win_pkg.py | 2 +- 2 files changed, 17 insertions(+), 10 deletions(-) diff --git a/salt/modules/win_pkg.py b/salt/modules/win_pkg.py index 3aa7c7919ab..e80dd193221 100644 --- a/salt/modules/win_pkg.py +++ b/salt/modules/win_pkg.py @@ -1298,7 +1298,7 @@ def _repo_process_pkg_sls(filename, short_path_name, ret, successful_verbose): successful_verbose[short_path_name] = [] -def _get_source_sum(source_hash, file_path, saltenv, **kwargs): +def _get_source_sum(source_hash, file_path, saltenv, verify_ssl=True): """ Extract the hash sum, whether it is in a remote hash file, or just a string. """ @@ -1315,7 +1315,7 @@ def _get_source_sum(source_hash, file_path, saltenv, **kwargs): # The source_hash is a file on a server try: cached_hash_file = __salt__["cp.cache_file"]( - source_hash, saltenv, verify_ssl=kwargs.get("verify_ssl", True) + source_hash, saltenv=saltenv, verify_ssl=verify_ssl ) except MinionError as exc: log.exception("Failed to cache %s", source_hash, exc_info=exc) @@ -1671,7 +1671,7 @@ def install(name=None, refresh=False, pkgs=None, **kwargs): try: cached_file = __salt__["cp.cache_file"]( cache_file, - saltenv, + saltenv=saltenv, verify_ssl=kwargs.get("verify_ssl", True), ) except MinionError as exc: @@ -1686,7 +1686,7 @@ def install(name=None, refresh=False, pkgs=None, **kwargs): try: cached_file = __salt__["cp.cache_file"]( cache_file, - saltenv, + saltenv=saltenv, verify_ssl=kwargs.get("verify_ssl", True), ) except MinionError as exc: @@ -1706,7 +1706,9 @@ def install(name=None, refresh=False, pkgs=None, **kwargs): # It's not cached. Cache it, mate. try: cached_pkg = __salt__["cp.cache_file"]( - installer, saltenv, verify_ssl=kwargs.get("verify_ssl", True) + installer, + saltenv=saltenv, + verify_ssl=kwargs.get("verify_ssl", True), ) except MinionError as exc: msg = "Failed to cache {}".format(installer) @@ -1730,7 +1732,7 @@ def install(name=None, refresh=False, pkgs=None, **kwargs): try: cached_pkg = __salt__["cp.cache_file"]( installer, - saltenv, + saltenv=saltenv, verify_ssl=kwargs.get("verify_ssl", True), ) except MinionError as exc: @@ -1754,7 +1756,12 @@ def install(name=None, refresh=False, pkgs=None, **kwargs): # Compare the hash sums source_hash = pkginfo[version_num].get("source_hash", False) if source_hash: - source_sum = _get_source_sum(source_hash, cached_pkg, saltenv, **kwargs) + source_sum = _get_source_sum( + source_hash, + cached_pkg, + saltenv=saltenv, + verify_ssl=kwargs.get("verify_ssl", True), + ) log.debug( "pkg.install: Source %s hash: %s", source_sum["hash_type"], @@ -2126,7 +2133,7 @@ def remove(name=None, pkgs=None, **kwargs): try: cached_pkg = __salt__["cp.cache_file"]( uninstaller, - saltenv, + saltenv=saltenv, verify_ssl=kwargs.get("verify_ssl", True), ) except MinionError as exc: @@ -2150,7 +2157,7 @@ def remove(name=None, pkgs=None, **kwargs): try: cached_pkg = __salt__["cp.cache_file"]( uninstaller, - saltenv, + saltenv=saltenv, verify_ssl=kwargs.get("verify_ssl", True), ) except MinionError as exc: diff --git a/tests/pytests/unit/modules/test_win_pkg.py b/tests/pytests/unit/modules/test_win_pkg.py index 76234fb77e3..6d435f00a54 100644 --- a/tests/pytests/unit/modules/test_win_pkg.py +++ b/tests/pytests/unit/modules/test_win_pkg.py @@ -262,7 +262,7 @@ def test_pkg_install_verify_ssl_false(): result = win_pkg.install(name="nsis", version="3.02", verify_ssl=False) mock_cp.assert_called_once_with( "http://download.sourceforge.net/project/nsis/NSIS%203/3.02/nsis-3.02-setup.exe", - "base", + saltenv="base", verify_ssl=False, ) assert expected == result From 13d571807c416365e2332a81bd91ab09b475a674 Mon Sep 17 00:00:00 2001 From: Pedro Algarvio Date: Thu, 27 Apr 2023 16:15:07 +0100 Subject: [PATCH 31/37] Add regression test for https://github.com/saltstack/salt/issues/64118 Signed-off-by: Pedro Algarvio --- tests/pytests/unit/states/test_pkg.py | 46 ++++++++++++++++++++++++--- 1 file changed, 41 insertions(+), 5 deletions(-) diff --git a/tests/pytests/unit/states/test_pkg.py b/tests/pytests/unit/states/test_pkg.py index b852f27b008..f58be11011f 100644 --- a/tests/pytests/unit/states/test_pkg.py +++ b/tests/pytests/unit/states/test_pkg.py @@ -3,6 +3,7 @@ import logging import pytest import salt.modules.beacons as beaconmod +import salt.modules.cp as cp import salt.modules.pkg_resource as pkg_resource import salt.modules.yumpkg as yumpkg import salt.states.beacon as beaconstate @@ -15,19 +16,28 @@ log = logging.getLogger(__name__) @pytest.fixture -def configure_loader_modules(): +def configure_loader_modules(minion_opts): return { + cp: { + "__opts__": minion_opts, + }, pkg: { "__env__": "base", "__salt__": {}, "__grains__": {"os": "CentOS", "os_family": "RedHat"}, - "__opts__": {"test": False, "cachedir": ""}, + "__opts__": minion_opts, "__instance_id__": "", "__low__": {}, "__utils__": {"state.gen_tag": state_utils.gen_tag}, }, - beaconstate: {"__salt__": {}, "__opts__": {}}, - beaconmod: {"__salt__": {}, "__opts__": {}}, + beaconstate: { + "__salt__": {}, + "__opts__": minion_opts, + }, + beaconmod: { + "__salt__": {}, + "__opts__": minion_opts, + }, pkg_resource: { "__salt__": {}, "__grains__": {"os": "CentOS", "os_family": "RedHat"}, @@ -35,7 +45,7 @@ def configure_loader_modules(): yumpkg: { "__salt__": {}, "__grains__": {"osarch": "x86_64", "osmajorrelease": 7}, - "__opts__": {}, + "__opts__": minion_opts, }, } @@ -563,6 +573,32 @@ def test_installed_with_changes_test_true(list_pkgs): assert ret["changes"] == expected +def test_installed_with_sources(list_pkgs, tmp_path): + """ + Test pkg.installed with passing `sources` + """ + + list_pkgs = MagicMock(return_value=list_pkgs) + pkg_source = tmp_path / "pkga-package-0.3.0.deb" + + with patch.dict( + pkg.__salt__, + { + "cp.cache_file": cp.cache_file, + "pkg.list_pkgs": list_pkgs, + "pkg_resource.pack_sources": pkg_resource.pack_sources, + "lowpkg.bin_pkg_info": MagicMock(), + }, + ), patch("salt.fileclient.get_file_client", return_value=MagicMock()): + try: + ret = pkg.installed("install-pkgd", sources=[{"pkga": str(pkg_source)}]) + assert ret["result"] is False + except TypeError as exc: + if "got multiple values for keyword argument 'saltenv'" in str(exc): + pytest.fail(f"TypeError should have not been raised: {exc}") + raise exc from None + + @pytest.mark.parametrize("action", ["removed", "purged"]) def test_removed_purged_with_changes_test_true(list_pkgs, action): """ From 1a63beb50633350b95993ac7f52230db48c825cc Mon Sep 17 00:00:00 2001 From: Pedro Algarvio Date: Thu, 27 Apr 2023 16:19:59 +0100 Subject: [PATCH 32/37] Add changelog file Signed-off-by: Pedro Algarvio --- changelog/64118.fixed.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog/64118.fixed.md diff --git a/changelog/64118.fixed.md b/changelog/64118.fixed.md new file mode 100644 index 00000000000..e7251827e97 --- /dev/null +++ b/changelog/64118.fixed.md @@ -0,0 +1 @@ +Stop passing `**kwargs` and be explicit about the keyword arguments to pass, namely, to `cp.cache_file` call in `salt.states.pkg` From 25821ae58a31a4af7c5a66e360acdf97cf68bd68 Mon Sep 17 00:00:00 2001 From: Pedro Algarvio Date: Wed, 19 Apr 2023 17:25:49 +0100 Subject: [PATCH 33/37] Make sure the file client is destroyed upon used Signed-off-by: Pedro Algarvio --- salt/client/ssh/wrapper/saltcheck.py | 106 ++++----- salt/fileclient.py | 11 - salt/modules/dockermod.py | 17 +- salt/pillar/__init__.py | 6 +- salt/states/ansiblegate.py | 11 +- salt/utils/asynchronous.py | 2 +- salt/utils/jinja.py | 18 ++ salt/utils/mako.py | 7 + salt/utils/templates.py | 315 ++++++++++++++------------- 9 files changed, 250 insertions(+), 243 deletions(-) diff --git a/salt/client/ssh/wrapper/saltcheck.py b/salt/client/ssh/wrapper/saltcheck.py index d47b5cf6883..5812d531957 100644 --- a/salt/client/ssh/wrapper/saltcheck.py +++ b/salt/client/ssh/wrapper/saltcheck.py @@ -9,6 +9,7 @@ import tarfile import tempfile from contextlib import closing +import salt.fileclient import salt.utils.files import salt.utils.json import salt.utils.url @@ -28,65 +29,66 @@ def update_master_cache(states, saltenv="base"): # Setup for copying states to gendir gendir = tempfile.mkdtemp() trans_tar = salt.utils.files.mkstemp() - if "cp.fileclient_{}".format(id(__opts__)) not in __context__: - __context__[ - "cp.fileclient_{}".format(id(__opts__)) - ] = salt.fileclient.get_file_client(__opts__) + cp_fileclient_ctx_key = "cp.fileclient_{}".format(id(__opts__)) + if cp_fileclient_ctx_key not in __context__: + __context__[cp_fileclient_ctx_key] = salt.fileclient.get_file_client(__opts__) - # generate cp.list_states output and save to gendir - cp_output = salt.utils.json.dumps(__salt__["cp.list_states"]()) - cp_output_file = os.path.join(gendir, "cp_output.txt") - with salt.utils.files.fopen(cp_output_file, "w") as fp: - fp.write(cp_output) + with __context__[cp_fileclient_ctx_key] as cp_fileclient: - # cp state directories to gendir - already_processed = [] - sls_list = salt.utils.args.split_input(states) - for state_name in sls_list: - # generate low data for each state and save to gendir - state_low_file = os.path.join(gendir, state_name + ".low") - state_low_output = salt.utils.json.dumps( - __salt__["state.show_low_sls"](state_name) - ) - with salt.utils.files.fopen(state_low_file, "w") as fp: - fp.write(state_low_output) + # generate cp.list_states output and save to gendir + cp_output = salt.utils.json.dumps(__salt__["cp.list_states"]()) + cp_output_file = os.path.join(gendir, "cp_output.txt") + with salt.utils.files.fopen(cp_output_file, "w") as fp: + fp.write(cp_output) - state_name = state_name.replace(".", os.sep) - if state_name in already_processed: - log.debug("Already cached state for %s", state_name) - else: - file_copy_file = os.path.join(gendir, state_name + ".copy") - log.debug("copying %s to %s", state_name, gendir) - qualified_name = salt.utils.url.create(state_name, saltenv) - # Duplicate cp.get_dir to gendir - copy_result = __context__["cp.fileclient_{}".format(id(__opts__))].get_dir( - qualified_name, gendir, saltenv + # cp state directories to gendir + already_processed = [] + sls_list = salt.utils.args.split_input(states) + for state_name in sls_list: + # generate low data for each state and save to gendir + state_low_file = os.path.join(gendir, state_name + ".low") + state_low_output = salt.utils.json.dumps( + __salt__["state.show_low_sls"](state_name) ) - if copy_result: - copy_result = [dir.replace(gendir, state_cache) for dir in copy_result] - copy_result_output = salt.utils.json.dumps(copy_result) - with salt.utils.files.fopen(file_copy_file, "w") as fp: - fp.write(copy_result_output) - already_processed.append(state_name) + with salt.utils.files.fopen(state_low_file, "w") as fp: + fp.write(state_low_output) + + state_name = state_name.replace(".", os.sep) + if state_name in already_processed: + log.debug("Already cached state for %s", state_name) else: - # If files were not copied, assume state.file.sls was given and just copy state - state_name = os.path.dirname(state_name) file_copy_file = os.path.join(gendir, state_name + ".copy") - if state_name in already_processed: - log.debug("Already cached state for %s", state_name) + log.debug("copying %s to %s", state_name, gendir) + qualified_name = salt.utils.url.create(state_name, saltenv) + # Duplicate cp.get_dir to gendir + copy_result = cp_fileclient.get_dir(qualified_name, gendir, saltenv) + if copy_result: + copy_result = [ + dir.replace(gendir, state_cache) for dir in copy_result + ] + copy_result_output = salt.utils.json.dumps(copy_result) + with salt.utils.files.fopen(file_copy_file, "w") as fp: + fp.write(copy_result_output) + already_processed.append(state_name) else: - qualified_name = salt.utils.url.create(state_name, saltenv) - copy_result = __context__[ - "cp.fileclient_{}".format(id(__opts__)) - ].get_dir(qualified_name, gendir, saltenv) - if copy_result: - copy_result = [ - dir.replace(gendir, state_cache) for dir in copy_result - ] - copy_result_output = salt.utils.json.dumps(copy_result) - with salt.utils.files.fopen(file_copy_file, "w") as fp: - fp.write(copy_result_output) - already_processed.append(state_name) + # If files were not copied, assume state.file.sls was given and just copy state + state_name = os.path.dirname(state_name) + file_copy_file = os.path.join(gendir, state_name + ".copy") + if state_name in already_processed: + log.debug("Already cached state for %s", state_name) + else: + qualified_name = salt.utils.url.create(state_name, saltenv) + copy_result = cp_fileclient.get_dir( + qualified_name, gendir, saltenv + ) + if copy_result: + copy_result = [ + dir.replace(gendir, state_cache) for dir in copy_result + ] + copy_result_output = salt.utils.json.dumps(copy_result) + with salt.utils.files.fopen(file_copy_file, "w") as fp: + fp.write(copy_result_output) + already_processed.append(state_name) # turn gendir into tarball and remove gendir try: diff --git a/salt/fileclient.py b/salt/fileclient.py index fef5154a0be..f01a86dd0d4 100644 --- a/salt/fileclient.py +++ b/salt/fileclient.py @@ -849,7 +849,6 @@ class Client: kwargs.pop("env") kwargs["saltenv"] = saltenv - url_data = urllib.parse.urlparse(url) sfn = self.cache_file(url, saltenv, cachedir=cachedir) if not sfn or not os.path.exists(sfn): return "" @@ -1165,13 +1164,8 @@ class RemoteClient(Client): if not salt.utils.platform.is_windows(): hash_server, stat_server = self.hash_and_stat_file(path, saltenv) - try: - mode_server = stat_server[0] - except (IndexError, TypeError): - mode_server = None else: hash_server = self.hash_file(path, saltenv) - mode_server = None # Check if file exists on server, before creating files and # directories @@ -1214,13 +1208,8 @@ class RemoteClient(Client): if dest2check and os.path.isfile(dest2check): if not salt.utils.platform.is_windows(): hash_local, stat_local = self.hash_and_stat_file(dest2check, saltenv) - try: - mode_local = stat_local[0] - except (IndexError, TypeError): - mode_local = None else: hash_local = self.hash_file(dest2check, saltenv) - mode_local = None if hash_local == hash_server: return dest2check diff --git a/salt/modules/dockermod.py b/salt/modules/dockermod.py index 6870c26b0e6..3e35700788d 100644 --- a/salt/modules/dockermod.py +++ b/salt/modules/dockermod.py @@ -6644,14 +6644,6 @@ def script_retcode( )["retcode"] -def _mk_fileclient(): - """ - Create a file client and add it to the context. - """ - if "cp.fileclient" not in __context__: - __context__["cp.fileclient"] = salt.fileclient.get_file_client(__opts__) - - def _generate_tmp_path(): return os.path.join("/tmp", "salt.docker.{}".format(uuid.uuid4().hex[:6])) @@ -6665,11 +6657,10 @@ def _prepare_trans_tar(name, sls_opts, mods=None, pillar=None, extra_filerefs="" # reuse it from salt.ssh, however this function should # be somewhere else refs = salt.client.ssh.state.lowstate_file_refs(chunks, extra_filerefs) - _mk_fileclient() - trans_tar = salt.client.ssh.state.prep_trans_tar( - __context__["cp.fileclient"], chunks, refs, pillar, name - ) - return trans_tar + with salt.fileclient.get_file_client(__opts__) as fileclient: + return salt.client.ssh.state.prep_trans_tar( + fileclient, chunks, refs, pillar, name + ) def _compile_state(sls_opts, mods=None): diff --git a/salt/pillar/__init__.py b/salt/pillar/__init__.py index 5a3f5388b40..d324fcee192 100644 --- a/salt/pillar/__init__.py +++ b/salt/pillar/__init__.py @@ -9,7 +9,6 @@ import logging import os import sys import traceback -import uuid import salt.channel.client import salt.ext.tornado.gen @@ -1341,6 +1340,11 @@ class Pillar: if self._closing: return self._closing = True + if self.client: + try: + self.client.destroy() + except AttributeError: + pass # pylint: disable=W1701 def __del__(self): diff --git a/salt/states/ansiblegate.py b/salt/states/ansiblegate.py index 7fd4deb6c2a..9abd418c42c 100644 --- a/salt/states/ansiblegate.py +++ b/salt/states/ansiblegate.py @@ -32,12 +32,10 @@ state: - state: installed """ - import logging import os import sys -# Import salt modules import salt.fileclient import salt.utils.decorators.path from salt.utils.decorators import depends @@ -108,13 +106,6 @@ def __virtual__(): return __virtualname__ -def _client(): - """ - Get a fileclient - """ - return salt.fileclient.get_file_client(__opts__) - - def _changes(plays): """ Find changes in ansible return data @@ -171,7 +162,7 @@ def playbooks(name, rundir=None, git_repo=None, git_kwargs=None, ansible_kwargs= } if git_repo: if not isinstance(rundir, str) or not os.path.isdir(rundir): - with _client() as client: + with salt.fileclient.get_file_client(__opts__) as client: rundir = client._extrn_path(git_repo, "base") log.trace("rundir set to %s", rundir) if not isinstance(git_kwargs, dict): diff --git a/salt/utils/asynchronous.py b/salt/utils/asynchronous.py index 2a858feee98..0c645bbc3bb 100644 --- a/salt/utils/asynchronous.py +++ b/salt/utils/asynchronous.py @@ -131,7 +131,7 @@ class SyncWrapper: result = io_loop.run_sync(lambda: getattr(self.obj, key)(*args, **kwargs)) results.append(True) results.append(result) - except Exception as exc: # pylint: disable=broad-except + except Exception: # pylint: disable=broad-except results.append(False) results.append(sys.exc_info()) diff --git a/salt/utils/jinja.py b/salt/utils/jinja.py index fcc5aec497e..4946ab921d3 100644 --- a/salt/utils/jinja.py +++ b/salt/utils/jinja.py @@ -221,6 +221,24 @@ class SaltCacheLoader(BaseLoader): # there is no template file within searchpaths raise TemplateNotFound(template) + def destroy(self): + for attr in ("_cached_client", "_cached_pillar_client"): + client = getattr(self, attr, None) + if client is not None: + try: + client.destroy() + except AttributeError: + # PillarClient and LocalClient objects do not have a destroy method + pass + setattr(self, attr, None) + + def __enter__(self): + self.file_client() + return self + + def __exit__(self, *args): + self.destroy() + class PrintableDict(OrderedDict): """ diff --git a/salt/utils/mako.py b/salt/utils/mako.py index 69618de9837..037d5d86deb 100644 --- a/salt/utils/mako.py +++ b/salt/utils/mako.py @@ -97,3 +97,10 @@ if HAS_MAKO: self.cache[fpath] = self.file_client().get_file( fpath, "", True, self.saltenv ) + + def destroy(self): + if self.client: + try: + self.client.destroy() + except AttributeError: + pass diff --git a/salt/utils/templates.py b/salt/utils/templates.py index 4947b820a36..4a8adf2a14f 100644 --- a/salt/utils/templates.py +++ b/salt/utils/templates.py @@ -362,163 +362,169 @@ def render_jinja_tmpl(tmplstr, context, tmplpath=None): elif tmplstr.endswith("\n"): newline = "\n" - if not saltenv: - if tmplpath: - loader = jinja2.FileSystemLoader(os.path.dirname(tmplpath)) - else: - loader = salt.utils.jinja.SaltCacheLoader( - opts, - saltenv, - pillar_rend=context.get("_pillar_rend", False), - _file_client=file_client, - ) - - env_args = {"extensions": [], "loader": loader} - - if hasattr(jinja2.ext, "with_"): - env_args["extensions"].append("jinja2.ext.with_") - if hasattr(jinja2.ext, "do"): - env_args["extensions"].append("jinja2.ext.do") - if hasattr(jinja2.ext, "loopcontrols"): - env_args["extensions"].append("jinja2.ext.loopcontrols") - env_args["extensions"].append(salt.utils.jinja.SerializerExtension) - - opt_jinja_env = opts.get("jinja_env", {}) - opt_jinja_sls_env = opts.get("jinja_sls_env", {}) - - opt_jinja_env = opt_jinja_env if isinstance(opt_jinja_env, dict) else {} - opt_jinja_sls_env = opt_jinja_sls_env if isinstance(opt_jinja_sls_env, dict) else {} - - # Pass through trim_blocks and lstrip_blocks Jinja parameters - # trim_blocks removes newlines around Jinja blocks - # lstrip_blocks strips tabs and spaces from the beginning of - # line to the start of a block. - if opts.get("jinja_trim_blocks", False): - log.debug("Jinja2 trim_blocks is enabled") - log.warning( - "jinja_trim_blocks is deprecated and will be removed in a future release," - " please use jinja_env and/or jinja_sls_env instead" - ) - opt_jinja_env["trim_blocks"] = True - opt_jinja_sls_env["trim_blocks"] = True - if opts.get("jinja_lstrip_blocks", False): - log.debug("Jinja2 lstrip_blocks is enabled") - log.warning( - "jinja_lstrip_blocks is deprecated and will be removed in a future release," - " please use jinja_env and/or jinja_sls_env instead" - ) - opt_jinja_env["lstrip_blocks"] = True - opt_jinja_sls_env["lstrip_blocks"] = True - - def opt_jinja_env_helper(opts, optname): - for k, v in opts.items(): - k = k.lower() - if hasattr(jinja2.defaults, k.upper()): - log.debug("Jinja2 environment %s was set to %s by %s", k, v, optname) - env_args[k] = v - else: - log.warning("Jinja2 environment %s is not recognized", k) - - if "sls" in context and context["sls"] != "": - opt_jinja_env_helper(opt_jinja_sls_env, "jinja_sls_env") - else: - opt_jinja_env_helper(opt_jinja_env, "jinja_env") - - if opts.get("allow_undefined", False): - jinja_env = jinja2.sandbox.SandboxedEnvironment(**env_args) - else: - jinja_env = jinja2.sandbox.SandboxedEnvironment( - undefined=jinja2.StrictUndefined, **env_args - ) - - indent_filter = jinja_env.filters.get("indent") - jinja_env.tests.update(JinjaTest.salt_jinja_tests) - jinja_env.filters.update(JinjaFilter.salt_jinja_filters) - if salt.utils.jinja.JINJA_VERSION >= Version("2.11"): - # Use the existing indent filter on Jinja versions where it's not broken - jinja_env.filters["indent"] = indent_filter - jinja_env.globals.update(JinjaGlobal.salt_jinja_globals) - - # globals - jinja_env.globals["odict"] = OrderedDict - jinja_env.globals["show_full_context"] = salt.utils.jinja.show_full_context - - jinja_env.tests["list"] = salt.utils.data.is_list - - decoded_context = {} - for key, value in context.items(): - if not isinstance(value, str): - if isinstance(value, NamedLoaderContext): - decoded_context[key] = value.value() - else: - decoded_context[key] = value - continue - - try: - decoded_context[key] = salt.utils.stringutils.to_unicode( - value, encoding=SLS_ENCODING - ) - except UnicodeDecodeError as ex: - log.debug( - "Failed to decode using default encoding (%s), trying system encoding", - SLS_ENCODING, - ) - decoded_context[key] = salt.utils.data.decode(value) - - jinja_env.globals.update(decoded_context) try: - template = jinja_env.from_string(tmplstr) - output = template.render(**decoded_context) - except jinja2.exceptions.UndefinedError as exc: - trace = traceback.extract_tb(sys.exc_info()[2]) - line, out = _get_jinja_error(trace, context=decoded_context) - if not line: - tmplstr = "" - raise SaltRenderError("Jinja variable {}{}".format(exc, out), line, tmplstr) - except ( - jinja2.exceptions.TemplateRuntimeError, - jinja2.exceptions.TemplateSyntaxError, - jinja2.exceptions.SecurityError, - ) as exc: - trace = traceback.extract_tb(sys.exc_info()[2]) - line, out = _get_jinja_error(trace, context=decoded_context) - if not line: - tmplstr = "" - raise SaltRenderError( - "Jinja syntax error: {}{}".format(exc, out), line, tmplstr - ) - except (SaltInvocationError, CommandExecutionError) as exc: - trace = traceback.extract_tb(sys.exc_info()[2]) - line, out = _get_jinja_error(trace, context=decoded_context) - if not line: - tmplstr = "" - raise SaltRenderError( - "Problem running salt function in Jinja template: {}{}".format(exc, out), - line, - tmplstr, - ) - except Exception as exc: # pylint: disable=broad-except - tracestr = traceback.format_exc() - trace = traceback.extract_tb(sys.exc_info()[2]) - line, out = _get_jinja_error(trace, context=decoded_context) - if not line: - tmplstr = "" + if not saltenv: + if tmplpath: + loader = jinja2.FileSystemLoader(os.path.dirname(tmplpath)) else: - tmplstr += "\n{}".format(tracestr) - log.debug("Jinja Error") - log.debug("Exception:", exc_info=True) - log.debug("Out: %s", out) - log.debug("Line: %s", line) - log.debug("TmplStr: %s", tmplstr) - log.debug("TraceStr: %s", tracestr) + loader = salt.utils.jinja.SaltCacheLoader( + opts, + saltenv, + pillar_rend=context.get("_pillar_rend", False), + _file_client=file_client, + ) - raise SaltRenderError( - "Jinja error: {}{}".format(exc, out), line, tmplstr, trace=tracestr + env_args = {"extensions": [], "loader": loader} + + if hasattr(jinja2.ext, "with_"): + env_args["extensions"].append("jinja2.ext.with_") + if hasattr(jinja2.ext, "do"): + env_args["extensions"].append("jinja2.ext.do") + if hasattr(jinja2.ext, "loopcontrols"): + env_args["extensions"].append("jinja2.ext.loopcontrols") + env_args["extensions"].append(salt.utils.jinja.SerializerExtension) + + opt_jinja_env = opts.get("jinja_env", {}) + opt_jinja_sls_env = opts.get("jinja_sls_env", {}) + + opt_jinja_env = opt_jinja_env if isinstance(opt_jinja_env, dict) else {} + opt_jinja_sls_env = ( + opt_jinja_sls_env if isinstance(opt_jinja_sls_env, dict) else {} ) + + # Pass through trim_blocks and lstrip_blocks Jinja parameters + # trim_blocks removes newlines around Jinja blocks + # lstrip_blocks strips tabs and spaces from the beginning of + # line to the start of a block. + if opts.get("jinja_trim_blocks", False): + log.debug("Jinja2 trim_blocks is enabled") + log.warning( + "jinja_trim_blocks is deprecated and will be removed in a future release," + " please use jinja_env and/or jinja_sls_env instead" + ) + opt_jinja_env["trim_blocks"] = True + opt_jinja_sls_env["trim_blocks"] = True + if opts.get("jinja_lstrip_blocks", False): + log.debug("Jinja2 lstrip_blocks is enabled") + log.warning( + "jinja_lstrip_blocks is deprecated and will be removed in a future release," + " please use jinja_env and/or jinja_sls_env instead" + ) + opt_jinja_env["lstrip_blocks"] = True + opt_jinja_sls_env["lstrip_blocks"] = True + + def opt_jinja_env_helper(opts, optname): + for k, v in opts.items(): + k = k.lower() + if hasattr(jinja2.defaults, k.upper()): + log.debug( + "Jinja2 environment %s was set to %s by %s", k, v, optname + ) + env_args[k] = v + else: + log.warning("Jinja2 environment %s is not recognized", k) + + if "sls" in context and context["sls"] != "": + opt_jinja_env_helper(opt_jinja_sls_env, "jinja_sls_env") + else: + opt_jinja_env_helper(opt_jinja_env, "jinja_env") + + if opts.get("allow_undefined", False): + jinja_env = jinja2.sandbox.SandboxedEnvironment(**env_args) + else: + jinja_env = jinja2.sandbox.SandboxedEnvironment( + undefined=jinja2.StrictUndefined, **env_args + ) + + indent_filter = jinja_env.filters.get("indent") + jinja_env.tests.update(JinjaTest.salt_jinja_tests) + jinja_env.filters.update(JinjaFilter.salt_jinja_filters) + if salt.utils.jinja.JINJA_VERSION >= Version("2.11"): + # Use the existing indent filter on Jinja versions where it's not broken + jinja_env.filters["indent"] = indent_filter + jinja_env.globals.update(JinjaGlobal.salt_jinja_globals) + + # globals + jinja_env.globals["odict"] = OrderedDict + jinja_env.globals["show_full_context"] = salt.utils.jinja.show_full_context + + jinja_env.tests["list"] = salt.utils.data.is_list + + decoded_context = {} + for key, value in context.items(): + if not isinstance(value, str): + if isinstance(value, NamedLoaderContext): + decoded_context[key] = value.value() + else: + decoded_context[key] = value + continue + + try: + decoded_context[key] = salt.utils.stringutils.to_unicode( + value, encoding=SLS_ENCODING + ) + except UnicodeDecodeError: + log.debug( + "Failed to decode using default encoding (%s), trying system encoding", + SLS_ENCODING, + ) + decoded_context[key] = salt.utils.data.decode(value) + + jinja_env.globals.update(decoded_context) + try: + template = jinja_env.from_string(tmplstr) + output = template.render(**decoded_context) + except jinja2.exceptions.UndefinedError as exc: + trace = traceback.extract_tb(sys.exc_info()[2]) + line, out = _get_jinja_error(trace, context=decoded_context) + if not line: + tmplstr = "" + raise SaltRenderError("Jinja variable {}{}".format(exc, out), line, tmplstr) + except ( + jinja2.exceptions.TemplateRuntimeError, + jinja2.exceptions.TemplateSyntaxError, + jinja2.exceptions.SecurityError, + ) as exc: + trace = traceback.extract_tb(sys.exc_info()[2]) + line, out = _get_jinja_error(trace, context=decoded_context) + if not line: + tmplstr = "" + raise SaltRenderError( + "Jinja syntax error: {}{}".format(exc, out), line, tmplstr + ) + except (SaltInvocationError, CommandExecutionError) as exc: + trace = traceback.extract_tb(sys.exc_info()[2]) + line, out = _get_jinja_error(trace, context=decoded_context) + if not line: + tmplstr = "" + raise SaltRenderError( + "Problem running salt function in Jinja template: {}{}".format( + exc, out + ), + line, + tmplstr, + ) + except Exception as exc: # pylint: disable=broad-except + tracestr = traceback.format_exc() + trace = traceback.extract_tb(sys.exc_info()[2]) + line, out = _get_jinja_error(trace, context=decoded_context) + if not line: + tmplstr = "" + else: + tmplstr += "\n{}".format(tracestr) + log.debug("Jinja Error") + log.debug("Exception:", exc_info=True) + log.debug("Out: %s", out) + log.debug("Line: %s", line) + log.debug("TmplStr: %s", tmplstr) + log.debug("TraceStr: %s", tracestr) + + raise SaltRenderError( + "Jinja error: {}{}".format(exc, out), line, tmplstr, trace=tracestr + ) finally: - if loader and hasattr(loader, "_file_client"): - if hasattr(loader._file_client, "destroy"): - loader._file_client.destroy() + if loader and isinstance(loader, salt.utils.jinja.SaltCacheLoader): + loader.destroy() # Workaround a bug in Jinja that removes the final newline # (https://github.com/mitsuhiko/jinja2/issues/75) @@ -569,9 +575,8 @@ def render_mako_tmpl(tmplstr, context, tmplpath=None): except Exception: # pylint: disable=broad-except raise SaltRenderError(mako.exceptions.text_error_template().render()) finally: - if lookup and hasattr(lookup, "_file_client"): - if hasattr(lookup._file_client, "destroy"): - lookup._file_client.destroy() + if lookup and isinstance(lookup, SaltMakoTemplateLookup): + lookup.destroy() def render_wempy_tmpl(tmplstr, context, tmplpath=None): From 3e5c06b4fb8a8465f52f50a798ba45bd9610989e Mon Sep 17 00:00:00 2001 From: Pedro Algarvio Date: Wed, 19 Apr 2023 19:32:41 +0100 Subject: [PATCH 34/37] Don't cache the file client Signed-off-by: Pedro Algarvio --- salt/client/ssh/wrapper/saltcheck.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/salt/client/ssh/wrapper/saltcheck.py b/salt/client/ssh/wrapper/saltcheck.py index 5812d531957..b0b94593809 100644 --- a/salt/client/ssh/wrapper/saltcheck.py +++ b/salt/client/ssh/wrapper/saltcheck.py @@ -29,11 +29,7 @@ def update_master_cache(states, saltenv="base"): # Setup for copying states to gendir gendir = tempfile.mkdtemp() trans_tar = salt.utils.files.mkstemp() - cp_fileclient_ctx_key = "cp.fileclient_{}".format(id(__opts__)) - if cp_fileclient_ctx_key not in __context__: - __context__[cp_fileclient_ctx_key] = salt.fileclient.get_file_client(__opts__) - - with __context__[cp_fileclient_ctx_key] as cp_fileclient: + with salt.fileclient.get_file_client(__opts__) as cp_fileclient: # generate cp.list_states output and save to gendir cp_output = salt.utils.json.dumps(__salt__["cp.list_states"]()) From 3962fcd281a0a77ea0b1b8cfbd53cc0f78e7470c Mon Sep 17 00:00:00 2001 From: "Daniel A. Wozniak" Date: Thu, 20 Apr 2023 16:33:14 -0700 Subject: [PATCH 35/37] Add regression test for #64111 Test importing jinja files --- .../integration/states/test_include.py | 38 +++++++++++++++++++ 1 file changed, 38 insertions(+) create mode 100644 tests/pytests/integration/states/test_include.py diff --git a/tests/pytests/integration/states/test_include.py b/tests/pytests/integration/states/test_include.py new file mode 100644 index 00000000000..f524957a7b7 --- /dev/null +++ b/tests/pytests/integration/states/test_include.py @@ -0,0 +1,38 @@ +""" +Integration tests for the jinja includes in states +""" +import logging + +import pytest + +log = logging.getLogger(__name__) + + +@pytest.mark.slow_test +def test_issue_64111(salt_master, salt_minion, salt_call_cli): + + macros_jinja = """ + {% macro a_jinja_macro(arg) -%} + {{ arg }} + {%- endmacro %} + """ + + init_sls = """ + include: + - common.file1 + """ + + file1_sls = """ + {% from 'common/macros.jinja' import a_jinja_macro with context %} + + a state id: + cmd.run: + - name: echo {{ a_jinja_macro("hello world") }} + """ + tf = salt_master.state_tree.base.temp_file + + with tf("common/macros.jinja", macros_jinja): + with tf("common/init.sls", init_sls): + with tf("common/file1.sls", file1_sls): + ret = salt_call_cli.run("state.apply", "common") + assert ret.returncode == 0 From c94b36c4417a5cc234b9438a4f00349b26a8c5eb Mon Sep 17 00:00:00 2001 From: Pedro Algarvio Date: Mon, 24 Apr 2023 17:44:54 +0100 Subject: [PATCH 36/37] Don't cache the file client at the class level Signed-off-by: Pedro Algarvio --- salt/utils/jinja.py | 52 ++++++------------- .../integration/states/test_include.py | 2 + .../utils/jinja/test_salt_cache_loader.py | 13 ----- 3 files changed, 18 insertions(+), 49 deletions(-) diff --git a/salt/utils/jinja.py b/salt/utils/jinja.py index 4946ab921d3..f788a6e4dd3 100644 --- a/salt/utils/jinja.py +++ b/salt/utils/jinja.py @@ -58,19 +58,6 @@ class SaltCacheLoader(BaseLoader): and only loaded once per loader instance. """ - _cached_pillar_client = None - _cached_client = None - - @classmethod - def shutdown(cls): - for attr in ("_cached_client", "_cached_pillar_client"): - client = getattr(cls, attr, None) - if client is not None: - # PillarClient and LocalClient objects do not have a destroy method - if hasattr(client, "destroy"): - client.destroy() - setattr(cls, attr, None) - def __init__( self, opts, @@ -93,8 +80,7 @@ class SaltCacheLoader(BaseLoader): log.debug("Jinja search path: %s", self.searchpath) self.cached = [] self._file_client = _file_client - # Instantiate the fileclient - self.file_client() + self._close_file_client = _file_client is None def file_client(self): """ @@ -108,18 +94,9 @@ class SaltCacheLoader(BaseLoader): or not hasattr(self._file_client, "opts") or self._file_client.opts["file_roots"] != self.opts["file_roots"] ): - attr = "_cached_pillar_client" if self.pillar_rend else "_cached_client" - cached_client = getattr(self, attr, None) - if ( - cached_client is None - or not hasattr(cached_client, "opts") - or cached_client.opts["file_roots"] != self.opts["file_roots"] - ): - cached_client = salt.fileclient.get_file_client( - self.opts, self.pillar_rend - ) - setattr(SaltCacheLoader, attr, cached_client) - self._file_client = cached_client + self._file_client = salt.fileclient.get_file_client( + self.opts, self.pillar_rend + ) return self._file_client def cache_file(self, template): @@ -222,15 +199,18 @@ class SaltCacheLoader(BaseLoader): raise TemplateNotFound(template) def destroy(self): - for attr in ("_cached_client", "_cached_pillar_client"): - client = getattr(self, attr, None) - if client is not None: - try: - client.destroy() - except AttributeError: - # PillarClient and LocalClient objects do not have a destroy method - pass - setattr(self, attr, None) + if self._close_file_client is False: + return + if self._file_client is None: + return + file_client = self._file_client + self._file_client = None + + try: + file_client.destroy() + except AttributeError: + # PillarClient and LocalClient objects do not have a destroy method + pass def __enter__(self): self.file_client() diff --git a/tests/pytests/integration/states/test_include.py b/tests/pytests/integration/states/test_include.py index f524957a7b7..f814328c5e4 100644 --- a/tests/pytests/integration/states/test_include.py +++ b/tests/pytests/integration/states/test_include.py @@ -10,6 +10,8 @@ log = logging.getLogger(__name__) @pytest.mark.slow_test def test_issue_64111(salt_master, salt_minion, salt_call_cli): + # This needs to be an integration test. A functional test does not trigger + # the issue fixed. macros_jinja = """ {% macro a_jinja_macro(arg) -%} diff --git a/tests/pytests/unit/utils/jinja/test_salt_cache_loader.py b/tests/pytests/unit/utils/jinja/test_salt_cache_loader.py index 38c5ce5b724..0cb95c1748b 100644 --- a/tests/pytests/unit/utils/jinja/test_salt_cache_loader.py +++ b/tests/pytests/unit/utils/jinja/test_salt_cache_loader.py @@ -222,16 +222,3 @@ def test_file_client_kwarg(minion_opts, mock_file_client): mock_file_client.opts = minion_opts loader = SaltCacheLoader(minion_opts, _file_client=mock_file_client) assert loader._file_client is mock_file_client - - -def test_cache_loader_shutdown(minion_opts, mock_file_client): - """ - The shudown method can be called without raising an exception when the - file_client does not have a destroy method - """ - assert not hasattr(mock_file_client, "destroy") - mock_file_client.opts = minion_opts - loader = SaltCacheLoader(minion_opts, _file_client=mock_file_client) - assert loader._file_client is mock_file_client - # Shutdown method should not raise any exceptions - loader.shutdown() From 2e3f98a95c1b2641d2f10cb6767e5e2c08de6ec9 Mon Sep 17 00:00:00 2001 From: Pedro Algarvio Date: Wed, 26 Apr 2023 19:23:17 +0100 Subject: [PATCH 37/37] Add unit test to validate logic on calls to `destroy()` Signed-off-by: Pedro Algarvio --- salt/utils/jinja.py | 1 + .../utils/jinja/test_salt_cache_loader.py | 46 ++++++++++++++++++- 2 files changed, 46 insertions(+), 1 deletion(-) diff --git a/salt/utils/jinja.py b/salt/utils/jinja.py index f788a6e4dd3..a6a8a279605 100644 --- a/salt/utils/jinja.py +++ b/salt/utils/jinja.py @@ -97,6 +97,7 @@ class SaltCacheLoader(BaseLoader): self._file_client = salt.fileclient.get_file_client( self.opts, self.pillar_rend ) + self._close_file_client = True return self._file_client def cache_file(self, template): diff --git a/tests/pytests/unit/utils/jinja/test_salt_cache_loader.py b/tests/pytests/unit/utils/jinja/test_salt_cache_loader.py index 0cb95c1748b..e0f5fa158ff 100644 --- a/tests/pytests/unit/utils/jinja/test_salt_cache_loader.py +++ b/tests/pytests/unit/utils/jinja/test_salt_cache_loader.py @@ -15,7 +15,7 @@ import salt.utils.json # pylint: disable=unused-import import salt.utils.stringutils # pylint: disable=unused-import import salt.utils.yaml # pylint: disable=unused-import from salt.utils.jinja import SaltCacheLoader -from tests.support.mock import Mock, patch +from tests.support.mock import Mock, call, patch @pytest.fixture @@ -222,3 +222,47 @@ def test_file_client_kwarg(minion_opts, mock_file_client): mock_file_client.opts = minion_opts loader = SaltCacheLoader(minion_opts, _file_client=mock_file_client) assert loader._file_client is mock_file_client + + +def test_cache_loader_passed_file_client(minion_opts, mock_file_client): + """ + The shudown method can be called without raising an exception when the + file_client does not have a destroy method + """ + # Test SaltCacheLoader creating and destroying the file client created + file_client = Mock() + with patch("salt.fileclient.get_file_client", return_value=file_client): + loader = SaltCacheLoader(minion_opts) + assert loader._file_client is None + with loader: + assert loader._file_client is file_client + assert loader._file_client is None + assert file_client.mock_calls == [call.destroy()] + + # Test SaltCacheLoader reusing the file client passed + file_client = Mock() + file_client.opts = {"file_roots": minion_opts["file_roots"]} + with patch("salt.fileclient.get_file_client", return_value=Mock()): + loader = SaltCacheLoader(minion_opts, _file_client=file_client) + assert loader._file_client is file_client + with loader: + assert loader._file_client is file_client + assert loader._file_client is file_client + assert file_client.mock_calls == [] + + # Test SaltCacheLoader creating a client even though a file client was + # passed because the "file_roots" option is different, and, as such, + # the destroy method on the new file client is called, but not on the + # file client passed in. + file_client = Mock() + file_client.opts = {"file_roots": ""} + new_file_client = Mock() + with patch("salt.fileclient.get_file_client", return_value=new_file_client): + loader = SaltCacheLoader(minion_opts, _file_client=file_client) + assert loader._file_client is file_client + with loader: + assert loader._file_client is not file_client + assert loader._file_client is new_file_client + assert loader._file_client is None + assert file_client.mock_calls == [] + assert new_file_client.mock_calls == [call.destroy()]