Stop loading utils modules into __utils__ if they don't define `__v…

…irtual__`

Refs saltstack#62191

Signed-off-by: Pedro Algarvio <palgarvio@vmware.com>

salt/loader/__init__.py

@@ -146,7 +146,7 @@ def _module_dirs(
     ext_type_types = []
     if ext_dirs:
         if ext_type_dirs is None:
-            ext_type_dirs = "{}_dirs".format(tag)
+            ext_type_dirs = f"{tag}_dirs"
         if ext_type_dirs in opts:
             ext_type_types.extend(opts[ext_type_dirs])
         if ext_type_dirs and load_extensions is True:
@@ -246,7 +246,7 @@ def _module_dirs(
             cli_module_dirs.insert(0, maybe_dir)
             continue
 
-        maybe_dir = os.path.join(_dir, "_{}".format(ext_type))
+        maybe_dir = os.path.join(_dir, f"_{ext_type}")
         if os.path.isdir(maybe_dir):
             cli_module_dirs.insert(0, maybe_dir)
 
@@ -539,6 +539,7 @@ def utils(
         pack={"__context__": context, "__proxy__": proxy or {}},
         pack_self=pack_self,
         loaded_base_name=loaded_base_name,
+        _ast_dunder_virtual_inspect=True,
         _only_pack_properly_namespaced_functions=False,
     )
 
@@ -1208,7 +1209,7 @@ def grains(opts, force_refresh=False, proxy=None, context=None, loaded_base_name
                     import salt.modules.cmdmod
 
                     # Make sure cache file isn't read-only
-                    salt.modules.cmdmod._run_quiet('attrib -R "{}"'.format(cfn))
+                    salt.modules.cmdmod._run_quiet(f'attrib -R "{cfn}"')
                 with salt.utils.files.fopen(cfn, "w+b") as fp_:
                     try:
                         salt.payload.dump(grains_data, fp_)

salt/loader/lazy.py

@@ -1,3 +1,4 @@
+import ast
 import copy
 import functools
 import importlib
@@ -149,7 +150,7 @@ def __call__(self, *args, **kwargs):
         return self.loader.run(run_func, *args, **kwargs)
 
     def __repr__(self):
-        return "<{} name={!r}>".format(self.__class__.__name__, self.name)
+        return f"<{self.__class__.__name__} name={self.name!r}>"
 
 
 class LoadedMod:
@@ -172,10 +173,10 @@ def __getattr__(self, name):
         Run the wrapped function in the loader's context.
         """
         try:
-            return self.loader["{}.{}".format(self.mod, name)]
+            return self.loader[f"{self.mod}.{name}"]
         except KeyError:
             raise AttributeError(
-                "No attribute by the name of {} was found on {}".format(name, self.mod)
+                f"No attribute by the name of {name} was found on {self.mod}"
             )
 
     def __repr__(self):
@@ -230,7 +231,8 @@ def __init__(
         virtual_funcs=None,
         extra_module_dirs=None,
         pack_self=None,
-        # Once we get rid of __utils__, the keyword argument bellow should be removed
+        # Once we get rid of __utils__, the keyword arguments bellow should be removed
+        _ast_dunder_virtual_inspect=False,
         _only_pack_properly_namespaced_functions=True,
     ):  # pylint: disable=W0231
         """
@@ -261,6 +263,7 @@ def __init__(
         self._gc_finalizer = None
         self.loaded_base_name = loaded_base_name or LOADED_BASE_NAME
         self.mod_type_check = mod_type_check or _mod_type
+        self._ast_dunder_virtual_inspect = _ast_dunder_virtual_inspect
         self._only_pack_properly_namespaced_functions = (
             _only_pack_properly_namespaced_functions
         )
@@ -311,10 +314,10 @@ def __init__(
 
         super().__init__()  # late init the lazy loader
         # create all of the import namespaces
-        _generate_module("{}.int".format(self.loaded_base_name))
-        _generate_module("{}.int.{}".format(self.loaded_base_name, tag))
-        _generate_module("{}.ext".format(self.loaded_base_name))
-        _generate_module("{}.ext.{}".format(self.loaded_base_name, tag))
+        _generate_module(f"{self.loaded_base_name}.int")
+        _generate_module(f"{self.loaded_base_name}.int.{tag}")
+        _generate_module(f"{self.loaded_base_name}.ext")
+        _generate_module(f"{self.loaded_base_name}.ext.{tag}")
 
     def clean_modules(self):
         """
@@ -372,19 +375,19 @@ def missing_fun_string(self, function_name):
         """
         mod_name = function_name.split(".")[0]
         if mod_name in self.loaded_modules:
-            return "'{}' is not available.".format(function_name)
+            return f"'{function_name}' is not available."
         else:
             try:
                 reason = self.missing_modules[mod_name]
             except KeyError:
-                return "'{}' is not available.".format(function_name)
+                return f"'{function_name}' is not available."
             else:
                 if reason is not None:
                     return "'{}' __virtual__ returned False: {}".format(
                         mod_name, reason
                     )
                 else:
-                    return "'{}' __virtual__ returned False".format(mod_name)
+                    return f"'{mod_name}' __virtual__ returned False"
 
     def _refresh_file_mapping(self):
         """
@@ -497,7 +500,7 @@ def _replace_pre_ext(obj):
                         for suffix in self.suffix_order:
                             if "" == suffix:
                                 continue  # Next suffix (__init__ must have a suffix)
-                            init_file = "__init__{}".format(suffix)
+                            init_file = f"__init__{suffix}"
                             if init_file in subfiles:
                                 break
                         else:
@@ -651,6 +654,24 @@ def __clean_sys_path(self):
     def _load_module(self, name):
         mod = None
         fpath, suffix = self.file_mapping[name][:2]
+        if suffix == ".py" and self._ast_dunder_virtual_inspect:
+            with salt.utils.files.fopen(fpath) as rfh:
+                tree = ast.parse(rfh.read())
+                for node in tree.body:
+                    if not isinstance(node, ast.FunctionDef):
+                        continue
+                    if node.name == "__virtual__":
+                        # The module defines a __virtual__ function.
+                        # Continue regular loading.
+                        break
+                else:
+                    # The module did not define a __virtual__ function, stop
+                    # processing the module
+                    log.debug(
+                        "Not loading %r because it does not define a __virtual__ function",
+                        name,
+                    )
+                    return False
         # if the fpath has `.cpython-3x` in it, but the running Py version
         # is 3.y, the following will cause us to return immediately and we won't try to import this .pyc.
         # This is for the unusual case where several Python versions share a single
@@ -990,7 +1011,7 @@ def _load_module(self, name):
                 try:
                     full_funcname = ".".join((tgt_mod, funcname))
                 except TypeError:
-                    full_funcname = "{}.{}".format(tgt_mod, funcname)
+                    full_funcname = f"{tgt_mod}.{funcname}"
                 # Save many references for lookups
                 # Careful not to overwrite existing (higher priority) functions
                 if full_funcname not in self._dict:
@@ -1017,7 +1038,7 @@ def _load(self, key):
         if not isinstance(key, str):
             raise KeyError("The key must be a string.")
         if "." not in key:
-            raise KeyError("The key '{}' should contain a '.'".format(key))
+            raise KeyError(f"The key '{key}' should contain a '.'")
         mod_name, _ = key.split(".", 1)
         with self._lock:
             # It is possible that the key is in the dictionary after

tests/pytests/functional/loader/test_utils.py

@@ -0,0 +1,48 @@
+import warnings
+
+import pytest
+from saltfactories.utils import random_string
+
+import salt.config
+import salt.loader
+from salt.loader.lazy import LazyLoader
+
+
+@pytest.fixture(scope="module")
+def loaded_base_name():
+    return random_string(f"{__name__}.", digits=False, uppercase=False)
+
+
+@pytest.fixture
+def loader(minion_opts, loaded_base_name):
+    loader = salt.loader.utils(minion_opts, loaded_base_name=loaded_base_name)
+    try:
+        with warnings.catch_warnings():
+            warnings.simplefilter("ignore")
+            # Force loading all functions
+            list(loader)
+            yield loader
+    finally:
+        if not isinstance(loader, LazyLoader):
+            for loaded_func in loader.values():
+                loader = loaded_func.loader
+                break
+        if isinstance(loader, LazyLoader):
+            loader.clean_modules()
+
+
+def test_ast_inspect_loading(loader):
+    loaded_functions = list(loader)
+    # Check that utils modules defining __virtual__ are loaded by the loader
+    # Of course, we can only check modules which load in most/all circumstances.
+    assert "ansible.targets" in loaded_functions
+    # However, modules which do not define a __virtual__ function should not load,
+    # at all!
+    modules_which_do_not_define_dunder_virtual = (
+        "args.",
+        "environment.",
+        "entrypoints.",
+        "process.",
+    )
+    for loaded_func in loaded_functions:
+        assert not loaded_func.startswith(modules_which_do_not_define_dunder_virtual)