Copilot commented on code in PR #13807:
URL: https://github.com/apache/trafficserver/pull/13807#discussion_r4263570867
##########
plugins/experimental/jax_fingerprint/plugin.cc:
##########
@@ -27,195 +27,187 @@
#include "header.h"
#include "log.h"
-#ifdef ENABLE_JAX_METHOD_JA4
-#include "ja4/method.h"
-#endif
-#ifdef ENABLE_JAX_METHOD_JA4H
-#include "ja4h/method.h"
-#endif
-#ifdef ENABLE_JAX_METHOD_JA3
-#include "ja3/method.h"
-#endif
-
#include <ts/apidefs.h>
#include <ts/ts.h>
#include <ts/remap.h>
#include <ts/remap_version.h>
-#include <getopt.h>
-
-#include <cstddef>
-#include <cstdint>
#include <cstdio>
-#include <cstring>
#include <memory>
#include <string>
#include <string_view>
+#include <vector>
#include <version>
DbgCtl dbg_ctl{PLUGIN_NAME};
namespace
{
-constexpr Method const *METHODS[] = {
-#ifdef ENABLE_JAX_METHOD_JA4
- &ja4::method,
-#endif
-#ifdef ENABLE_JAX_METHOD_JA4H
- &ja4h::method,
-#endif
-#ifdef ENABLE_JAX_METHOD_JA3
- &ja3::method,
-#endif
-};
-} // namespace
+bool
+prepare_config(PluginConfig &config)
+{
+ if (!config.log_filename.empty()) {
+ if (!create_log_file(config.log_filename, config.log_handle)) {
+ TSError("[%s] Failed to create log.", PLUGIN_NAME);
+ return false;
+ }
+ Dbg(dbg_ctl, "Created log file.");
+ }
+
+ if (reserve_user_arg(config) == TS_ERROR) {
+ TSError("[%s] Failed to reserve user arg index.", PLUGIN_NAME);
+ return false;
+ }
+
+ return true;
+}
-static bool
-read_config_option(int argc, char const *argv[], PluginConfig &config)
+/** Prepare every configuration, releasing any log file already created if one
of them fails. */
+bool
+prepare_configs(PluginConfigs &configs)
{
- const struct option longopts[] = {
- {"standalone", no_argument, nullptr, 's'},
- {"method", required_argument, nullptr, 'M'}, // JA4, JA4H, or JA3
- {"mode", required_argument, nullptr, 'm'}, // overwrite, keep, or
append
- {"header", required_argument, nullptr, 'h'},
- {"via-header", required_argument, nullptr, 'v'},
- {"log-filename", required_argument, nullptr, 'f'},
- {"log-field", required_argument, nullptr, 'l'},
- {"servernames", required_argument, nullptr, 'S'},
- {"export", required_argument, nullptr, 'e'},
- {nullptr, 0, nullptr, 0 }
- };
-
- optind = 0;
- int opt{0};
- while ((opt = getopt_long(argc, const_cast<char *const *>(argv), "",
longopts, nullptr)) >= 0) {
- switch (opt) {
- case '?':
- Dbg(dbg_ctl, "Unrecognized command argument.");
- break;
- case 'M': {
- bool found = false;
- for (auto const *m : METHODS) {
- if (m->name == optarg) {
- config.method = *m;
- found = true;
- break;
+ for (auto const &config : configs) {
+ if (!prepare_config(*config)) {
+ for (auto const &created : configs) {
+ if (created->log_handle != nullptr) {
+ flush_log_file(created->log_handle);
+ created->log_handle = nullptr;
}
}
- if (!found) {
- Dbg(dbg_ctl, "Unexpected method: %s", optarg);
- return false;
- }
- } break;
- case 'm':
- if (strcmp("overwrite", optarg) == 0) {
- config.mode = Mode::OVERWRITE;
- } else if (strcmp("keep", optarg) == 0) {
- config.mode = Mode::KEEP;
- } else if (strcmp("append", optarg) == 0) {
- config.mode = Mode::APPEND;
- } else {
- Dbg(dbg_ctl, "Unexpected mode: %s", optarg);
- return false;
- }
- break;
- case 'h':
- config.header_name = {optarg, strlen(optarg)};
- break;
- case 'v':
- config.via_header_name = {optarg, strlen(optarg)};
- break;
- case 'f':
- config.log_filename = {optarg, strlen(optarg)};
- break;
- case 's':
- config.standalone = true;
- break;
- case 'S':
- for (std::string_view input(optarg, strlen(optarg)); !input.empty();) {
- auto pos = input.find(',');
- config.servernames.emplace(input.substr(0, pos));
- input.remove_prefix(pos == std::string_view::npos ? input.size() : pos
+ 1);
- }
- break;
- case 'l':
- config.log_symbol = {optarg, strlen(optarg)};
- break;
- case 'e':
- config.export_name = {optarg, strlen(optarg)};
- if (config.export_name.empty()) {
- TSError("[%s] --export requires a non-empty user arg name",
PLUGIN_NAME);
- return false;
- }
- break;
- case 0:
- case -1:
- break;
- default:
- Dbg(dbg_ctl, "Unexpected options error.");
return false;
}
}
+ return true;
+}
- if (config.method.name == "uninitialized") {
- TSError("[%s] Method must be specified", PLUGIN_NAME);
- return false;
+constexpr std::string_view RELOAD_TAG = "jax_fingerprint.reload";
+
+/** Register a remap configuration file as a child of the remap configuration
so that editing it triggers a remap reload. */
+void
+register_remap_config_file(std::string_view filename)
+{
+ std::string const path = resolve_config_path(filename);
+ TSMgmtString parent = nullptr;
+ if (TSMgmtStringGet("proxy.config.url_remap.filename", &parent) !=
TS_SUCCESS) {
+ TSWarning("[%s] Could not retrieve the remap configuration filename; %s
changes require a remap.config change to reload",
+ PLUGIN_NAME, path.c_str());
+ return;
+ }
+ TSMgmtConfigFileAdd(parent, path.c_str());
+ TSfree(parent);
+}
+
+/** The configuration loaded by one plugin.config line, which can be reloaded
at runtime. */
+struct GlobalState {
+ std::string config_filename;
+ std::vector<PluginConfig *> configs;
+};
+
+void
+reload_global_config(GlobalState &state)
+{
+ char const *filename = state.config_filename.c_str();
+ Dbg(dbg_ctl, "Reloading configuration from %s", filename);
+
+ PluginConfigs updated;
+ if (!load_config_file(state.config_filename, PluginType::GLOBAL, updated)) {
+ TSError("[%s] Configuration reload from %s failed. Keeping current
configuration.", PLUGIN_NAME, filename);
+ return;
}
- Dbg(dbg_ctl, "JAx method is %s", config.method.name.data());
- Dbg(dbg_ctl, "JAx mode is %d", static_cast<int>(config.mode));
- Dbg(dbg_ctl, "JAx header is %s", !config.header_name.empty() ?
config.header_name.c_str() : "DISABLED");
- Dbg(dbg_ctl, "JAx via-header is %s", !config.via_header_name.empty() ?
config.via_header_name.c_str() : "DISABLED");
- Dbg(dbg_ctl, "JAx log file is %s", !config.log_filename.empty() ?
config.log_filename.c_str() : "DISABLED");
- Dbg(dbg_ctl, "JAx export registry is %s", !config.export_name.empty() ?
config.export_name.c_str() : PLUGIN_NAME);
- Dbg(dbg_ctl, "JAx standalone mode is %s", config.standalone ? "ENABLED" :
"DISABLED");
- for (auto &&servername : config.servernames) {
- Dbg(dbg_ctl, "%s", servername.c_str());
+ std::string reason;
+ if (!is_reload_compatible(state.configs, updated, reason)) {
+ TSError("[%s] Configuration reload from %s rejected: %s. Keeping current
configuration.", PLUGIN_NAME, filename,
+ reason.c_str());
+ return;
}
- return true;
+ for (size_t i = 0; i < updated.size(); ++i) {
+ state.configs[i]->set_settings(updated[i]->get_settings());
+ }
+ TSNote("[%s] Configuration reloaded successfully from %s", PLUGIN_NAME,
filename);
+}
+
+int
+handle_lifecycle_msg(TSCont contp, TSEvent /* event ATS_UNUSED */, void *edata)
+{
+ auto const *msg = static_cast<TSPluginMsg const *>(edata);
+ if (msg->tag != nullptr && msg->tag == RELOAD_TAG) {
+ reload_global_config(*static_cast<GlobalState *>(TSContDataGet(contp)));
+ }
+ return TS_SUCCESS;
}
+void
+register_log_field(PluginConfig *config)
+{
+ std::string name = "jax_fingerprint-";
+ name += config->method.name;
+ TSReturnCode const result = TSLogFieldRegister(
+ name.c_str(), config->log_symbol, TS_LOG_TYPE_STRING,
Review Comment:
The registered log field *name* is only based on `method` (e.g.,
`jax_fingerprint-JA4`). Since the YAML format allows listing the same method
more than once across different `export` registries, two entries can
legitimately both set `log_field` and then collide on the `name` passed to
`TSLogFieldRegister`, causing registration to fail. Make the field name unique
per configuration entry (e.g., include `export_name` and/or `log_symbol`, or
append a stable index) so multiple `log_field` registrations for the same
method don’t conflict.
##########
tests/gold_tests/pluginTest/jax_fingerprint/jax_fingerprint_reload.test.py:
##########
@@ -0,0 +1,219 @@
+'''
+Verify reloading the jax_fingerprint plugin configuration at runtime.
+
+Covers traffic_ctl plugin msg jax_fingerprint.reload for configurations
+loaded from plugin.config, and traffic_ctl config reload for remap
+configuration files.
+'''
+# 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
+
+Test.Summary = __doc__
+Test.SkipUnless(Condition.PluginExists('jax_fingerprint.so'))
+exec(open(os.path.join(Test.TestDirectory,
'jax_fingerprint_common.py')).read())
+
+
+class ReloadTest:
+ '''Verify that traffic_ctl plugin msg jax_fingerprint.reload re-reads the
configuration.
+
+ The servernames allowlist is switched at runtime from jax.server.test to
+ jax-filtered.server.test, and a reload that changes a startup-only
+ setting is rejected while the current configuration stays in effect.
+ A second plugin.config line with its own configuration file verifies
+ that each line reloads its own file independently.
+ '''
+
+ _replay_file: str = 'jax_fingerprint_reload.replay.yaml'
+ _config_name: str = 'jax_fingerprint.yaml'
+ _second_config_name: str = 'jax_fingerprint_ja4h.yaml'
+ _client_counter: int = 0
+
+ def __init__(self, name: str) -> None:
+ '''Configure the reload test runs.'''
+ self._name = name
+ tr = Test.AddTestRun(f'{name}: initial servernames')
+ self._dns = Test.MakeDNServer('dns_reload', default='127.0.0.1')
+ self._server = Test.MakeVerifierServerProcess('server_reload',
self._replay_file)
+ self._configure_trafficserver()
+ self._add_client_run(tr, 'initial-allowed initial-filtered',
start_processes=True)
+
+ self._add_reload_run(f'{name}: reload new servernames',
'jax_fingerprint_reloaded.yaml')
+ Test.AddAwaitFileContainsTestRun(
+ f'{name}: await the reload', self._ts.Disk.diags_log.AbsPath,
+ f'Configuration reloaded successfully from {self._config_name}')
+ self._add_client_run(Test.AddTestRun(f'{name}: reloaded servernames'),
'reloaded-allowed reloaded-filtered')
+
+ self._add_reload_run(f'{name}: reload a startup-only change',
'jax_fingerprint_incompatible.yaml')
+ Test.AddAwaitFileContainsTestRun(
+ f'{name}: await the rejected reload',
self._ts.Disk.diags_log.AbsPath,
+ f'Configuration reload from {self._config_name} rejected')
+ self._add_client_run(Test.AddTestRun(f'{name}: servernames kept after
rejection'), 'reloaded-allowed reloaded-filtered')
+
+ def _configure_trafficserver(self) -> None:
+ '''Configure Traffic Server with the initial and the reloaded
configurations.'''
+ self._ts = Test.MakeATSProcess('ts_reload', enable_cache=False,
enable_tls=True)
+ self._ts.addDefaultSSLFiles()
+ self._ts.Disk.ssl_multicert_yaml.AddLines(
+ """
+ssl_multicert:
+ - dest_ip: "*"
+ ssl_cert_name: server.pem
+ ssl_key_name: server.key
+""".split("\n"))
+ 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',
+ })
+
+ def fingerprint(method: str, servername: str) -> dict[str, str | bool
| list[str]]:
+ return {
+ 'method': method,
+ 'standalone': True,
+ 'header': 'x-jax',
+ 'via_header': 'x-jax-via',
+ 'servernames': [servername],
+ }
+
+
self._ts.Disk.MakeConfigFile(self._config_name).update(make_config([fingerprint('JA4',
'jax.server.test')]))
+ self._ts.Disk.MakeConfigFile('jax_fingerprint_reloaded.yaml').update(
+ make_config([fingerprint('JA4', 'jax-filtered.server.test')]))
+
self._ts.Disk.MakeConfigFile('jax_fingerprint_incompatible.yaml').update(
+ make_config([fingerprint('JA3', 'jax.server.test')]))
+ self._ts.Disk.MakeConfigFile(self._second_config_name).update(
+ make_config([{
+ 'method': 'JA4H',
+ 'standalone': True,
+ 'header': 'x-ja4h'
+ }]))
+ self._ts.Disk.plugin_config.AddLines(
+ [
+ f'jax_fingerprint.so {self._config_name}',
+ f'jax_fingerprint.so {self._second_config_name}',
+ ])
+
+ server_port = self._server.Variables.https_port
+ for host in ('jax.server.test', 'jax-filtered.server.test'):
+ self._ts.Disk.remap_config.AddLine(f'map https://{host}
https://jax.backend.test:{server_port}')
+
+ # Replace the default "no errors" check since the rejected reload is
logged as an error.
+ self._ts.Disk.diags_log.Content = Testers.ContainsExpression(
+ r"Configuration reload from jax_fingerprint\.yaml rejected:
'method' of fingerprint entry 1 changed",
+ 'Verify the reload that changes a startup-only setting is
rejected.')
+ self._ts.Disk.diags_log.Content += Testers.ContainsExpression(
+ r'Configuration reloaded successfully from
jax_fingerprint_ja4h\.yaml',
+ 'Verify the second plugin.config line reloads its own
configuration.')
+
+ def _add_reload_run(self, name: str, config_name: str) -> None:
+ '''Install a configuration file and ask the plugin to reload it.'''
+ config_dir = self._ts.Variables.CONFIGDIR
+ tr = Test.AddTestRun(name)
+ tr.Processes.Default.Command = (
+ f'cp {config_dir}/{config_name} {config_dir}/{self._config_name}
&& '
+ 'traffic_ctl plugin msg jax_fingerprint.reload')
+ tr.Processes.Default.Env = self._ts.Env
+ tr.Processes.Default.ReturnCode = 0
+ tr.StillRunningAfter = self._ts
+
+ def _add_client_run(self, tr: 'TestRun', keys: str, start_processes: bool
= False) -> None:
+ '''Run the replay sessions selected by keys.'''
+ p = tr.AddVerifierClientProcess(
+ f'client_reload{ReloadTest._client_counter}',
+ self._replay_file,
+ http_ports=[self._ts.Variables.port],
+ https_ports=[self._ts.Variables.ssl_port],
+ keys=keys)
+ if start_processes:
+ p.StartBefore(self._dns)
+ p.StartBefore(self._server)
+ p.StartBefore(self._ts)
+ ReloadTest._client_counter += 1
+ tr.StillRunningAfter = self._ts
+ tr.StillRunningAfter = self._server
+
+
+ReloadTest('Reload servernames')
+
+
+class RemapReloadTest:
+ '''Verify that editing only a remap configuration file and running
traffic_ctl config reload reloads it.'''
+
+ _replay_file: str = 'jax_fingerprint_remap_reload.replay.yaml'
+ _config_name: str = 'jax_fingerprint_remap.yaml'
+
+ def __init__(self, name: str) -> None:
+ '''Configure the remap reload test runs.'''
+ self._dns = Test.MakeDNServer('dns_remap_reload', default='127.0.0.1')
+ self._server = Test.MakeVerifierServerProcess('server_remap_reload',
self._replay_file)
+ self._configure_trafficserver()
+
+ tr = Test.AddTestRun(f'{name}: initial header')
+ client = self._add_client(tr, 'client_remap_reload0', 'remap-initial')
+ client.StartBefore(self._dns)
+ client.StartBefore(self._server)
+ client.StartBefore(self._ts)
+
+ config_dir = self._ts.Variables.CONFIGDIR
+ traffic_out = self._ts.Disk.traffic_out.AbsPath
+ loading = f'Loading configuration from
{config_dir}/{self._config_name}'
+ tr = Test.AddTestRun(f'{name}: change only the remap configuration and
reload')
+ tr.Processes.Default.Command = (
+ f'cp {config_dir}/jax_fingerprint_remap_reloaded.yaml
{config_dir}/{self._config_name} && '
+ 'traffic_ctl config reload && '
+ f'for i in $$(seq 60); do [ "$$(grep -c "{loading}"
{traffic_out})" -ge 2 ] && exit 0; sleep 1; done; exit 1')
Review Comment:
This shell command has broken quoting: the f-string expands into `\"$$(grep
-c \"Loading ...\" file)\"`, so the inner `\"{loading}\"` double-quotes
prematurely terminate the outer quotes and will cause a syntax error. Use a
quoting approach that doesn’t nest unescaped double-quotes (e.g., wrap the grep
pattern in single quotes, or escape the inner quotes). Also consider `grep
-F`/`-cF` so `{loading}` is treated as a fixed string rather than a regex.
--
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]
