fix postgres module to use config.get

changelog/69971.fixed.md

@@ -0,0 +1,2 @@
+- Made postgres module to use config.get instead of relying on config.option
+- Made tests for postgres module to ensure nested keys are used and backwardly compatible

salt/modules/postgres.py

@@ -140,7 +140,9 @@ def _find_pg_binary(util):
 
     Helper function to locate various psql related binaries
     """
-    pg_bin_dir = __salt__["config.option"]("postgres.bins_dir")
+    pg_bin_dir = __salt__["config.option"]("postgres.bins_dir") or __salt__[
+        "config.get"
+    ]("postgres:bins_dir")
     if pg_bin_dir:
         util_bin = salt.utils.path.which(os.path.join(pg_bin_dir, util))
         if util_bin:
@@ -153,16 +155,15 @@ def _run_psql(cmd, runas=None, password=None, host=None, port=None, user=None):
     Helper function to call psql, because the password requirement
     makes this too much code to be repeated in each function below
     """
-    kwargs = {
-        "reset_system_locale": False,
-        "clean_env": True,
-        "timeout": __salt__["config.option"](
-            "postgres.timeout", default=_DEFAULT_COMMAND_TIMEOUT_SECS
-        ),
-    }
+    timeout = __salt__["config.option"]("postgres.timeout") or __salt__["config.get"](
+        "postgres:timeout", default=_DEFAULT_COMMAND_TIMEOUT_SECS
+    )
+    kwargs = {"reset_system_locale": False, "clean_env": True, "timeout": timeout}
     if runas is None:
         if not host:
-            host = __salt__["config.option"]("postgres.host")
+            host = __salt__["config.option"]("postgres.host") or __salt__["config.get"](
+                "postgres:host"
+            )
         if not host or host.startswith("/"):
             if "FreeBSD" in __grains__["os_family"]:
                 runas = "postgres"
@@ -178,7 +179,9 @@ def _run_psql(cmd, runas=None, password=None, host=None, port=None, user=None):
         kwargs["runas"] = runas
 
     if password is None:
-        password = __salt__["config.option"]("postgres.pass")
+        password = __salt__["config.option"]("postgres.pass") or __salt__["config.get"](
+            "postgres:pass"
+        )
     if password is not None:
         pgpassfile = salt.utils.files.mkstemp(text=True)
         with salt.utils.files.fopen(pgpassfile, "w") as fp_:
@@ -259,13 +262,11 @@ def _run_initdb(
             __salt__["file.chown"](pgpassfile, runas, "")
         cmd.extend([f"--pwfile={pgpassfile}"])
 
-    kwargs = dict(
-        runas=runas,
-        clean_env=True,
-        timeout=__salt__["config.option"](
-            "postgres.timeout", default=_DEFAULT_COMMAND_TIMEOUT_SECS
-        ),
+    timeout = __salt__["config.option"]("postgres.timeout") or __salt__["config.get"](
+        "postgres:timeout", default=_DEFAULT_COMMAND_TIMEOUT_SECS
     )
+
+    kwargs = dict(runas=runas, clean_env=True, timeout=timeout)
     cmdstr = shlex.join(cmd)
     ret = __salt__["cmd.run_all"](cmdstr, python_shell=False, **kwargs)
 
@@ -344,13 +345,21 @@ def _connection_defaults(user=None, host=None, port=None, maintenance_db=None):
     values assigned to missing values.
     """
     if not user:
-        user = __salt__["config.option"]("postgres.user")
+        user = __salt__["config.option"]("postgres.user") or __salt__["config.get"](
+            "postgres:user"
+        )
     if not host:
-        host = __salt__["config.option"]("postgres.host")
+        host = __salt__["config.option"]("postgres.host") or __salt__["config.get"](
+            "postgres:host"
+        )
     if not port:
-        port = __salt__["config.option"]("postgres.port")
+        port = __salt__["config.option"]("postgres.port") or __salt__["config.get"](
+            "postgres:port"
+        )
     if not maintenance_db:
-        maintenance_db = __salt__["config.option"]("postgres.maintenance_db")
+        maintenance_db = __salt__["config.option"](
+            "postgres.maintenance_db"
+        ) or __salt__["config.get"]("postgres:maintenance_db")
 
     return (user, host, port, maintenance_db)
 

tests/pytests/unit/modules/test_postgres.py

@@ -72,6 +72,7 @@ def configure_loader_modules():
         postgres: {
             "__grains__": {"os_family": "Linux"},
             "__salt__": {
+                "config.get": MagicMock(),
                 "config.option": MagicMock(),
                 "cmd.run_all": MagicMock(),
                 "file.chown": MagicMock(),
@@ -186,9 +187,81 @@ def test_has_privileges_with_function(get_test_privileges_list_function_csv):
         )
 
 
+def test__connection_defaults():
+    """
+    test to ensure toplevel keys for connection defaults are backwards compatible
+    """
+
+    def config_option(key, default=None):
+        if key in config:
+            return config[key]
+        return default
+
+    config_option_mock = MagicMock(side_effect=config_option)
+    postgres_opts = {
+        "config.get": configmod.get,
+        "config.option": config_option_mock,
+    }
+    config = {
+        "postgres.user": "user",
+        "postgres.host": "host",
+        "postgres.port": 9999,
+        "postgres.maintenance_db": "maintenance_db",
+    }
+
+    with patch.dict(postgres.__salt__, postgres_opts):
+        with patch.dict(configmod.__opts__, config):
+            result = postgres._connection_defaults()
+            assert result == ("user", "host", 9999, "maintenance_db")
+            assert config_option_mock.call_count == 4
+
+
+def test__connection_defaults_nested():
+    """
+    test to ensure nested keys for connection defaults work
+    """
+
+    def config_get(key, default=None, delimiter=":"):
+        current_config_level = config
+        for split_key in key.split(delimiter):
+            if split_key in current_config_level:
+                current_config_level = current_config_level[split_key]
+            else:
+                return default
+        if current_config_level == config:
+            return default
+        return current_config_level
+
+    def config_option(key, default=None):
+        if key in config:
+            return config[key]
+        return default
+
+    config_get_mock = MagicMock(side_effect=config_get)
+    postgres_opts = {
+        "config.get": config_get_mock,
+        "config.option": MagicMock(side_effect=config_option),
+    }
+    config = {
+        "postgres": {
+            "user": "user",
+            "host": "host",
+            "port": 9999,
+            "maintenance_db": "maintenance_db",
+        },
+    }
+
+    with patch.dict(postgres.__salt__, postgres_opts):
+        with patch.dict(configmod.__opts__, config):
+            result = postgres._connection_defaults()
+            assert result == ("user", "host", 9999, "maintenance_db")
+            assert config_get_mock.call_count == 4
+
+
 def test__runpsql_with_timeout():
     cmd_run_mock = MagicMock()
     postgres_opts = {
+        "config.get": configmod.get,
         "config.option": configmod.option,
         "cmd.run_all": cmd_run_mock,
     }
@@ -200,18 +273,22 @@ def test__runpsql_with_timeout():
     }
     with patch.dict(postgres.__salt__, postgres_opts):
         with patch.dict(
-            configmod.__opts__, {"postgres.timeout": 60, "postgres.pass": None}
+            configmod.__opts__,
+            {"postgres.timeout": 60, "postgres.pass": None, "postgres": {"pass": None}},
         ):
             postgres._run_psql("fakecmd", runas="saltuser")
             cmd_run_mock.assert_called_with("fakecmd", timeout=60, **kwargs)
-        with patch.dict(configmod.__opts__, {"postgres.pass": None}):
+        with patch.dict(
+            configmod.__opts__, {"postgres.pass": None, "postgres": {"pass": None}}
+        ):
             postgres._run_psql("fakecmd", runas="saltuser")
             cmd_run_mock.assert_called_with("fakecmd", timeout=0, **kwargs)
 
 
 def test__run_initdb_with_timeout():
     cmd_run_mock = MagicMock(return_value={})
     postgres_opts = {
+        "config.get": configmod.get,
         "config.option": configmod.option,
         "cmd.run_all": cmd_run_mock,
     }
@@ -224,11 +301,18 @@ def test__run_initdb_with_timeout():
     with patch.dict(postgres.__salt__, postgres_opts):
         with patch.object(postgres, "_find_pg_binary", return_value="/fake/path"):
             with patch.dict(
-                configmod.__opts__, {"postgres.timeout": 60, "postgres.pass": None}
+                configmod.__opts__,
+                {
+                    "postgres.timeout": 60,
+                    "postgres.pass": None,
+                    "postgres": {"pass": None},
+                },
             ):
                 postgres._run_initdb("fakename", runas="saltuser")
                 cmd_run_mock.assert_called_with(cmd_str, timeout=60, **kwargs)
-            with patch.dict(configmod.__opts__, {"postgres.pass": None}):
+            with patch.dict(
+                configmod.__opts__, {"postgres.pass": None, "postgres": {"pass": None}}
+            ):
                 postgres._run_initdb("fakename", runas="saltuser")
                 cmd_run_mock.assert_called_with(cmd_str, timeout=0, **kwargs)
 
@@ -2663,6 +2747,34 @@ def which_side_effect(path):
     assert result == "/usr/pgsql-15/bin/psql"
 
 
+def test_find_pg_binary_bins_dir_nested_preferred_over_path():
+    """
+    When postgres.bins_dir is configured (nested), _find_pg_binary should return
+    the binary from bins_dir even when a psql binary is also present on the
+    system PATH (GitHub issue #53190).
+    """
+
+    def which_side_effect(path):
+        if path == "/usr/pgsql-15/bin/psql":
+            return "/usr/pgsql-15/bin/psql"
+        if path == "psql":
+            return "/usr/bin/psql"
+        return None
+
+    def config_option(key, default=None, **kwargs):
+        return default
+
+    with patch.dict(
+        postgres.__salt__,
+        {
+            "config.get": MagicMock(return_value="/usr/pgsql-15/bin"),
+            "config.option": MagicMock(side_effect=config_option),
+        },
+    ), patch("salt.utils.path.which", side_effect=which_side_effect):
+        result = postgres._find_pg_binary("psql")
+    assert result == "/usr/pgsql-15/bin/psql"
+
+
 def test_find_pg_binary_bins_dir_used_when_not_on_path():
     """
     When postgres.bins_dir is configured and psql is not on the system PATH,
@@ -2682,6 +2794,31 @@ def which_side_effect(path):
     assert result == "/usr/pgsql-15/bin/psql"
 
 
+def test_find_pg_binary_bins_dir_nested_used_when_not_on_path():
+    """
+    When postgres.bins_dir is configured (nested) and psql is not on the system
+    PATH, _find_pg_binary should still find the binary via bins_dir.
+    """
+
+    def which_side_effect(path):
+        if path == "/usr/pgsql-15/bin/psql":
+            return "/usr/pgsql-15/bin/psql"
+        return None
+
+    def config_option(key, default=None, **kwargs):
+        return default
+
+    with patch.dict(
+        postgres.__salt__,
+        {
+            "config.get": MagicMock(return_value="/usr/pgsql-15/bin"),
+            "config.option": MagicMock(side_effect=config_option),
+        },
+    ), patch("salt.utils.path.which", side_effect=which_side_effect):
+        result = postgres._find_pg_binary("psql")
+    assert result == "/usr/pgsql-15/bin/psql"
+
+
 def test_find_pg_binary_falls_back_to_path_when_bins_dir_not_set():
     """
     When postgres.bins_dir is not configured, _find_pg_binary should fall
@@ -2695,6 +2832,26 @@ def test_find_pg_binary_falls_back_to_path_when_bins_dir_not_set():
     assert result == "/usr/bin/psql"
 
 
+def test_find_pg_binary_falls_back_to_path_when_bins_dir_nested_not_set():
+    """
+    When postgres.bins_dir is not configured (nested), _find_pg_binary should
+    fall back to the system PATH (regression guard).
+    """
+
+    def config_option(key, default=None, **kwargs):
+        return default
+
+    with patch.dict(
+        postgres.__salt__,
+        {
+            "config.get": MagicMock(return_value="/usr/pgsql-15/bin"),
+            "config.option": MagicMock(side_effect=config_option),
+        },
+    ), patch("salt.utils.path.which", MagicMock(return_value="/usr/bin/psql")):
+        result = postgres._find_pg_binary("psql")
+    assert result == "/usr/bin/psql"
+
+
 def test_find_pg_binary_falls_back_to_path_when_not_in_bins_dir():
     """
     When postgres.bins_dir is configured but the binary is not found there,
@@ -2714,6 +2871,31 @@ def which_side_effect(path):
     assert result == "/usr/bin/psql"
 
 
+def test_find_pg_binary_falls_back_to_path_when_not_in_bins_dir_nested():
+    """
+    When postgres.bins_dir is configured (nested) but the binary is not found
+    there, _find_pg_binary should fall back to the system PATH.
+    """
+
+    def which_side_effect(path):
+        if path == "psql":
+            return "/usr/bin/psql"
+        return None
+
+    def config_option(key, default=None, **kwargs):
+        return default
+
+    with patch.dict(
+        postgres.__salt__,
+        {
+            "config.get": MagicMock(return_value="/usr/pgsql-15/bin"),
+            "config.option": MagicMock(side_effect=config_option),
+        },
+    ), patch("salt.utils.path.which", side_effect=which_side_effect):
+        result = postgres._find_pg_binary("psql")
+    assert result == "/usr/bin/psql"
+
+
 def test_find_pg_binary_returns_none_when_not_found_anywhere():
     """
     When psql cannot be found in bins_dir or on the system PATH,
@@ -2727,6 +2909,26 @@ def test_find_pg_binary_returns_none_when_not_found_anywhere():
     assert result is None
 
 
+def test_find_pg_binary_returns_none_when_not_found_anywhere_nested():
+    """
+    When psql cannot be found in bins_dir (nested) or on the system PATH,
+    _find_pg_binary should return None so the caller can handle the error.
+    """
+
+    def config_option(key, default=None, **kwargs):
+        return default
+
+    with patch.dict(
+        postgres.__salt__,
+        {
+            "config.get": MagicMock(return_value="/usr/pgsql-15/bin"),
+            "config.option": MagicMock(side_effect=config_option),
+        },
+    ), patch("salt.utils.path.which", MagicMock(return_value=None)):
+        result = postgres._find_pg_binary("psql")
+    assert result is None
+
+
 def test_tablespace_remove():
     with patch(
         "salt.modules.postgres._run_psql", Mock(return_value={"retcode": 0})