diff --git a/changelog/66603.fixed.md b/changelog/66603.fixed.md new file mode 100644 index 000000000000..a3bd1d948e9f --- /dev/null +++ b/changelog/66603.fixed.md @@ -0,0 +1,9 @@ +Fixed a regression where setting ``ipv6: true`` in the minion configuration +caused the minion to fail to start on Windows. Three IPC socket paths in the +TCP transport hardcoded ``AF_INET`` or ``127.0.0.1`` regardless of the IPv6 +setting: the IPC publish server/client addresses in ``salt.transport.base``, +the ``TCPPuller`` server socket, and the ``_TCPPubServerPublisher`` client +socket. On Windows, mixing an ``AF_INET6`` socket with the IPv4 loopback +address (or vice-versa) is rejected by the OS. All three paths now use +``::1`` with ``AF_INET6`` when ``ipv6: true`` is set, and ``127.0.0.1`` +with ``AF_INET`` otherwise. diff --git a/conf/master b/conf/master index ab37e280ef2b..b72d89ef00d1 100644 --- a/conf/master +++ b/conf/master @@ -16,6 +16,10 @@ # Whether the master should listen for IPv6 connections. If this is set to True, # the interface option must be adjusted, too. (For example: "interface: '::'") +# When enabled, Salt also uses the IPv6 loopback address (::1) for internal +# IPC connections between daemon processes instead of 127.0.0.1. On Windows, +# this is required because Windows does not permit binding an AF_INET6 socket +# to an IPv4 address. #ipv6: False # The tcp port used by the publisher: diff --git a/conf/minion b/conf/minion index 70cbe8934a45..323299a04feb 100644 --- a/conf/minion +++ b/conf/minion @@ -63,6 +63,10 @@ #master_failback_interval: 0 # Set whether the minion should connect to the master via IPv6: +# When set to True, Salt also uses the IPv6 loopback address (::1) for +# internal IPC connections instead of 127.0.0.1. On Windows, this is +# required because Windows does not permit binding an AF_INET6 socket to +# an IPv4 address. #ipv6: False # Set the number of seconds to wait before attempting to resolve diff --git a/doc/ref/configuration/master.rst b/doc/ref/configuration/master.rst index 06cf14bbf742..c147bdae1449 100644 --- a/doc/ref/configuration/master.rst +++ b/doc/ref/configuration/master.rst @@ -46,7 +46,12 @@ The local interface to bind to, must be an IP address. Default: ``False`` Whether the master should listen for IPv6 connections. If this is set to True, -the interface option must be adjusted too (for example: ``interface: '::'``) +the interface option must be adjusted too (for example: ``interface: '::'``). + +When enabled, Salt also uses the IPv6 loopback address (``::1``) for internal +IPC connections between daemon processes instead of ``127.0.0.1``. On Windows, +this is required because Windows does not permit binding an ``AF_INET6`` socket +to an IPv4 address. .. code-block:: yaml diff --git a/doc/ref/configuration/minion.rst b/doc/ref/configuration/minion.rst index 590320b0b052..8441baa2b662 100644 --- a/doc/ref/configuration/minion.rst +++ b/doc/ref/configuration/minion.rst @@ -123,6 +123,11 @@ Default: ``None`` Whether the master should be connected over IPv6. By default salt minion will try to automatically detect IPv6 connectivity to master. +When set to ``True``, Salt also uses the IPv6 loopback address (``::1``) for +internal IPC connections instead of ``127.0.0.1``. On Windows, this is +required because Windows does not permit binding an ``AF_INET6`` socket to an +IPv4 address. + .. code-block:: yaml ipv6: True diff --git a/pkg/common/conf/master b/pkg/common/conf/master index 4f0fa646d495..9869057fea59 100644 --- a/pkg/common/conf/master +++ b/pkg/common/conf/master @@ -16,6 +16,10 @@ # Whether the master should listen for IPv6 connections. If this is set to True, # the interface option must be adjusted, too. (For example: "interface: '::'") +# When enabled, Salt also uses the IPv6 loopback address (::1) for internal +# IPC connections between daemon processes instead of 127.0.0.1. On Windows, +# this is required because Windows does not permit binding an AF_INET6 socket +# to an IPv4 address. #ipv6: False # The tcp port used by the publisher: diff --git a/salt/transport/base.py b/salt/transport/base.py index 6b9e2636f017..c185d2cb7a3b 100644 --- a/salt/transport/base.py +++ b/salt/transport/base.py @@ -188,18 +188,30 @@ def _minion_hash(hash_type, minion_id): return hasher(salt.utils.stringutils.to_bytes(minion_id)).hexdigest()[:10] +def _ipc_loopback(opts): + """ + Return the loopback address appropriate for the configured IP family. + + IPC connections always bind to loopback. When ``ipv6: true`` is set the + socket layer creates an ``AF_INET6`` socket, so the address must be the + IPv6 loopback ``::1`` rather than the IPv4 ``127.0.0.1``; binding an + ``AF_INET6`` socket to ``127.0.0.1`` fails on Windows. + """ + return "::1" if opts.get("ipv6", False) else "127.0.0.1" + + def ipc_publish_client(node, opts, io_loop): # Default to TCP for now kwargs = {"transport": "tcp", "ssl": None} if opts["ipc_mode"] == "tcp": if node == "master": kwargs.update( - host="127.0.0.1", + host=_ipc_loopback(opts), port=int(opts["tcp_master_pub_port"]), ) else: kwargs.update( - host="127.0.0.1", + host=_ipc_loopback(opts), port=int(opts["tcp_pub_port"]), ) else: @@ -232,16 +244,16 @@ def ipc_publish_server(node, opts): if opts["ipc_mode"] == "tcp": if node == "master": kwargs.update( - pub_host="127.0.0.1", + pub_host=_ipc_loopback(opts), pub_port=int(opts["tcp_master_pub_port"]), - pull_host="127.0.0.1", + pull_host=_ipc_loopback(opts), pull_port=int(opts["tcp_master_pull_port"]), ) else: kwargs.update( - pub_host="127.0.0.1", + pub_host=_ipc_loopback(opts), pub_port=int(opts["tcp_pub_port"]), - pull_host="127.0.0.1", + pull_host=_ipc_loopback(opts), pull_port=int(opts["tcp_pull_port"]), ) else: diff --git a/salt/transport/tcp.py b/salt/transport/tcp.py index d51aa7bbf652..4f0f43b345d1 100644 --- a/salt/transport/tcp.py +++ b/salt/transport/tcp.py @@ -1222,7 +1222,8 @@ def start(self): self.sock = tornado.netutil.bind_unix_socket(self.path, self.mode) else: log.trace("IPCServer: binding to socket: %s:%s", self.host, self.port) - self.sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) + family = socket.AF_INET6 if ":" in (self.host or "") else socket.AF_INET + self.sock = socket.socket(family, socket.SOCK_STREAM) self.sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) self.sock.setblocking(0) self.sock.bind((self.host, self.port)) @@ -1652,7 +1653,7 @@ async def _connect(self, timeout=None): sock_addr = self.path log.debug("Publisher connecting to %s", self.path) else: - sock_type = socket.AF_INET + sock_type = socket.AF_INET6 if ":" in (self.host or "") else socket.AF_INET sock_addr = (self.host, self.port) log.debug("Publisher connecting to %s:%s", self.host, self.port) diff --git a/tests/pytests/unit/transport/test_base.py b/tests/pytests/unit/transport/test_base.py index 5d541073d676..dbd24a105ee5 100644 --- a/tests/pytests/unit/transport/test_base.py +++ b/tests/pytests/unit/transport/test_base.py @@ -7,13 +7,133 @@ import pytest import salt.transport.base -from tests.support.mock import patch +from tests.support.mock import MagicMock, patch pytestmark = [ pytest.mark.core_test, ] +# --------------------------------------------------------------------------- +# _ipc_loopback +# --------------------------------------------------------------------------- + + +def test_ipc_loopback_ipv4(): + assert salt.transport.base._ipc_loopback({}) == "127.0.0.1" + assert salt.transport.base._ipc_loopback({"ipv6": False}) == "127.0.0.1" + + +def test_ipc_loopback_ipv6(): + assert salt.transport.base._ipc_loopback({"ipv6": True}) == "::1" + + +# --------------------------------------------------------------------------- +# ipc_publish_client +# --------------------------------------------------------------------------- + +_IPC_TCP_OPTS = { + "ipc_mode": "tcp", + "ipv6": False, + "tcp_pub_port": 4510, + "tcp_pull_port": 4511, + "tcp_master_pub_port": 4512, + "tcp_master_pull_port": 4513, + "hash_type": "sha256", + "id": "test-minion", + "sock_dir": "/var/run/salt", +} + + +def test_ipc_publish_client_minion_ipv4(): + """ipc_publish_client uses 127.0.0.1 for minion when ipv6 is False.""" + opts = {**_IPC_TCP_OPTS, "ipv6": False} + mock_client = MagicMock() + with patch("salt.transport.base.publish_client", return_value=mock_client) as mock: + salt.transport.base.ipc_publish_client("minion", opts, io_loop=None) + _, kwargs = mock.call_args + assert kwargs["host"] == "127.0.0.1" + + +def test_ipc_publish_client_minion_ipv6(): + """ipc_publish_client uses ::1 for minion when ipv6 is True.""" + opts = {**_IPC_TCP_OPTS, "ipv6": True} + mock_client = MagicMock() + with patch("salt.transport.base.publish_client", return_value=mock_client) as mock: + salt.transport.base.ipc_publish_client("minion", opts, io_loop=None) + _, kwargs = mock.call_args + assert kwargs["host"] == "::1" + + +def test_ipc_publish_client_master_ipv4(): + """ipc_publish_client uses 127.0.0.1 for master when ipv6 is False.""" + opts = {**_IPC_TCP_OPTS, "ipv6": False} + mock_client = MagicMock() + with patch("salt.transport.base.publish_client", return_value=mock_client) as mock: + salt.transport.base.ipc_publish_client("master", opts, io_loop=None) + _, kwargs = mock.call_args + assert kwargs["host"] == "127.0.0.1" + + +def test_ipc_publish_client_master_ipv6(): + """ipc_publish_client uses ::1 for master when ipv6 is True.""" + opts = {**_IPC_TCP_OPTS, "ipv6": True} + mock_client = MagicMock() + with patch("salt.transport.base.publish_client", return_value=mock_client) as mock: + salt.transport.base.ipc_publish_client("master", opts, io_loop=None) + _, kwargs = mock.call_args + assert kwargs["host"] == "::1" + + +# --------------------------------------------------------------------------- +# ipc_publish_server +# --------------------------------------------------------------------------- + + +def test_ipc_publish_server_minion_ipv4(): + """ipc_publish_server uses 127.0.0.1 for minion when ipv6 is False.""" + opts = {**_IPC_TCP_OPTS, "ipv6": False} + mock_server = MagicMock() + with patch("salt.transport.base.publish_server", return_value=mock_server) as mock: + salt.transport.base.ipc_publish_server("minion", opts) + _, kwargs = mock.call_args + assert kwargs["pub_host"] == "127.0.0.1" + assert kwargs["pull_host"] == "127.0.0.1" + + +def test_ipc_publish_server_minion_ipv6(): + """ipc_publish_server uses ::1 for minion when ipv6 is True.""" + opts = {**_IPC_TCP_OPTS, "ipv6": True} + mock_server = MagicMock() + with patch("salt.transport.base.publish_server", return_value=mock_server) as mock: + salt.transport.base.ipc_publish_server("minion", opts) + _, kwargs = mock.call_args + assert kwargs["pub_host"] == "::1" + assert kwargs["pull_host"] == "::1" + + +def test_ipc_publish_server_master_ipv4(): + """ipc_publish_server uses 127.0.0.1 for master when ipv6 is False.""" + opts = {**_IPC_TCP_OPTS, "ipv6": False} + mock_server = MagicMock() + with patch("salt.transport.base.publish_server", return_value=mock_server) as mock: + salt.transport.base.ipc_publish_server("master", opts) + _, kwargs = mock.call_args + assert kwargs["pub_host"] == "127.0.0.1" + assert kwargs["pull_host"] == "127.0.0.1" + + +def test_ipc_publish_server_master_ipv6(): + """ipc_publish_server uses ::1 for master when ipv6 is True.""" + opts = {**_IPC_TCP_OPTS, "ipv6": True} + mock_server = MagicMock() + with patch("salt.transport.base.publish_server", return_value=mock_server) as mock: + salt.transport.base.ipc_publish_server("master", opts) + _, kwargs = mock.call_args + assert kwargs["pub_host"] == "::1" + assert kwargs["pull_host"] == "::1" + + def test_unclosed_warning(): transport = salt.transport.base.Transport() diff --git a/tests/pytests/unit/transport/test_tcp.py b/tests/pytests/unit/transport/test_tcp.py index 68f9165935ff..f5b2adda3a3e 100644 --- a/tests/pytests/unit/transport/test_tcp.py +++ b/tests/pytests/unit/transport/test_tcp.py @@ -119,6 +119,92 @@ def test_get_bind_addr(): assert res == ("192.168.0.1", 1) +def test_tcppuller_start_ipv4(): + """TCPPuller uses AF_INET when host is an IPv4 address.""" + puller = salt.transport.tcp.TCPPuller(host="127.0.0.1", port=4511) + created_sockets = [] + + def fake_socket(family, *args, **kwargs): + sock = MagicMock() + sock.family = family + created_sockets.append(sock) + return sock + + with patch("salt.transport.tcp.socket.socket", side_effect=fake_socket): + with patch("tornado.netutil.add_accept_handler"): + puller.start() + + assert len(created_sockets) == 1 + assert created_sockets[0].family == socket.AF_INET + + +def test_tcppuller_start_ipv6(): + """TCPPuller uses AF_INET6 when host is an IPv6 address.""" + puller = salt.transport.tcp.TCPPuller(host="::1", port=4511) + created_sockets = [] + + def fake_socket(family, *args, **kwargs): + sock = MagicMock() + sock.family = family + created_sockets.append(sock) + return sock + + with patch("salt.transport.tcp.socket.socket", side_effect=fake_socket): + with patch("tornado.netutil.add_accept_handler"): + puller.start() + + assert len(created_sockets) == 1 + assert created_sockets[0].family == socket.AF_INET6 + + +def test_tcppubserverpublisher_connect_ipv4(): + """_TCPPubServerPublisher uses AF_INET when connecting to an IPv4 address.""" + io_loop = tornado.ioloop.IOLoop() + publisher = salt.transport.tcp._TCPPubServerPublisher( + host="127.0.0.1", port=4511, path=None, io_loop=io_loop + ) + captured_family = [] + + def fake_socket(family, *args, **kwargs): + captured_family.append(family) + raise OSError("test abort") + + publisher._connecting_future = tornado.concurrent.Future() + + with patch("salt.transport.tcp.socket.socket", fake_socket): + try: + io_loop.run_sync(publisher._connect, timeout=3) + except Exception: + pass + + io_loop.close() + assert captured_family == [socket.AF_INET] + + +def test_tcppubserverpublisher_connect_ipv6(): + """_TCPPubServerPublisher uses AF_INET6 when connecting to an IPv6 address.""" + io_loop = tornado.ioloop.IOLoop() + publisher = salt.transport.tcp._TCPPubServerPublisher( + host="::1", port=4511, path=None, io_loop=io_loop + ) + captured_family = [] + + def fake_socket(family, *args, **kwargs): + captured_family.append(family) + raise OSError("test abort") + + publisher._connecting_future = tornado.concurrent.Future() + + with patch("salt.transport.tcp.socket.socket", fake_socket): + try: + io_loop.run_sync(publisher._connect, timeout=3) + except Exception: + pass + + io_loop.close() + assert captured_family == [socket.AF_INET6] + + @pytest.mark.usefixtures("_squash_exepected_message_client_warning") async def test_message_client_cleanup_on_close(client_socket, temp_salt_master): """ diff --git a/tools/pkg/salt_build_backend.py b/tools/pkg/salt_build_backend.py index 1901df771a43..cd501c5206da 100644 --- a/tools/pkg/salt_build_backend.py +++ b/tools/pkg/salt_build_backend.py @@ -7,6 +7,10 @@ sys.path.insert(0, PROJECT_ROOT) from setuptools import build_meta as _orig +from setuptools.build_meta import build_editable as setuptools_build_editable +from setuptools.build_meta import ( + prepare_metadata_for_build_editable as setuptools_prepare_metadata, +) # PEP 517 hooks prepare_metadata_for_build_wheel = _orig.prepare_metadata_for_build_wheel @@ -16,6 +20,18 @@ get_requires_for_build_sdist = _orig.get_requires_for_build_sdist +def build_editable(wheel_directory, config_settings=None, metadata_directory=None): + # Delegate to the actual setuptools implementation + return setuptools_build_editable( + wheel_directory, config_settings, metadata_directory + ) + + +def prepare_metadata_for_build_editable(metadata_directory, config_settings=None): + # Delegate to setuptools to get the metadata + return setuptools_prepare_metadata(metadata_directory, config_settings) + + def _parse_requirements_file(requirements_file): parsed_requirements = [] if not os.path.exists(requirements_file):