Provide infrastructure to migrate legacy analyzers to Spicy inside the Zeek tree.

[rsmmr]

As initial examples, this branch ports the Syslog and Finger analyzers
over. We leave the old analyzers in place for now and activate them
iff we compile without any Spicy.

Needs zeek-spicy-infra branches in zeek/spicy#1333, zeek/spicy-plugin#155, zeek/cmake#57, and https://github.com/zeek/zeek-testing-private/pull/11.

[awelzel]

Hey - I've tried to build branch locally and am running into an issue:

[error] /home/awelzel/corelight-oss/zeek/src/analyzer/protocol/finger/finger.spicy:42:22-46:2: call does not match any function: spicy_rt::initializeParsedUnit(<value_ref<spicy_rt::ParsedUnit>>, <value_ref<Finger::Reply>>, <const __library_type("::hilti::rt::TypeInfo const*")>)

This seems like it should be something local, but I did the recursive submodule update and otherwise usual build.l

set -x
PREFIX=${PREFIX:-/opt/zeek-dev}

export CFLAGS="-ggdb -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer"
export CXXFLAGS="-ggdb -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer"
export LDFLAGS="-fuse-ld=lld"

./configure \
    --ccache \
    --generator=Ninja \
    --build-dir=./build \
    --prefix=$PREFIX \
    --enable-debug \
    --build-type=debug \
    --disable-zkg \
    --disable-btest \
    --disable-python \
    --disable-zeekctl \
    --disable-zeek-client \
    --disable-broker-tests \
    --enable-static-binpac  \
    --enable-static-broker \
    --binary-package \
    --sanitizers=address \
    --enable-fuzzers \
    $*

[awelzel]

This seems like it should be something local, but I did the recursive submodule update and otherwise usual build.

Okay: I've had an existing installation in /opt/zeek-dev and appears that spicyz picks the installed version of spicy_rt.hlt over the one that can be found through the provided -L <...>/zeek/auxil/spicy/spicy/spicy/lib from the source tree.

That seems slightly surprising (when compared to -I from cc which will preceed system directories) and likely sub-optimal for the builtin case here. After removing /opt/zeek-dev locally the error from above is gone.

[rsmmr]

That seems slightly surprising (when compared to -I from cc which will preceed system directories) and likely sub-optimal for the builtin case here. After removing /opt/zeek-dev locally the error from above is gone.

Ack, I'll look into that.

[rsmmr]

This is ready, but I'll start the merge process with the subprojects and will file a few individual PRs there to get their pieces in place first. Once that's all in, I'll turn the main Zeek PR to ready for review.

[rsmmr]

This is ready for review.

[bbannier]

Before going into the details I wanted to check whether it supports all the use cases we wanted (not all tested in CI). We need to be able to build against an externally installed Spicy (shared or static) and/or against a different version of the plugin, configured with --with-spicy and --with-spicy-plugin. It looks like this is currently broken already when only using one of external Spicy/spicy-plugin, even at configure time.

The migration of the Finger analyzer looks okay to me, but CI for that currently hinges on us never making the different sanitizer CI jobs use Spicy (they are currently --disable-spicy). Maybe we need a dedicated CI for testing legacy analyzers in a job explicitly without Spicy (maybe as a nightly job @awelzel started putting infra in?).

[awelzel]

and/or against a different version of the plugin, configured with --with-spicy and --with-spicy-plugin. It looks like this is currently broken already when only using one of external Spicy/spicy-plugin, even at configure time.

Mostly FWIW: I could see us removing --with-spicy-plugin and ask developers for manual bumping/pointing the submodule instead. For AF_Packet we do not have a similar option and I'd like to avoid the proliferation.

The --with-spicy should continue to work, it's such a time-saver when doing one-off container builds that one can just install Spicy's binary packages directly.

Maybe we need a dedicated CI for testing legacy analyzers in a job explicitly without Spicy (maybe as a nightly job @awelzel started putting infra in?).

I don't think there's actual infra, but I might be missing a bit what you're referring to.

[rsmmr]

Arne:

Mostly FWIW: I could see us removing --with-spicy-plugin

Agree with that.

The --with-spicy should continue to work, it's such a time-saver when doing one-off container builds that one can just install Spicy's binary packages directly.

Actually, I'd prefer for that to go, too. We have such a proliferation of Zeek/Spicy/Plugin versions and ways to install them, it's hard to keep that all working in all combinations. In particular, having one extern but the other not, feels like we could skip.

We do have --disable-spicy, and that's actually the only one I use. When I don't want the internal Spicy, I use that and then install Spicy (source or binary) and the plugin separately. Would that work for you (and your container builds), too?

Maybe we need a dedicated CI for testing legacy analyzers in a job explicitly without Spicy (maybe as a nightly job @awelzel started putting infra in?).

I don't think there's actual infra, but I might be missing a bit what you're referring to.

[rsmmr]

Benjamin:

The migration of the Finger analyzer looks okay to me, but CI for that currently hinges on us never making the different sanitizer CI jobs use Spicy (they are currently --disable-spicy). Maybe we need a dedicated CI for testing legacy analyzers in a job explicitly without Spicy (maybe as a nightly job @awelzel started putting infra in?).

Yep, I noticed that too: looks like we should have a dedicated (non-sanitizer) CI run with --disable-spicy.

[awelzel]

We do have --disable-spicy, and that's actually the only one I use. When I don't want the internal Spicy, I use that and then install Spicy (source or binary) and the plugin separately. Would that work for you (and your container builds), too?

Hmm, cringing a bit, but if it pains you too much I'll adapt or use more ccache magic.

Other thought is that bundling dependencies isn't great for distributions (some packaging guidelines suggest to submit upstream patches to allow for external dependencies), but we're probably far from that and Zeek/Spicy are rather closely coupled so that whatever a distribution ships may not actually work with the latest Zeek version anyway (or analyzers).

As of spicy-plugin: There's another thought to go even further: In the scenario that one passes --disable-spicy and builds everything externally, do we provide a way to leverage Spicy analyzers maintained within Zeek's source tree? If not (or if that would just complicate things), I could also see us merging the plugin completely for Zeek 6.0 with the sentiment that in-tree Spicy analyzers can only be leveraged with the builtin spicy plugin. If we want to support in-tree analyzers with an external plugin, that's a different story, but might be a scenario to avoid.

Sorry for all the text, I'll try to look, run this tomorrow 🤞

[timwoj]

I gave it a cursory glance and it all looks ok to me. My only question is whether we have a plan to deprecate/eventually remove the "legacy" version of the analyzer. Given that we're always building with spicy by default, how long should we expect the old version to live on?

Along those same lines, should there be some sort of warning during configure if there are in-tree spicy plugins, but you've disabled spicy?

[rsmmr]

Ok, let's do this: For now, I'll try to get the options back to working so that we don't need to make this call right now; I think I see what broken them. But longer term, we probably still want to think about what combinations we want to keep supporting.

[rsmmr]

My only question is whether we have a plan to deprecate/eventually remove the "legacy" version of the analyzer. Given that we're always building with spicy by default, how long should we expect the old version to live on?

Something to talk about, but my take is that at some point we should stop supporting running without Spicy at all (i.e., remove —disable-spicy), and then it becomes an individual per-analyzer decision as we port them over. Then we either immediately remove legacy analyzers once we have Spicy version, or we allow each a grace period of, say, one major release where we keep a knob to turn them back on.

Along those same lines, should there be some sort of warning during configure if there are in-tree spicy plugins, but you've disabled spicy?

Sure, I'll add something.

[rsmmr]

Updated this with (1) supporting external Spicy/plugin versions again (tested in all combinations on macOS), and (2) add warnings about legacy/missing analyzers without Spicy.

[bbannier]

From the Spicy and CMake side this looks okay to me, only left trivial issues.

[timwoj]

Something to talk about, but my take is that at some point we should stop supporting running without Spicy at all (i.e., remove —disable-spicy), and then it becomes an individual per-analyzer decision as we port them over. Then we either immediately remove legacy analyzers once we have Spicy version, or we allow each a grace period of, say, one major release where we keep a knob to turn them back on.

I'd agree with removing --disable-spicy, if we can figure out how to make the builds work on lower-memory machines. For example, my NUC with 8GB of RAM cannot complete a build unless I turn off spicy. The build eventually hangs, and then the compiler crashes due to being OOM.

[rsmmr]

For example, my NUC with 8GB of RAM cannot complete a build unless I turn off spicy. The build eventually hangs, and then the compiler crashes due to being OOM.

Is that even with -j1?