falco

falco copied to clipboard

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

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

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

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

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.

/milestone 0.38.0

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

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

LGTM!

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

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

Thanks! Can you squash the commits together?

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.

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:

• ~~OWNERS~~ [FedeDP]

Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment

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

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

@FedeDP :eye:

@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 :)

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

Thanks Andre!

/assign @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

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

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.

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

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

/remove-lifecycle stale