-
Notifications
You must be signed in to change notification settings - Fork 411
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
eBPF control plane signals #3237
eBPF control plane signals #3237
Conversation
@AlonZivony FYI, if this is merged before the process tree, it might be a good idea to connect it to this instead of the regular event buffer. If you see any limitations for that LMK here so I can address them. @yanivagman @rafaeldtinoco Currently we automatically pass cgroup_mkdir/rmdir in all scopes with no filter check, WDYT about removing the bypass in this PR? |
7d1ddd4
to
29a8fed
Compare
Yes please. |
pathDecoder := New(trimmedPath) | ||
sunPath, err = readStringVarFromBuff(pathDecoder, 108) | ||
} | ||
sunPath, err := readStringVarFromBuff(ebpfMsgDecoder, 108) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There were reasons for the above additions like having the extra NULL bytes, why do you think it is ok to remove those?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I saw it doing it in another buffer was unnecessary overhead (in terms of readability mostly). By moving the cursor it has the same effect in the buffer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yanivagman About this and the below comment, this is a one time used function for this specific flow, I don't think it's necessary to formalize it for this PR. Maybe in another one, but it's a very niche function not used for anything else.
res = append(res, byte(char)) | ||
err = decoder.DecodeInt8(&char) | ||
if err != nil { | ||
return "", errfmt.Errorf("error reading null terminated string: %v", err) | ||
} | ||
} | ||
res = bytes.TrimLeft(res[:], "\000") | ||
decoder.cursor += max - count // move cursor to end of buffer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We never access the cursor directly in this file but always using the DecodeXXX decoder methods.
Maybe we should consider converting this function to be a decoder method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well then the package boundary may be wrong. I saw these function as utils which shouldn't be in the decoder itself but do have access to it. But maybe it should be added to the decoder as you suggest.
@@ -89,7 +89,7 @@ func (t *Tracee) enrichContainerEvents(ctx gocontext.Context, in <-chan *trace.E | |||
cgroupId := uint64(event.CgroupID) | |||
// CgroupMkdir: pick EventID from the event itself | |||
if eventID == events.CgroupMkdir { | |||
cgroupId, _ = parse.ArgVal[uint64](event, "cgroup_id") | |||
cgroupId, _ = parse.ArgVal[uint64](event.Args, "cgroup_id") | |||
} | |||
// CgroupRmdir: clean up remaining events and maps | |||
if eventID == events.CgroupRmdir { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the below code related to event enrichment of cgroup mkdir/rmdir or should we move it to be handled in the control plane?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could add an enrich call at the end of the control plane logic so the code here could take less time (since enrichment would already be done), that's a good idea.
But the change done here is just to align with the parse.ArgVal
function signature change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By starting the EnrichCgroupInfo
sooner (in the control plane), we will help the pipeline (that might receive same related cgroupid events a bit later because of its channel buffer). I think its a good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added now, will test to see if it works right, please review it as well @rafaeldtinoco.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So if we now enrich using the control plane messages - do we still need to perform the code below (currently the lines below are not changed in this PR)
I want to review this one pls. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some minor comments but LGTM, its a nice change, thank you!
ab31b88
to
748e303
Compare
@yanivagman @rafaeldtinoco I believe i've addressed the problems raised in the PR relevant to its scope, please re-review. |
748e303
to
0b9b75b
Compare
0b9b75b
to
a03ff26
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM,
we can potentially remove more logic from the events pipeline
@@ -89,7 +89,7 @@ func (t *Tracee) enrichContainerEvents(ctx gocontext.Context, in <-chan *trace.E | |||
cgroupId := uint64(event.CgroupID) | |||
// CgroupMkdir: pick EventID from the event itself | |||
if eventID == events.CgroupMkdir { | |||
cgroupId, _ = parse.ArgVal[uint64](event, "cgroup_id") | |||
cgroupId, _ = parse.ArgVal[uint64](event.Args, "cgroup_id") | |||
} | |||
// CgroupRmdir: clean up remaining events and maps | |||
if eventID == events.CgroupRmdir { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So if we now enrich using the control plane messages - do we still need to perform the code below (currently the lines below are not changed in this PR)
Thanks @NDStrahilevitz, well done! |
a03ff26
to
052707e
Compare
@NDStrahilevitz Im re-checking with your changes, and will +1 soon. |
@NDStrahilevitz needs rebasing. |
052707e
to
21d480d
Compare
Take arguments array directly instead of the entire event.
The Control Plane's intention is to process "signal programs" which would previously depend on the event submission success. Instead, a separate buffer and slimmer type is used for these critical event processes.
21d480d
to
49324f3
Compare
Is there a reason the last commit is separate (fix id override)? |
Because it was a bug that I think existed before the features introduced here, and I just found it while testing it with container enrichment. So I decided to include it in a separate commit and fix it opportunistically as part of the PR. |
makes sense, thanks |
In this case the PR should have closed the bug report issue in addition to the feature issue (ok to close tow issues with one PR) and that bug issue should have been included in the milestone. Not a huge deal just taking the learning opportunity |
Add a control plane package which attaches specialized shorter eBPF probes to guarantee lifecycle events reach tracee.
Currently, only container lifecycle events are handled.
Resolve #3086