Copilot commented on code in PR #13383: URL: https://github.com/apache/trafficserver/pull/13383#discussion_r3581862998
########## tests/tools/plugins/client_packet_mark_mask.cc: ########## @@ -0,0 +1,164 @@ +/** @file + + Test plugin for the TSHttpTxnClientPacketMarkSet API. + + On each request it reads a target mark from request headers, applies it to the + client-side connection via TSHttpTxnClientPacketMarkSet, then reads the mark + back off the client socket with getsockopt(SO_MARK) and echoes the observed + value into the X-Client-Packet-Mark response header for the AuTest to assert + on. + + @section license License + + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + */ + +#include <ts/ts.h> + +extern "C" { +#include <sys/socket.h> +} + Review Comment: Include the generated config header so feature-test macros like TS_HAS_SO_MARK are available (this avoids relying on platform headers alone when gating SO_MARK logic). ########## tests/gold_tests/pluginTest/client_packet_mark/client_packet_mark_mask.test.py: ########## @@ -0,0 +1,100 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import os +import socket + +Test.Summary = ''' +Verify the TSHttpTxnClientPacketMarkSet overload replaces the entire client-side +firewall mark with the supplied value, using a test plugin that reads the applied +mark back off the client socket. +''' Review Comment: The summary text refers to a TSHttpTxnClientPacketMarkSet "overload", but this PR doesn’t add any overloads (and the API is a single function). This wording is likely to confuse readers when interpreting what is being validated. ########## tests/tools/plugins/client_packet_mark_mask.cc: ########## @@ -0,0 +1,164 @@ +/** @file + + Test plugin for the TSHttpTxnClientPacketMarkSet API. + + On each request it reads a target mark from request headers, applies it to the + client-side connection via TSHttpTxnClientPacketMarkSet, then reads the mark + back off the client socket with getsockopt(SO_MARK) and echoes the observed + value into the X-Client-Packet-Mark response header for the AuTest to assert + on. + + @section license License + + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + */ + +#include <ts/ts.h> + +extern "C" { +#include <sys/socket.h> +} + +#include <cstdint> +#include <cstdio> +#include <cstdlib> +#include <optional> +#include <string> +#include <string_view> + +namespace +{ +constexpr std::string_view PLUGIN_NAME = "client_packet_mark_mask"; +constexpr std::string_view MARK_HEADER = "X-Set-Mark"; +constexpr std::string_view ECHO_HEADER = "X-Client-Packet-Mark"; + +DbgCtl dbg_ctl{PLUGIN_NAME.data()}; + +/** Read a header field and interpret its value as a 32-bit unsigned quantity. + + Values are parsed with strtoul (base 0), so "0x0000000A" and "10" are both + accepted. Returns std::nullopt if the header is absent. */ +std::optional<uint32_t> +get_uint_header(TSMBuffer bufp, TSMLoc hdr_loc, std::string_view header) +{ + TSMLoc field_loc = TSMimeHdrFieldFind(bufp, hdr_loc, header.data(), static_cast<int>(header.length())); + if (field_loc == TS_NULL_MLOC) { + return std::nullopt; + } + + int value_len = 0; + const char *value_str = TSMimeHdrFieldValueStringGet(bufp, hdr_loc, field_loc, -1, &value_len); + uint32_t result = 0; + if (value_str != nullptr && value_len > 0) { + std::string value(value_str, value_len); + result = static_cast<uint32_t>(strtoul(value.c_str(), nullptr, 0)); + } + TSHandleMLocRelease(bufp, hdr_loc, field_loc); + return result; +} + +/** Create the echo header on the response with the value formatted as 0x%08x. */ +void +set_echo_header(TSMBuffer bufp, TSMLoc hdr_loc, uint32_t value) +{ + char formatted[16]; + int len = snprintf(formatted, sizeof(formatted), "0x%08x", value); + + TSMLoc field_loc = TS_NULL_MLOC; + if (TSMimeHdrFieldCreateNamed(bufp, hdr_loc, ECHO_HEADER.data(), static_cast<int>(ECHO_HEADER.length()), &field_loc) == + TS_SUCCESS) { + TSMimeHdrFieldValueStringSet(bufp, hdr_loc, field_loc, -1, formatted, len); + TSMimeHdrFieldAppend(bufp, hdr_loc, field_loc); + TSHandleMLocRelease(bufp, hdr_loc, field_loc); + } +} + +int +handle_send_response(TSCont /* contp ATS_UNUSED */, TSEvent event, void *edata) +{ + TSHttpTxn txnp = static_cast<TSHttpTxn>(edata); + + if (event != TS_EVENT_HTTP_SEND_RESPONSE_HDR) { + TSHttpTxnReenable(txnp, TS_EVENT_HTTP_CONTINUE); + return 0; + } + + TSMBuffer req_bufp = nullptr; + TSMLoc req_loc = TS_NULL_MLOC; + if (TSHttpTxnClientReqGet(txnp, &req_bufp, &req_loc) != TS_SUCCESS) { + TSError("[%s] Failed to get client request headers", PLUGIN_NAME.data()); + TSHttpTxnReenable(txnp, TS_EVENT_HTTP_CONTINUE); + return 0; + } + + std::optional<uint32_t> mark = get_uint_header(req_bufp, req_loc, MARK_HEADER); + TSHandleMLocRelease(req_bufp, TS_NULL_MLOC, req_loc); + + if (mark.has_value()) { + Dbg(dbg_ctl, "Calling unmasked set: mark=0x%08x", *mark); + TSHttpTxnClientPacketMarkSet(txnp, static_cast<int>(*mark)); + } + + uint32_t observed = 0; +#if defined(SO_MARK) + int client_fd = -1; + if (TSHttpTxnClientFdGet(txnp, &client_fd) == TS_SUCCESS && client_fd >= 0) { + socklen_t optlen = sizeof(observed); + if (getsockopt(client_fd, SOL_SOCKET, SO_MARK, &observed, &optlen) != 0) { + TSError("[%s] getsockopt(SO_MARK) failed on fd %d", PLUGIN_NAME.data(), client_fd); + } + } else { + TSError("[%s] Failed to obtain client fd", PLUGIN_NAME.data()); + } +#else + // SO_MARK is Linux-only. On other platforms the accompanying AuTest is skipped + // via Test.SkipUnless, so this readback path is never exercised; keep it + // compilable so the plugin still builds everywhere. + TSError("[%s] SO_MARK is not supported on this platform", PLUGIN_NAME.data()); +#endif Review Comment: This readback path is gated on `defined(SO_MARK)`, but ATS’ own runtime capability is expressed via the generated `TS_HAS_SO_MARK` macro (used when applying the option). Using `TS_HAS_SO_MARK` here keeps the test plugin consistent with how ATS is built/configured. ########## tests/gold_tests/pluginTest/client_packet_mark/client_packet_mark_mask.test.py: ########## @@ -0,0 +1,100 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import os +import socket + +Test.Summary = ''' +Verify the TSHttpTxnClientPacketMarkSet overload replaces the entire client-side +firewall mark with the supplied value, using a test plugin that reads the applied +mark back off the client socket. +''' + + +def _can_set_so_mark() -> bool: + """Probe whether SO_MARK can actually be set on this host. + + Setting SO_MARK is Linux-only and requires CAP_NET_ADMIN. On any host that + lacks the capability (or the platform), setsockopt raises, and the applied + value would be unobservable -- so the test is skipped rather than failed. + """ + if not hasattr(socket, "SO_MARK"): + return False + try: + with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as probe: + probe.setsockopt(socket.SOL_SOCKET, socket.SO_MARK, 0x1) + return True + except (OSError, PermissionError): + return False + + +Test.SkipUnless( + Condition.IsPlatform("linux"), + Condition(_can_set_so_mark, "Setting SO_MARK requires Linux with CAP_NET_ADMIN", True), + Condition.PluginExists('client_packet_mark_mask.so'), +) + + +class ClientPacketMarkTest: + """Drive the unmasked overload 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. + """ Review Comment: The class docstring says "unmasked overload", but the test is exercising the existing TSHttpTxnClientPacketMarkSet API behavior (not an overload). Updating this wording will keep the test intent accurate. ########## tests/tools/plugins/CMakeLists.txt: ########## @@ -15,6 +15,7 @@ # ####################### +add_autest_plugin(client_packet_mark_mask client_packet_mark_mask.cc) Review Comment: The plugin/test name includes "_mask", but the new test plugin only exercises TSHttpTxnClientPacketMarkSet’s unmasked behavior (no mask is applied). This is likely to confuse readers and make it harder to find the right test once a real masked variant is added. ########## include/ts/ts.h: ########## @@ -1585,10 +1585,19 @@ TSReturnCode TSHttpSsnClientFdGet(TSHttpSsn ssnp, int *fdp); /* TS-1008 END */ /** Change packet firewall mark for the client side connection - * - @note The change takes effect immediately - @return TS_SUCCESS if the client connection was modified + Sets the entire client-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 when a + client connection is present, but setting the mark has no effect at the OS layer (it is a safe + no-op). + Review Comment: The docs imply TS_SUCCESS means the OS-layer firewall mark was actually applied. In the implementation, apply_options() calls setsockopt(SO_MARK) via safe_setsockopt(), but the return value is ignored; TSHttpTxnClientPacketMarkSet can return TS_SUCCESS even if the OS rejects the setsockopt (e.g. due to insufficient privileges). The API docs should clarify that OS-layer failures are not reported. ########## doc/developer-guide/api/functions/TSHttpTxnClientPacketMarkSet.en.rst: ########## @@ -32,11 +32,24 @@ Synopsis Description =========== -Change packet firewall :arg:`mark` for the client side connection. +Change the packet firewall :arg:`mark` for the client side connection. The +entire firewall mark is replaced with :arg:`mark`, which is interpreted as a +32-bit unsigned bit pattern. + +Returns :const:`TS_SUCCESS` when the client connection was modified, and +:const:`TS_ERROR` when there is no client connection to modify. + +.. note:: + + 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` when a client connection is + present, but setting the mark has no effect at the OS layer (it is a safe + no-op). Review Comment: Similar to the header doc: TSHttpTxnClientPacketMarkSet returns TS_SUCCESS once a client NetVConnection exists, but the underlying setsockopt(SO_MARK) return value is not surfaced to the caller. The docs should note that the OS may reject the mark (e.g. insufficient privileges) and this is not reported by the API. ########## tests/tools/plugins/client_packet_mark_mask.cc: ########## @@ -0,0 +1,164 @@ +/** @file + + Test plugin for the TSHttpTxnClientPacketMarkSet API. + + On each request it reads a target mark from request headers, applies it to the + client-side connection via TSHttpTxnClientPacketMarkSet, then reads the mark + back off the client socket with getsockopt(SO_MARK) and echoes the observed + value into the X-Client-Packet-Mark response header for the AuTest to assert + on. + + @section license License + + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + */ + +#include <ts/ts.h> + +extern "C" { +#include <sys/socket.h> +} + +#include <cstdint> +#include <cstdio> +#include <cstdlib> +#include <optional> +#include <string> +#include <string_view> + +namespace +{ +constexpr std::string_view PLUGIN_NAME = "client_packet_mark_mask"; +constexpr std::string_view MARK_HEADER = "X-Set-Mark"; +constexpr std::string_view ECHO_HEADER = "X-Client-Packet-Mark"; + +DbgCtl dbg_ctl{PLUGIN_NAME.data()}; + +/** Read a header field and interpret its value as a 32-bit unsigned quantity. + + Values are parsed with strtoul (base 0), so "0x0000000A" and "10" are both + accepted. Returns std::nullopt if the header is absent. */ +std::optional<uint32_t> +get_uint_header(TSMBuffer bufp, TSMLoc hdr_loc, std::string_view header) +{ + TSMLoc field_loc = TSMimeHdrFieldFind(bufp, hdr_loc, header.data(), static_cast<int>(header.length())); + if (field_loc == TS_NULL_MLOC) { + return std::nullopt; + } + + int value_len = 0; + const char *value_str = TSMimeHdrFieldValueStringGet(bufp, hdr_loc, field_loc, -1, &value_len); + uint32_t result = 0; + if (value_str != nullptr && value_len > 0) { + std::string value(value_str, value_len); + result = static_cast<uint32_t>(strtoul(value.c_str(), nullptr, 0)); + } + TSHandleMLocRelease(bufp, hdr_loc, field_loc); + return result; +} + +/** Create the echo header on the response with the value formatted as 0x%08x. */ +void +set_echo_header(TSMBuffer bufp, TSMLoc hdr_loc, uint32_t value) +{ + char formatted[16]; + int len = snprintf(formatted, sizeof(formatted), "0x%08x", value); + + TSMLoc field_loc = TS_NULL_MLOC; + if (TSMimeHdrFieldCreateNamed(bufp, hdr_loc, ECHO_HEADER.data(), static_cast<int>(ECHO_HEADER.length()), &field_loc) == + TS_SUCCESS) { + TSMimeHdrFieldValueStringSet(bufp, hdr_loc, field_loc, -1, formatted, len); + TSMimeHdrFieldAppend(bufp, hdr_loc, field_loc); + TSHandleMLocRelease(bufp, hdr_loc, field_loc); + } +} + +int +handle_send_response(TSCont /* contp ATS_UNUSED */, TSEvent event, void *edata) +{ + TSHttpTxn txnp = static_cast<TSHttpTxn>(edata); + + if (event != TS_EVENT_HTTP_SEND_RESPONSE_HDR) { + TSHttpTxnReenable(txnp, TS_EVENT_HTTP_CONTINUE); + return 0; + } + + TSMBuffer req_bufp = nullptr; + TSMLoc req_loc = TS_NULL_MLOC; + if (TSHttpTxnClientReqGet(txnp, &req_bufp, &req_loc) != TS_SUCCESS) { + TSError("[%s] Failed to get client request headers", PLUGIN_NAME.data()); + TSHttpTxnReenable(txnp, TS_EVENT_HTTP_CONTINUE); + return 0; + } + + std::optional<uint32_t> mark = get_uint_header(req_bufp, req_loc, MARK_HEADER); + TSHandleMLocRelease(req_bufp, TS_NULL_MLOC, req_loc); + + if (mark.has_value()) { + Dbg(dbg_ctl, "Calling unmasked set: mark=0x%08x", *mark); + TSHttpTxnClientPacketMarkSet(txnp, static_cast<int>(*mark)); + } Review Comment: TSHttpTxnClientPacketMarkSet can return TS_ERROR (e.g. if there is no UA/client connection). The test plugin should check the return value and log a clear error so failures are easier to diagnose in AuTest output. ########## tests/gold_tests/pluginTest/client_packet_mark/client_packet_mark_mask.test.py: ########## @@ -0,0 +1,100 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import os +import socket + +Test.Summary = ''' +Verify the TSHttpTxnClientPacketMarkSet overload replaces the entire client-side +firewall mark with the supplied value, using a test plugin that reads the applied +mark back off the client socket. +''' + + +def _can_set_so_mark() -> bool: + """Probe whether SO_MARK can actually be set on this host. + + Setting SO_MARK is Linux-only and requires CAP_NET_ADMIN. On any host that + lacks the capability (or the platform), setsockopt raises, and the applied + value would be unobservable -- so the test is skipped rather than failed. + """ + if not hasattr(socket, "SO_MARK"): + return False + try: + with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as probe: + probe.setsockopt(socket.SOL_SOCKET, socket.SO_MARK, 0x1) + return True + except (OSError, PermissionError): + return False + + +Test.SkipUnless( + Condition.IsPlatform("linux"), + Condition(_can_set_so_mark, "Setting SO_MARK requires Linux with CAP_NET_ADMIN", True), + Condition.PluginExists('client_packet_mark_mask.so'), +) + + +class ClientPacketMarkTest: + """Drive the unmasked overload 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. + """ + + # Value the plugin sets; the full mark is expected to become exactly this. + SET_MARK = 0x0000000A + + def __init__(self): + self._server = self._make_server() + self._ts = self._make_ats("ts", seed_mark=0x0000FF00) + + def _make_server(self) -> 'Process': + server = Test.MakeOriginServer("server") + 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) + return server + + def _make_ats(self, name: str, seed_mark: int) -> 'Process': + ts = Test.MakeATSProcess(name, enable_cache=False) + ts.Disk.records_config.update( + { + 'proxy.config.net.sock_packet_mark_in': seed_mark, + 'proxy.config.diags.debug.enabled': 1, + 'proxy.config.diags.debug.tags': 'http|client_packet_mark_mask', + 'proxy.config.url_remap.remap_required': 0, + }) + 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_mask.so'), ts) + return ts + + def run(self): + # Unmasked baseline: the two-argument overload replaces the entire mark + # with the supplied value, regardless of the seeded starting mark. Review Comment: The inline comment calls this a "two-argument overload". This is just the normal C API signature; calling it an overload is misleading given this PR does not add new overloads. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
