diff --git a/changelog/69971.fixed.md b/changelog/69971.fixed.md new file mode 100644 index 000000000000..6b4cc9c61d7b --- /dev/null +++ b/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 diff --git a/salt/modules/postgres.py b/salt/modules/postgres.py index b0503385e63f..5a944fd2bf74 100644 --- a/salt/modules/postgres.py +++ b/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) diff --git a/tests/pytests/unit/modules/test_postgres.py b/tests/pytests/unit/modules/test_postgres.py index e7fa5913b802..63ea1f3ca508 100644 --- a/tests/pytests/unit/modules/test_postgres.py +++ b/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,11 +273,14 @@ 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) @@ -212,6 +288,7 @@ def test__runpsql_with_timeout(): 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})