Fix TCP PublishServer crash on IPv6-enabled masters

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.

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)

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")