Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Broker::peer_websocket() #3597

Open
ckreibich opened this issue Feb 2, 2024 · 7 comments
Open

Add Broker::peer_websocket() #3597

ckreibich opened this issue Feb 2, 2024 · 7 comments
Labels
Area: Broker Implementation: Core Implementation requires modification of the Zeek core Implementation: Scripts Implementation requires Zeek scripting Type: Enhancement

Comments

@ckreibich
Copy link
Member

For some applications that integrate external services with a Zeek cluster via Broker communication, it makes more sense for Zeek nodes to connect out to those services, rather than the other way around. There's currently no way to do that via the WebSocket transport — we have Broker::listen() and Broker::listen_websocket(), but only Broker::peer(). With the new Broker bindings ( zeek/broker#380) supporting only the WebSocket transport, that's not going to work, so we should add this.

@ckreibich ckreibich added Type: Enhancement Area: Broker Implementation: Scripts Implementation requires Zeek scripting Implementation: Core Implementation requires modification of the Zeek core labels Feb 2, 2024
@Neverlord
Copy link
Member

Would you mind explaining the use case a bit? What kind of Broker-based service would run outside of Zeek but would not allow/want us to use the native peering?

At the very least, I would not call this Broker::peer_websocket(), because it does not establish a peering relation. The WebSocket interface allows publishing messages to a remote node and remotely subscribing. That's not the same as peering. If two nodes are peering, they do a handshake to detect (and discard) multiple connections, they can update their subscriptions dynamically, and they forward messages (unless configured not to).

I'm also confused as to how this is related to the new bindings. The bindings are for external tools to connect to Zeek. Not something that Zeek itself would use to connect to an external system.

@awelzel
Copy link
Contributor

awelzel commented Feb 27, 2024

it makes more sense for Zeek nodes to connect out to those services, rather than the other way around.

Would you mind explaining the use case a bit?

I actually had the same reaction. This is a setup where a user implements and runs an external WebSocket server which Zeek then connects to? And the first message from that server in response would contain subscriptions they want to setup?

To me seems quite backwards to provide this officially. How did this come up?

If someone really needs this, they could possibly run such a custom connector as a side-car process next to Zeek (or within Zeek via JavaScript) that does the translation/plumbing.

@ckreibich
Copy link
Member Author

Sorry for the delay here. I am trying to address the use case of folks who currently implement a third-party service via Broker's native bindings, where the connectivity is naturally many-to-one from Zeek to that service (say because all workers need that connection), and who are frustrated by having to use Broker's native format, for the usual reasons. Case in point: Dubem's ML service.

I don't see this as "Broker-based" in the long term (I clearly phrased that poorly), because the future we're working toward doesn't require Broker. I'd like there to be a transport that third parties can implement easily (that's websockets) using a rendering that is portable (our future JSON representation). Neither of them needs to imply anything about directionality of the connection establishment.

Did I just confuse you because I oversimplified it as Broker::peer_websocket(), or are we not even on the same page that this is a use case that needs addressing?

@awelzel
Copy link
Contributor

awelzel commented Mar 12, 2024

where the connectivity is naturally many-to-one from Zeek to that service (say because all workers need that connection)

I see. I guess the alternative is to funnel everything through the manager and the external service connects to the manager, then plumb up events to forward to workers.

Dubem's ML service.

He's aware this means he needs to implement a websocket server on their side? :-)


I'm at the point where this just squarely falls into "cluster-enabled scripts need to be broker topology aware" today. It surfaces in various places, e.g. broadcast from one worker to other workers needs to be explicitly implemented as re-publishing through manager/proxies, because "it's known" that a worker publishing directly to "zeek/cluster/workers" won't make it to other workers.

If we had a central message bus, one wouldn't need to be aware of the topology, you'd just work with topics and subscriptions and expect messages that other nodes publish (regardless of connectivity properties between nodes).

@Neverlord
Copy link
Member

Did I just confuse you because I oversimplified it as Broker::peer_websocket(), or are we not even on the same page that this is a use case that needs addressing?

That did confuse me, yes. From what you describe, this would basically be a script opening up a WebSocket connection and exchanging Zeek events over it? That would have nothing to do with Broker at all. The Zeek script would be the client in this case and we would have to represent a WebSocket connection in Zeek script land. Probably some handle to send to and registering a callback that's being called whenever receiving something?

If we had a central message bus, one wouldn't need to be aware of the topology, you'd just work with topics and subscriptions and expect messages that other nodes publish (regardless of connectivity properties between nodes).

I don't get that part. You don't need to know about the topology with Broker. There are topics that are only subscribed to by individual workers to do a 1:1 "send" over a pub/sub layer. But that has nothing to do with the topology. Or is this an artifact when building a topology with forwarding disabled?

@awelzel
Copy link
Contributor

awelzel commented Mar 13, 2024

Or is this an artifact when building a topology with forwarding disabled?

I do need to admit I'm not familiar with Broker::forward_messages. Also not with Broker::forward(). The former is off by default and the docs are very concise. Using Broker::forward() appears to imply topology awareness and users should not be exposed to it.

You don't need to know about the topology with Broker. There are topics that are only subscribed to by individual workers to do a 1:1 "send" over a pub/sub layer.

The following script today doesn't work as I'd expect it from a "normal" pub-sub behavior. The naive expectation is that a call to Broker::publish(worker_topic, ...) on any node has the effect that a request_message event arrives on all nodes subscribing to worker_topic. Possibly even itself if that matches, but that's another discussion. This behavior isn't observed for worker nodes, neither by default, nor with forward_messages set to T.

The call to Broker::publish(Cluster::node_topic("worker-1"), ...), targeting a single worker node has no effect either on a worker node. Publishing from a worker to the manager_topic works and the manager prints "hello_message" whenever any worker sees a http_request.

@load base/frameworks/broker
@load base/frameworks/cluster

redef Broker::forward_messages = T;

module TestBroker;

export {
        global request_message: event(from: string, to: string, uri: string);
}

@if ( Cluster::local_node_type() == Cluster::WORKER )

event http_request(c: connection, method: string, original_URI: string, unescaped_URI: string, version: string) {
        print Cluster::node, "http_request", original_URI;
        Broker::publish(Cluster::worker_topic, request_message, Cluster::node, "workers", original_URI);
        Broker::publish(Cluster::node_topic("worker-1"), request_message, Cluster::node, "workers", original_URI);
        Broker::publish(Cluster::manager_topic, request_message, Cluster::node, "workers", original_URI);
}
@endif

event request_message(from: string, to: string, uri: string)
        {
        print "hello_message", "from", from, "to", to, "uri", uri;
        }

To raise request_message on the workers, the common pattern I've seen is that on the manager node an explicit re-publish happens. And that pattern is what I'm stamping as topology awareness.

event request_message(from: string, to: string, uri: string)
        {
        if ( Cluster::local_node_type() == Cluster::MANAGER )
                Broker::publish(Cluster::worker_topic, request_message, Cluster::node, "workers-via-manager", uri);
        }

Digging a bit finding mentions of routing loops and quoting the commit that disabled forwarding by default c73bb8f:

    Disable broker message forwarding by default
    
    Still finding it to not be foolproof enough to enable generally for all
    nodes in a cluster.  Specific/advanced use-cases may still consider
    enabling, possibly just for specific nodes.

...which just seems a user would need to very well know what they are doing.

IMO, a Broker::publish(Cluster::worker_topic, ...) should've always reached all subscribed worker nodes. Yes, that's obviously easier with a central message bus, but now there are workarounds in scripts to deal with this.

Am I completely off path?

@Neverlord
Copy link
Member

To raise request_message on the workers, the common pattern I've seen is that on the manager node an explicit re-publish happens. And that pattern is what I'm stamping as topology awareness.

Yikes, that's not how Broker (or pub/sub in general) is supposed to work. I'm more than happy to discuss / dig into this, but let's not hijack this ticket. 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Broker Implementation: Core Implementation requires modification of the Zeek core Implementation: Scripts Implementation requires Zeek scripting Type: Enhancement
Projects
Status: No status
Development

No branches or pull requests

3 participants