refactor(events): new event definitions (mutable vs immutable data) #3340

rafaeldtinoco · 2023-07-26T05:48:58Z

Observation

This is not fully done. I can't make all the refactor at once, I need things merged first to understand future needs for the Extensions approach (and also to give some "briefing moment" in between the big changes, so others can suggest things as they use and also provide feedback).

This is true for the EventState struct, which will be fully encapsulated inside the EventGroup, for example (@josedonizetti FYI)

Changes

401c336 fix(events): fix arm64 tailcalls indexes for unsupported syscalls
1aded80 chore(cmd): improve list of events output using definitions
ea4247e tests(definition): include GetDefinitions tests
f89a8c3 refactor(events): make Events become Definition
24f5051 refactor(events): Events.GetEventByID returns only the event
4a69b53 refactor(events): encapsulate more event dependencies
c9ea982 refactor(events): start core event group for extensions
9d2495e refactor(events): rename types to Event and EventGroup
de4c87a fix(events): remove leftover from tailcalls initialization
4208b5c fix(events): fix syntax and static checks issues
dbe9eed chore(revive): no arg limit check
0160880 refactor(events): the only mutable type is event definition group
90bf9f3 refactor(events): make tailcall dependencies immutable
9512b86 refactor(events): ksymbols and probes are immutable
564dd53 refactor(events): begin of event definition refactor
9d3f96e refactor(events): start re-organizing event definitions
01e4aeb fix(filter): remove unneeded workaround

commit 401c336

fix(events): fix arm64 tailcalls indexes for unsupported syscalls

Bring back the workaround not to index, in tailcall maps, the eBPF
programs with event (syscall) indexes that are unsupported (other
architectures should flag unsupported syscalls with IDs >= 10000).

commit 1aded80

chore(cmd): improve list of events output using definitions

- tabled event list output
- include --wide option for better readability
- use recent factored event definitions and event groups concept

commit ea4247e

tests(definition): include GetDefinitions tests

commit f89a8c3

refactor(events): make Events become Definition

- differentiate Events and Event Definitions
- rename Event Group to Definition Group

commit 24f5051

refactor(events): Events.GetEventByID returns only the event

- Creates IsEventDefined() for event ID checking
- Keeps a single GetEventByID() method
- Refactor method usage
- Minor styling changes

commit 4a69b53

refactor(events): encapsulate more event dependencies

Encapsulates Event Definition Dependencies:
- KSymbols
- Probes
- Capabilities (and simplify its type)

Also:
- Unify Events.NewEvent() and NewEventFull()
- Rename methods as reviews are done
- Change direct field access for the encapsulation methods

commit c9ea982

refactor(events): start core event group for extensions

commit 9d2495e

refactor(events): rename types to Event and EventGroup

- normalize syscall names to be exported (for future extensions)

commit de4c87a

fix(events): remove leftover from tailcalls initialization

commit 4208b5c

fix(events): fix syntax and static checks issues

commit dbe9eed

chore(revive): no arg limit check

commit 0160880

refactor(events): the only mutable type is event definition group

commit 90bf9f3

refactor(events): make tailcall dependencies immutable

commit 9512b86

refactor(events): ksymbols and probes are immutable

commit 564dd53

refactor(events): begin of event definition refactor

1. create CoreEventDefinitionGroup (TODO: "core" extension).
2. EventDefinition and EventDefinitionGroup types.
3. Make EventDefinition encapsulated.
4. Keep temporary EvtDef instances (due to Dependencies).

commit 9d3f96e

refactor(events): start re-organizing event definitions

- turn the event dependency ID type just a slice of IDs
- split code into multiple files (might still change) to organize

commit 01e4aeb

fix(filter): remove unneeded workaround

Since 4.19 support was dropped (non CORE) there is no need for this
workaround to exist anymore.

rafaeldtinoco · 2023-07-26T20:32:14Z

Failing in arm64 because of:

-                       for _, index := range tailCall.GetIndexes() {
-                               if index >= uint32(events.MaxCommonID) {
-                                       logger.Debugw(
-                                               "Removing index from tail call (over max event id)",
-                                               "tail_call_map", tailCall.GetMapName(),
-                                               "index", index,
-                                               "max_event_id", events.MaxCommonID,
-                                               "pkgName", pkgName,
-                                       )
-                                       tailCall.DelIndex(index) // remove undef syscalls (eg. arm64)
-                               }
-                       }

So I'll need to do a per arch event definition declaration (since we're now doing immutable declarations). might split the "syscall event group definition" for architecture (common, amd64, arm64 files, for example, for them).

josedonizetti

@rafaeldtinoco I added a couple of questions, most about naming, nothing huge, let me know what you think

pkg/events/event.go.go

josedonizetti · 2023-07-27T14:44:29Z

pkg/events/group.go

+
+// TODO: add states to the EventGroup struct (to keep states of events from that group)
+type EventState struct {
+	Submit uint64 // event that should be submitted to userspace (by policies bitmap)


why make this public and not private with get/sets?

This will be in the next parts of the refactor (this PR got too big). You can consider this PR a "1st phase". Its hard to make all changes in a single PR (it would take quite sometime and its better to have the refactor rolling so other PRs can rebase and suggest things/changes.

This EventState will be a full encapsulated type, etc etc.

pkg/events/group.go

josedonizetti · 2023-07-27T14:46:27Z

pkg/events/event.go.go

+	return e.id32Bit
+}
+
+func (e *Event) GetName() string {


For getters of private fields, do you think we always need GetXXX, could it be Name()? wdyt?

I like GetXXXX because of IDE completion (I want to get something I type Get and it shows me a list of what I can Get). I think its a good practice and I hope we can make it the default if others accept (on other refactors as well).

yanivagman

I like this change very much
Mostly nit comments ahead

yanivagman · 2023-07-27T15:14:10Z

pkg/events/dependencies.go

+// Event is a struct describing an event configuration
+type Event struct {
+	ID32Bit      ID
+	Name         string
+	DocPath      string // Relative to the 'doc/events' directory
+	Internal     bool
+	Syscall      bool
+	Dependencies Dependencies
+	Sets         []string
+	Params       []trace.ArgMeta
+}


This struct should probably be moved to either events.go or definitions.go

It is now called "Definitions" and placed under definitions.go.
A event group has turned into "definitions group".
This way, we can clearly see what is an "event definition" and what is an "event".
All the places calling the methods had their variables renamed as well (to eventDefinition or equivalent).

yanivagman · 2023-07-27T15:25:34Z

pkg/cmd/flags/filter.go

@@ -107,8 +107,8 @@ func prepareEventsToTrace(eventFilter eventFilter, eventsNameToID map[string]eve
 	isExcluded := make(map[events.ID]bool)

 	// build a map: k:set, v:eventID
-	for id, event := range events.Definitions.Events() {
-		for _, set := range event.Sets {
+	for id, event := range events.CoreEventDefinitionGroup.Events() {


How are events from other groups being prepared if we only iterate the core group?
Same question for any other place in the code where we only iterate or refer to the core group

Yep, not done yet, part of future changes (when extensions is started and their event groups are created).

pkg/events/dependencies.go

pkg/ebpf/tracee.go

pkg/bufferdecoder/decoder.go

pkg/events/event.go.go

yanivagman · 2023-07-27T16:01:12Z

pkg/events/ids.go

In a previous PR you took the IDs into their own file (which I thought is nice) and now you merge them back to the event file. Any reason for that?

I might re-do it, it will become more clear what is the best once we have extensions (because extensions will have to have the same structure for their events (so we will know which filenames we should use for each extension). Thats why its not "that important" now (but will be once we have the first extension made, so we can decide how all others will look like).

rafaeldtinoco · 2023-07-28T03:34:26Z

I marked addressed comments as solved. Still need to fix arm64. All the other comments are almost addressed, will be finishing this soon.

yanivagman

Some commits in this PR undo or add on top of previous commits of this PR so they can probably be squashed.

I also think that this PR (that is already big enough) tries to do things that can be on their own PR like remove network workaround for older kernel (the first commit) or reformatting the list of events to be in a table. Consider taking them out of this PR into their own.

yanivagman · 2023-07-31T08:30:59Z

pkg/ebpf/tracee.go

@@ -1191,9 +1191,9 @@ func getTailCalls(eventConfigs map[events.ID]eventConfig) ([]*events.TailCall, e
 	tailCalls := []*events.TailCall{}

 	for e, cfg := range eventConfigs {
-		def := events.Definitions.Get(e)
+		def := events.CoreEventDefinitionGroup.Get(e)


I had the same concern as before, so I'm asking again to understand -
when we will add other groups of events (non core) we will need to refactor all places that get the core definition group. So won't it be better if we already treat these parts (that are not specific to core group) in a generic way and write a wrapper function to get all definitions from all groups of events?

I'm going to add an array (or map, or complex type abstracting them) to the singleton. This way "tracee" will have a way to access all the group of events. I really need "this code to get in first", so I can start to "picture" how Im going to do the rest. I think many parts of this refactor will still change (or be improved) until the "final extensions idea" is finished.

pkg/cmd/flags/filter.go

yanivagman · 2023-07-31T10:33:20Z

pkg/ebpf/tracee.go

-			if err != nil {
-				return errfmt.WrapError(err)
+			// Optimization: enable enter/exit probes only if at least one syscall is enabled.
+			once.Do(func() {


Why do we need that?
In the future, we may want to use this function dynamically to load new events that have tail call dependencies

Agreed. I just added the once.Do() because it makes no sense calling it over and over for the same thing (so there is a refactor need here). Also, the code was already here and I agree to your statement.

- turn the event dependency ID type just a slice of IDs - split code into multiple files (might still change) to organize

1. create CoreEventDefinitionGroup (TODO: "core" extension). 2. EventDefinition and EventDefinitionGroup types. 3. Make EventDefinition encapsulated. 4. Keep temporary EvtDef instances (due to Dependencies).

- normalize syscall names to be exported (for future extensions)

Encapsulates Event Definition Dependencies: - KSymbols - Probes - Capabilities (and simplify its type) Also: - Unify Events.NewEvent() and NewEventFull() - Rename methods as reviews are done - Change direct field access for the encapsulation methods

- Creates IsEventDefined() for event ID checking - Keeps a single GetEventByID() method - Refactor method usage - Minor styling changes

- differentiate Events and Event Definitions - rename Event Group to Definition Group

- tabled event list output - include --wide option for better readability - use recent factored event definitions and event groups concept

Bring back the workaround not to index, in tailcall maps, the eBPF programs with event (syscall) indexes that are unsupported (other architectures should flag unsupported syscalls with IDs >= 10000).

yanivagman

LGTM

github-actions bot added area/ebpf area/testing area/UX area/events area/filtering area/flags labels Jul 26, 2023

github-actions bot assigned rafaeldtinoco Jul 26, 2023

josedonizetti reviewed Jul 27, 2023

View reviewed changes

rafaeldtinoco marked this pull request as ready for review July 27, 2023 15:03

yanivagman reviewed Jul 27, 2023

View reviewed changes

github-actions bot added the area/build label Jul 29, 2023

rafaeldtinoco requested review from josedonizetti and yanivagman July 30, 2023 07:29

yanivagman reviewed Jul 31, 2023

View reviewed changes

rafaeldtinoco added 14 commits July 31, 2023 10:35

refactor(events): start re-organizing event definitions

7bf0c57

- turn the event dependency ID type just a slice of IDs - split code into multiple files (might still change) to organize

refactor(events): begin of event definition refactor

4b80c4b

1. create CoreEventDefinitionGroup (TODO: "core" extension). 2. EventDefinition and EventDefinitionGroup types. 3. Make EventDefinition encapsulated. 4. Keep temporary EvtDef instances (due to Dependencies).

refactor(events): ksymbols and probes are immutable

a9ee8c8

refactor(events): make tailcall dependencies immutable

c38dd29

refactor(events): the only mutable type is event definition group

f95d58b

chore(revive): no arg limit check

0763d8b

fix(events): fix syntax and static checks issues

8511b59

refactor(events): rename types to Event and EventGroup

02a9c2d

- normalize syscall names to be exported (for future extensions)

refactor(events): start core event group for extensions

ea7968c

refactor(events): Events.GetEventByID returns only the event

8d2b598

- Creates IsEventDefined() for event ID checking - Keeps a single GetEventByID() method - Refactor method usage - Minor styling changes

refactor(events): make Events become Definition

a989e3a

- differentiate Events and Event Definitions - rename Event Group to Definition Group

tests(definition): include GetDefinitions tests

b8b0a59

chore(cmd): improve list of events output using definitions

d23e0af

- tabled event list output - include --wide option for better readability - use recent factored event definitions and event groups concept

fix(events): fix arm64 tailcalls indexes for unsupported syscalls

5b627d1

Bring back the workaround not to index, in tailcall maps, the eBPF programs with event (syscall) indexes that are unsupported (other architectures should flag unsupported syscalls with IDs >= 10000).

yanivagman approved these changes Jul 31, 2023

View reviewed changes

rafaeldtinoco merged commit da0db0d into aquasecurity:main Jul 31, 2023
25 checks passed

rafaeldtinoco deleted the new-event-definitions branch July 31, 2023 18:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(events): new event definitions (mutable vs immutable data) #3340

refactor(events): new event definitions (mutable vs immutable data) #3340

rafaeldtinoco commented Jul 26, 2023 •

edited

Loading

rafaeldtinoco commented Jul 26, 2023

josedonizetti left a comment

josedonizetti Jul 27, 2023 •

edited

Loading

rafaeldtinoco Jul 27, 2023

josedonizetti Jul 27, 2023

rafaeldtinoco Jul 27, 2023

yanivagman left a comment

yanivagman Jul 27, 2023

rafaeldtinoco Jul 29, 2023

yanivagman Jul 27, 2023

rafaeldtinoco Jul 28, 2023

yanivagman Jul 27, 2023

rafaeldtinoco Jul 27, 2023

rafaeldtinoco commented Jul 28, 2023 •

edited

Loading

yanivagman left a comment

yanivagman Jul 31, 2023

rafaeldtinoco Jul 31, 2023

yanivagman Jul 31, 2023

rafaeldtinoco Jul 31, 2023

yanivagman left a comment

refactor(events): new event definitions (mutable vs immutable data) #3340

refactor(events): new event definitions (mutable vs immutable data) #3340

Conversation

rafaeldtinoco commented Jul 26, 2023 • edited Loading

Observation

Changes

rafaeldtinoco commented Jul 26, 2023

josedonizetti left a comment

Choose a reason for hiding this comment

josedonizetti Jul 27, 2023 • edited Loading

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

yanivagman 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

rafaeldtinoco commented Jul 28, 2023 • edited Loading

yanivagman 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

yanivagman left a comment

Choose a reason for hiding this comment

rafaeldtinoco commented Jul 26, 2023 •

edited

Loading

josedonizetti Jul 27, 2023 •

edited

Loading

rafaeldtinoco commented Jul 28, 2023 •

edited

Loading