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

tetragon declutter sensors and decouple spec from core #88

Merged
merged 12 commits into from
May 27, 2022

Conversation

jrfastab
Copy link
Contributor

The sensors pkg was a bit of a monolith this PR breaks that down into smaller components to make it more manageable.

Then the CRD specs were embedded into the sensor core as well this pulls the spec handling logic out of sensors and core components so we can support multiple versions of the spec or new specs entirely for new sensors as we write them without having to work on core components that manage the lifetime of the sensors.

Finally I had to disable some of the stack trace functionality as it was embedded into the sensor pkg and also reading specs directly. (ugh) To fix I suggest we pull this into a new sensor ./pkg/sensor/stackTrace or such so it can be easily configured and not part of the core bits.

Unload logic is part of sensors now, but can be easily isolated to its
own unloader pkg. Making sensors pkg slightly smaller.

Signed-off-by: John Fastabend <[email protected]>
@jrfastab jrfastab requested a review from a team as a code owner May 26, 2022 23:08
Sensors pkg is a bit of a monolith lets break it down into its components.
This adds a programs pkg to manage the Program and Map objects.

Signed-off-by: John Fastabend <[email protected]>
@jrfastab jrfastab force-pushed the pr/jrfastab/sensorRefactor branch 3 times, most recently from 5835d7c to 5b4b19f Compare May 27, 2022 04:57
@kkourt
Copy link
Contributor

kkourt commented May 27, 2022

here seem to be some errors in the tests: TestFork and TestCopyFD. The latter is #82 and plan to fix this today. The former, seems to be due to the changes.

@willfindlay
Copy link
Contributor

Static checker also seems quite upset here. FWIW if these are false positives we can always ignore them with a //nolint comment. Although I haven't looked in detail yet so maybe some of them do need fixing 🤷‍♂️

jrfastab added 7 commits May 27, 2022 08:01
Remove these unused bits, they were orphaned by an earlier refactor. If
we need them its easy to pull them back in.

Signed-off-by: John Fastabend <[email protected]>
Setfilter and friends are not used anymore and are old code.

Signed-off-by: John Fastabend <[email protected]>
We can treat the base set of programs and maps as a sensor. This create
a ./pkg/sensors/base to collect all this logic and pull it out of the
monolith sensors.

Signed-off-by: John Fastabend <[email protected]>
Extract some config logic out of sensors, this makes sensors a bit nicer.
In the process we can then pull a bit more logic into the base sensor.

Signed-off-by: John Fastabend <[email protected]>
As an overall goal sensor writers and spec/CRD developers should not need
to mangle the core sensors logic in order to add/remove fields. Further,
as we get more sensors I expect more CRDs to manage them and I want the
core pkgs (sensors, observer, base, etc) to be agnostic of this. We can
even have different expectations of stability this way.

We have at least two examples under development already. First a user
friendly CRD is under development and then also the stack tracing
logic (currently embedded in a few places) can be abstracted into a
sensor and made to work in general case. Both of these are likely to be
new CRDs.

We lose some type checking but that can be done at CRD parsing time and
in the sensors themselves in the next patches.

Signed-off-by: John Fastabend <[email protected]>
The stack trace tree bits are infecting lots of pkgs convoluting the
pkg dependency links. A stack trace object should be its own sensor
and anyways its not fully hooked up at the moment. So lets remove
these references that are breaking pkging.

Signed-off-by: John Fastabend <[email protected]>
Sensors track the lifetime of the registered sensors and also manage the
CRD and config file lifetimes. However, the sensor should not need to know
anything about the config file layout or CRD layout. This is ideal because
as we add new sensors (stack trace sensor) with new configurations or add
more user friendly configuration (yes we know writing your own CRD is
painful at times) we want to keep the exact same core logic.

Lift the CRD and configfile logic out of sensors so next patches for
stack traces can avoid mangling sensors logic to add new features.

Signed-off-by: John Fastabend <[email protected]>
@jrfastab jrfastab force-pushed the pr/jrfastab/sensorRefactor branch 3 times, most recently from eccd23c to 31a7fcc Compare May 27, 2022 16:19
jrfastab added 2 commits May 27, 2022 09:45
This allows spec definitions to evolve independent of the underlying
sensors. The concrete example is to add different sensors with varying
levels of quality and expose them with different specs. This way the
base/core (and stable) specs can be concrete, but user/developers can
explore using other specs.

The two examples on the horizon are one to roll a new (more user
friendly CRD) and call it v2. But with this we can still consume
both v1 and v2 without refactoring our sensors. The second example
is to add a new stack tracing sensor with its own CRD. Now we can
do this and sensor creator can happily do this without touching
my core sensor, observer, watcher logic.

With this fixup tracing sensors continues to happily run and consume
CRDs even if receiving different versions of the CRD.

Signed-off-by: John Fastabend <[email protected]>
Typing program.ProgramBuilder is redundant lets shorter the builder name
to just program.Builder. This makes CI happy as well.

Signed-off-by: John Fastabend <[email protected]>
@jrfastab jrfastab force-pushed the pr/jrfastab/sensorRefactor branch from c255ab5 to 31e47b6 Compare May 27, 2022 16:45
Update the mods.

Signed-off-by: John Fastabend <[email protected]>
@jrfastab jrfastab merged commit 9fe09ca into main May 27, 2022
@jrfastab jrfastab deleted the pr/jrfastab/sensorRefactor branch May 27, 2022 19:03
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