Skip to content

new(falco): add json_container support and http_output authorization support #3591

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

arirubinstein
Copy link

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:
This is a simple POC for #3561
It implements the concept of json_containers and 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/commits

Which issue(s) this PR fixes:

Fixes #3561

Special notes for your reviewer:

This is a simple POC of a json_container feature 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.


@poiana
Copy link
Contributor

poiana commented May 30, 2025

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.

@poiana
Copy link
Contributor

poiana commented May 30, 2025

Welcome @arirubinstein! It looks like this is your first PR to falcosecurity/falco 🎉

@poiana poiana added the size/M label May 30, 2025
Copy link

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

@poiana
Copy link
Contributor

poiana commented May 30, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: arirubinstein
Once this PR has been reviewed and has the lgtm label, please assign sgaist for approval. For more information see the Kubernetes Code Review Process.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@poiana poiana requested a review from jasondellaluce May 30, 2025 21:02
@poiana poiana requested a review from Kaizhe May 30, 2025 21:02
Copy link
Contributor

@incertum incertum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: ""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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")) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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", "");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@incertum incertum Jun 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

Introduce the concept of "formatting strategy" for http_output
3 participants