-
Notifications
You must be signed in to change notification settings - Fork 873
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
chore(userspace/engine): changed format of the 'output' field in the JSON payload #3037
base: master
Are you sure you want to change the base?
Conversation
This PR may bring feature or behavior changes in the Falco engine and may require the engine version to be bumped. Please double check userspace/engine/falco_engine_version.h file. See versioning for FALCO_ENGINE_VERSION. /hold |
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.
Thank you for this!
Linking here an example of what this change means #2985 (comment). Read the full issue to gather full context.
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 per the original proposal, this behavior must be opt-in for users.
So we need to add a new config key in falco.yaml
to enable this behavior. The previous format must be preserved when this new setting is off (default).
@leogr here is the discussion about this https://kubernetes.slack.com/archives/CMWH3EH32/p1706275031447379?thread_ts=1705577834.276719&cid=CMWH3EH32 @Andreagit97 proposed that we don't really need current output format and removing timestamp tag permanently would be good for user experience, WDYT about it @leogr 🤔 |
Sorry, I missed that discussion 👼 So, ignore my previous comment for now. If we get a consensus on that, it will be ok for me, too. Just a note for maintainers: if we agree to change the JSON "output" field without an opt-in, this will introduce a (minor) breaking change that needs to be signaled in the release note. |
Yes! Let's see what other maintainers think about that @falcosecurity/falco-maintainers |
I agree, it should be signaled in the rn, and i'd add an entry in the 0.38.0 breaking changes; even if minor, it is a breaking change. |
/milestone 0.38.0 |
if all maintainers agree, I think we can go with this :) |
Ok, for me. Does anyone disagree with introducing this small breaking change? |
LGTM! |
Can you fix the build? |
@FedeDP @leogr @Andreagit97 @incertum build is fixed😀. please take a look into it and provide your feedback or any changes if required. |
i should have fixed it, let's see if the CI passes |
@FedeDP @Andreagit97 , I squashed all three commits. Sorry for the delay in fixing this build 😅, as I've also started learning and contributing to falcosecurity/event-generator for GSoC project 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.
/approve
LGTM label has been added. Git tree hash: cd62c0fb4bb94198646d2778667113493d8858e4
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: FedeDP, h4l0gen 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 |
Updated PR body and PR title to adhere to the https://github.com/falcosecurity/.github/blob/main/CONTRIBUTING.md#commit-convention (that we use for |
oh since the format is changed we need to change all the associated tests :/ we will take care of that, don't worry! |
@Andreagit97 if output field format also documented somewhere , we need to take care of that too :) |
|
@leogr yes, we need to fix 3 tests on https://github.com/falcosecurity/testing framework:
@h4l0gen :) |
folks, i've already written this message, i will take care of these tests when i have time. Let's avoid to ping everyone in the discussion |
Thanks Andre! |
/assign @Andreagit97 |
uhm fixing tests i noticed that probably we are obtaining something that we don't want... {"hostname":"test-falco-hostname","output":"2016-08-04T16:17:57.881781397+0000: Warning An open was seen (command=cat /dev/null)","priority":"Warning","rule":"open_from_cat","source":"syscall","tags":["filesystem","process","testing"],"time":"2016-08-04T16:17:57.881781397Z", "output_fields": {"evt.time.iso8601":1470327477881781397,"proc.cmdline":"cat /dev/null"}} While now the output is: {"hostname":"test-falco-hostname","output":"{\"evt.time.iso8601\":1470327477881781397,\"proc.cmdline\":\"cat /dev/null\"}","priority":"Warning","rule":"open_from_cat","source":"syscall","tags":["filesystem","process","testing"],"time":"2016-08-04T16:17:57.881781397Z", "output_fields": {"evt.time.iso8601":1470327477881781397,"proc.cmdline":"cat /dev/null"}} As you can see the |
@Andreagit97 thanks for checking, wasn't aware we changed that to JSON. No this is not what we want and what is advertised as change in this PR. |
Totally agree. This PR should not introduce such a breaking change. |
Since there is no agreement here, i will move this to the next milestone. |
What type of PR is this?
/kind feature
Any specific area of the project related to this PR?
/area engine
What this PR does / why we need it:
'timestamp' and 'priority' format of the 'output' field in the JSON format has been removed.
Which issue(s) this PR fixes:
Change the format of the 'output' field in the JSON payload
Fixes #2985
Special notes for your reviewer:
Does this PR introduce a user-facing change?: