Skip to content
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

Merged
merged 26 commits into from
Oct 13, 2020
Merged

Falco outputs refactoring #1412

merged 26 commits into from
Oct 13, 2020

Conversation

leogr
Copy link
Member

@leogr leogr commented Sep 24, 2020

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, and syslog 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:

  • avoiding glue code (that was needed to call functions to and from LUA)
  • avoiding locking mechanism (to call LUA functions, see here)
  • reducing the function call stack

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

grpc after

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:

  • Remove/Add outputs
  • Increase/Decrease the performance of any output
  • Change the behaviors of the current outputs
  • Introduce new features

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:

  • setvbuf - that's pretty similar to the LUA implementation
  • explicit flush with std::flush or std::endl
    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 and buffered_outputs: false. However, in general, the buffering has no effect when keep_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 when buffered_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:

program_output:
  enabled: true
  keep_alive: false
  program: "cat - > /tmp/falco.out"

then use tail -f /tmp/falco.out.

Does this PR introduce a user-facing change?:

NONE

@leogr
Copy link
Member Author

leogr commented Sep 29, 2020

/cc @leodido
/cc @ldegio
/cc @fntlnz

fntlnz
fntlnz previously approved these changes Oct 9, 2020
@poiana
Copy link

poiana commented Oct 9, 2020

LGTM label has been added.

Git tree hash: 9866b50f6696ac0d86773a78c2eaa6e965e008ae

@poiana poiana added the approved label Oct 9, 2020
@leogr
Copy link
Member Author

leogr commented Oct 9, 2020

I have added some integration tests in #1436 to be merged before this, so
/hold

Once #1436 got merged, I will then rebase this on top of that

fntlnz
fntlnz previously approved these changes Oct 12, 2020
@poiana poiana added the lgtm label Oct 12, 2020
@poiana
Copy link

poiana commented Oct 12, 2020

LGTM label has been added.

Git tree hash: 19d636e5bd7e1a426f15dd28cdecd053b6e06f2b

leodido
leodido previously approved these changes Oct 12, 2020
Signed-off-by: Leonardo Grasso <me@leonardograsso.com>
@leogr leogr dismissed stale reviews from leodido and fntlnz via 8a1a45f October 12, 2020 21:54
@leogr leogr requested review from leodido and fntlnz October 13, 2020 06:50
@poiana poiana added the lgtm label Oct 13, 2020
@poiana
Copy link

poiana commented Oct 13, 2020

LGTM label has been added.

Git tree hash: 8c83bbf992afb1d24fee06a85d0ae8719c0c0663

@poiana
Copy link

poiana commented Oct 13, 2020

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

@poiana poiana merged commit d07f18a into master Oct 13, 2020
@poiana poiana deleted the refactor/outputs branch October 13, 2020 09:12
@fntlnz fntlnz mentioned this pull request Oct 13, 2020
4 tasks
leogr added a commit that referenced this pull request Oct 15, 2020
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 <me@leonardograsso.com>
leogr added a commit that referenced this pull request Oct 16, 2020
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 <me@leonardograsso.com>
poiana pushed a commit that referenced this pull request Oct 27, 2020
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 <me@leonardograsso.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New flag --alternate-lua-dir does not account for falco_outputs Lua files
5 participants