Pull in changes from #66931

Make sure we only call chmod when pull_path or pub_path is used. Add
regression tests for this behavior.

changelog/66931.fixed.md

@@ -0,0 +1,4 @@
+transports.tcp: ensure pull path is being used before attempting chmod.
+The fix prevents an unnecessary traceback when the TCP transport is
+not using unix sockets. No functionaly has changed as the traceback
+occurs when an async task was about to exit anyway.

salt/transport/tcp.py

@@ -10,7 +10,6 @@ import asyncio.exceptions
 import errno
 import logging
 import multiprocessing
-import os
 import queue
 import select
 import socket
@@ -1150,7 +1149,13 @@ class TCPPuller:
     """
 
     def __init__(
-        self, host=None, port=None, path=None, io_loop=None, payload_handler=None
+        self,
+        host=None,
+        port=None,
+        path=None,
+        mode=0o600,
+        io_loop=None,
+        payload_handler=None,
     ):
         """
         Create a new Tornado IPC server
@@ -1170,6 +1175,7 @@ class TCPPuller:
         self.host = host
         self.port = port
         self.path = path
+        self.mode = mode
         self._started = False
         self.payload_handler = payload_handler
 
@@ -1187,7 +1193,7 @@ class TCPPuller:
         # Start up the ioloop
         if self.path:
             log.trace("IPCServer: binding to socket: %s", self.path)
-            self.sock = tornado.netutil.bind_unix_socket(self.path)
+            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)
@@ -1421,8 +1427,9 @@ class PublishServer(salt.transport.base.DaemonizedPublishServer):
                 "Publish server binding pub to %s ssl=%r", self.pub_path, self.ssl
             )
             with salt.utils.files.set_umask(0o177):
-                sock = tornado.netutil.bind_unix_socket(self.pub_path)
-                os.chmod(self.pub_path, self.pub_path_perms)
+                sock = tornado.netutil.bind_unix_socket(
+                    self.pub_path, self.pub_path_perms
+                )
         else:
             log.debug(
                 "Publish server binding pub to %s:%s ssl=%r",
@@ -1451,18 +1458,17 @@ class PublishServer(salt.transport.base.DaemonizedPublishServer):
             pull_host = self.pull_host
             pull_port = self.pull_port
 
-        self.pull_sock = TCPPuller(
-            host=self.pull_host,
-            port=self.pull_port,
-            path=self.pull_path,
-            io_loop=io_loop,
-            payload_handler=publish_payload,
-        )
-
-        # Securely create socket
         with salt.utils.files.set_umask(0o177):
+            self.pull_sock = TCPPuller(
+                host=self.pull_host,
+                port=self.pull_port,
+                path=self.pull_path,
+                mode=self.pull_path_perms,
+                io_loop=io_loop,
+                payload_handler=publish_payload,
+            )
+            # Securely create socket
             self.pull_sock.start()
-            os.chmod(self.pull_path, self.pull_path_perms)
         self.started.set()
 
     def pre_fork(self, process_manager):

tests/pytests/unit/transport/test_tcp.py

@@ -685,3 +685,62 @@ async def test_pub_server_publish_payload_closed_stream(master_opts, io_loop):
     server.clients = {client}
     await server.publish_payload(package, topic_list)
     assert server.clients == set()
+
+
+async def test_pub_server_pull_path_no_perms(master_opts, io_loop):
+    def publish_payload(payload):
+        return payload
+
+    pubserv = salt.transport.tcp.PublishServer(
+        master_opts,
+        pub_host="127.0.0.1",
+        pub_port=5151,
+        pull_host="127.0.0.1",
+        pull_port=5152,
+    )
+    with patch("os.chmod") as p:
+        await pubserv.publisher(publish_payload)
+        assert p.call_count == 0
+
+
+async def test_pub_server_publisher_pull_path_perms(master_opts, io_loop, tmp_path):
+    def publish_payload(payload):
+        return payload
+
+    pull_path = tmp_path / "pull.ipc"
+    pull_path_perms = 0o664
+    pubserv = salt.transport.tcp.PublishServer(
+        master_opts,
+        pub_host="127.0.0.1",
+        pub_port=5151,
+        pull_host=None,
+        pull_port=None,
+        pull_path=str(pull_path),
+        pull_path_perms=pull_path_perms,
+    )
+    with patch("os.chmod") as p:
+        await pubserv.publisher(publish_payload)
+        assert p.call_count == 1
+        assert p.call_args.args == (pubserv.pull_path, pubserv.pull_path_perms)
+
+
+async def test_pub_server_publisher_pub_path_perms(master_opts, io_loop, tmp_path):
+    def publish_payload(payload):
+        return payload
+
+    pub_path = tmp_path / "pub.ipc"
+    pub_path_perms = 0o664
+    pubserv = salt.transport.tcp.PublishServer(
+        master_opts,
+        pub_host=None,
+        pub_port=None,
+        pub_path=str(pub_path),
+        pub_path_perms=pub_path_perms,
+        pull_host="127.0.0.1",
+        pull_port=5151,
+        pull_path=None,
+    )
+    with patch("os.chmod") as p:
+        await pubserv.publisher(publish_payload)
+        assert p.call_count == 1
+        assert p.call_args.args == (pubserv.pub_path, pubserv.pub_path_perms)