Copilot commented on code in PR #12938:
URL: https://github.com/apache/trafficserver/pull/12938#discussion_r2885430250
##########
doc/admin-guide/plugins/ja4_fingerprint.en.rst:
##########
@@ -78,13 +78,22 @@ Key Differences from JA3
Plugin Configuration
====================
-The plugin operates as a global plugin and has no configuration options.
+The plugin operates as a global plugin.
To enable the plugin, add the following line to :file:`plugin.config`::
ja4_fingerprint.so
-No additional parameters are required or supported.
+
+.. option:: --preserve
+
+This option controls whether the plugin preserves any existing JA4 header. If
the option is specified the plugin keep the header
+intact. If the option is not speficied, the plugins appends a generated
fingerprint to the existing header value.
Review Comment:
Typo in documentation: "speficied" should be "specified".
```suggestion
intact. If the option is not specified, the plugins appends a generated
fingerprint to the existing header value.
```
##########
doc/admin-guide/plugins/ja4_fingerprint.en.rst:
##########
@@ -110,7 +119,7 @@ Log Output
==========
The plugin writes to ``ja4_fingerprint.log`` in the Traffic Server log
-directory (typically ``/var/log/trafficserver/``).
+directory (typically ``/var/log/trafficserver/``) if the feature is not
disabled.
Review Comment:
"if the feature is not disabled" is unclear—this appears to refer
specifically to `--nologging`. Consider rephrasing to explicitly mention
`--nologging`, and also update earlier sections (e.g., the "Plugin Behavior"
bullet that says it always "Log to File") to reflect the conditional logging
behavior.
##########
plugins/experimental/ja4_fingerprint/plugin.cc:
##########
@@ -207,8 +212,11 @@ handle_client_hello(TSCont /* cont ATS_UNUSED */, TSEvent
event, void *edata)
} else {
auto data{std::make_unique<JA4_data>()};
data->fingerprint = get_fingerprint(ch);
+ Dbg(dbg_ctl, "JA4 fingerprint: %s", data->fingerprint.c_str());
get_IP(TSNetVConnRemoteAddrGet(ssl_vc), data->IP_addr);
- log_fingerprint(data.get());
+ if (!global_nologging_enabled) {
+ log_fingerprint(data.get());
+ }
// The VCONN_CLOSE handler is now responsible for freeing the resource.
TSUserArgSet(ssl_vc, *get_user_arg_index(), static_cast<void
*>(data.release()));
}
Review Comment:
`TSUserArgSet()` stores a `JA4_data*` on the vconn, but other hooks treat
this user arg as a `std::string*` (see `TSUserArgGet()` usage in
`handle_read_request_hdr()` and the `delete` in `handle_vconn_close()`).
Deleting/using a `JA4_data*` as a `std::string*` is undefined behavior even if
the `std::string` is the first member. Please make the stored type consistent
(e.g., store `JA4_data*` and update the later casts/deletes, or store only a
`std::string*` and adjust `log_fingerprint()` accordingly).
##########
doc/admin-guide/plugins/ja4_fingerprint.en.rst:
##########
@@ -78,13 +78,22 @@ Key Differences from JA3
Plugin Configuration
====================
-The plugin operates as a global plugin and has no configuration options.
+The plugin operates as a global plugin.
To enable the plugin, add the following line to :file:`plugin.config`::
ja4_fingerprint.so
-No additional parameters are required or supported.
+
+.. option:: --preserve
+
+This option controls whether the plugin preserves any existing JA4 header. If
the option is specified the plugin keep the header
+intact. If the option is not speficied, the plugins appends a generated
fingerprint to the existing header value.
+
+.. option:: --nologging
+
+This option disables log output.
Review Comment:
"This option disables log output" is ambiguous now that debug logging
(`Dbg`) still emits fingerprints when debug tags are enabled. Please clarify
that `--nologging` disables writing to `ja4_fingerprint.log` (text log), and
whether debug output is still expected.
```suggestion
This option disables writing fingerprints to the :file:`ja4_fingerprint.log`
text log file. It does not affect Traffic Server debug
logging (``Dbg``); if relevant debug tags are enabled, fingerprints may
still appear in debug logs.
```
##########
plugins/experimental/ja4_fingerprint/plugin.cc:
##########
@@ -148,16 +151,18 @@ TSPluginInit(int argc, char const **argv)
TSError("[%s] Failed to register.", PLUGIN_NAME);
return;
}
- if (!read_config_option(argc, argv, global_preserve_enabled)) {
+ if (!read_config_option(argc, argv, global_preserve_enabled,
global_nologging_enabled)) {
TSError("[%s] Failed to parse options.", PLUGIN_NAME);
return;
}
reserve_user_arg();
- if (!create_log_file()) {
- TSError("[%s] Failed to create log.", PLUGIN_NAME);
- return;
- } else {
- Dbg(dbg_ctl, "Created log file.");
+ if (!global_nologging_enabled) {
+ if (!create_log_file()) {
+ TSError("[%s] Failed to create log.", PLUGIN_NAME);
+ return;
+ } else {
+ Dbg(dbg_ctl, "Created log file.");
+ }
}
Review Comment:
The new `--nologging` behavior isn't covered by existing tests. Since there
are already gold tests for this plugin, please add a test case that runs with
`ja4_fingerprint.so --nologging` and asserts (1) no `ja4_fingerprint.log` is
created/written and (2) JA4 headers are still injected.
##########
doc/admin-guide/plugins/ja4_fingerprint.en.rst:
##########
@@ -78,13 +78,22 @@ Key Differences from JA3
Plugin Configuration
====================
-The plugin operates as a global plugin and has no configuration options.
+The plugin operates as a global plugin.
To enable the plugin, add the following line to :file:`plugin.config`::
ja4_fingerprint.so
-No additional parameters are required or supported.
+
+.. option:: --preserve
+
+This option controls whether the plugin preserves any existing JA4 header. If
the option is specified the plugin keep the header
+intact. If the option is not speficied, the plugins appends a generated
fingerprint to the existing header value.
Review Comment:
The `--preserve` option description has several grammar issues and is hard
to read as a single long line (e.g., "the plugin keep", "the plugins appends").
Please fix the wording and wrap the text consistently with the rest of this RST
file.
```suggestion
This option controls whether the plugin preserves any existing JA4 header.
If the option is specified, the plugin keeps the header intact. If the
option is not specified, the plugin appends a generated fingerprint to the
existing header value.
```
--
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]
