From 96f2d5d369f90746ea590341657a1eb762711403 Mon Sep 17 00:00:00 2001 From: Arne Welzel Date: Mon, 26 May 2025 16:56:32 +0200 Subject: [PATCH] Event/init-bare: Add add_missing_remote_network_timestamp logic Make defaulting to the local network timestamp for remote events opt-in. --- NEWS | 7 +++ scripts/base/init-bare.zeek | 10 ++++ src/Event.cc | 46 +++++++++++++------ src/const.bif | 1 + .../btest/broker/remote_event_ts_compat.zeek | 5 ++ 5 files changed, 54 insertions(+), 15 deletions(-) diff --git a/NEWS b/NEWS index 1204e667f1..72bae2258c 100644 --- a/NEWS +++ b/NEWS @@ -30,6 +30,13 @@ Breaking Changes dispatched. Previously this would've likely been 0.0, or the previously dispatched event. +- Missing network timestamp metadata on remote events is not set to the local + network time anymore by default. This potentially hid useful debugging information + about another node not sending timestamp metadata. The old behavior can be + re-enabled as follows: + + redef EventMetadata::add_missing_remote_network_timestamp = T; + New Functionality ----------------- diff --git a/scripts/base/init-bare.zeek b/scripts/base/init-bare.zeek index 9fd71dae1a..cfa9dfd1e8 100644 --- a/scripts/base/init-bare.zeek +++ b/scripts/base/init-bare.zeek @@ -601,6 +601,16 @@ export { ## might be a value before the network_time() when the event ## was actually dispatched. const add_network_timestamp: bool = F &redef; + + ## By default, remote events without network timestamp metadata + ## will yield a negative zeek:see:`current_event_time` during + ## processing. To have the receiving Zeek node set the event's + ## network timestamp metadata with its current local network time, + ## set this option to true. + ## + ## This setting is only in effect if :zeek:see:`EventMetadata::add_network_timestamp` + ## is also set to true. + const add_missing_remote_network_timestamp: bool = F &redef; } module FTP; diff --git a/src/Event.cc b/src/Event.cc index d8694aebe7..1fc707327a 100644 --- a/src/Event.cc +++ b/src/Event.cc @@ -11,6 +11,7 @@ #include "zeek/Val.h" #include "zeek/iosource/Manager.h" #include "zeek/plugin/Manager.h" +#include "zeek/util.h" #include "const.bif.netvar_h" #include "event.bif.netvar_h" @@ -165,12 +166,19 @@ void EventMgr::Enqueue(const EventHandlerPtr& h, Args vl, util::detail::SourceID detail::EventMetadataVectorPtr meta; double ts = double(deprecated_ts); - if ( src == util::detail::SOURCE_LOCAL && BifConst::EventMetadata::add_network_timestamp ) { - // If this is a local event and EventMetadata::add_network_timestamp is - // enabled automatically set the network timestamp for this event to the - // current network time when it is < 0 (default is -1.0). - // - // See the other Enqueue() implementation for the local motivation. + + // If this is a local event and EventMetadata::add_network_timestamp is + // enabled, automatically set the network timestamp for this event to the + // current network time when it is < 0 (default of deprecated_ts is -1.0). + // + // See the other Enqueue() implementation for the local vs broker/remote + // motivation of want_network_timestamp. + bool want_network_timestamp = + BifConst::EventMetadata::add_network_timestamp && + ((src == util::detail::SOURCE_LOCAL) || + (src == util::detail::SOURCE_BROKER && BifConst::EventMetadata::add_missing_remote_network_timestamp)); + + if ( want_network_timestamp ) { if ( ts < 0.0 ) ts = run_state::network_time; @@ -189,15 +197,23 @@ void EventMgr::Enqueue(const EventHandlerPtr& h, Args vl, util::detail::SourceID void EventMgr::Enqueue(detail::EventMetadataVectorPtr meta, const EventHandlerPtr& h, Args vl, util::detail::SourceID src, analyzer::ID aid, Obj* obj) { - if ( src == util::detail::SOURCE_LOCAL && BifConst::EventMetadata::add_network_timestamp ) { - // If all events are supposed to have a network time attached, ensure - // that the meta vector was passed *and* contains a network timestamp. - // - // This is only done for local events, however. For remote events (src == BROKER) - // that do not hold network timestamp metadata, it seems less surprising to keep - // it unset. If it is required that a remote node sends *their* network timestamp, - // defaulting to this node's network time seems more confusing and error prone - // than just leaving it unset and having the consumer deal with the situation. + // Attach network timestamps to all events if EventMetadata::add_network_timestamp is T and + // + // 1) this event is locally generated + // or + // 2) this is a remote event and EventMetadata::add_missing_remote_network_timestamp is T + // + // Why so complicated? It seems less surprising behavior to keep network timestamp metadata unset + // if a remote event didn't have any attached. It should help to more easily figure out what's + // actually going on compared to setting it to the local network time. If all nodes are required to + // send *their* network timestamp, filling it with this node's network time seems more confusing + // and error prone compared to just leaving it unset and having the consumer deal with the situation. + bool want_network_timestamp = + BifConst::EventMetadata::add_network_timestamp && + ((src == util::detail::SOURCE_LOCAL) || + (src == util::detail::SOURCE_BROKER && BifConst::EventMetadata::add_missing_remote_network_timestamp)); + + if ( want_network_timestamp ) { bool has_time = false; if ( ! meta ) { diff --git a/src/const.bif b/src/const.bif index 482a325588..7874352d6d 100644 --- a/src/const.bif +++ b/src/const.bif @@ -36,3 +36,4 @@ const Log::write_buffer_size: count; const Storage::expire_interval: interval; const EventMetadata::add_network_timestamp: bool; +const EventMetadata::add_missing_remote_network_timestamp: bool; diff --git a/testing/btest/broker/remote_event_ts_compat.zeek b/testing/btest/broker/remote_event_ts_compat.zeek index 991e425759..36a00a74c9 100644 --- a/testing/btest/broker/remote_event_ts_compat.zeek +++ b/testing/btest/broker/remote_event_ts_compat.zeek @@ -19,6 +19,11 @@ redef exit_only_after_terminate = T; redef allow_network_time_forward = F; redef EventMetadata::add_network_timestamp = T; +# This is needed so that the receiving node sets its +# own local network timestamp on remote events that do +# not have network timestamp metadata. +redef EventMetadata::add_missing_remote_network_timestamp = T; + event zeek_init() { Broker::subscribe(getenv("TOPIC"));