-
Notifications
You must be signed in to change notification settings - Fork 912
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
Falco outputs refactoring #1412
Conversation
b0b0631
to
0d20dbc
Compare
9d426c5
to
a019a76
Compare
LGTM label has been added. Git tree hash: 9866b50f6696ac0d86773a78c2eaa6e965e008ae
|
LGTM label has been added. Git tree hash: 19d636e5bd7e1a426f15dd28cdecd053b6e06f2b
|
Signed-off-by: Leonardo Grasso <[email protected]>
LGTM label has been added. Git tree hash: 8c83bbf992afb1d24fee06a85d0ae8719c0c0663
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fntlnz, leodido 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 |
Previously, formatters were freed by LUA code when re-opening outputs. Since now, outputs are not controlling anymore the falco_formats class (see #1412), we just free formatters only if were already initialized. That is needed when the engine restarts (see #1446). By doing so, we also ensure that correct inspector instance is set to the formatter cache. Signed-off-by: Leonardo Grasso <[email protected]>
Previously, formatters were freed by LUA code when re-opening outputs. Since now, outputs are not controlling anymore the falco_formats class (see #1412), we just free formatters only if were already initialized. That is needed when the engine restarts (see #1446). By doing so, we also ensure that correct inspector instance is set to the formatter cache. Signed-off-by: Leonardo Grasso <[email protected]>
Previously, formatters were freed by LUA code when re-opening outputs. Since now, outputs are not controlling anymore the falco_formats class (see #1412), we just free formatters only if were already initialized. That is needed when the engine restarts (see #1446). By doing so, we also ensure that correct inspector instance is set to the formatter cache. Signed-off-by: Leonardo Grasso <[email protected]>
What type of PR is this?
/kind cleanup
/kind design
Any specific area of the project related to this PR?
/area engine
Why we need it:
While trying to figure out how to make all Falco outputs non-blocking (see #1417), I found out that the current LUA implementation for the outputs is not strictly required and counterproductive in some cases. For example,
grpc
,http
, andsyslog
outputs are actually implemented in C++ though passing forth and back thru (almost empty) LUA functions. So, I realized that by just porting all outputs to C++, we could reduce the overall complexity by:By doing so, we can also achieve other benefits like:
What this PR does:
This PR ports the LUA outputs code to C++, cleans up all the unnecessary code and introduces a shared interface for implementing Falco output classes.
As an example, please find below two sequence diagrams to visualize how the gRPC output has been simplified.
![grpc before](https://user-images.githubusercontent.com/3390997/94276253-447d1e00-ff48-11ea-91da-7f7c105e067c.png)
In a similar way, other outputs are simplified too.
Note that the main goal here is just to refactoring the code and maintaining function parity with the previous implementation.
Non-goals:
Further improvements (like a non-blocking mechanism described in #1417) needs to be implemented in a follow-up PR since this PR is already big enough.
Special notes for your reviewer:
Please, note the non-goals (above).
Furthermore, this PR incidentally
Fixes #1438
Special note regarding the
output_stdout
implementation:Before I opted for the current implement (that relies on std::unitbuf), I had tried several approaches, including:
Also note that in any case, the buffer needs to be disabled (or flushed) every time an output occurs, since
In the end, I chose
std::unitbuf
because of its simplicity and low overhead.Anyway, the implementation behaves exactly how the LUA ones did, especially when
buffered_outputs: false
and the stdout refers to a non-interactive device (ie., no TTY). Btw, the easiest way to try Falco standard output with no TTY:sudo userspace/falco/falco -c /etc/falco/falco.yaml > /tmp/falco.out
then use
tail -f /tmp/falco.out
in another shell.Special note regarding the
output_program
implementation:The original implementation (LUA) disables the buffering only when
keep_live: true
andbuffered_outputs: false
. However, in general, the buffering has no effect whenkeep_live: false
since the output is performed in a single operation, and then the pipe is closed. Thus, the newer implementation simplifies by disabling the buffering just whenbuffered_outputs: false
.Furthermore, both implementations do not check that the program ran successfully.
In general, both implementations behave precisely the same.
Finally, an easy way to test that is by configuring the
program_output
so:then use
tail -f /tmp/falco.out
.Does this PR introduce a user-facing change?: