From d828e08a9edb2645d4b8e05a3e4b93af466ea242 Mon Sep 17 00:00:00 2001 From: Arne Welzel Date: Mon, 26 May 2025 12:29:39 +0200 Subject: [PATCH 1/3] cluster and broker: Propagate zero-timestamp as metadata, too. This will be cleaned up later to just pass all contained metadata from a cluster event to the queued event, but for now do this here, otherwise we break some internal tests. --- src/broker/Manager.cc | 2 +- src/cluster/Backend.cc | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/broker/Manager.cc b/src/broker/Manager.cc index e42bed29ab..2e36509a2f 100644 --- a/src/broker/Manager.cc +++ b/src/broker/Manager.cc @@ -1657,7 +1657,7 @@ void Manager::ProcessMessage(std::string_view topic, broker::zeek::Event& ev) { if ( vl.size() == args.size() ) { zeek::detail::EventMetadataVectorPtr meta; - if ( ts > 0.0 ) + if ( ts >= 0.0 ) meta = zeek::detail::MakeEventMetadataVector(ts); event_mgr.Enqueue(std::move(meta), handler, std::move(vl), util::detail::SOURCE_BROKER); diff --git a/src/cluster/Backend.cc b/src/cluster/Backend.cc index f7300e22d6..ae23a5fbf1 100644 --- a/src/cluster/Backend.cc +++ b/src/cluster/Backend.cc @@ -24,7 +24,7 @@ using namespace zeek::cluster; bool detail::LocalEventHandlingStrategy::DoProcessEvent(std::string_view topic, detail::Event e) { zeek::detail::EventMetadataVectorPtr meta; - if ( auto ts = e.Timestamp(); ts > 0.0 ) + if ( auto ts = e.Timestamp(); ts >= 0.0 ) meta = zeek::detail::MakeEventMetadataVector(e.Timestamp()); zeek::event_mgr.Enqueue(std::move(meta), e.Handler(), std::move(e.Args()), util::detail::SOURCE_BROKER); From a9d22611d05970e0645a8c1d8669e9e86e85bcef Mon Sep 17 00:00:00 2001 From: Arne Welzel Date: Mon, 26 May 2025 12:37:09 +0200 Subject: [PATCH 2/3] EventMgr/Enqueue: Add automatic timestamp metadata to local events, only It seems less surprising if only local events receive automatic network timestamp metadata. For remote events the automatic value will most likely be misleading. --- src/Event.cc | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/src/Event.cc b/src/Event.cc index c6af18be0d..8164ba3dd8 100644 --- a/src/Event.cc +++ b/src/Event.cc @@ -165,8 +165,13 @@ void EventMgr::Enqueue(const EventHandlerPtr& h, Args vl, util::detail::SourceID detail::EventMetadataVectorPtr meta; double ts = double(deprecated_ts); - if ( BifConst::EventMetadata::add_network_timestamp ) { - if ( ts < 0.0 ) // default -1.0, modify to current network_time + 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 ( ts < 0.0 ) ts = run_state::network_time; // In v8.1 when the deprecated_ts parameters is gone: Just use run_state::network_time directly here. @@ -184,9 +189,15 @@ 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 ( BifConst::EventMetadata::add_network_timestamp ) { + 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. bool has_time = false; if ( ! meta ) { From 277c3f524534036629be1f5f25e37c6b68a1ddca Mon Sep 17 00:00:00 2001 From: Arne Welzel Date: Mon, 26 May 2025 13:23:19 +0200 Subject: [PATCH 3/3] btest: Add test for Cluster::hello zero-timestamp --- .../..manager..stdout | 3 ++ .../..worker-1..stdout | 3 ++ .../cluster-hello-zero-network-timestamp.zeek | 48 +++++++++++++++++++ 3 files changed, 54 insertions(+) create mode 100644 testing/btest/Baseline/cluster.broker.cluster-hello-zero-network-timestamp/..manager..stdout create mode 100644 testing/btest/Baseline/cluster.broker.cluster-hello-zero-network-timestamp/..worker-1..stdout create mode 100644 testing/btest/cluster/broker/cluster-hello-zero-network-timestamp.zeek diff --git a/testing/btest/Baseline/cluster.broker.cluster-hello-zero-network-timestamp/..manager..stdout b/testing/btest/Baseline/cluster.broker.cluster-hello-zero-network-timestamp/..manager..stdout new file mode 100644 index 0000000000..d07af135ca --- /dev/null +++ b/testing/btest/Baseline/cluster.broker.cluster-hello-zero-network-timestamp/..manager..stdout @@ -0,0 +1,3 @@ +### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. +Cluster::hello name=worker-1 is_remote_event=T metadata=[[id=EventMetadata::NETWORK_TIMESTAMP, val=0.0]] +Cluster::node_up name=worker-1 is_remote_event=F metadata=[[id=EventMetadata::NETWORK_TIMESTAMP, val=XXXXXXXXXX.XXXXXX]] diff --git a/testing/btest/Baseline/cluster.broker.cluster-hello-zero-network-timestamp/..worker-1..stdout b/testing/btest/Baseline/cluster.broker.cluster-hello-zero-network-timestamp/..worker-1..stdout new file mode 100644 index 0000000000..09db3fb744 --- /dev/null +++ b/testing/btest/Baseline/cluster.broker.cluster-hello-zero-network-timestamp/..worker-1..stdout @@ -0,0 +1,3 @@ +### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. +Cluster::hello name=manager is_remote_event=T metadata=[[id=EventMetadata::NETWORK_TIMESTAMP, val=XXXXXXXXXX.XXXXXX]] +Cluster::node_up name=manager is_remote_event=F metadata=[[id=EventMetadata::NETWORK_TIMESTAMP, val=0.0]] diff --git a/testing/btest/cluster/broker/cluster-hello-zero-network-timestamp.zeek b/testing/btest/cluster/broker/cluster-hello-zero-network-timestamp.zeek new file mode 100644 index 0000000000..49541794ae --- /dev/null +++ b/testing/btest/cluster/broker/cluster-hello-zero-network-timestamp.zeek @@ -0,0 +1,48 @@ +# @TEST-DOC: Ensure Cluster::hello sent by a worker with a zero network time is observed with a zero network timestamp by the manager. +# +# @TEST-PORT: BROKER_MANAGER_PORT +# @TEST-PORT: BROKER_WORKER1_PORT +# +# @TEST-EXEC: cp $FILES/broker/cluster-layout.zeek . +# +# @TEST-EXEC: zeek -b --parse-only %INPUT +# +# @TEST-EXEC: btest-bg-run manager "ZEEKPATH=$ZEEKPATH:.. && CLUSTER_NODE=manager zeek -b %INPUT" +# @TEST-EXEC: btest-bg-run worker-1 "ZEEKPATH=$ZEEKPATH:.. && CLUSTER_NODE=worker-1 zeek -b %INPUT" +# +# @TEST-EXEC: btest-bg-wait 30 +# @TEST-EXEC: btest-diff ./manager/.stdout +# @TEST-EXEC: btest-diff ./worker-1/.stdout + +redef allow_network_time_forward = F; +redef EventMetadata::add_network_timestamp = T; + +event do_terminate() + { + terminate(); + } + +event zeek_init() + { + # Set the manager's time to non-zero, the worker continues to be at 0.0. + if ( Cluster::local_node_type() == Cluster::MANAGER ) + set_network_time(double_to_time(1748256346)); + } + +event Cluster::hello(name: string, id: string) + { + print fmt("Cluster::hello name=%s is_remote_event=%s metadata=%s", name, is_remote_event(), EventMetadata::current_all()); + } + +event Cluster::node_up(name: string, id: string) + { + print fmt("Cluster::node_up name=%s is_remote_event=%s metadata=%s", name, is_remote_event(), EventMetadata::current_all()); + + if ( Cluster::local_node_type() == Cluster::MANAGER ) + Cluster::publish(Cluster::worker_topic, do_terminate); + } + +event Cluster::node_down(name: string, id: string) + { + terminate(); + }