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 longstanding deprecations #243

Closed
11 tasks done
jsiwek opened this issue Jan 11, 2019 · 12 comments · Fixed by #434
Closed
11 tasks done

Remove longstanding deprecations #243

jsiwek opened this issue Jan 11, 2019 · 12 comments · Fixed by #434
Assignees
Milestone

Comments

@jsiwek
Copy link
Contributor

jsiwek commented Jan 11, 2019

There's deprecated functionality that's been marked as such for a while, but never removed. Need to consider doing that for 2.7 finally. Maybe first warrants a policy decision on what length of time to allow before removing deprecations. My opinion is they should be guaranteed to exist for a single release (and its patch lifecycle).

We need to remove the following:

  • All deprecated functions/bifs
  • All deprecated events
  • &synchronized
  • &persistent, .state/state.bst
  • Other deprecated attributes: &encrypt, &mergeable, &rotate_interval, &rotate_size
  • && and || operators for patterns
  • BroControl's update command
  • Communication framework (RemoteSerializer and other things linked to that)
  • Value serialization
  • Potentially: when support for watching globals (see Consider removing "when" support for monitoring globals #319)

To make this possible, we need to add:

@jsiwek jsiwek added this to the 2.7 milestone Jan 11, 2019
@rsmmr
Copy link
Member

rsmmr commented Apr 29, 2019

These are all the functions that were deprecated as of 2.6:

function find_ip_addresses(input: string): string_array &deprecated
function cat_string_array%(a: string_array%): string &deprecated
function cat_string_array_n%(a: string_array, start: count, end: count%): string &deprecated
function complete_handshake%(p: event_peer%) : bool &deprecated
function connect%(ip: addr, zone_id: string, p: port, our_class: string, retry: interval, ssl: bool%) : count &deprecated
function decode_base64_custom%(s: string, a: string%): string &deprecated
function disconnect%(p: event_peer%) : bool &deprecated
function enable_communication%(%): any &deprecated
function encode_base64_custom%(s: string, a: string%): string &deprecated
function get_event_peer%(%) : event_peer &deprecated
function get_local_event_peer%(%) : event_peer &deprecated
function join_string_array%(sep: string, a: string_array%): string &deprecated
function listen%(ip: addr, p: port, ssl: bool, ipv6: bool, zone_id: string, retry_interval: interval%) : bool &deprecated
function merge_pattern%(p1: pattern, p2: pattern%): pattern &deprecated
function request_remote_events%(p: event_peer, handlers: pattern%) : bool &deprecated
function request_remote_logs%(p: event_peer%) : bool &deprecated
function request_remote_sync%(p: event_peer, auth: bool%) : bool &deprecated
function resume_state_updates%(%) : any &deprecated
function send_capture_filter%(p: event_peer, s: string%) : bool &deprecated
function send_current_packet%(p: event_peer%) : bool &deprecated
function send_id%(p: event_peer, id: string%) : bool &deprecated
function send_ping%(p: event_peer, seq: count%) : bool &deprecated
function set_accept_state%(p: event_peer, accept: bool%) : bool &deprecated
function set_compression_level%(p: event_peer, level: count%) : bool &deprecated
function sort_string_array%(a: string_array%): string_array &deprecated
function split1%(str: string, re: pattern%): string_array &deprecated
function split_all%(str: string, re: pattern%): string_array &deprecated
function split%(str: string, re: pattern%): string_array &deprecated
function suspend_state_updates%(%) : any &deprecated
function terminate_communication%(%) : bool &deprecated

These are all the events that were deprecated as of 2.6:

event ssl_server_curve%(c: connection, curve: count%) &deprecated;
global dhcp_ack: event(c: connection, msg: dhcp_msg, mask: addr, router: dhcp_router_list, lease: interval, serv_addr: addr, host_name: string) &deprecated;
global dhcp_decline: event(c: connection, msg: dhcp_msg, host_name: string) &deprecated;
global dhcp_discover: event(c: connection, msg: dhcp_msg, req_addr: addr, host_name: string) &deprecated;
global dhcp_inform: event(c: connection, msg: dhcp_msg, host_name: string) &deprecated;
global dhcp_nak: event(c: connection, msg: dhcp_msg, host_name: string) &deprecated;
global dhcp_offer: event(c: connection, msg: dhcp_msg, mask: addr, router: dhcp_router_list, lease: interval, serv_addr: addr, host_name: string) &deprecated;
global dhcp_release: event(c: connection, msg: dhcp_msg, host_name: string) &deprecated;
global dhcp_request: event(c: connection, msg: dhcp_msg, req_addr: addr, serv_addr: addr, host_name: string) &deprecated;

@0xxon 0xxon self-assigned this May 2, 2019
0xxon added a commit that referenced this issue May 2, 2019
This commit removed functions/events that have been deprecated in Bro
2.6. It also removes the detection code that checks if the old
communication framework is used (since all the functions that are
checked were removed).

Addresses parts of GH-243
0xxon added a commit that referenced this issue May 6, 2019
Broccoli was still present in the source in a few places, debug outputs
that do no longer exist were too.

Part of GH-243
@0xxon 0xxon mentioned this issue May 6, 2019
@jsiwek jsiwek added this to Unassigned / Todo in Release 3.0.0 via automation May 13, 2019
@jsiwek jsiwek moved this from Unassigned / Todo to Assigned / In Progress in Release 3.0.0 May 13, 2019
jsiwek added a commit to zeek/zeekctl that referenced this issue May 15, 2019
0xxon added a commit that referenced this issue May 20, 2019
To be more exact: &encrypt, &mergeable, &rotate_interval, &rotate_size

Also removes no longer used redef-able constants:
log_rotate_interval, log_max_size, log_encryption_key

GH-243
@0xxon
Copy link
Member

0xxon commented May 24, 2019

I have the removal of value serialization mostly done in the branch topic/johanna/remove-serializer. The new clone implementation is merged into the branch and most tests pass.

The missing piece (which I sadly did not get done this week) is that we need serialization/deserialization for the opaque types. My plan was to use the BinarySerialization format for this, since broker already uses it - the code for opaque value serialization would probably look a lot like the code that I just removed.

Apart from that it looks like everything works. There might be a few additional places where other codeparts are now no longer needed because value serialization does not exist anymore - but all the big obvious pieces like the serializer, chunkedio, the serialization functions, type cloning are removed or implemented.

I am happy to continue this in a few weeks - if anyone feels like taking over in the meantime that is also fine.

@0xxon
Copy link
Member

0xxon commented May 24, 2019

Another piece where we need to think about what to do is event playback. Zeek supported event playback, where one could record events into a file and play it back. This is a little known about but kind of neat feature.

Since this used the serialization framework, it does not work anymore. We can either completely remove it, let the remnants in the code for later re-implementation (reserving the keywords and command line option), or re-implement it, e.g. using the Binary Serialization format directly.

@rsmmr
Copy link
Member

rsmmr commented Jun 8, 2019

Then reworked when statement is in topic/robin/gh59-when, see #319.

@rsmmr
Copy link
Member

rsmmr commented Jun 11, 2019

I'm taking a stab at the opaque serialization

@rsmmr rsmmr self-assigned this Jun 11, 2019
@rsmmr
Copy link
Member

rsmmr commented Jun 11, 2019

So the use case for this serialization is Broker, right? Sending opaques over Broker used to go through the serializer. Readding this will require bringing back some of the serialization machinery that we just removed. In addition to those virtual Serialize()/Unserialize() methods, we'll need a global registry of all OpaqueTypes that provides a factory function for each to create instances (we can't do that statically as we need to support plugins).

A question is if, instead of going back to producing binary blobs through the BinarySerializationFormat, we should instead create broker::data directly, as that's what it'll be converted into anyways. Then the serialize methods could use standard Broker types to assemble that.

Second thing here is that if we wanted to bring back the event replay, we'd need to extend serialization to all Val classes again---which is possible, but brings additional complexities: we'd need to restore reference cycles, and we'd need restore the type information as well (which could be done mostly by type name I believe, but not fully sure). It'd be quite a bit of work, probably not really worth it.

Thoughts on any of this?

@jsiwek
Copy link
Contributor Author

jsiwek commented Jun 12, 2019

A question is if, instead of going back to producing binary blobs through the BinarySerializationFormat, we should instead create broker::data directly, as that's what it'll be converted into anyways. Then the serialize methods could use standard Broker types to assemble that.

Having methods to (de)serialize via broker::data sounds Good to me.

Second thing here is that if we wanted to bring back the event replay, we'd need to extend serialization to all Val classes again

I don't think event replay is worth much effort? I agree it's a "neat" thing that used to exist, but never really encountered a use-case for it?

Another idea/hack for re-implementing it differently could be to convert the stream of events into broker::data (which we can obviously already do) and shove those into a Broker data store (e.g. using the sqlite backend provides the equivalent of persisting the event stream in a file on disk).

@rsmmr
Copy link
Member

rsmmr commented Jun 17, 2019

I've went ahead with the direct serialization to Broker in topic/robin/gh-243-opaque-serialization. This is based off of Johanna's branch. The code is complete, but some tests are still failing, probably some bug in the serialization left. Also needs a bit more clean up.

@0xxon
Copy link
Member

0xxon commented Jun 17, 2019

Great - I will take a look and probably just merge that back into my branch :)

0xxon added a commit that referenced this issue Jun 18, 2019
…ization' into topic/johanna/remove-serializer

* origin/topic/robin/gh-243-opaque-serialization:
  Reimplement serialization infrastructure for OpaqueVals.
  Couple of compile fixes.
@jsiwek
Copy link
Contributor Author

jsiwek commented Jun 21, 2019

Maybe 2 remaining things to do here:

  • grep for "Deprecated" in init-bare.zeek: should we remove those or be safe and mark &deprecated ? Didn't look closely, but assume they were labeled that way before &deprecated even existed.

  • rotate_file_by_name is something we just &deprecated, but we still have a usage: trim-trace-file.zeek, which seems standalone zeekctl may use. What to do there? Just un-deprecate for now until we can deprecate zeekctl later on ?

@jsiwek jsiwek assigned jsiwek and unassigned 0xxon and rsmmr Jun 21, 2019
@jsiwek
Copy link
Contributor Author

jsiwek commented Jun 21, 2019

I'll look into wrapping this up:

  • Stuff we added "Deprecated" to in a comment I guess should be considered as having been officially announced as so a long time ago and ok to remove now.
  • The one rotate_file_by_name usage in trim-trace-file.zeek can be replaced by something like a new restart_packet_dump BIF that simply closes/re-opens the filing being written to by -w

@vpax
Copy link
Contributor

vpax commented Jun 21, 2019

@jsiwek It would certainly be helpful if trim-trace-file.zeek, or something similar, exists so that trace files can indeed be rotated. Restarting the dump is also handy, but being able to take long-running pcaps is vital in some contexts.

@jsiwek jsiwek moved this from Assigned / In Progress to Pending Review / Merge in Release 3.0.0 Jun 22, 2019
@0xxon 0xxon closed this as completed in #434 Jul 1, 2019
Release 3.0.0 automation moved this from Pending Review / Merge to Done Jul 1, 2019
0xxon added a commit that referenced this issue Jul 1, 2019
…ecation-removal'

* origin/topic/jsiwek/gh-243-wrap-up-deprecation-removal:
  Improve deprecation warning messages
  Remove deprecated DNS events
  Remove BackDoor analyzer
  Remove InterConn analyzer
  Remove deprecated/unused irc_servers option
  Remove deprecated print_hook event
  Remove dead code: dump_used_event_handlers
  Remove unused software_version_found events
  Remove deprecated open_log_file and log_file_name functions
  Remove deprecated/unused "packet" type
  Un-deprecate anonymizer BIFs
  Un-deprecate file rotation functions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Release 3.0.0
  
Done
Development

Successfully merging a pull request may close this issue.

4 participants