diff --git a/changelog/66567.fixed.md b/changelog/66567.fixed.md new file mode 100644 index 000000000000..104c0ce5f9bf --- /dev/null +++ b/changelog/66567.fixed.md @@ -0,0 +1 @@ +Fix TCP transport PublishServer crash on IPv6-enabled masters: the publisher now normalizes the configured ``interface`` address (which master startup brackets for ZeroMQ URIs) and selects the address family matching the actual address, so the publish port binds correctly instead of dying with ``gaierror`` and leaving the master unable to publish jobs to minions. diff --git a/salt/transport/tcp.py b/salt/transport/tcp.py index f626beecce7f..8c7b6be22745 100644 --- a/salt/transport/tcp.py +++ b/salt/transport/tcp.py @@ -61,10 +61,36 @@ async def _null_callback(*args, **kwargs): pass -def _get_socket(opts): - family = socket.AF_INET - if opts.get("ipv6", False): - family = socket.AF_INET6 +def _normalize_bind_host(host): + """ + Strip ZeroMQ-style brackets from an IP literal (master startup adds + them to ``interface`` for ZeroMQ URIs; see salt/cli/daemons.py). + Anything that is not an IP literal (e.g. a hostname) is returned + unchanged so it can still be resolved by name. + """ + if host is None: + return None + try: + return ip_bracket(host, strip=True) + except ValueError: + return host + + +def _get_socket(opts, host=None): + """ + Return a TCP socket for binding or connecting to *host*. + + When *host* is given, the address family is derived from the actual + address (after stripping ZeroMQ-style brackets added at startup), so + an IPv4 literal does not end up on an AF_INET6 socket when ``ipv6`` is + enabled. Without *host*, the family follows the ``ipv6`` config flag. + """ + if host is not None: + family = _socket_family(_normalize_bind_host(host)) + else: + family = socket.AF_INET + if opts.get("ipv6", False): + family = socket.AF_INET6 return socket.socket(family, socket.SOCK_STREAM) @@ -232,7 +258,7 @@ def run(self): """ Start the load balancer """ - self._socket = _get_socket(self.opts) + self._socket = _get_socket(self.opts, self.opts["interface"]) self._socket.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) _set_tcp_keepalive(self._socket, self.opts) self._socket.setblocking(1) @@ -814,7 +840,7 @@ def pre_fork(self, process_manager, *args, **kwargs): self._socket.bind(ipc_path) os.chmod(ipc_path, 0o600) else: - self._socket = _get_socket(self.opts) + self._socket = _get_socket(self.opts, self.opts["interface"]) self._socket.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) _set_tcp_keepalive(self._socket, self.opts) self._socket.setblocking(0) @@ -873,7 +899,7 @@ def post_fork(self, message_handler, io_loop, **kwargs): ) else: if salt.utils.platform.is_windows(): - self._socket = _get_socket(self.opts) + self._socket = _get_socket(self.opts, self.opts["interface"]) self._socket.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) _set_tcp_keepalive(self._socket, self.opts) self._socket.setblocking(0) @@ -2316,11 +2342,15 @@ async def publisher( self.pub_port, self.ssl, ) - sock = _get_socket(self.opts) + # The master startup brackets IPv6 ``interface`` values for + # ZeroMQ URIs (salt/cli/daemons.py); normalize before binding + # and let the address family follow the actual address. + pub_host = _normalize_bind_host(self.pub_host) + sock = _get_socket(self.opts, pub_host) sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) _set_tcp_keepalive(sock, self.opts) sock.setblocking(0) - sock.bind((self.pub_host, self.pub_port)) + sock.bind((pub_host, self.pub_port)) sock.listen(self.backlog) # pub_server will take ownership of the socket self.pub_server.add_socket(sock) diff --git a/tests/pytests/unit/transport/test_tcp.py b/tests/pytests/unit/transport/test_tcp.py index ce39982de4eb..582da1164dd6 100644 --- a/tests/pytests/unit/transport/test_tcp.py +++ b/tests/pytests/unit/transport/test_tcp.py @@ -111,6 +111,134 @@ def test_get_socket(): assert int(socket.family) == 2 +def _run_publisher_with_fake_socket(server, payload=lambda *args, **kwargs: None): + """ + Drive ``PublishServer.publisher()`` with the socket machinery mocked out, + returning the sockets created through ``salt.transport.tcp.socket.socket``. + """ + created_sockets = [] + + def fake_socket(family, *args, **kwargs): + sock = MagicMock() + sock.family = family + created_sockets.append(sock) + return sock + + io_loop = tornado.ioloop.IOLoop() + + async def run(): + with patch("salt.transport.tcp.socket.socket", side_effect=fake_socket): + with patch("salt.transport.tcp.PubServer"), patch( + "salt.transport.tcp.TCPPuller" + ): + await server.publisher(payload, io_loop=io_loop) + + try: + io_loop.run_sync(run, timeout=5) + finally: + io_loop.close() + return created_sockets + + +def test_publish_server_publisher_binds_stripped_ipv6(tmp_path): + """ + PublishServer.publisher() normalizes a bracketed IPv6 pub_host before + binding. The master startup brackets IPv6 ``interface`` values for + ZeroMQ URIs (salt/cli/daemons.py); without normalization the TCP + publisher crashes with gaierror and the publish port never opens. + + Regression test for https://github.com/saltstack/salt/issues/66567 + """ + opts = {"ipv6": True, "publish_port": 4506, "sock_dir": str(tmp_path)} + server = salt.transport.tcp.PublishServer( + opts, pub_host="[::1]", pub_port=4512, pull_path=str(tmp_path / "pull.ipc") + ) + + created_sockets = _run_publisher_with_fake_socket(server) + + assert created_sockets[0].family == socket.AF_INET6 + created_sockets[0].bind.assert_called_once_with(("::1", 4512)) + + +def test_publish_server_publisher_binds_ipv4_with_ipv6_flag(tmp_path): + """ + With ``ipv6: True`` but an IPv4 ``interface`` (for example the default + 0.0.0.0), the publisher must bind an AF_INET socket to the actual + address instead of crashing with gaierror on an AF_INET6 socket. + + Regression test for https://github.com/saltstack/salt/issues/66567 + """ + opts = {"ipv6": True, "publish_port": 4506, "sock_dir": str(tmp_path)} + server = salt.transport.tcp.PublishServer( + opts, pub_host="127.0.0.1", pub_port=4512, pull_path=str(tmp_path / "pull.ipc") + ) + + created_sockets = _run_publisher_with_fake_socket(server) + + assert created_sockets[0].family == socket.AF_INET + created_sockets[0].bind.assert_called_once_with(("127.0.0.1", 4512)) + + +def test_get_socket_with_host_derives_family_from_address(): + """ + When a host is given, the socket family follows the actual address, + not the ``ipv6`` config flag - an IPv4 literal must not end up on an + AF_INET6 socket, and bracketed ZeroMQ-style IPv6 literals are + normalized. Regression test for + https://github.com/saltstack/salt/issues/66567 + """ + sock = salt.transport.tcp._get_socket({"ipv6": True}, "0.0.0.0") + assert sock.family == socket.AF_INET + sock.close() + + sock = salt.transport.tcp._get_socket({"ipv6": True}, "[::]") + assert sock.family == socket.AF_INET6 + sock.close() + + sock = salt.transport.tcp._get_socket({"ipv6": False}, "::1") + assert sock.family == socket.AF_INET6 + sock.close() + + +def test_get_socket_with_hostname_does_not_raise(): + """ + Non-literal hosts (hostnames) are resolved by name instead of raising + in ip_bracket, preserving the pre-existing contract for callers that + pass resolvable names. + """ + sock = salt.transport.tcp._get_socket({"ipv6": True}, "localhost") + assert sock.family in (socket.AF_INET, socket.AF_INET6) + sock.close() + + # Unresolvable non-literal: _socket_family falls back to the + # colon heuristic (no ":" here) -> AF_INET, mirroring prior + # flag-independent behavior for unknown names. + sock = salt.transport.tcp._get_socket({"ipv6": True}, "nonexistent.invalid") + assert sock.family == socket.AF_INET + sock.close() + + +def test_req_server_bind_pattern_matches_family_and_address(): + """ + The request-server / load-balancer binds (Unix pre-fork and Windows + post-fork share this exact pattern) must produce a socket whose + family matches the normalized bind address for both the bracketed + IPv6 wildcard the master startup produces and an IPv4 interface used + together with ``ipv6: True``. + """ + opts = {"ipv6": True, "interface": "[::]", "ret_port": 4505} + sock = salt.transport.tcp._get_socket(opts, opts["interface"]) + assert sock.family == socket.AF_INET6 + assert salt.transport.tcp._get_bind_addr(opts, "ret_port") == ("::", 4505) + sock.close() + + opts = {"ipv6": True, "interface": "0.0.0.0", "ret_port": 4505} + sock = salt.transport.tcp._get_socket(opts, opts["interface"]) + assert sock.family == socket.AF_INET + assert salt.transport.tcp._get_bind_addr(opts, "ret_port") == ("0.0.0.0", 4505) + sock.close() + + def test_get_bind_addr(): opts = {"interface": "192.168.0.1", "tcp": 1} res = salt.transport.tcp._get_bind_addr(opts=opts, port_type="tcp")