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

Remove Python bindings #303

Open
timwoj opened this issue Jan 4, 2023 · 4 comments
Open

Remove Python bindings #303

timwoj opened this issue Jan 4, 2023 · 4 comments

Comments

@timwoj
Copy link
Member

timwoj commented Jan 4, 2023

Now that the websocket support is in place and seems like it's working pretty well, we should revisit removing the python binding code entirely. This was on the roadmap in #212, and we have a lot of other open issues involving the bindings that would simply go away (#18, #24, #33, #96, #156, #215, #241, #280).

@ckreibich
Copy link
Member

I agree. I am wondering what we should provide instead. Over in zeek-client there's an implementation of Broker's websocket data model that is ready for reuse. It'd be useful to provide that in a dedicated Python package. I'm wondering what additional functionality we should add if we do this. It's tempting to say "none, just use an existing WebSocket library", but that isn't optimal, because depending on your choice you still need to write a fair amount of specific code for error handling, etc. I could see adding support for a one or two common libraries to start with, keeping them all optional.

There are two popular packages for WebSockets, websocket-client and websockets. They're both active, with differences mainly in API design.

We have one known user of the old bindings at the moment — a zeekctl API for sending/receiving events, to interact with Zeek's control framework. Certainly wouldn't be prohibitive to port it. Shout if there are others!

@ckreibich ckreibich changed the title Remove Python bindings? Remove Python bindings Jan 7, 2023
@Neverlord
Copy link
Member

I'm all for deprecating (and eventually removing) the existing bindings and instead have a lightweight, Python-only package that makes it easy to work with the WebSocket/JSON interface. The data model implementation you have looks like a great starting point. 🙂

The Python package (via pypy?) would probably also live in a separate repository, which disentangles it nicely from the C++ code base. The whole point of offering the WebSocket API was to have a stable, versioned representation for clients.

We have one known user of the old bindings at the moment — a zeekctl API for sending/receiving events

Just one thing worth mentioning: if we do port zeekctl to the WebSocket API, it would mean that it would only work if Zeek has been configured to expose WebSocket. Simply connecting to the regular peering port won't work. Would that be acceptable?

@ckreibich
Copy link
Member

it would only work if Zeek has been configured to expose WebSocket

That's a good point — plus, the differing port would mean that people possibly need to adjust their port filtering/monitoring/etc, which might be a significant hassle at some sites. I'd argue for the removal regardless, though. But it'd clearly need calling out pretty strongly in the release notes. We can also ask folks in Slack whether it'd be too disruptive for them.

Dominik I'm curious — when you added WebSocket support, did you consider supporting both data models on the same port? Too messy?

@Neverlord
Copy link
Member

Dominik I'm curious — when you added WebSocket support, did you consider supporting both data models on the same port? Too messy?

I didn't consider it until now. It would make the code more complicated, but could be done if the extra port / setup is a showstopper otherwise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants