Improve testing and documentation for server firewall marks

doc/developer-guide/api/functions/TSHttpTxnServerPacketMarkSet.en.rst

@@ -20,8 +20,6 @@
 TSHttpTxnServerPacketMarkSet
 ****************************
 
-Change packet firewall mark for the server side connection.
-
 Synopsis
 ========
 
@@ -34,13 +32,25 @@ Synopsis
 Description
 ===========
 
+Change the packet firewall :arg:`mark` for the server side (origin) connection.
+The entire firewall mark is replaced with :arg:`mark`, which is interpreted as a
+32-bit unsigned bit pattern.
+
+Always returns :const:`TS_SUCCESS`, including when no server connection has been
+established yet.
+
 .. note::
 
-   The change takes effect immediately. If no OS connection has been
-   made, then this sets the mark that will be used. If an OS connection
-   is established
+   The firewall mark is only honored on platforms whose OS supports it,
+   specifically Linux via ``SO_MARK``. On platforms without ``SO_MARK`` support
+   the call still returns :const:`TS_SUCCESS`, but setting the mark has no effect
+   at the OS layer (it is a safe no-op).
+
+.. note::
 
-.. XXX Third sentence above needs to be completed.
+   If a live server connection exists, the mark is applied to it immediately; the
+   mark is also recorded on the transaction so that any subsequent server
+   connection for this transaction uses it.
 
 See Also
 ========

include/ts/ts.h

@@ -1602,12 +1602,18 @@ TSReturnCode           TSHttpSsnClientFdGet(TSHttpSsn ssnp, int *fdp);
 TSReturnCode TSHttpTxnClientPacketMarkSet(TSHttpTxn txnp, int mark);
 
 /** Change packet firewall mark for the server side connection
- *
-    @note The change takes effect immediately, if no OS connection has been
-    made, then this sets the mark that will be used IF an OS connection
-    is established
 
-    @return TS_SUCCESS if the (future?) server connection was modified
+    Sets the entire server-side packet firewall mark to @a mark; the whole mark is replaced. @a mark
+    is interpreted as a 32-bit unsigned bit pattern.
+
+    @note The firewall mark is only honored on platforms whose OS supports it, specifically Linux via
+    @c SO_MARK. On platforms without @c SO_MARK support the call still returns TS_SUCCESS, but setting
+    the mark has no effect at the OS layer (it is a safe no-op).
+
+    @note If a live server connection exists, the mark is applied to it immediately; the mark is also
+    recorded on the transaction so that any subsequent server connection for this transaction uses it.
+
+    @return TS_SUCCESS always, including when no server connection has been established yet.
 */
 TSReturnCode TSHttpTxnServerPacketMarkSet(TSHttpTxn txnp, int mark);
 

tests/gold_tests/pluginTest/packet_mark/packet_mark.test.py

@@ -18,9 +18,11 @@
 import socket
 
 Test.Summary = '''
-Verify TSHttpTxnClientPacketMarkSet sets the client-side firewall mark to the
-supplied value, using a test plugin that reads the applied mark back off the
-client socket.
+Verify TSHttpTxnClientPacketMarkSet and TSHttpTxnServerPacketMarkSet set the
+firewall mark on the client- and server-side connections respectively. Each is
+driven by a test plugin that applies the mark and reads it back off the relevant
+socket with getsockopt(SO_MARK), echoing the observed value into a response
+header this test asserts on.
 '''
 
 
@@ -44,60 +46,136 @@ def _can_set_so_mark() -> bool:
 
 Test.SkipUnless(
     Condition.IsPlatform("linux"),
-    Condition(_can_set_so_mark, "Setting SO_MARK requires Linux with CAP_NET_ADMIN or CAP_NET_RAW", True),
+    # pass_value defaults to True: run only when the probe reports SO_MARK is settable.
+    Condition(_can_set_so_mark, "Setting SO_MARK requires Linux with CAP_NET_ADMIN or CAP_NET_RAW"),
 )
 
+# SOCK_OPT_PACKET_MARK (0x10) | SOCK_OPT_NO_DELAY (0x1). The mark is only pushed
+# to the socket when the PACKET_MARK bit is set in the sock option flag.
+SOCK_OPT_FLAG_PACKET_MARK = 0x11
 
-class ClientPacketMarkTest:
-    """Drive TSHttpTxnClientPacketMarkSet through a test plugin and assert on the
-    firewall mark read back off the client socket.
 
-    The starting mark is seeded per process via
-    proxy.config.net.sock_packet_mark_in, applied at accept time.
+class PacketMarkTest:
+    """Drive a TSHttpTxn*PacketMarkSet API through a test plugin and assert on the
+    firewall mark read back off the relevant socket.
+
+    This base holds the shared skeleton -- process setup, the common records, the
+    curl-and-assert case runner. Each subclass supplies its plugin and echo
+    header and extends _configure() (via super()) with the side-specific mark and
+    flag records.
     """
 
     # Value the plugin sets; the mark is expected to become exactly this.
     SET_MARK = 0x0000000A
+    # Seeded starting mark, distinct from SET_MARK so a no-op would be visible.
+    SEED_MARK = 0x0000FF00
+
+    # Bumped per instance so each side gets uniquely-numbered processes.
+    _counter = 0
 
     def __init__(self):
+        self._num = PacketMarkTest._counter
+        PacketMarkTest._counter += 1
         self._server = self._make_server()
-        self._ts = self._make_ats("ts", seed_mark=0x0000FF00)
+        self._ts = self._make_ats()
+        self._configure(self._ts)
+        self._started = False
 
-    def _make_server(self) -> 'Process':
-        server = Test.MakeOriginServer("server")
+    def _make_server(self):
+        server = Test.MakeOriginServer(f"server{self._num}")
         request_header = {"headers": "GET / HTTP/1.1\r\nHost: example.com\r\n\r\n", "timestamp": "1469733493.993", "body": ""}
         response_header = {"headers": "HTTP/1.1 200 OK\r\nConnection: close\r\n\r\n", "timestamp": "1469733493.993", "body": ""}
-        server.addResponse("sessionlog.json", request_header, response_header)
+        for _ in range(2):
+            server.addResponse("sessionlog.json", request_header, response_header)
         return server
 
-    def _make_ats(self, name: str, seed_mark: int) -> 'Process':
-        ts = Test.MakeATSProcess(name, enable_cache=False)
+    def _make_ats(self):
+        return Test.MakeATSProcess(f"ts{self._num}", enable_cache=False)
+
+    def _configure(self, ts):
+        # Records and remap shared by both sides. Subclasses override to add the
+        # side-specific mark/flag records and load their plugin, calling super()
+        # for these.
         ts.Disk.records_config.update(
             {
-                'proxy.config.net.sock_packet_mark_in': seed_mark,
-                'proxy.config.net.sock_option_flag_in': 0x11,
-                'proxy.config.diags.debug.enabled': 1,
-                'proxy.config.diags.debug.tags': 'http|client_packet_mark',
                 'proxy.config.url_remap.remap_required': 0,
                 # Keep ATS running as the invoking user inside sudo (no privilege drop).
                 'proxy.config.admin.user_id': '#-1',
             })
         ts.Disk.remap_config.AddLine(f"map / http://127.0.0.1:{self._server.Variables.Port}")
-        Test.PrepareTestPlugin(os.path.join(Test.Variables.AtsTestPluginsDir, 'client_packet_mark.so'), ts)
-        return ts
 
-    def run(self):
+    def _add_case(self, echo_header: str, description: str, set_header: str):
         # The mark is set to the supplied value, regardless of the seeded
-        # starting mark.
-        tr = Test.AddTestRun("TSHttpTxnClientPacketMarkSet sets the mark")
+        # starting mark. The set is driven by whichever request header the plugin
+        # keys on; the observed mark is echoed into echo_header. The origin server
+        # and ATS are started before the first case, independent of the order in
+        # which cases are added.
+        tr = Test.AddTestRun(description)
         tr.Processes.Default.StartBefore(self._server)
         tr.Processes.Default.StartBefore(self._ts)
+        if not self._started:
+            tr.StillRunningAfter = self._server
+            tr.StillRunningAfter = self._ts
+            self._started = True
         tr.MakeCurlCommand(
-            f'--verbose --ipv4 --header "X-Set-Mark: 0x{self.SET_MARK:08x}" http://localhost:{self._ts.Variables.port}/',
+            f'--verbose --ipv4 --header "{set_header}: 0x{self.SET_MARK:08x}" http://localhost:{self._ts.Variables.port}/',
             ts=self._ts)
         tr.Processes.Default.ReturnCode = 0
         tr.Processes.Default.Streams.All += Testers.ContainsExpression(
-            f"X-Client-Packet-Mark: 0x{self.SET_MARK:08x}", f"Observed client packet mark should be 0x{self.SET_MARK:08x}")
+            f"{echo_header}: 0x{self.SET_MARK:08x}", f"Observed packet mark should be 0x{self.SET_MARK:08x}")
+
+
+class ClientPacketMarkTest(PacketMarkTest):
+    """Exercise TSHttpTxnClientPacketMarkSet. The client mark is seeded on the
+    inbound socket.
+    """
+
+    ECHO_HEADER = "X-Client-Packet-Mark"
+
+    def _configure(self, ts):
+        super()._configure(ts)
+        ts.Disk.records_config.update(
+            {
+                'proxy.config.net.sock_packet_mark_in': self.SEED_MARK,
+                'proxy.config.net.sock_option_flag_in': SOCK_OPT_FLAG_PACKET_MARK,
+                'proxy.config.diags.debug.enabled': 1,
+                'proxy.config.diags.debug.tags': 'http|client_packet_mark',
+            })
+        Test.PrepareTestPlugin(os.path.join(Test.Variables.AtsTestPluginsDir, f'client_packet_mark.so'), ts)
+
+    def run(self):
+        self._add_case(self.ECHO_HEADER, "client_packet_mark sets the client-side mark on the live connection", "X-Set-Mark")
+
+
+class ServerPacketMarkTest(PacketMarkTest):
+    """Exercise TSHttpTxnServerPacketMarkSet. The server mark is seeded on the
+    outbound socket.
+
+    The server API additionally records the mark for a *future* origin
+    connection (TSHttpTxnConfigIntSet on TS_CONFIG_NET_SOCK_PACKET_MARK_OUT),
+    which the client API has no equivalent of. The server plugin exposes this by
+    honoring X-Set-Mark-Preconnect at READ_REQUEST_HDR, before any origin
+    connection exists -- so the mark can only reach the socket via that seed.
+    """
+
+    ECHO_HEADER = "X-Server-Packet-Mark"
+
+    def _configure(self, ts):
+        super()._configure(ts)
+        ts.Disk.records_config.update(
+            {
+                'proxy.config.net.sock_packet_mark_out': self.SEED_MARK,
+                'proxy.config.net.sock_option_flag_out': SOCK_OPT_FLAG_PACKET_MARK,
+                'proxy.config.diags.debug.enabled': 1,
+                'proxy.config.diags.debug.tags': 'http|server_packet_mark',
+            })
+        Test.PrepareTestPlugin(os.path.join(Test.Variables.AtsTestPluginsDir, f'server_packet_mark.so'), ts)
+
+    def run(self):
+        self._add_case(self.ECHO_HEADER, "server_packet_mark sets the server-side mark on the live connection", "X-Set-Mark")
+        self._add_case(
+            self.ECHO_HEADER, "server_packet_mark seeds the mark for a future origin connection", "X-Set-Mark-Preconnect")
 
 
 ClientPacketMarkTest().run()
+ServerPacketMarkTest().run()

tests/tools/plugins/CMakeLists.txt

@@ -25,6 +25,7 @@ add_autest_plugin(fatal_shutdown fatal_shutdown.cc)
 add_autest_plugin(hook_add_plugin hook_add_plugin.cc)
 add_autest_plugin(missing_mangled_definition missing_mangled_definition_c.c missing_mangled_definition_cpp.cc)
 add_autest_plugin(missing_ts_plugin_init missing_ts_plugin_init.cc)
+add_autest_plugin(server_packet_mark server_packet_mark.cc packet_mark_common.cc)
 add_autest_plugin(ssl_client_verify_test ssl_client_verify_test.cc)
 add_autest_plugin(ssl_hook_test ssl_hook_test.cc)
 add_autest_plugin(ssl_secret_load_test ssl_secret_load_test.cc)

tests/tools/plugins/packet_mark_common.cc

@@ -1,6 +1,6 @@
 /** @file
 
-  Shared helpers for the packet-mark test plugins.
+  Shared helpers for the client_packet_mark and server_packet_mark test plugins.
 
   @section license License
 
@@ -118,9 +118,9 @@ namespace
   }
 
   // Parameterized on the exact tsapi function and kept private to this file,
-  // driven only by the named entry points below. The public API is split by
-  // client/server rather than taking the function as an argument so each plugin
-  // links against exactly the tsapi trio it exercises.
+  // driven only by the four named entry points below. See packet_mark_common.h
+  // for why the public API is split by client/server rather than taking the
+  // function as an argument.
   template <MarkSetter Setter>
   void
   apply_mark_from_header(const LogContext &log, TSHttpTxn txnp, std::string_view header)
@@ -178,10 +178,22 @@ apply_client_mark(const LogContext &log, TSHttpTxn txnp, std::string_view header
   apply_mark_from_header<TSHttpTxnClientPacketMarkSet>(log, txnp, header);
 }
 
+void
+apply_server_mark(const LogContext &log, TSHttpTxn txnp, std::string_view header)
+{
+  apply_mark_from_header<TSHttpTxnServerPacketMarkSet>(log, txnp, header);
+}
+
 void
 echo_client_mark(const LogContext &log, TSHttpTxn txnp, std::string_view echo_header)
 {
   echo_observed_mark<TSHttpTxnClientFdGet, TSHttpTxnClientRespGet>(log, txnp, echo_header);
 }
 
+void
+echo_server_mark(const LogContext &log, TSHttpTxn txnp, std::string_view echo_header)
+{
+  echo_observed_mark<TSHttpTxnServerFdGet, TSHttpTxnServerRespGet>(log, txnp, echo_header);
+}
+
 } // namespace packet_mark

tests/tools/plugins/packet_mark_common.h

@@ -1,10 +1,10 @@
 /** @file
 
-  Shared helpers for the packet-mark test plugins.
+  Shared helpers for the client_packet_mark and server_packet_mark test plugins.
 
-  The plugin reads a target mark out of a request header, applies it to a
-  connection via the tsapi under test, reads the applied mark back off the
-  relevant socket with getsockopt(SO_MARK), and echoes the observed value into a
+  Both plugins read a target mark out of a request header, apply it to a
+  connection via the tsapi under test, read the applied mark back off the
+  relevant socket with getsockopt(SO_MARK), and echo the observed value into a
   response header for the accompanying AuTest to assert on. Everything except
   the tsapi call and the fd getter is identical, so it lives here.
 
@@ -42,6 +42,10 @@ struct LogContext {
 
 void apply_client_mark(const LogContext &log, TSHttpTxn txnp, std::string_view header);
 
+void apply_server_mark(const LogContext &log, TSHttpTxn txnp, std::string_view header);
+
 void echo_client_mark(const LogContext &log, TSHttpTxn txnp, std::string_view echo_header);
 
+void echo_server_mark(const LogContext &log, TSHttpTxn txnp, std::string_view echo_header);
+
 } // namespace packet_mark