chore(userspace/engine): changed format of the 'output' field in the JSON payload

[h4l0gen]

What type of PR is this?

Uncomment one (or more) /kind <> lines:

/kind bug

/kind cleanup

/kind design

/kind documentation

/kind failing-test

/kind feature

/kind release

Any specific area of the project related to this PR?

Uncomment one (or more) /area <> lines:

/area build

/area engine

/area tests

/area proposals

/area CI

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?:

chore(userspace/engine): changed format of the 'output' field in the JSON payload

[github-actions]

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

[Andreagit97]

Thank you for this!

Linking here an example of what this change means #2985 (comment). Read the full issue to gather full context.

[leogr]

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).

[h4l0gen]

@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 🤔

[leogr]

@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.

[Andreagit97]

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

[FedeDP]

testing framework needs adaption.
Btw i think this is for 0.38.0, right?

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.

[FedeDP]

/milestone 0.38.0

[Andreagit97]

if all maintainers agree, I think we can go with this :)

[leogr]

if all maintainers agree, I think we can go with this :)

Ok, for me.

Does anyone disagree with introducing this small breaking change?
cc @incertum @LucaGuerra

[incertum]

LGTM!

[FedeDP]

/home/runner/work/falco/falco/userspace/engine/formats.cpp: In member function ‘std::string falco_formats::format_event(sinsp_evt*, const string&, const string&, const string&, const string&, const std::set<std::__cxx11::basic_string<char> >&, const string&) const’:
/home/runner/work/falco/falco/userspace/engine/formats.cpp:46:46: error: ‘gen_event_formatter’ has not been declared
   46 |         if(formatter->get_output_format() == gen_event_formatter::OF_JSON)
      |                                              ^~~~~~~~~~~~~~~~~~~
/home/runner/work/falco/falco/userspace/engine/formats.cpp:89:67: error: ‘gen_event_formatter’ has not been declared
   89 |                         formatter->tostring_withformat(evt, line, gen_event_formatter::OF_JSON);
      |                                                                   ^~~~~~~~~~~~~~~~~~~
/home/runner/work/falco/falco/userspace/engine/formats.cpp:130:59: error: ‘gen_event_formatter’ has not been declared
  130 |                 formatter->tostring_withformat(evt, line, gen_event_formatter::OF_NORMAL);
      |                                                           ^~~~~~~~~~~~~~~~~~~
[ 74%] Building CXX object userspace/engine/CMakeFiles/falco_engine.dir/filter_warning_resolver.cpp.o

Can you fix the build?

[h4l0gen]

@FedeDP @leogr @Andreagit97 @incertum build is fixed😀. please take a look into it and provide your feedback or any changes if required.

[FedeDP]

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 release-note block and PR title too!)

[Andreagit97]

oh since the format is changed we need to change all the associated tests :/ we will take care of that, don't worry!

[h4l0gen]

@Andreagit97 if output field format also documented somewhere , we need to take care of that too :)

[leogr]

@FedeDP 👁️

[FedeDP]

@leogr yes, we need to fix 3 tests on https://github.com/falcosecurity/testing framework:

  TestFalco_Legacy_StdoutOutputJsonStrict
  TestFalco_Legacy_NullOutputField
  TestFalco_Legacy_TimeIso8601

@h4l0gen :)

[Andreagit97]

oh since the format is changed we need to change all the associated tests :/ we will take care of that, don't worry!

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

[FedeDP]

Thanks Andre!

[leogr]

/assign @Andreagit97

[Andreagit97]

uhm fixing tests i noticed that probably we are obtaining something that we don't want...
Before this PR the output of a simple rule was:

{"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 output: is changed, but what scares me is that we don't have anymore (command=cat /dev/null) but we have "proc.cmdline\":\"cat /dev/null\", not sure this is what we want since this is a big breaking change because we are completely changing the output format, not just removing the 3 fields time, priority, rule

[incertum]

@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.

[leogr]

As you can see the output: is changed, but what scares me is that we don't have anymore (command=cat /dev/null) but we have "proc.cmdline\":\"cat /dev/null\", not sure this is what we want since this is a big breaking change because we are completely changing the output format, not just removing the 3 fields time, priority, rule

Totally agree. This PR should not introduce such a breaking change.

[FedeDP]

Since there is no agreement here, i will move this to the next milestone.
/milestone 0.39.0

[poiana]

Issues go stale after 90d of inactivity.

Mark the issue as fresh with /remove-lifecycle stale.

Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Provide feedback via https://github.com/falcosecurity/community.

/lifecycle stale

[leogr]

/remove-lifecycle stale

[leogr]

@lg @Issif can you take a look pls?

[FedeDP]

Moving to TBD milestone; feel free to ping me in case anybody is willing to work on this.
/milestone TBD

[leogr]

Superseded by #3314

/close

[poiana]

@leogr: Closed this PR.

In response to this:

Superseded by #3314

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.