Repository: saltstack/salt
Pull request: [master] Add syncing of custom ssh wrappers
### What does this PR do?
Adds `sync_wrapper` to the `saltutil` runner/execution/state modules.
From what I can tell, the loader is already configured to load them.

### What issues does this PR fix or reference?
Fixes: https://github.com/saltstack/salt/issues/45450
Reference: https://github.com/saltstack/salt/issues/61637

### Previous Behavior
`<cache_dir>/extmods/wrapper` is respected by the loader, but cannot be synced from the fileserver

### New Behavior
`<cache_dir>/extmods/wrapper` can be synced

### Merge requirements satisfied?
**[NOTICE] Bug fixes or features added to Salt require tests.**
<!-- Please review the [test documentation](https://docs.saltproject.io/en/master/topics/tutorials/writing_tests.html) for details on how to implement tests into Salt's test suite. -->
- [x] Docs
- [x] Changelog - https://docs.saltproject.io/en/master/topics/development/changelog.html
- [x] Tests written/updated

### Commits signed with GPG?
Yes

Problem statement:
Allow custom modules which run on master
### Description of Issue/Question

### Setup

I am using `salt-ssh` to deploy configuration to servers. On my computer (SSH master) I have in `states` directory also `_modules` with some simple Python functions I want to run to process values in Jinja templates. I do something like:

```
{% for user in pillar.users %}
  {% set user_groups = salt['servers.groups'](user.get('servers', {}), grains['id']) %}
  ...
```

`servers.groups` is a Python function which returns values from user's pillars based on target ID.

The issue is that this simple pure function runs over SSH on the minion. This is problematic because it really slows down things. Is there a way to configure that the module should be only used during rendering of the state and that it can be run locally on master?

### Versions Report

```
Salt Version:
           Salt: 2016.11.1
 
Dependency Versions:
           cffi: Not Installed
       cherrypy: Not Installed
       dateutil: Not Installed
          gitdb: Not Installed
      gitpython: Not Installed
          ioflo: Not Installed
         Jinja2: 2.10
        libgit2: Not Installed
        libnacl: Not Installed
       M2Crypto: Not Installed
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.4.8
   mysql-python: Not Installed
      pycparser: Not Installed
       pycrypto: 2.6.1
         pygit2: Not Installed
         Python: 2.7.14 (default, Sep 23 2017, 22:06:14)
   python-gnupg: Not Installed
         PyYAML: 3.12
          PyZMQ: 16.0.3
           RAET: Not Installed
          smmap: Not Installed
        timelib: Not Installed
        Tornado: 4.5.2
            ZMQ: 4.1.6
 
System Versions:
           dist: Ubuntu 17.10 artful
        machine: x86_64
        release: 4.13.0-21-generic
         system: Linux
        version: Ubuntu 17.10 artful
```

Hints from issue discussion:
The only way you could render it on the master is if you put all that information into pillar data, and even if you did that, it would still need to be rendered once per minion.  The states are not rendered at all on the master, we tar up the state tree into a `salt_states.tgz` on the salt master, then just send the tarball of files over and they get rendered on the minion in the `salt-call` command.

Also, if it was rendered on the master, how would it be able to look up which groups that user was in on that minion? it would have to login, and that is what would slow it down.

Probably best to just include the list of groups that you want that user to be in, and then include it in the user.present, so that any extra groups get removed as well.

Daniel
OK, maybe I am misunderstanding something. What I want in the end is to improve the performance of how quickly states are applied. Now, if I call `salt-ssh -l debug '<redacted>' state.highstate` I see in output many many entries of:

```
[DEBUG   ] Could not LazyLoad servers.match: 'servers.match' is not available.
[DEBUG   ] Performing shimmed, blocking command as follows:
servers.match <redacted> <redacted>
[DEBUG   ] Executed SHIM command. Command logged to TRACE
[DEBUG   ] Child Forked! PID: 17552  STDOUT_FD: 10  STDERR_FD: 12
[DEBUG   ] VT: Salt-SSH SHIM Terminal Command executed. Logged to TRACE
[DEBUG   ] RETCODE <redacted>: 0
[DEBUG   ] Could not LazyLoad servers.groups: 'servers.groups' is not available.
[DEBUG   ] Performing shimmed, blocking command as follows:
servers.groups <redacted> <redacted>
[DEBUG   ] Executed SHIM command. Command logged to TRACE
[DEBUG   ] Child Forked! PID: 17556  STDOUT_FD: 10  STDERR_FD: 12
[DEBUG   ] VT: Salt-SSH SHIM Terminal Command executed. Logged to TRACE
[DEBUG   ] RETCODE <redacted>: 0
[DEBUG   ] Could not LazyLoad servers.match: 'servers.match' is not available.
[DEBUG   ] Performing shimmed, blocking command as follows:
servers.match <redacted> <redacted>
[DEBUG   ] Executed SHIM command. Command logged to TRACE
[DEBUG   ] Child Forked! PID: 17560  STDOUT_FD: 10  STDERR_FD: 12
[DEBUG   ] VT: Salt-SSH SHIM Terminal Command executed. Logged to TRACE
[DEBUG   ] RETCODE <redacted>: 0
[DEBUG   ] Could not LazyLoad servers.groups: 'servers.groups' is not available.
[DEBUG   ] Performing shimmed, blocking command as follows:
servers.groups <redacted> <redacted>
[DEBUG   ] Executed SHIM command. Command logged to TRACE
[DEBUG   ] Child Forked! PID: 17564  STDOUT_FD: 10  STDERR_FD: 12
[DEBUG   ] VT: Salt-SSH SHIM Terminal Command executed. Logged to TRACE
[DEBUG   ] RETCODE <redacted>: 0
```

For all entries in `pillar.users`. (I have two such methods, `servers.groups`, and `servers.match`.)

I assumed that state is being rendered on the master and this is why it has to do all these SSH calls over to minions to evaluate code. But in this case code can be evaluated locally. It is just a pure Python function.

Now, if you are saying that state is evaluated on the minion, then I have no idea what all those calls are. Those calls are problematic because they are serial and when there is high latency (server on the other continent) this can take really a lot of time.
Hrm, maybe I am wrong, I was pretty certain we just shipped over the whole state tree and rendered it on the minon, but it appears that it does not.

https://github.com/saltstack/salt/blob/2017.7/salt/client/ssh/wrapper/state.py#L624

It would seem reasonable to add `salt.client.ssh.wrappers` as a loader system and then allow to be syncable

I am marking this as a feature request.

Yea, I was also surprised by this behavior. It really delays everything. So I do not care if everything is rendered on master or on minion, but it should just be done at one place without SSH connections going all the time.
@saltstack/team-ssh would this be even possible?

Thanks,
Daniel
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

If this issue is closed prematurely, please leave a comment and we will gladly reopen the issue.

This is still useful.
Thank you for updating this issue. It is no longer marked as stale.

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

If this issue is closed prematurely, please leave a comment and we will gladly reopen the issue.

Unstale.

Commit message: run pre-commit upgrade code for Py3.8+ again

Patch to review:
diff --git a/changelog/45450.added.md b/changelog/45450.added.md
new file mode 100644
index 000000000000..c7d6cd93074b
--- /dev/null
+++ b/changelog/45450.added.md
@@ -0,0 +1 @@
+Added syncing of custom salt-ssh wrappers
diff --git a/doc/topics/development/modules/index.rst b/doc/topics/development/modules/index.rst
index 3fc59f9d0b22..b9deaaa7ae7a 100644
--- a/doc/topics/development/modules/index.rst
+++ b/doc/topics/development/modules/index.rst
@@ -144,7 +144,7 @@ SDB          ``salt.sdb`` (:ref:`index <all-salt.sdb>`)                       ``
 Serializer   ``salt.serializers`` (:ref:`index <all-salt.serializers>`)       ``serializers`` [#no-fs]_ ``serializers_dirs``
 SPM pkgdb    ``salt.spm.pkgdb``                                               ``pkgdb`` [#no-fs]_       ``pkgdb_dirs``
 SPM pkgfiles ``salt.spm.pkgfiles``                                            ``pkgfiles`` [#no-fs]_    ``pkgfiles_dirs``
-SSH Wrapper  ``salt.client.ssh.wrapper``                                      ``wrapper`` [#no-fs]_     ``wrapper_dirs``
+SSH Wrapper  ``salt.client.ssh.wrapper``                                      ``wrapper``               ``wrapper_dirs``
 State        ``salt.states`` (:ref:`index <all-salt.states>`)                 ``states``                ``states_dirs``
 Thorium      ``salt.thorium`` (:ref:`index <all-salt.thorium>`)               ``thorium``               ``thorium_dirs``
 Tokens       ``salt.tokens``                                                  ``tokens``                ``tokens_dirs``
diff --git a/salt/modules/saltutil.py b/salt/modules/saltutil.py
index ecf467046aa2..bdfcea367cd4 100644
--- a/salt/modules/saltutil.py
+++ b/salt/modules/saltutil.py
@@ -86,9 +86,7 @@ def _get_top_file_envs():
                 else:
                     envs = "base"
             except SaltRenderError as exc:
-                raise CommandExecutionError(
-                    "Unable to render top file(s): {}".format(exc)
-                )
+                raise CommandExecutionError(f"Unable to render top file(s): {exc}")
         __context__["saltutil._top_file_envs"] = envs
         return envs
 
@@ -164,7 +162,7 @@ def update(version=None):
         try:
             version = app.find_update()
         except urllib.error.URLError as exc:
-            ret["_error"] = "Could not connect to update_url. Error: {}".format(exc)
+            ret["_error"] = f"Could not connect to update_url. Error: {exc}"
             return ret
     if not version:
         ret["_error"] = "No updates available"
@@ -172,21 +170,21 @@ def update(version=None):
     try:
         app.fetch_version(version)
     except EskyVersionError as exc:
-        ret["_error"] = "Unable to fetch version {}. Error: {}".format(version, exc)
+        ret["_error"] = f"Unable to fetch version {version}. Error: {exc}"
         return ret
     try:
         app.install_version(version)
     except EskyVersionError as exc:
-        ret["_error"] = "Unable to install version {}. Error: {}".format(version, exc)
+        ret["_error"] = f"Unable to install version {version}. Error: {exc}"
         return ret
     try:
         app.cleanup()
     except Exception as exc:  # pylint: disable=broad-except
-        ret["_error"] = "Unable to cleanup. Error: {}".format(exc)
+        ret["_error"] = f"Unable to cleanup. Error: {exc}"
     restarted = {}
     for service in __opts__["update_restart_services"]:
         restarted[service] = __salt__["service.restart"](service)
-    ret["comment"] = "Updated from {} to {}".format(oldversion, version)
+    ret["comment"] = f"Updated from {oldversion} to {version}"
     ret["restarted"] = restarted
     return ret
 
@@ -1016,6 +1014,55 @@ def sync_executors(
     return ret
 
 
+def sync_wrapper(
+    saltenv=None, refresh=True, extmod_whitelist=None, extmod_blacklist=None
+):
+    """
+    .. versionadded:: 3007.0
+
+    Sync salt-ssh wrapper modules from ``salt://_wrapper`` to the minion.
+
+    saltenv
+        The fileserver environment from which to sync. To sync from more than
+        one environment, pass a comma-separated list.
+
+        If not passed, then all environments configured in the :ref:`top files
+        <states-top>` will be checked for wrappers to sync. If no top files
+        are found, then the ``base`` environment will be synced.
+
+    refresh : True
+        If ``True``, refresh the available wrapper modules on the minion.
+        This refresh will be performed even if no wrappers are synced.
+        Set to ``False`` to prevent this refresh.
+
+    extmod_whitelist : None
+        comma-seperated list of modules to sync
+
+    extmod_blacklist : None
+        comma-seperated list of modules to blacklist based on type
+
+    .. note::
+        This function will raise an error if executed on a traditional (i.e.
+        not masterless) minion.
+
+    CLI Examples:
+
+    .. code-block:: bash
+
+        salt '*' saltutil.sync_wrapper
+        salt '*' saltutil.sync_wrapper saltenv=dev
+        salt '*' saltutil.sync_wrapper saltenv=base,dev
+    """
+    if __opts__["file_client"] != "local":
+        raise CommandExecutionError(
+            "Wrapper modules can only be synced to masterless minions"
+        )
+    ret = _sync("wrapper", saltenv, extmod_whitelist, extmod_blacklist)
+    if refresh:
+        refresh_modules()
+    return ret
+
+
 def sync_all(
     saltenv=None,
     refresh=True,
@@ -1105,6 +1152,9 @@ def sync_all(
     ret["matchers"] = sync_matchers(saltenv, False, extmod_whitelist, extmod_blacklist)
     if __opts__["file_client"] == "local":
         ret["pillar"] = sync_pillar(saltenv, False, extmod_whitelist, extmod_blacklist)
+        ret["wrapper"] = sync_wrapper(
+            saltenv, False, extmod_whitelist, extmod_blacklist
+        )
     if refresh:
         # we don't need to call refresh_modules here because it's done by refresh_pillar
         refresh_pillar(clean_cache=clean_pillar_cache)
@@ -1402,7 +1452,7 @@ def find_cached_job(jid):
                 " enable cache_jobs on this minion"
             )
         else:
-            return "Local jobs cache directory {} not found".format(job_dir)
+            return f"Local jobs cache directory {job_dir} not found"
     path = os.path.join(job_dir, "return.p")
     with salt.utils.files.fopen(path, "rb") as fp_:
         buf = fp_.read()
@@ -1604,7 +1654,7 @@ def _exec(
     kwarg,
     batch=False,
     subset=False,
-    **kwargs
+    **kwargs,
 ):
     fcn_ret = {}
     seen = 0
@@ -1660,7 +1710,7 @@ def cmd(
     ret="",
     kwarg=None,
     ssh=False,
-    **kwargs
+    **kwargs,
 ):
     """
     .. versionchanged:: 2017.7.0
@@ -1681,7 +1731,7 @@ def cmd(
     # if return is empty, we may have not used the right conf,
     # try with the 'minion relative master configuration counter part
     # if available
-    master_cfgfile = "{}master".format(cfgfile[:-6])  # remove 'minion'
+    master_cfgfile = f"{cfgfile[:-6]}master"  # remove 'minion'
     if (
         not fcn_ret
         and cfgfile.endswith("{}{}".format(os.path.sep, "minion"))
@@ -1704,7 +1754,7 @@ def cmd_iter(
     ret="",
     kwarg=None,
     ssh=False,
-    **kwargs
+    **kwargs,
 ):
     """
     .. versionchanged:: 2017.7.0
diff --git a/salt/runners/saltutil.py b/salt/runners/saltutil.py
index 1b777a0865f7..b9ca1682144e 100644
--- a/salt/runners/saltutil.py
+++ b/salt/runners/saltutil.py
@@ -146,6 +146,11 @@ def sync_all(saltenv="base", extmod_whitelist=None, extmod_blacklist=None):
         extmod_whitelist=extmod_whitelist,
         extmod_blacklist=extmod_blacklist,
     )
+    ret["wrapper"] = sync_wrapper(
+        saltenv=saltenv,
+        extmod_whitelist=extmod_whitelist,
+        extmod_blacklist=extmod_blacklist,
+    )
     ret["roster"] = sync_roster(
         saltenv=saltenv,
         extmod_whitelist=extmod_whitelist,
@@ -835,3 +840,34 @@ def sync_executors(saltenv="base", extmod_whitelist=None, extmod_blacklist=None)
         extmod_whitelist=extmod_whitelist,
         extmod_blacklist=extmod_blacklist,
     )[0]
+
+
+def sync_wrapper(saltenv="base", extmod_whitelist=None, extmod_blacklist=None):
+    """
+    .. versionadded:: 3000
+
+    Sync salt-ssh wrapper modules from ``salt://_wrapper`` to the master.
+
+    saltenv : base
+        The fileserver environment from which to sync. To sync from more than
+        one environment, pass a comma-separated list.
+
+    extmod_whitelist : None
+        comma-seperated list of modules to sync
+
+    extmod_blacklist : None
+        comma-seperated list of modules to blacklist based on type
+
+    CLI Example:
+
+    .. code-block:: bash
+
+        salt-run saltutil.sync_wrapper
+    """
+    return salt.utils.extmods.sync(
+        __opts__,
+        "wrapper",
+        saltenv=saltenv,
+        extmod_whitelist=extmod_whitelist,
+        extmod_blacklist=extmod_blacklist,
+    )[0]
diff --git a/salt/states/saltutil.py b/salt/states/saltutil.py
index 9442c5019ccc..e99ace9be77c 100644
--- a/salt/states/saltutil.py
+++ b/salt/states/saltutil.py
@@ -29,18 +29,18 @@ def _sync_single(name, module, **kwargs):
 
     if __opts__["test"]:
         ret["result"] = None
-        ret["comment"] = "saltutil.sync_{} would have been run".format(module)
+        ret["comment"] = f"saltutil.sync_{module} would have been run"
         return ret
 
     try:
-        sync_status = __salt__["saltutil.sync_{}".format(module)](**kwargs)
+        sync_status = __salt__[f"saltutil.sync_{module}"](**kwargs)
         if sync_status:
             ret["changes"][module] = sync_status
-            ret["comment"] = "Updated {}.".format(module)
+            ret["comment"] = f"Updated {module}."
     except Exception as e:  # pylint: disable=broad-except
         log.error("Failed to run saltutil.sync_%s: %s", module, e)
         ret["result"] = False
-        ret["comment"] = "Failed to run sync_{}: {}".format(module, e)
+        ret["comment"] = f"Failed to run sync_{module}: {e}"
         return ret
 
     if not ret["changes"]:
@@ -76,7 +76,7 @@ def sync_all(name, **kwargs):
     except Exception as e:  # pylint: disable=broad-except
         log.error("Failed to run saltutil.sync_all: %s", e)
         ret["result"] = False
-        ret["comment"] = "Failed to run sync_all: {}".format(e)
+        ret["comment"] = f"Failed to run sync_all: {e}"
         return ret
 
     if not ret["changes"]:
@@ -349,3 +349,17 @@ def sync_serializers(name, **kwargs):
             - refresh: True
     """
     return _sync_single(name, "serializers", **kwargs)
+
+
+def sync_wrapper(name, **kwargs):
+    """
+    Performs the same task as saltutil.sync_wrapper module
+    See :mod:`saltutil module for full list of options <salt.modules.saltutil>`
+
+    .. code-block:: yaml
+
+        sync_everything:
+          saltutil.sync_wrapper:
+            - refresh: True
+    """
+    return _sync_single(name, "wrapper", **kwargs)
diff --git a/tests/pytests/integration/runners/test_saltutil.py b/tests/pytests/integration/runners/test_saltutil.py
index 179d1b7bf6a7..dfa3b648e60d 100644
--- a/tests/pytests/integration/runners/test_saltutil.py
+++ b/tests/pytests/integration/runners/test_saltutil.py
@@ -31,6 +31,7 @@ def get_module_types():
         "tokens",
         "serializers",
         "executors",
+        "wrapper",
         "roster",
     ]
     return module_types
@@ -66,6 +67,7 @@ def module_sync_functions():
         "tokens": "eauth_tokens",
         "serializers": "serializers",
         "executors": "executors",
+        "wrapper": "wrapper",
         "roster": "roster",
     }
 
@@ -76,7 +78,7 @@ def test_sync(
     """
     Ensure modules are synced when various sync functions are called
     """
-    module_name = "hello_sync_{}".format(module_type)
+    module_name = f"hello_sync_{module_type}"
     module_contents = """
 def __virtual__():
     return "hello"
@@ -85,17 +87,17 @@ def world():
     return "world"
 """
 
-    test_moduledir = salt_master.state_tree.base.paths[0] / "_{}".format(module_type)
+    test_moduledir = salt_master.state_tree.base.paths[0] / f"_{module_type}"
     test_moduledir.mkdir(parents=True, exist_ok=True)
     module_tempfile = salt_master.state_tree.base.temp_file(
-        "_{}/{}.py".format(module_type, module_name), module_contents
+        f"_{module_type}/{module_name}.py", module_contents
     )
 
     with module_tempfile, test_moduledir:
-        salt_cmd = "saltutil.sync_{}".format(module_sync_functions[module_type])
+        salt_cmd = f"saltutil.sync_{module_sync_functions[module_type]}"
         ret = salt_run_cli.run(salt_cmd)
         assert ret.returncode == 0
-        assert "{}.hello".format(module_type) in ret.stdout
+        assert f"{module_type}.hello" in ret.stdout
 
 
 def _write_module_dir_and_file(module_type, salt_minion, salt_master):
@@ -111,11 +113,11 @@ def world():
     return "world"
 """
 
-    test_moduledir = salt_master.state_tree.base.paths[0] / "_{}".format(module_type)
+    test_moduledir = salt_master.state_tree.base.paths[0] / f"_{module_type}"
     test_moduledir.mkdir(parents=True, exist_ok=True)
 
     module_tempfile = salt_master.state_tree.base.temp_file(
-        "_{}/{}.py".format(module_type, module_name), module_contents
+        f"_{module_type}/{module_name}.py", module_contents
     )
 
     return module_tempfile
@@ -139,4 +141,4 @@ def test_sync_all(salt_run_cli, salt_minion, salt_master):
 
         assert ret.returncode == 0
         for module_type in get_module_types():
-            assert "{}.hello".format(module_type) in ret.stdout
+            assert f"{module_type}.hello" in ret.stdout
diff --git a/tests/pytests/unit/states/test_saltutil.py b/tests/pytests/unit/states/test_saltutil.py
index c0d251889f66..23f91caa0540 100644
--- a/tests/pytests/unit/states/test_saltutil.py
+++ b/tests/pytests/unit/states/test_saltutil.py
@@ -36,6 +36,7 @@ def test_saltutil_sync_all_nochange():
         "pillar": [],
         "matchers": [],
         "serializers": [],
+        "wrapper": [],
     }
     state_id = "somename"
     state_result = {
@@ -71,6 +72,7 @@ def test_saltutil_sync_all_test():
         "pillar": [],
         "matchers": [],
         "serializers": [],
+        "wrapper": [],
     }
     state_id = "somename"
     state_result = {
@@ -107,6 +109,7 @@ def test_saltutil_sync_all_change():
         "pillar": [],
         "matchers": [],
         "serializers": [],
+        "wrapper": [],
     }
     state_id = "somename"
     state_result = {
@@ -139,4 +142,4 @@ def test_saltutil_sync_states_should_match_saltutil_module():
     for fn in module_functions:
         assert (
             fn in state_functions
-        ), "modules.saltutil.{} has no matching state in states.saltutil".format(fn)
+        ), f"modules.saltutil.{fn} has no matching state in states.saltutil"


Code review comments:
diff_hunk: @@ -835,3 +840,34 @@ def sync_executors(saltenv="base", extmod_whitelist=None, extmod_blacklist=None)
         extmod_whitelist=extmod_whitelist,
         extmod_blacklist=extmod_blacklist,
     )[0]
+
+
+def sync_wrapper(saltenv="base", extmod_whitelist=None, extmod_blacklist=None):
+    """
+    .. versionadded:: 3000
path: salt/runners/saltutil.py
text: @user1:
```suggestion
    .. versionadded:: 3007.0
```