Skip to content

Conversation

@philrz
Copy link
Contributor

@philrz philrz commented Dec 29, 2023

The artifacts produced from what's in this repo should take the place of the the ones we've been using from https://github.com/brimdata/zeek for years and catch us up with current Zeek releases. It takes advantage of the official support for Zeek on Windows such that we no longer need to maintain our own fork of the Zeek source code. I've tested in Brimcap/Zui using such artifacts built in a personal repo https://github.com/philrz/build-zeek, so this PR is my attempt to bring that effort into the official Brim Data org.

The official Zeek-on-Windows support removed the need for many of the hacks we'd used to get our initial port working, though I've kept using some of our previous glue when it comes to adding Zeek packages and bundling the artifacts.

I'm adding inline PR comments to call attention to things I've done that look like hacks but were intentional.

@philrz philrz self-assigned this Dec 29, 2023
call "C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\VC\Auxiliary\Build\vcvarsall.bat" x86_amd64
mkdir zeek-src\build
cd zeek-src\build
cmake.exe .. -DCMAKE_BUILD_TYPE=release -DENABLE_ZEEK_UNIT_TESTS=yes -D CMAKE_INSTALL_PREFIX="C:\Program Files\Git\usr\local\zeek" -DLibMMDB_INCLUDE_DIR="C:\Program Files (x86)\maxminddb\include" -DLibMMDB_LIBRARY="C:\Program Files (x86)\maxminddb\lib\maxminddb.lib" -G Ninja
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unpacking to C:\Program Files\Git\usr\local\zeek makes it such that the later step that run in shell: sh will find the Windows Zeek install under /usr/local/bin just like on Linux/macOS.

Comment on lines 51 to 58
build_command=$(zkg_meta package build_command)
if [ "$build_command" ]; then
if [ "$OS" = Windows_NT ]; then
export LDFLAGS='-static -Wl,--allow-multiple-definition'
fi
sh -c "$build_command"
$sudo tar -xf build/*.tgz -C /usr/local/zeek/lib/zeek/plugins
fi
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This and other references to "plugins" are part of what came over from the glue in our older Zeek port. They should be effectively no-ops at this point because we now use the built-in Community ID support and hence don't need to compile the plugin like we did in the past. It was a close call in my mind as to whether I should try to remove all the plugin refs from this script and the "zeekrunners" or leave it there in case there's a reason to revive it in the future. Since everything builds and runs fine with what's currently in this PR I figured I'd save myself the extra iterations of removing and testing without it, but if someone feels strongly I can invest the time.

# of capture.
exec "$dir/bin/zeek" \
-C -r - \
--exec "event zeek_init() { Log::disable_stream(PacketFilter::LOG); Log::disable_stream(LoadedScripts::LOG); Log::disable_stream(Telemetry::LOG); }" \
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I spent some time looking at the additional logs that are produced by Zeek v6.0.2 relative to the ones that had been coming from our old Zeek v3.2.1 artifacts. Here I've added the telemetry ones to our exclusion list since they create a lot of noise (e.g., 800+ additional log lines when processing a pcap that contains only a single connection) and are intended to give insight to operational deployments (i.e., not our use case.)

@philrz philrz requested a review from nwt December 29, 2023 20:42
@philrz philrz marked this pull request as ready for review December 29, 2023 20:43
@philrz philrz requested a review from mattnibs January 2, 2024 18:08
Comment on lines +55 to +56
rename "C:\Program Files (x86)\maxminddb\include\maxminddb.h" maxminddb.h.bak
sed "/typedef ADDRESS_FAMILY sa_family_t/d" "C:\Program Files (x86)\maxminddb\include\maxminddb.h.bak" > "C:\Program Files (x86)\maxminddb\include\maxminddb.h"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did this in two steps rather than sed -i because on the Windows runner that failed with a cannot rename: Invalid cross-device link error referencing a temp directory on a different drive letter. This workaround is all temporary until they release another libmaxminddb, so I chose not to obsess over that.

@philrz
Copy link
Contributor Author

philrz commented Jan 3, 2024

This is ready for another look. Summary of changes since the last review (7a4b563...e6437d9):

  • The build workflow is now triggered by tags/branches rather than inputs in Workflow Dispatch
  • Go version is now specified in the Actions Workflow rather than go.mod
  • The libmaxminddb workaround on Windows now deletes the offending line from the include file post-install rather than doing patch pre-build
  • I removed the now-unused code we'd previously used for building plugins and now just flag plugins as unsupported. I briefly tested if what we had from before even still works on Linux/macOS and it failed due to a missing include. That could probably be healed with some work but since we don't need it currently I figured I'd remove it entirely since we can dig up the old code and hack if we ever need to revisit.
  • Tested that it runs ok on all platforms, with a focus on making sure the packages still work ok and no sign of problems due to dropping the empty plugins/ directory and references to ZEEK_PLUGIN_PATH from the zeekrunners

@philrz philrz requested a review from nwt January 3, 2024 20:49
@philrz philrz merged commit 6340b98 into main Jan 4, 2024
@philrz philrz deleted the init branch January 4, 2024 00:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants