new(falco): add json_container support and http_output authorization support #3591
Conversation
…support Signed-off-by: Ari Rubinstein <[email protected]>
|
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. 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. |
|
Welcome @arirubinstein! It looks like this is your first PR to falcosecurity/falco 🎉 |
|
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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: arirubinstein The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Thanks @arirubinstein for this PR!
I have left a few inline comments.
Once certain decisions have been made, we can determine which tests might be appropriate to add.
Some maintainers may be occupied this week with reviewing the new Falco release and potentially addressing necessary patches. Just wanted to give a heads-up that further review may be slightly delayed as a result.
| @@ -782,6 +794,8 @@ http_output: | |||
| keep_alive: false | |||
| # Maximum consecutive timeouts of libcurl to ignore | |||
| max_consecutive_timeouts: 5 | |||
| # if provided, adds the value to the Authorization header in the POST | |||
| authorization: "" | |||
There was a problem hiding this comment.
I would move these changes to a separate small PR that may be merged more quickly.
| nlohmann::json omsg; | ||
| omsg["text"] = json.dump(); | ||
| return omsg.dump(); | ||
| } else if(m_json_container == std::string("generic_event_encoded")) { |
There was a problem hiding this comment.
It appears that this is a highly custom format. Perhaps a better approach would be to allow users to supply a formal schema via the configuration, supporting only officially recognized schema formats as templates. I'm curious to hear the perspective of other Falco maintainers on this.
There seem to be multiple possibilities. For instance, if you check out the sinsp_evt_formatter in libs, it provides a more abstracted way of formatting / transforming outputs in the project. A different approach may be more suitable here — we can determine the best path forward after gathering a few viewpoints.
I believe this is the main discussion point for this feature.
| @@ -347,6 +348,7 @@ void falco_configuration::load_yaml(const std::string &config_name) { | |||
| m_config.get_scalar<bool>("json_include_message_property", false); | |||
| m_json_include_output_fields_property = | |||
| m_config.get_scalar<bool>("json_include_output_fields_property", true); | |||
| m_json_container = m_config.get_scalar<std::string>("json_container", ""); | |||
There was a problem hiding this comment.
From my perspective, in such scenarios, we typically support direct boolean configurations in the config, which later translate into direct boolean checks. Alternatively, string inputs could be converted into flags (typically preferred) — for instance, check out the metrics_flags gating, which offers more performant and maintainable gates compared to repeated string comparisons over time.
Adding validity checks on user input while loading the config may be beneficial for free-form string inputs.
There was a problem hiding this comment.
Good callout - I'll migrate this to a flags var so the runtime check is minimal for the default path and shouldn't impact performance
There was a problem hiding this comment.
I would wait until we have discussed the user input. Maybe some sort of enum could also be an option or maybe at the end one simple boolean variable is all that is needed to enable this new user input formatting ...
| @@ -201,7 +203,7 @@ void falco_outputs::handle_msg(uint64_t ts, | |||
| jmsg["hostname"] = m_hostname; | |||
| jmsg["source"] = s_internal_source; | |||
|
|
|||
| cmsg.msg = jmsg.dump(); | |||
| cmsg.msg = m_formats->format_json_container(jmsg, evttime); | |||
There was a problem hiding this comment.
One consideration is that, with these changes, even users who do not actively utilize this new feature may experience the new overhead of these checks for each event. It might be valuable to gather additional feedback.
There was a problem hiding this comment.
Let's double-check whether this formatting also applies to the internal metrics output rules or other internal messages. I recall it involving slightly separate message handling, but I might be mistaken.
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:
This is a simple POC for #3561
It implements the concept of
json_containersand also a simple authorization header addon to support the header-based Splunk HEC posts. PR is just for initial discussion and likely needs to be split up into two PRs/commitsWhich issue(s) this PR fixes:
Fixes #3561
Special notes for your reviewer:
This is a simple POC of a
json_containerfeature to allow enclosing a json output in a parent container to allow for more inline use of the http_output (and other) outputs directly with popular formats (splunk_hec, slack webhooks, etc). In lieu of introducing an entire templating engine, this may be simple enough to provide value to users without introducing too much complexity.Does this PR introduce a user-facing change?:
It introduces a new configuration variable which will need documentation. It is not breaking and is implemented in an additive fashion.