Copilot commented on code in PR #13082:
URL: https://github.com/apache/trafficserver/pull/13082#discussion_r3065259617
##########
tests/gold_tests/pluginTest/jax_fingerprint/jax_fingerprint.test.py:
##########
@@ -337,3 +337,100 @@ def _configure_client(self, tr: 'TestRun') -> None:
# Remap plugin sets headers on both routes, but only the SNI-allowed
# connection has a vconn context, so only that request gets headers.
JaxFingerprintTest('Hybrid JA4 servernames', 'JA4', 'hybrid',
servernames='jax.server.test')
+
+# ======================================================================
+# All Methods Test - Verify shared context map works with multiple methods
+# ======================================================================
+
+
+class AllMethodsTest:
+ '''Test multiple fingerprint methods loaded simultaneously.
+
+ When multiple jax_fingerprint instances are loaded, they share user arg
+ slots via a ContextMap. This test verifies that all methods produce
+ fingerprints when sharing the SSL_CLIENT_HELLO_HOOK.
Review Comment:
The docstring claims all methods share the `SSL_CLIENT_HELLO_HOOK`, but JA4H
is a request-based method (no client-hello hook). Consider rewording to reflect
that the test exercises multiple methods sharing user-arg slots (vconn for
JA3/JA4, txn for JA4H) rather than implying a single shared hook.
```suggestion
slots via a ContextMap. This test verifies that JA3/JA4 and JA4H can
coexist and produce fingerprints while sharing that context machinery
across their respective vconn and txn storage.
```
##########
tests/gold_tests/pluginTest/jax_fingerprint/jax_fingerprint.test.py:
##########
@@ -337,3 +337,100 @@ def _configure_client(self, tr: 'TestRun') -> None:
# Remap plugin sets headers on both routes, but only the SNI-allowed
# connection has a vconn context, so only that request gets headers.
JaxFingerprintTest('Hybrid JA4 servernames', 'JA4', 'hybrid',
servernames='jax.server.test')
+
+# ======================================================================
+# All Methods Test - Verify shared context map works with multiple methods
+# ======================================================================
+
+
+class AllMethodsTest:
+ '''Test multiple fingerprint methods loaded simultaneously.
+
+ When multiple jax_fingerprint instances are loaded, they share user arg
+ slots via a ContextMap. This test verifies that all methods produce
+ fingerprints when sharing the SSL_CLIENT_HELLO_HOOK.
+ '''
+
+ _dns_counter: int = 0
+ _server_counter: int = 0
+ _ts_counter: int = 0
+ _client_counter: int = 0
+
+ def __init__(self, name: str) -> None:
+ '''Configure test with multiple methods loaded.'''
+ self._name = name
+ self._replay_file = 'jax_fingerprint_all_methods.replay.yaml'
+
+ tr = Test.AddTestRun(name)
+ self._configure_dns(tr)
+ self._configure_server(tr)
+ self._configure_trafficserver()
+ self._configure_client(tr)
+
+ def _configure_dns(self, tr: 'TestRun') -> None:
+ '''Configure a nameserver for the test.'''
+ name = f'dns_all{AllMethodsTest._dns_counter}'
+ self._dns = tr.MakeDNServer(name, default='127.0.0.1')
+ AllMethodsTest._dns_counter += 1
+
+ def _configure_server(self, tr: 'TestRun') -> None:
+ '''Configure the origin (verifier) server.'''
+ name = f'server_all{AllMethodsTest._server_counter}'
+ self._server = tr.AddVerifierServerProcess(name, self._replay_file)
+ AllMethodsTest._server_counter += 1
+
+ # Verify all headers were forwarded to the origin.
+ for header in ['x-ja3', 'x-ja4', 'x-ja4h']:
+ self._server.Streams.All += Testers.ContainsExpression(
+ rf'{header}:', f'Verify {header} header was forwarded.',
reflags=re.IGNORECASE)
+
+ def _configure_trafficserver(self) -> None:
+ '''Configure Traffic Server with multiple methods.'''
+ name = f'ts_all{AllMethodsTest._ts_counter}'
+ self._ts = Test.MakeATSProcess(name, enable_cache=False,
enable_tls=True)
+ AllMethodsTest._ts_counter += 1
+
+ self._ts.addDefaultSSLFiles()
+ self._ts.Disk.ssl_multicert_config.AddLine('dest_ip=*
ssl_cert_name=server.pem ssl_key_name=server.key')
+
+ server_port = self._server.Variables.https_port
+
+ self._ts.Disk.records_config.update(
+ {
+ 'proxy.config.ssl.server.cert.path': self._ts.Variables.SSLDir,
+ 'proxy.config.ssl.server.private_key.path':
self._ts.Variables.SSLDir,
+ 'proxy.config.ssl.client.verify.server.policy': 'PERMISSIVE',
+ 'proxy.config.dns.nameservers':
f"127.0.0.1:{self._dns.Variables.Port}",
+ 'proxy.config.dns.resolv_conf': 'NULL',
+ 'proxy.config.proxy_name': 'test.proxy.test',
+ 'proxy.config.diags.debug.enabled': 1,
+ 'proxy.config.diags.debug.tags': 'jax_fingerprint',
+ })
+
+ # Load multiple methods - all share the same user arg slot via
ContextMap.
+ self._ts.Disk.plugin_config.AddLines(
+ [
+ 'jax_fingerprint.so --method JA3 --header x-ja3 --standalone',
+ 'jax_fingerprint.so --method JA4 --header x-ja4 --standalone',
+ 'jax_fingerprint.so --method JA4H --header x-ja4h
--standalone',
+ ])
Review Comment:
This test is intended to validate the user-arg slot sharing fix, but as
written it only asserts that headers are present when multiple methods are
loaded. That would also pass even if each method still reserved its own slot
(as long as slots aren’t exhausted). Consider adding an assertion that the
shared slot reservation happens once per type (e.g., via debug log
expectations), or otherwise constructing the test so it would fail when
additional per-method slot reservations occur.
##########
plugins/experimental/jax_fingerprint/context_map.h:
##########
@@ -0,0 +1,142 @@
+/** @file
+ *
+ * Shared context map for jax_fingerprint plugin instances.
+ *
+ * When multiple jax_fingerprint.so instances are loaded (e.g., one per
+ * fingerprinting method), they share a single user arg slot. This map
+ * stores the JAxContext for each method, keyed by method name.
+ *
+ * @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.
+ */
+
+#pragma once
+
+#include "config.h"
+#include "context.h"
+
+#include <string>
+#include <string_view>
+#include <unordered_map>
+
+/**
+ * @brief Container holding JAxContext instances for multiple methods.
+ *
+ * ATS has a limited number of user arg slots (~4 per type). When loading
+ * many jax_fingerprint instances, we share a single slot and store all
+ * contexts in this map, keyed by method name.
+ */
+class ContextMap
+{
+public:
+ ~ContextMap()
+ {
+ for (auto &pair : m_contexts) {
+ delete pair.second;
+ }
+ }
+
+ /**
+ * @brief Store a context for a method.
+ * @param[in] method_name The method name (e.g., "JA3", "JA4").
+ * @param[in] ctx The context to store. Ownership is transferred.
+ */
+ void
+ set(std::string_view method_name, JAxContext *ctx)
+ {
+ auto it = find_context(method_name);
+ if (it != m_contexts.end()) {
+ delete it->second;
+ it->second = ctx;
+ } else {
+ m_contexts.emplace(std::string{method_name}, ctx);
+ }
+ }
+
+ /**
+ * @brief Retrieve a context for a method.
+ * @param[in] method_name The method name.
+ * @return The context, or nullptr if not found.
+ */
+ JAxContext *
+ get(std::string_view method_name)
+ {
+ auto it = find_context(method_name);
+ return it != m_contexts.end() ? it->second : nullptr;
+ }
+
+ /**
+ * @brief Remove a context for a method.
+ * @param[in] method_name The method name.
+ */
+ void
+ remove(std::string_view method_name)
+ {
+ auto it = find_context(method_name);
+ if (it != m_contexts.end()) {
+ delete it->second;
+ m_contexts.erase(it);
+ }
+ }
+
+ /**
+ * @brief Check if the map is empty.
+ * @return True if no contexts are stored.
+ */
+ bool
+ empty() const
+ {
+ return m_contexts.empty();
+ }
+
+private:
+ using ContextStorage = std::unordered_map<std::string, JAxContext *,
StringHash, std::equal_to<>>;
+
+ /** Find context by method name with C++20 generic lookup fallback.
+ *
+ * C++20 generic unordered lookup allows finding with std::string_view in a
+ * std::unordered_map<std::string, ...> without creating a temporary string.
+ * For standard libraries without this feature, we fall back to constructing
+ * a std::string for the lookup.
+ *
+ * @param[in] method_name The method name to look up.
+ * @return Iterator to the found element, or end() if not found.
+ */
+ ContextStorage::iterator
+ find_context(std::string_view method_name)
+ {
+#ifdef __cpp_lib_generic_unordered_lookup
+ return m_contexts.find(method_name);
+#else
+ return m_contexts.find(std::string{method_name});
+#endif
Review Comment:
`ContextMap` uses `__cpp_lib_generic_unordered_lookup` to choose the
heterogeneous `unordered_map::find` fast-path, but this header doesn't include
`<version>`, so that feature-test macro may be undefined even when the library
supports it (causing the fallback path to be used unconditionally). Include
`<version>` here (or avoid the macro and rely on transparent hash/equal +
overload availability) to make the intent deterministic.
##########
tests/gold_tests/pluginTest/jax_fingerprint/jax_fingerprint_all_methods.replay.yaml:
##########
@@ -0,0 +1,67 @@
+# 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.
+
+# Used by: All methods test.
+#
+# The jax_fingerprint plugin is loaded multiple times with different methods.
+# All TLS-based fingerprint headers should be present and non-empty.
+
Review Comment:
The header comment says "All TLS-based fingerprint headers" but this replay
asserts `x-ja4h` as well, which is generated by the JA4H request-based method
(not TLS ClientHello based). Consider adjusting the wording to just "all
fingerprint headers" (and, if you truly want non-empty, make that explicit in
the assertions).
--
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]
