bneradt commented on code in PR #13838:
URL: https://github.com/apache/trafficserver/pull/13838#discussion_r4457132779
##########
plugins/experimental/jax_fingerprint/plugin.cc:
##########
@@ -310,11 +310,81 @@ handle_read_request_hdr(void *edata, PluginConfig &config)
return TS_SUCCESS;
}
+int
+handle_ssn_start(void *edata, PluginConfig &config)
+{
+ TSHttpSsn ssnp = static_cast<TSHttpSsn>(edata);
+
+ if (TSVConn vconn = TSHttpSsnClientVConnGet(ssnp); vconn != nullptr) {
+ if (JAxContext *ctx = get_user_arg(vconn, config); ctx != nullptr) {
+ copy_to_session_user_arg(ssnp, config, *ctx);
+ }
+ }
+
+ TSHttpSsnReenable(ssnp, TS_EVENT_HTTP_CONTINUE);
+ return TS_SUCCESS;
+}
+
+int
+handle_ssn_close(void *edata, PluginConfig &config)
+{
+ TSHttpSsn ssnp = static_cast<TSHttpSsn>(edata);
+
+ cleanup_session_user_arg(ssnp, config);
+
+ TSHttpSsnReenable(ssnp, TS_EVENT_HTTP_CONTINUE);
+ return TS_SUCCESS;
+}
+
+/** Find the context whose fingerprint the --log-field callback logs for a
transaction.
+ *
+ * @param[in] txnp The transaction being logged.
+ * @param[in] config The plugin configuration.
+ * @return The context, or nullptr if no fingerprint was generated for the
transaction.
+ */
+JAxContext *
+get_log_context(TSHttpTxn txnp, PluginConfig &config)
+{
+ TSHttpSsn ssnp = TSHttpTxnSsnGet(txnp);
+
+ if (config.method.type != Method::Type::CONNECTION_BASED) {
+ if (JAxContext *ctx = get_user_arg(txnp, config); ctx != nullptr) {
+ return ctx;
+ }
+ if (ssnp == nullptr) {
+ return nullptr;
+ }
+ // A later transaction on the session may have replaced the copy made at
TXN_CLOSE.
+ JAxContext *ctx = get_session_user_arg(ssnp, config);
+ return ctx != nullptr && ctx->get_txn_id() == TSHttpTxnIdGet(txnp) ? ctx :
nullptr;
Review Comment:
Accepted as a known limitation and documented: `jax_fingerprint.en.rst` now
says an HTTP/2 stream whose logging is delayed may log `-` for JA4H. The
transaction id check means it never logs another stream's fingerprint. There is
no post-logging hook to free per-transaction entries, so a per-txn map would
grow until session close.
##########
plugins/experimental/jax_fingerprint/userarg.cc:
##########
@@ -36,85 +36,136 @@ user_arg_type_name(TSUserArgType type)
switch (type) {
case TS_USER_ARGS_VCONN:
return "vconn";
+ case TS_USER_ARGS_SSN:
+ return "ssn";
case TS_USER_ARGS_TXN:
return "txn";
default:
return "unknown";
}
}
-} // anonymous namespace
-
int
-reserve_user_arg(PluginConfig &config)
+reserve_slot(TSUserArgType type, PluginConfig const &config, int &index)
{
- TSUserArgType type = config.method.type == Method::Type::CONNECTION_BASED ?
TS_USER_ARGS_VCONN : TS_USER_ARGS_TXN;
- const char *name = config.export_name.empty() ? PLUGIN_NAME :
config.export_name.c_str();
+ const char *name = config.export_name.empty() ? PLUGIN_NAME :
config.export_name.c_str();
const char *description = nullptr;
- if (TSUserArgIndexNameLookup(type, name, &config.user_arg_index,
&description) == TS_SUCCESS) {
+ if (TSUserArgIndexNameLookup(type, name, &index, &description) ==
TS_SUCCESS) {
if (description == nullptr || std::strcmp(description,
jax_fingerprint::REGISTRY_DESCRIPTION) != 0) {
TSError("[%s] User arg '%s' is already reserved with an incompatible
data contract", PLUGIN_NAME, name);
return TS_ERROR;
}
- } else if (TSUserArgIndexReserve(type, name,
jax_fingerprint::REGISTRY_DESCRIPTION, &config.user_arg_index) == TS_SUCCESS) {
- Dbg(dbg_ctl, "Reserved shared user_arg slot: type=%s, name=%s, index=%d",
user_arg_type_name(type), name,
- config.user_arg_index);
+ } else if (TSUserArgIndexReserve(type, name,
jax_fingerprint::REGISTRY_DESCRIPTION, &index) == TS_SUCCESS) {
+ Dbg(dbg_ctl, "Reserved shared user_arg slot: type=%s, name=%s, index=%d",
user_arg_type_name(type), name, index);
} else {
Dbg(dbg_ctl, "Failed to reserve shared user_arg slot: type=%s, name=%s",
user_arg_type_name(type), name);
return TS_ERROR;
}
Dbg(dbg_ctl, "Using shared user_arg: type=%s, name=%s, method=%.*s,
index=%d", user_arg_type_name(type), name,
- static_cast<int>(config.method.name.size()), config.method.name.data(),
config.user_arg_index);
+ static_cast<int>(config.method.name.size()), config.method.name.data(),
index);
return TS_SUCCESS;
}
+ContextMap *
+get_map(void *container, int index)
+{
+ auto *registry = static_cast<jax_fingerprint::RegistryV1
*>(TSUserArgGet(container, index));
+ return registry == nullptr ? nullptr : static_cast<ContextMap *>(registry);
+}
+
void
-set_user_arg(void *container, PluginConfig &config, JAxContext *ctx)
+set_context(void *container, int index, std::string_view method_name,
JAxContext *ctx)
{
- auto *registry = static_cast<jax_fingerprint::RegistryV1
*>(TSUserArgGet(container, config.user_arg_index));
- ContextMap *map = registry == nullptr ? nullptr :
static_cast<ContextMap *>(registry);
+ ContextMap *map = get_map(container, index);
if (map == nullptr) {
map = new ContextMap();
- TSUserArgSet(container, config.user_arg_index,
static_cast<jax_fingerprint::RegistryV1 *>(map));
+ TSUserArgSet(container, index, static_cast<jax_fingerprint::RegistryV1
*>(map));
}
- map->set(config.method.name, ctx);
+ map->set(method_name, ctx);
}
JAxContext *
-get_user_arg(void *container, PluginConfig &config)
+get_context(void *container, int index, std::string_view method_name)
{
- auto *registry = static_cast<jax_fingerprint::RegistryV1
*>(TSUserArgGet(container, config.user_arg_index));
- ContextMap *map = registry == nullptr ? nullptr :
static_cast<ContextMap *>(registry);
- if (map == nullptr) {
- return nullptr;
+ ContextMap *map = get_map(container, index);
+ return map == nullptr ? nullptr : map->get(method_name);
+}
+
+void
+cleanup_context(void *container, int index, std::string_view method_name)
+{
+ ContextMap *map = get_map(container, index);
+ if (map != nullptr) {
+ // Remove this plugin's context from the map.
+ map->remove(method_name);
+
+ // If the map is now empty, delete it and clear the user arg.
+ if (map->empty()) {
+ delete map;
+ TSUserArgSet(container, index, nullptr);
+ }
}
- return map->get(config.method.name);
+}
+
+} // anonymous namespace
+
+int
+reserve_user_arg(PluginConfig &config)
+{
+ TSUserArgType type = config.method.type == Method::Type::CONNECTION_BASED ?
TS_USER_ARGS_VCONN : TS_USER_ARGS_TXN;
+ return reserve_slot(type, config, config.user_arg_index);
+}
+
+void
+set_user_arg(void *container, PluginConfig &config, JAxContext *ctx)
+{
+ set_context(container, config.user_arg_index, config.method.name, ctx);
+}
+
+JAxContext *
+get_user_arg(void *container, PluginConfig &config)
+{
+ return get_context(container, config.user_arg_index, config.method.name);
}
void
refresh_user_arg(void *container, PluginConfig &config)
{
- auto *registry = static_cast<jax_fingerprint::RegistryV1
*>(TSUserArgGet(container, config.user_arg_index));
- if (registry != nullptr) {
- static_cast<ContextMap *>(registry)->refresh(config.method.name);
+ if (ContextMap *map = get_map(container, config.user_arg_index); map !=
nullptr) {
+ map->refresh(config.method.name);
}
}
void
cleanup_user_arg(void *container, PluginConfig &config)
{
- auto *registry = static_cast<jax_fingerprint::RegistryV1
*>(TSUserArgGet(container, config.user_arg_index));
- ContextMap *map = registry == nullptr ? nullptr :
static_cast<ContextMap *>(registry);
- if (map != nullptr) {
- // Remove this plugin's context from the map.
- map->remove(config.method.name);
+ cleanup_context(container, config.user_arg_index, config.method.name);
+}
- // If the map is now empty, delete it and clear the user arg.
- if (map->empty()) {
- delete map;
- TSUserArgSet(container, config.user_arg_index, nullptr);
- }
- }
+int
+reserve_session_user_arg(PluginConfig &config)
+{
+ return reserve_slot(TS_USER_ARGS_SSN, config, config.session_user_arg_index);
+}
+
+void
+copy_to_session_user_arg(TSHttpSsn ssnp, PluginConfig &config, JAxContext
const &ctx, uint64_t txn_id)
+{
+ auto *copy = new JAxContext(ctx);
+ copy->set_txn_id(txn_id);
+ set_context(ssnp, config.session_user_arg_index, config.method.name, copy);
+}
Review Comment:
The session copy no longer goes through `ContextMap`.
`SessionFingerprints::set` reuses the method's existing entry and assigns the
string in place, so nothing is allocated or leaked per transaction.
(`ContextMap::set` also deletes the context it replaces.)
##########
src/api/InkAPI.cc:
##########
@@ -9688,7 +9688,8 @@ TSLogStringUnmarshal(char **buf, char *dest, int len)
// of the alignment.
if (int l = strlen(*buf); l < len) {
memcpy(dest, *buf, l);
- return {l, l};
+ // TSLogStringMarshal also wrote the terminating NUL, so the field
occupies one more byte than was written to dest.
+ return {l + 1, l};
} else {
Review Comment:
The `strlen` predates this PR, which only changes the returned consumed
length. The log buffer always holds the NUL that `TSLogStringMarshal` wrote,
and a bounded scan belongs in its own change, so I'm leaving this thread open
rather than changing it here.
--
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]
