implemented broker and consume API for tracee-ebpf #793

mtcherni95 · 2021-06-27T09:51:05Z

This commit includes:

a Broker API with the Streamer interface and implementation of IOStreamer (aka printer).
the creation of Consume() API exposed by Tracee.

mtcherni95 · 2021-06-27T10:17:37Z

tracee-ebpf/tracee/tracee.go

@@ -889,12 +863,19 @@ func (t *Tracee) writeProfilerStats(wr io.Writer) error {
 	return nil
 }

+func (t *Tracee) Consume() (<-chan external.Event, error) {


The model proposed implies that the ownership of the channel should be on the producer (indeed who writes on the channel should be responsible to close it). IMO this is a better design compared to a "pass channel from configuration" design, where the consumer is supposed to be the owner of the channel. Moreover, in the model proposed by the commit we will still be able in future to add multiple channels (just return a struct/map of channels).

itaysk

great job undertaking this complex task. It is a major change to tracee to some newly introduced concepts might need fine-tuning

itaysk · 2021-06-27T11:16:11Z

tracee-ebpf/tracee/broker/broker.go

+
+import (
+	"fmt"
+	cmap "github.com/orcaman/concurrent-map"


what benefits does this library have over stdlib's sync.Map?

The stdlib sync.Map is designed for append-only scenarios. So if you want to use the map for something more like in-memory db, you might benefit from using our version. You can read more about it in the golang repo, for example here:golang/go#21035 and here: https://stackoverflow.com/questions/11063473/map-with-concurrent-access

I don't think this matters enough to make the map synchronous. Most (all?) implementations will register 1/2/maybe 3 streamers in a sequential manner. Even if you want to make the map thread safe, "append-only" sounds reasonable to our use case. Also, the worst case with sync.Map is a slight performance penalty but only in the insertion (and this is for a scenario that is very unlikely to happen). I don't think we need an in-memory high-throughput cache here

itaysk · 2021-06-27T11:24:24Z

tracee-ebpf/tracee/broker/broker.go

+	"github.com/aquasecurity/tracee/tracee-ebpf/tracee/streamers"
+)
+
+type Broker struct {


I think we should have a NewBroker function that does all the one-time setup here instead of having the consumer do it.

func NewBroker(channel chan Event, stats Stats) (Broker, error) { // initialize a new broker // initialize it's internal cmap // save channel and stats on the struct // return struct }

itaysk · 2021-06-27T11:26:34Z

tracee-ebpf/main.go

+			b := broker.Broker{Streamers: cmap.New()}
+			if err := b.Register(iostreamer); err != nil {
+				return fmt.Errorf("failed to register IOStreamer: %w", err)
+			}
+			b.ChanEvents, err = t.Consume()
+			if err != nil {
+				return fmt.Errorf("failed to consume event channels: %w", err)
+			}


I think the consumer has too much visibility and control into the broker. we should simplify things for the consumer so it instantiates a new broker and the setup is done in the broker package (see my other comment in broker.go)

itaysk · 2021-06-27T11:29:37Z

tracee-ebpf/tracee/broker/broker.go

+	if ok := b.Streamers.SetIfAbsent(strconv.FormatUint(b.nextAvailStreamerId, 10), streamer); !ok {
+		return fmt.Errorf("failed to subscribe streamer")
+	}
+	streamer.SetId(b.nextAvailStreamerId)


why should the streamer care which id the broker gave it? the id is purely for disambiguation in the broker's internal map

mainly because the streamer can call the broker.Unregister API by passing its ID.

this explains why the broker should care about the IDs, but I asked why the streamer cares

itaysk · 2021-06-27T11:31:20Z

tracee-ebpf/tracee/broker/broker.go

+		return fmt.Errorf("failed to subscribe streamer")
+	}
+	streamer.SetId(b.nextAvailStreamerId)
+	streamer.Preamble()


preamble should be called in start, not in register

register is considered as the "init" phase for the streamer. The moment is registered it starts its lifetime.

itaysk · 2021-06-27T11:38:16Z

tracee-ebpf/tracee/broker/broker.go

+	go func() {
+		defer close(errc)
+		for printEvent := range b.ChanEvents {
+			stats.EventCounter.Increment()


increment should be done after the event was handled (to avoid double counting)

how could double counting happen?

you incremented, then some error in the streamer occurred which cause the event to be "lost". so it's not in the output but still in the counter

itaysk · 2021-06-27T11:39:33Z

tracee-ebpf/tracee/broker/broker.go

+	errc := make(chan error, 1)
+	go func() {
+		defer close(errc)
+		for printEvent := range b.ChanEvents {


printEvent sounds suggestive in this generic broker. maybe event or e?

itaysk · 2021-06-27T12:35:38Z

tracee-ebpf/tracee/broker/broker.go

+		for printEvent := range b.ChanEvents {
+			stats.EventCounter.Increment()
+			cb := func(key string, v interface{}) {
+				v.(streamers.Streamer).Stream(&printEvent)


It sounds to me that "Stream" is a read only operation on the event, why do we pass a pointer?

on purpose we pass a pointer and not a copy, we are reading only.

if you meant to avoid a copy for micro-optimization, then I'm not sure you made it better by creating a closure here

itaysk · 2021-06-27T12:44:52Z

tracee-ebpf/tracee/streamers/streamers.go

@@ -0,0 +1,329 @@
+package streamers


calling it streamers is a big suggestive, they don't have to "stream" the event they handle, they just need to do whatever with it. what do you think about "handle" instead of "stream"?

sounds good to me.

itaysk · 2021-06-27T13:33:17Z

tracee-ebpf/tracee/broker/broker.go

@@ -0,0 +1,64 @@
+package broker


this broker design creates a (loose) coupling from tracee-ebpf to the consumers, a coupling which did not exist until now. The coupling is demonstrated at the function call from the broker (broker is a tracee-ebpf concept) to the registered consumer. a function call in this direction is not ideal because it's synchronously called within the critical path of event processing. If one time once streamer is stuck for one event, it could clog the entire pipeline. I would rather keep tracee-ebpf oblivious to it's consumers. To do this, the broker should be a simpler "fan out" machine, that duplicates the incoming event into consumer-specific channels. An example of this pattern can be found in tracee-rules' engine (dispatching code), here's a simpler example: https://stackoverflow.com/a/36418082/310298

we can just go streamer.Stream() without having a blocking operation.
Copying here is a bad idea in my opinion.

so you suggest to change the broker code to invoke the callback in a goroutine? this will create a whole lot of different challenges, without seeing the code, I can expect ordering issues and resource contention for starts

Yes, you are right. Let's go with the "fan out" machine.

entity can read from the channel without any dependencies with the tracee printers.

itaysk

done channel is ignored, which is not your problem, just making a note for future

mtcherni95 force-pushed the consume-and-broker-api branch 2 times, most recently from 0520bc9 to 70a6ce9 Compare June 27, 2021 10:11

mtcherni95 commented Jun 27, 2021

View reviewed changes

itaysk self-assigned this Jun 27, 2021

itaysk reviewed Jun 27, 2021

View reviewed changes

mtcherni95 force-pushed the consume-and-broker-api branch 3 times, most recently from c777e2a to 2579020 Compare June 28, 2021 12:00

Adding exportable channel into Config struct. In this way a third party

5455088

entity can read from the channel without any dependencies with the tracee printers.

mtcherni95 force-pushed the consume-and-broker-api branch from 2579020 to 5455088 Compare June 28, 2021 12:06

itaysk approved these changes Jun 28, 2021

View reviewed changes

yanivagman mentioned this pull request Jun 28, 2021

Implemented broker and consume API for tracee as a library (#768) #789

Closed

itaysk merged commit ae07c82 into aquasecurity:main Jun 30, 2021

yanivagman mentioned this pull request Jul 4, 2021

Tracee consume API #768

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implemented broker and consume API for tracee-ebpf #793

implemented broker and consume API for tracee-ebpf #793

mtcherni95 commented Jun 27, 2021

mtcherni95 Jun 27, 2021 •

edited

Loading

itaysk left a comment

itaysk Jun 27, 2021

mtcherni95 Jun 27, 2021

itaysk Jun 27, 2021

itaysk Jun 27, 2021

itaysk Jun 27, 2021

itaysk Jun 27, 2021

mtcherni95 Jun 27, 2021

itaysk Jun 27, 2021

itaysk Jun 27, 2021

mtcherni95 Jun 27, 2021

itaysk Jun 27, 2021

mtcherni95 Jun 27, 2021

itaysk Jun 27, 2021

itaysk Jun 27, 2021

itaysk Jun 27, 2021

mtcherni95 Jun 27, 2021

itaysk Jun 27, 2021

itaysk Jun 27, 2021

mtcherni95 Jun 28, 2021

itaysk Jun 27, 2021

mtcherni95 Jun 27, 2021

itaysk Jun 27, 2021

mtcherni95 Jun 28, 2021

itaysk left a comment

implemented broker and consume API for tracee-ebpf #793

implemented broker and consume API for tracee-ebpf #793

Conversation

mtcherni95 commented Jun 27, 2021

mtcherni95 Jun 27, 2021 • edited Loading

Choose a reason for hiding this comment

itaysk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itaysk left a comment

Choose a reason for hiding this comment

mtcherni95 Jun 27, 2021 •

edited

Loading