-
Notifications
You must be signed in to change notification settings - Fork 11
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
Test For command falco -i (ignore default events) #8
Conversation
Signed-off-by: Rohith-Raju <rohithraju488@gmail.com>
Welcome @Rohith-Raju! It looks like this is your first PR to falcosecurity/testing 🎉 |
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.
Thanks a ton for your first contribution! Much appreciated, specially in this repo!!
tests/falco/events.json
Outdated
@@ -0,0 +1,278 @@ | |||
{ |
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.
This makes sense! What about putting this under a tests/data/outputs
directory, and masking this through run.NewStringFileAccessor
? That would allow the framework to carry around the test file in a runner-agnostic way (it will work whether Falco runs in a container or in the local machine). Accordingly, I would also add the deserialization code in the same package (the part of code responsible of converting the JSON in a Go struct). Take a look at how some of the existing test data is handled.
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.
Sure, will do
tests/falco/commands_test.go
Outdated
} | ||
|
||
runner := tests.NewFalcoExecutableRunner(t) | ||
t.Run("all-events-ignored-by-default", func(t *testing.T) { |
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.
Since we don't have other tests here, I would avoid having a sub-test with t.Run
and would just proceed with the test code in the main TestFalco_Print_IgnoredEvents
test body, what do you think?
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.
Test the behaviour and expected output of the Falco CLI depending on its options, considered both individually and in combinations:
Yes, since there no combinations for -i
we can proceed with the main TestFalco_Print_IgnoredEvents
tests/falco/commands_test.go
Outdated
type Data struct { | ||
Events []string `json:"events"` | ||
} | ||
evntfile, err := os.Open("events.json") | ||
if err != nil { | ||
panic(err) | ||
} | ||
defer evntfile.Close() | ||
|
||
var events Data | ||
err = json.NewDecoder(evntfile).Decode(&events) | ||
if err != nil { | ||
panic(err) | ||
} |
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.
Look at the comment below: I think this should be part of the test data package under tests/data/outputs
, so that it could be accessible by other tests as well and reused.
tests/falco/commands_test.go
Outdated
`Ignored\sEvent\(s\):\n`, | ||
), res.Stdout()) | ||
for _, event := range events.Events { | ||
assert.Regexp(t, regexp.MustCompile(`\b`+event+`\b`), res.Stdout()) |
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.
Maybe assert.Contains
would be more lightweight here, since events are simple known strings. Remember, the more we make tests optimized, the less the CI will take to run them!
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.
Sure!
tests/falco/events.json
Outdated
@@ -0,0 +1,278 @@ | |||
{ | |||
"events": [ |
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.
Out of curiosity, which Falco version did you use to generate this list?
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.
Falco version: 0.34.1
Libs version: 0.10.4
Plugin API: 2.0.0
Engine: 16
Driver:
API version: 3.0.0
Schema version: 2.0.0
Default driver: 4.0.0+driver
Falco version: 0.34.1
Hi, @jasondellaluce thanks for reviewing my pr. I'll make the requested changes by Saturday as I'm caught up with college exams until tomorrow 😄 |
Signed-off-by: Rohith-Raju <rohithraju488@gmail.com>
Signed-off-by: Rohith-Raju <rohithraju488@gmail.com>
tests/data/outputs/event.go
Outdated
Events []string `json:"events"` | ||
} | ||
|
||
func deserialize() string { |
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.
Since we have a fairly recent version of Go, let me suggest considering go:embed https://pkg.go.dev/embed. I think it's a better approach than going through runtime.Caller
and reinvent the wheel in this case! What do you think?
Plus, if the only point of having a JSON file is to deserialize it and encode the inner list with comma-separated concatenation, why not just encoding all strings as comma-separated in a text file right away? This would also save you the trouble of defining a Data
struct. I see the attempt of making something reusable, but I would suggest to 1) tackle reusability later when needed and keep the scope of this PR smaller, and 2) the JSON encoding is arbitrary, so it would be less intuitive for future contributors to reproduce.
TL;DR: Great work! My last suggestions are:
- Encode the file as a comma-separated text file containing the list of event names
- Include it in the project using go:embed, and then making it available through the
NewStringFileAccessor
construct like you already do.
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.
Since we have a fairly recent version of Go, let me suggest considering go:embed https://pkg.go.dev/embed. I think it's a better approach than going through runtime.Caller and reinvent the wheel in this case! What do you think?
Oh, wow I didn't know something like this existed 😮 . Yes, this is great.
- Encode the file as a comma-separated text file containing the list of event names
- Include it in the project using go:embed, and then making it available through the NewStringFileAccessor construct like you already do.
Will do!!
Signed-off-by: Rohith-Raju <rohithraju488@gmail.com>
Looks good now! Your test is failing in the latest version of Falco master. That's reasonable, because we're changing the logic with which events are selected/ignored for the next Falco release. Please make sure you use the latest |
Signed-off-by: Rohith-Raju <rohithraju488@gmail.com>
Hey @jasondellaluce, So I
I took these and re-generated the event file. I wasn't able to build |
What error are you encountering when compiling Falco? Either way, you can install locally the latest dev version by downloading it from https://download.falco.org/?prefix=packages/bin-dev/. Then, once installed, the test suite should run successfully by following the readme instructions. Either way, the tests are now just failing in the CI because your regex must be updated to match |
Signed-off-by: Rohith-Raju <rohithraju488@gmail.com>
Oh, I see that there was a new image 14h ago. let me retry |
Signed-off-by: Rohith-Raju <rohithraju488@gmail.com>
tests/data/outputs/events.txt
Outdated
@@ -0,0 +1 @@ | |||
recv,pwrite64,pread64,sendfile64,read,write,writev,pwritev,readv,sendfile,preadv,sendto,recvfrom,send,sendmsg,recvmsg,sendmmsg,recvmmsg |
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.
recv,pwrite64,pread64,sendfile64,read,write,writev,pwritev,readv,sendfile,preadv,sendto,recvfrom,send,sendmsg,recvmsg,sendmmsg,recvmmsg | |
sendfile,pwritev,preadv,writev,read,pwrite,write,sendto,recv,send,sendmsg,recvfrom,recvmsg,pread,sendmmsg,recvmmsg,readv |
The latest Falco dev fixed a couple of things, this should be the most up to date and hopefully definitive list.
Signed-off-by: Rohith-Raju <rohithraju488@gmail.com>
@@ -0,0 +1 @@ | |||
sendfile,recvfrom,readv,sendto,send,read,recvmmsg,write,recvmsg,pwrite,sendmmsg,sendmsg,pread,writev,recv,pwritev,preadv |
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.
@jasondellaluce my list is now matching with the latest list.
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.
What error are you encountering when compiling Falco? Either way, you can install locally the latest dev version by downloading it from https://download.falco.org/?prefix=packages/bin-dev/. Then, once installed, the test suite should run successfully by following the readme instructions.
There was some deps issue but now I've solved it. I built falco
master and used the same for the test and it has since been passing. I think we are good for now @jasondellaluce
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.
/approve
Thanks a lot and congrats for your first contribution here 🎉
LGTM label has been added. Git tree hash: 884a0c6714a4cd4aabb129d0c1fd34ac6d47e46a
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jasondellaluce, Rohith-Raju The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Thank you more to come 😄 |
This issue is related to #7,
Checkoff task
Implementation:
events.json
file. The reason behind this was that if we were to store all the events in the test, it would look dirty.event.json
file, loop over the events, and match with the regex expression.