Deprecating Broker::auto_publish() ?

[awelzel]

We're re-visiting pub-sub and the Broker::auto_publish() feature came up.

It's a thorn in my eye in terms of reasoning which events are published to which remote topics. Basically, using Broker::auto_publish() makes publishing events to topics implicit for event statements, rather than being explicit through a Broker::publish() call. These Broker::auto_publish() may be in different places or even different files, so you may never know they exists when looking at a new code base.

In conversations with @rsmmr, he didn't sound super fond of it. I heard @JustinAzoff mentioning it makes things complicated, maybe his opinion is stronger. I personally think Broker::auto_publish() is an obfuscation tool. Cluster programming is difficult enough, adding some implicit twists make it even harder to reason and review code.

A direct replacement of an event myevent() would be to explicitly publish it at the same point:

# file1.zeek
event zeek_init() {
    Broker::auto_publish(manager_topic, myevent);
}

# file2.zeek
event http_request(...) {
    event myevent();  # implicitly publishes to manager_topic
}

The replacement would be as follows:

# file2.zeek
event http_request(...) {
   Broker::publish(manager_topic, myevent);
   event myevent();
}

If an event is published to various topics, then explicit publishes are somewhat less convenient compared to registering multiple auto-publishes for different topics. At the same time, maybe a shared topic for the specific purpose and a single publish() invocation would be the better replacement. Here, interested parties would then subscribe() to the shared topic, rather than having the producer explicitly publish() to the individual topics of interested parties.

As an example, we have Cluster::broadcast_topics which one could auto_publish() or publish() individually. The idea would be to have an actual broadcast_topic = "zeek/cluster/broadcast which every node type subscribes to and only a single publish() would be required.

There's traces of the auto-publish feature within the EventManager, too, which would be nice to rip out :-)

Does anyone have a historic view on Broker::auto_publish()? When or why it's useful? I'm not writing a lot of cluster code, but I'm in a fairly opinionated corner where I'd never use it.

[rsmmr]

I support letting this go, it makes it really hard to understand what's going. Couples notes:

The replacement would be as follows: ``` # file2.zeek event http_request(...) { Broker::publish(manager_topic, myevent); event myevent(); }

I think it would be nice to still have a way to (explicitly) *forward* an existing event. That's were `auto_publish()` came from: Zeek B needs to see (a subset of) what Zeek A sees, like: all http_requests. Not a must-have, though.

Does anyone have a historic view on `Broker::auto_publish()`? When or why it's useful?

Going back all the way, the original approach to Zeek communication was to make things as transparent as possible: events and state get automatically exchanged between Zeeks so that everybody sees the same (subset of) stuff. However, over time I think it's become pretty clear that that's not a good model (too much magic) and communication has become more and more explicit, which is a good thing.

…

-- Robin Sommer * Corelight, Inc. * ***@***.*** * www.corelight.com

[J-Gras]

I'd be fine with removing Broker::auto_publish(). My argumentation would just be slightly different.

I personally think Broker::auto_publish() is an obfuscation tool. Cluster programming is difficult enough, adding some implicit twists make it even harder to reason and review code.

I don't see how getting rid of Broker::auto_publish() will help here. Is there anything that would prevent me from doing the following? I'd argue that this pattern is even harder to spot and review.

# file2.zeek
event http_request(...) {
    event myevent(); # implicitly publishes to manager_topic again
}

# file3.zeek
event myevent() {
   Broker::publish(manager_topic, myevent);
}

That being said, removing Broker::auto_publish() makes sense to me just to reduce the number of APIs. As a (new) developer I wouldn't need to think about the difference between Broker::auto_publish() and Broker::publish() to make a decision, which will fit my use case best.

As an example, we have Cluster::broadcast_topics which one could auto_publish() or publish() individually. The idea would be to have an actual broadcast_topic = "zeek/cluster/broadcast which every node type subscribes to and only a single publish() would be required.

If I remember correctly, using a broadcast topic like zeek/cluster/broadcast was my preferred solution, when I refactored the cluster framework. Unfortunately, this approach led to routing loops when combined with Broker's forwarding feature (I think there is a btest for this). Hence, I ended up introducing the Cluster::broadcast_topics set. Another option might be to have an Broker::publish overload that accepts a set of topics.

[awelzel]

I think it would be nice to still have a way to (explicitly) forward
an existing event. That's were auto_publish() came from: Zeek B
needs to see (a subset of) what Zeek A sees, like: all http_requests.
Not a must-have, though.

Hmm, interesting. You're thinking something like Cluster::publish_current_event(topic)? I could see it, but do wonder how often that happens and if "re-packing" the event is really so bad. In fact, any event with a c: connection parameter is also fairly "dangerous" or "heavy" due to all the things that possibly hang onto connection. I'd lean towards explicitness here as well.

I don't see how getting rid of Broker::auto_publish() will help here. Is there anything that would prevent me from doing the following? I'd argue that this pattern is even harder to spot and review.

I don't find the example all that questionable. Mainly, there's no magic happening behind the scenes. Of course, we cannot prevent someone from complicating/obfuscating code. We can remove tools and patterns from the base scripts that help obfuscation. For me it's really on that level. With Broker::auto_publish() being available and used in the base scripts, I'd go so far and say we promote its usage (people copying scripts, taking inspiration, ...).

If I remember correctly, using a broadcast topic like zeek/cluster/broadcast was my preferred solution, when I refactored the cluster framework. Unfortunately, this approach led to routing loops when combined with Broker's forwarding feature

Ah, thanks, you did mention something in that direction back then. Sorry for picking this example, I had just looked at the file earlier and that one stuck. I'm currently looking at things independently of broker and from that perspective seems like a bug with the forward_messages option.

[JustinAzoff]

Past me has some opinions on this subject :-)

http://mailman.icsi.berkeley.edu/pipermail/bro-dev/2017-February/012386.html

datanodes turned into proxies and send_event_hashed became publish_hrw.

In your example where you have

# file2.zeek
event http_request(...) {
   Broker::publish(manager_topic, myevent);
   event myevent();
}

in many cases this would not be the replacement, but instead it would be

# file2.zeek
event http_request(...) {
   Broker::publish(manager_topic, myevent);
}

because often the event isn't needed on the local process. Much of the current use of @if is to workaround this sort of thing. On a standalone process we can do things like make

   Broker::publish(manager_topic, myevent);

equivalent to

   event myevent();

without the user having to use @if everywhere.

[awelzel]

in many cases this would not be the replacement, but instead it would be
...
because often the event isn't needed on the local process.

Can also paint this as another short-coming of auto_publish(): Making intention clear.

601         # Delegate removal if we are on a worker                                                                                                                                                                             
602         if ( !have_full_data )                                                                                                                                                                                               
603                 {                                                                                                                                                                                                            
604                 event Intel::remove_item(item, purge_indicator);

Without that comment it would not be clear if Intel::remove_item() is intended for local handling or not. And the comment might be stale, too and maybe it's now also okay to use locally. That particular event is not in an export section, so that helps, but Broker::publish(manager_topic, ...) would just avoid this question.

[awelzel]

because often the event isn't needed on the local process. Much of the current use of @if is to workaround this sort of thing. On a > standalone process we can do things like make

Broker::publish(manager_topic, myevent); equivalent to event myevent();
without the user having to use @if everywhere.

I find that interesting. I'm dabbling with a new Cluster::publish() BiF and plugable cluster backends and could certainly see a Cluster::CLUSTER_BACKEND_LOOPBACK implementation that would just raise the published event as a local event (with or without subscription awareness, not sure).

It seems a bit tricky though. How would that behave for the verbose pattern of publish() and event? That would result in two local events in non-cluster modee. Maybe that's not actually a common pattern (just seems so due to the auto_publish() usage) and a separate function that does both might be acceptable:

Cluster::publish(manager_topic, myevent)
event myevent();

# to
Cluster::publish_and_queue(manager_topic, myevent)

+1 to that - this is a key missing piece, some sort of publishing semantics that just directly do the right thing when running non-clusterized.

the right thing would be great, not sure it's easy. One result was Broker::auto_publish(). Maybe loopback could work for most cases, but any more magic and it'll be hard to grasp or explain IMO.

[J-Gras]

Imho, a loopback backend would be exactly the same level of magic as auto_publish(). It's just flipped: Instead of a local event that magically triggers publish, publish magically triggers a local event 😆

[JustinAzoff]

hmm.. I suppose it's the same amount of magic, (one magic), but to me they are not equivalent. With

    Cluster::publish(manager_topic, myevent)

You can see that an event is supposed to be pushed up to the manager - or locally if not in cluster mode. The one line tells you everything that you need to know.

When auto_publish is in use we have

    event myevent();

and somewhere else in the script or in another file entirely there is a call to auto_publish that says where to send the event, and then inevitably @if statements to prevent the event from being handled locally. Understanding how things work means first finding all calls to auto_publish, then matching them up with any event calls for those events. The one magic thing is still there, but it's decoupled from the event generation itself makes things harder to reason about.

[ckreibich]

It seems to me that abstracting away the cluster via a combination of topic subscriptions in the same node, plus only using Cluster::publish(), is far preferable. @J-Gras is this what you mean by "loopback backend"?

Has anybody actually tried centralizing scripts that were designed to work distributed this way?

Really interesting discussion. 👍

[ckreibich]

I remember a discussion with Robin during the early management framework phase about auto_publish() vs explicit publishing, and we also landed on preferring explicit publishing, simply because it made it clear what's going on. That case was easy, though, because it naturally fit specific distributed communication patterns.

My sense is that whether to have auto_publish() goes back to a basic confusion we have everywhere in our cluster at the moment: is it tightly coupled or not? Is there a notion of the cluster being "up"? Etc. In a world where you're mostly working with specific, predictable communication patterns (RPCs, 2-phase-commit-style multicast-with-responses, etc), you more naturally want explicit calls to publish, and the more you think of yourself as loosely coupled, the more things like auto_publish() make sense.

I think in practice we've long been effectively tightly coupled. Nothing in Zeek clusters naturally comes, goes, fails over, or auto-scales, we have plenty of examples of having built RPCs with eventing, etc. So most of the time, publish() strikes me as the right approach. There's a separate question whether we've spent enough time building out abstractions that let you do specific communication patterns — the answer seems a clear no, since everybody who first hits this stumbles over pub/sub details, addressing specific endpoints, writing the eventing, clusterized-vs-centralized incompatibilities, etc. With such abstractions in place, the question of whether publish() or auto_publish() is right might feel less pressing.

I could see starting by strongly cautioning against the use of auto_publish(), and seeing whether we can naturally remove it from our own code. Use of publish() vs auto_publish() in the current Zeek packages looks pretty mixed.