Manage EventStates via Policies
#3848
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This comment was marked as outdated.
This comment was marked as outdated.
geyslan
force-pushed
the
the-one-with-the-events-states
branch
from
a2bafba
to
423e7d3
Compare
geyslan
commented
Feb 6, 2024
geyslan
commented
Feb 7, 2024
geyslan
force-pushed
the
the-one-with-the-events-states
branch
from
423e7d3
to
053fd4a
Compare
geyslan
commented
Feb 7, 2024
geyslan
commented
Feb 7, 2024
geyslan
commented
Feb 7, 2024
geyslan
force-pushed
the
the-one-with-the-events-states
branch
3 times, most recently
from
e67593a
to
62f562d
Compare
This comment was marked as outdated.
This comment was marked as outdated.
geyslan
force-pushed
the
the-one-with-the-events-states
branch
3 times, most recently
from
81c8d51
to
6885f48
Compare
geyslan
commented
Feb 18, 2024
geyslan
force-pushed
the
the-one-with-the-events-states
branch
2 times, most recently
from
cc51615
to
bb5b727
Compare
geyslan
commented
Feb 20, 2024
geyslan
force-pushed
the
the-one-with-the-events-states
branch
from
b244f08
to
10a4989
Compare
geyslan
changed the title
EventFlags via PoliciesEventStates via Policies
geyslan
force-pushed
the
the-one-with-the-events-states
branch
from
10a4989
to
48c85dc
Compare
geyslan
changed the title
EventStates via PoliciesEventStates via Policies
geyslan
force-pushed
the
the-one-with-the-events-states
branch
2 times, most recently
from
ae99512
to
734e3aa
Compare
geyslan
changed the title
EventStates via PoliciesEventStates via Policies
geyslan
commented
Mar 26, 2024
| @@ -430,9 +300,28 @@ func (t *Tracee) Init(ctx gocontext.Context) error { | |||
| t.contPathResolver = containers.InitContainerPathResolver(&t.pidsInMntns) | |||
| t.contSymbolsLoader = sharedobjs.InitContainersSymbolsLoader(t.contPathResolver, 1024) | |||
|
|
|||
| // Initialize event flags | |||
There was a problem hiding this comment.
Suggested change
| // Initialize event flags | |
| // Initialize event properties |
geyslan
commented
Mar 26, 2024
| defID := def.GetID() | ||
| eProperties := &events.Properties{} | ||
|
|
||
| // Set event flags based on its dependencies |
There was a problem hiding this comment.
Suggested change
| // Set event flags based on its dependencies | |
| // Set event properties based on its dependencies |
geyslan
force-pushed
the
the-one-with-the-events-states
branch
2 times, most recently
from
59c3cef
to
734e3aa
Compare
PoliciesConfig is a new struct that holds the configuration used on the Policies computation. This also optimises cmd.GetContainerMode() and starts to decouple Policies from the config.Config.
Properties holds the static properties of an event wich are not
hardcoded in the event definition and have to be computed at runtime.
This also introduces a new file `dependencies.go` in the `events` package
with the helper:
`func GetAllEventsDependencies(givenEvtId ID) map[ID]struct{}`
- Rename: MaxPolicies to PolicyMax AllPoliciesOn to PolicyAll AlwaysSubmit to submitAllPolicies - Create PolicyNone const
- Moves all EventState management into policy package, introducing policy.EventState and policy.EventStates interfaces. - Changes DefinitionGroup.GetTailCalls() signature to deal only with subimittable events. This avoids circular import. - Removes Config.Policies, forcing to get policies from Snapshot and inject it when required.
Move it into policy package.
As this is a transitionary effort, we need to keep track of what needs to be done.
geyslan
force-pushed
the
the-one-with-the-events-states
branch
from
734e3aa
to
d068d9b
Compare
|
E2E tests 1428 is green. |
AlonZivony
reviewed
Mar 27, 2024
| import "sync/atomic" | ||
|
|
||
| type Properties struct { | ||
| requiredBySignature atomic.Bool |
There was a problem hiding this comment.
Why do we need an atomic here?
Atomic is good for comparing and switching, but for a specific value it doesn't give any advantage. Once I thought it helps with caches coherency, but in modern memory systems it is done without atomic operations as well:
on cache-coherent architectures (read - on all modern commodity architectures - IA-32, Intel 64, IA-64, SPARC, POWER) visibility is ensured automatically. Namely, each write is automatically propagated to all other processors/cores in a best-effort manner. There are no ways to prevent nor to speed it up. Period.
Anyways I think that either you want to do some compare and switch logic here, and then using an atomic is a good idea, you should just keep and atomic reference to the struct and work with RCU.
There was a problem hiding this comment.
It's just a computed value in runtime, for now, just initialized. But I used it as atomic since "if" changes occur "somehow" in other code path, it's thread-safe to load and store.
|
Closing due to upcoming architecture changes. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Close: #3849
1. Explain what the PR does
d068d9b chore: add TODOs
56dcdc8 chore: remove unused functions
362969c chore: refactor policies computation
203ad45 chore: minor renaming
860924d chore: move attachProbes to policy package
600351a chore: relocate validateKallsymsDependencies
ffb6520 chore: move UpdateCapabilitiesRings to policy pkg
1897129 chore: add EventState and EventsStates interfaces
7f1405d chore: introduces ksymbols.go with singleton
f816b92 chore: rename consts
4ffcd9d chore: introduce events.Properties type
f79fe97 chore: introduce config.PoliciesConfig
d068d9b chore: add TODOs
600351a chore: relocate validateKallsymsDependencies
1897129 chore: add EventState and EventsStates interfaces
f816b92 chore: rename consts
4ffcd9d chore: introduce events.Properties type
f79fe97 chore: introduce config.PoliciesConfig
2. Explain how to test it
3. Other comments
This is a transitionary PR focused in bring the
EventStateandEventStatesread-only interfaces - used by Policies internals. It also introducesevents.Propertiestype as a holder for non hardcoded information about events.