add kprobe read/write e2e test #3374
Conversation
Signed-off-by: sat0ken <[email protected]>
There was a problem hiding this comment.
Thanks for the PR!
Please find some comments below.
On a more generic note, I'm ambivalent if adding this test covers some gap in our testing. On the one hand, we do not have kprobe e2e tests, but on the other hand, kprobes (and tracepoints) are extensively tested in unit tests, which are, generally speaking, faster and more reliable. I would like to hear @will-isovalent and other folks' opinions on this.
| // Copyright Authors of Tetragon | ||
|
|
||
| // This package contains a simple test skeleton that can be copied, pasted, and modified | ||
| // to create new Tetragon e2e tests. |
There was a problem hiding this comment.
This comment is probably from the skeleton test, so maybe it's worth removing it.
| - index: 0 | ||
| type: "int" | ||
| - index: 1 | ||
| type: "char_buf" |
There was a problem hiding this comment.
Reading the buffer from the user-space buffer is generally an unsafe pattern. There is no issue for the test, but I don't know if adding this anti-pattern to our code is a good idea.
We also don't seem to validate the buffer so maybe it would maybe sense to remove this argument?
| type: "int" | ||
| - index: 1 | ||
| type: "char_buf" | ||
| sizeArgIndex: 3 |
| followForks: true | ||
| isNamespacePID: true | ||
| values: | ||
| - 1 |
There was a problem hiding this comment.
Why is the matchPIDs filter needed?
There was a problem hiding this comment.
I pasted from example write.yaml.
I removed it.
| } | ||
| if ev.GetFunctionName() == "__x64_sys_read" && ev.GetProcess().GetBinary() == "cat" { | ||
| k.matches++ | ||
| } |
There was a problem hiding this comment.
One thing to note is that the cat or echo implementations might change to use other system calls.
There was a problem hiding this comment.
cat and echo moved to variables.
| } | ||
|
|
||
| func (k *kprobeCheker) FinalCheck(logger *logrus.Logger) error { | ||
| if k.matches > 0 { |
There was a problem hiding this comment.
Shouldn't we check == 2 or at least >= 2 here?
Signed-off-by: sat0ken <[email protected]>
Signed-off-by: sat0ken <[email protected]>
Signed-off-by: sat0ken <[email protected]>
Signed-off-by: sat0ken <[email protected]>
Fixes #230
Description
I wrote test code use e2e test framework about kprobe/kretprobe test (read/write a file).