flag(output): new flag format #5008
Conversation
|
|
LucaRocco
force-pushed
the
message-output-routing
branch
3 times, most recently
from
776eea3 to
13f9161
Compare
josedonizetti
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5008 +/- ##
==========================================
+ Coverage 29.70% 30.29% +0.59%
==========================================
Files 234 234
Lines 26180 26437 +257
==========================================
+ Hits 7776 8009 +233
- Misses 17864 17870 +6
- Partials 540 558 +18
🚀 New features to boost your workflow:
|
|
Hi @josedonizetti, Thanks for taking care of the PR. The coding part is done. I’ll go through everything again and add a few more tests before requesting a review. Just a few points:
Luca |
LucaRocco
force-pushed
the
message-output-routing
branch
from
73b2958 to
90fd199
Compare
| if !isStructured("output") { | ||
| outputFlags, err := GetFlagsFromViper("output") | ||
| if err != nil { | ||
| return runner, err | ||
| } | ||
|
|
||
| output, err := flags.PrepareOutput(outputFlags) | ||
| if err != nil { | ||
| return runner, err | ||
| } | ||
|
|
||
| streams := []config.Stream{} | ||
| for _, destination := range output.DestinationConfigs { | ||
| streams = append(streams, config.Stream{ | ||
| Name: destination.Name + "-stream", | ||
| Destinations: []config.Destination{destination}, | ||
| }) | ||
| } | ||
| output.TraceeConfig.Streams = streams | ||
|
|
||
| cfg.Output = output.TraceeConfig | ||
| } else { | ||
| outputConfig, err := GetStructuredOutputConfig() | ||
| if err != nil { | ||
| return runner, err | ||
| } | ||
|
|
||
| outputTraceeConfig, err := prepareTraceeConfig( | ||
| *outputConfig, | ||
| cmd.GetContainerMode(containerFilterEnabled(), cfg.NoContainersEnrich)) | ||
| if err != nil { | ||
| return runner, err | ||
| } | ||
|
|
||
| cfg.Output = outputTraceeConfig |
There was a problem hiding this comment.
This is the biggest issue for me here. At least with all PRs for flags I've been creating, we are trying to be consistent with flags and config file.
If we do such, it means, GetFlagsFromViper is already prepared to return the proper outputFlags doesn't matter if structured or not https://github.com/aquasecurity/tracee/blob/main/pkg/cmd/cobra/config.go#L20-L70, which means flags.PrepareOutput can handle both flags and config.
Ideally, we would do the same for output flags.
Can u take a look at how I'm implementing the other flags, for example, a simple one:
https://github.com/josedonizetti/tracee/blob/67c263ca24d942d05dfb48a6775787e21d08b548/pkg/cmd/cobra/cobra.go
https://github.com/josedonizetti/tracee/blob/67c263ca24d942d05dfb48a6775787e21d08b548/pkg/cmd/cobra/config.go
https://github.com/aquasecurity/tracee/pull/5021/files
Let me know if u have any questions.
There was a problem hiding this comment.
Hi @josedonizetti, thank you for the comment. My idea to implement it this ways came from the discussion I had with @yanivagman. Specifically, I am referring to this.
It is completely fine for me changing the way we handle it but at this point we should decide the form of the flags. Do you have any suggestions? I was thinking something like the following:
--stream stream1 --stream stream1:buffer.size=1024 --stream stream1:buffer.mode=block --stream stream1:destinations=d1,d2 --stream1:filters.policies=p1,p2 --stream1:filters.events=e1,e2what do you think?
There was a problem hiding this comment.
@LucaRocco We were thinking of:
--output sort-events (bool flags don't need value)
--output destination.<destination_name>.type=
--output destination.<destination_name>.path= (for file type)
--output stream.<stream_name>.destination=<destination_name>
--output stream.<stream_name>.format=
--output stream.<stream_name>.fields=
--output stream.<stream_name>.filters=
--output stream.<stream_name>.parse-data
So the dotted structure, matches a config file, eg:
output:
sort-events: true
destination:
<name>:
type: xx
path: xx
This is how we are doing for all other flags. WDYT?
There was a problem hiding this comment.
Only to clarify what I meant in the discussion by "cli flags auto-creates a default stream/destination under the hood" is that we don't need to support more than one stream from the cli.
So I don't think we need stream1, stream2, etc. from the cli, just create one stream and keep the flag simple:
--output stream.destinatios=d1,d2
A user who wants more than one stream can then use the config file.
What do you guys think? Should we support more than one stream from the CLI?
There was a problem hiding this comment.
Hi @yanivagman,
The new implementation automatically creates a stream under the hood when an --output <type> flag is specified. Users don’t need to explicitly declare a stream when using the CLI.
If we want this change to continue following the standard configuration flow (StructuredConfig → converted to flags by implementing cliFlagger interface → flags.PrepareOutput), we’ll need to:
- Define a CLI flag structure that supports the declaration of multiple streams. Currently, the code derives flags from the structured configuration and passes them to the same function that typically parses CLI flags. To maintain this behavior, we must be able to generate flags that support all features available in the configuration file. This is why I decided to separate the two flows.
- Update
flags.PrepareOutputto handle the new flags and generate the corresponding Stream structure.
Let me know if you agree with this approach, and I’ll proceed accordingly. I’m a bit busy this week and next, so I’ll likely be able to work on it in about 10 days.
Thanks everyone, much appreciated!
There was a problem hiding this comment.
I'm also fine with supporting more than one stream from the CLI, and I think the syntax Jose suggested is good, as long as it won't overcomplicate things.
There was a problem hiding this comment.
We can go ahead with supporting multiple streams from CLI, I will keep you posted on the implementation. Let me add a new task in the PR and I put it in draft again.
Thanks
There was a problem hiding this comment.
About the internal implementation, I think having a strandard configuration flow is a good idea. But I'll leave this discussion for you and @josedonizetti .
I just want us to agree about the interface exposed to the user through the CLI.
No worries about the time @LucaRocco, thanks for working on this, looks good!
There was a problem hiding this comment.
Hi @josedonizetti,
Will old flags still be available? For example: --output webhook:http://localhost:8080. Keeping both behavior will make the code pretty confusing in my opinion. Do you want me to implement it so that we can decide how to proceed?
|
@LucaRocco finished testing this today, great contribution! Thanks! The only consideration for now is indeed to match flags/configfile. |
|
Hi @josedonizetti thank you for the time you are spending reviewing the PR. Appreciated. Happy that the contribution is valid. Luca |
4 participants
1. Explain what the PR does
This PR implements what was discussed in this issue (#4887)
The main goal is to improve how Tracee prints and delivers events. Below is the list of the main features included in this PR:
dropandblockstrategies.check-proutput:2. Explain how to test it
Once the implementation is complete, a full test scenario will be provided.
Currently, to test the stream and destination handling, I am using the following configuration file and policies:
config.yaml
json-policy.yaml
webhook-policy.yaml
Command to run Tracee
To run Tracee, I use a variation of the following command (depending on what I want to test, I may add or remove policies and adjust the configuration to match the policies in use)
3. Other comments
Below is a checklist of the implemented and pending features, along with the activities I plan to complete before considering this PR finished: