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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

falco: add healthz endpoint #1546

Merged
merged 1 commit into from
Feb 11, 2021
Merged

Conversation

cpanato
Copy link
Member

@cpanato cpanato commented Jan 30, 2021

What type of PR is this?
/kind feature

Any specific area of the project related to this PR?

What this PR does / why we need it:

Add an initial implementation to have a heatlhz endpoint, this is useful when running the workload in kubernetes so we can add liveness/read probes to do the checks and fail when is not responding.

In the future, we can have some deep checks if needed.

This PR is the initial to fix this issue: falcosecurity/charts#53

tried my best with c++ 馃槃

/assign @fntlnz @leodido

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

new: add healthz endpoint to the webserver

@cpanato
Copy link
Member Author

cpanato commented Jan 30, 2021

/unassign @lorenzo-david
/assign @fntlnz

@poiana poiana assigned fntlnz and unassigned lorenzo-david Jan 30, 2021
Copy link
Contributor

@fntlnz fntlnz left a comment

Choose a reason for hiding this comment

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

Good job @cpanato !

Just a little nit then LGTM


bool k8s_healthz_handler::handleGet(CivetServer *server, struct mg_connection *conn)
{
std::string status_body = "{\"status\": \"ok\"}";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
std::string status_body = "{\"status\": \"ok\"}";
const std::string status_body = "{\"status\": \"ok\"}";

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks @fntlnz for your review, did the change and also update https://github.com/falcosecurity/falco/pull/1546/files#diff-759cf16077c5a03cbf4e9b6dd8af34208252447adb0f3f894e5aadb6f218335dR168 to match your feedback here 馃槃

PTAL

Copy link
Member

@leogr leogr left a comment

Choose a reason for hiding this comment

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

Overall it seems good to me, but I'm not sure about one thing.

We assume the (already implemented) Falco web server is enabled, so /heathz endpoint works. But this assumption can be false under some circumstances since the web server can easily be disabled by the config (see https://github.com/falcosecurity/falco/blob/master/falco.yaml#L156 ).
It might also be confusing as the only purpose of the webserver has been to ingest K8s Auditing log events until now.

So, IMHO we should either:

  • implement the /healthz endpoint on another web server instance (btw, I don't like this idea)
    OR
  • document very well the new purposes of the webserver (making it clear that it's not for k8s events only, my vote goes for this option)

PS
well done, @cpanato ! We really missed this feature, thank you! 馃憦

@leogr leogr added this to the 0.28.0 milestone Feb 1, 2021
@cpanato
Copy link
Member Author

cpanato commented Feb 2, 2021

@leogr @fntlnz Agree with documentation the endpoint, please let me know what y'all prefer then I will update the PR accordingly

@leogr
Copy link
Member

leogr commented Feb 3, 2021

@leogr @fntlnz Agree with documentation the endpoint, please let me know what y'all prefer then I will update the PR accordingly

Hey @cpanato

I think that just adding the new configuration option to falco.yaml (with the usage description in the comments) would be the right way to document it.

@cpanato cpanato force-pushed the add-healthz branch 4 times, most recently from a802aa3 to 1863cd0 Compare February 4, 2021 15:31
Signed-off-by: Carlos Panato <ctadeu@gmail.com>
@cpanato
Copy link
Member Author

cpanato commented Feb 4, 2021

@leogr PTAL

@cpanato cpanato requested review from leogr and fntlnz February 11, 2021 14:51
@cpanato
Copy link
Member Author

cpanato commented Feb 11, 2021

ping 馃槃 @leogr @fntlnz

Copy link
Member

@leogr leogr left a comment

Choose a reason for hiding this comment

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

LGTM

@poiana
Copy link

poiana commented Feb 11, 2021

LGTM label has been added.

Git tree hash: e82c4de66d881105a1d38cf85943eb3ac3c375aa

@poiana
Copy link

poiana commented Feb 11, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: leodido, leogr

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 f140cdf into falcosecurity:master Feb 11, 2021
@cpanato cpanato deleted the add-healthz branch February 12, 2021 09:42
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.

None yet

6 participants