-
Notifications
You must be signed in to change notification settings - Fork 902
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: add healthz endpoint #1546
Conversation
/unassign @lorenzo-david |
There was a problem hiding this 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
userspace/falco/webserver.cpp
Outdated
|
||
bool k8s_healthz_handler::handleGet(CivetServer *server, struct mg_connection *conn) | ||
{ | ||
std::string status_body = "{\"status\": \"ok\"}"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
std::string status_body = "{\"status\": \"ok\"}"; | |
const std::string status_body = "{\"status\": \"ok\"}"; |
There was a problem hiding this comment.
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
There was a problem hiding this 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! 👏
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. |
a802aa3
to
1863cd0
Compare
Signed-off-by: Carlos Panato <[email protected]>
@leogr PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
LGTM label has been added. Git tree hash: e82c4de66d881105a1d38cf88633eb3ac3c375aa
|
[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 |
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?: