Change /ping endpoint to /healthz
#167
Conversation
Signed-off-by: KeisukeYamashita <[email protected]>
Signed-off-by: KeisukeYamashita <[email protected]>
|
I get your point, I prefer to have both, for avoiding breaking changes.
And we use wdyt? |
|
Hi, |
|
We can have both endpoints for now and add in changelog and README that |
Signed-off-by: KeisukeYamashita <[email protected]>
Signed-off-by: KeisukeYamashita <[email protected]>
KeisukeYamashita
force-pushed
the
change-ping-endpoint-to-health
branch
from
1cbb0e2
to
545bd9e
Compare
handlers.go
Outdated
| @@ -73,6 +73,11 @@ func pingHandler(w http.ResponseWriter, r *http.Request) { | |||
| w.Write([]byte("pong\n")) | |||
| } | |||
|
|
|||
| // healthHandler is a simple handler to test if daemon is UP. | |||
| func healthHandler(w http.ResponseWriter, r *http.Request) { | |||
| w.WriteHeader(http.StatusOK) | |||
There was a problem hiding this comment.
@Issif Do you want me to respond a JSON as you suggested? I think the 200 status code is enough, it will tell the same thing as {"status": "ok"}. Moreover, many health check features (Kubernetes HTTPGet Probe, Google Load Balancer, etc) checked the status code and won't check the resp body.
WDYT?
There was a problem hiding this comment.
It is good to send the body as well, because in the future we can add more info, for example, we can check if the AWS is still valid and respond on that as well like
{
"status": "ok",
"aws": "ok",
"pagerduty": "ok"
...
}for now the /healtz is a dummy which is fine
There was a problem hiding this comment.
We also can add the ready endpoint to say that the APP is ready, but for that, we will need a bit more logic
There was a problem hiding this comment.
Thanks for your reply.
It is good to send the body as well, because in the future we can add more info, for example, we can check if the AWS is still valid and respond on that as well like
Totally agree.
for now the /healtz is a dummy which is fine
Okay, let's go with this change for now 👍
Thank you.
|
For now I'm ok for a dummy We can create an issue for discussing about an enhanced endpoint, with more details. We need to agree about what informations have to be inside (version? enabled outputs?). I think this a point for 3.0.0, I really need to release 2.20.0 now, both of you have added so much great things 🙏 . I'm not a big fan of @KeisukeYamashita btw the message about deprecation of |
Signed-off-by: KeisukeYamashita <[email protected]>
|
@Issif Thank you for your comment.
Okay, now it the health endpoint will return
Got it, let's discuss for later release 👍 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Issif 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 |
|
LGTM label has been added. Git tree hash: c84d08414bc8dd336a2403cbce4dc53d34502301
|
What type of PR is this?
/kind design
Any specific area of the project related to this PR?
/area outputs
What this PR does / why we need it:
Unify the health check endpoint with cloud native conversion.
This endpoint is used for health checking. See the chart implementation too https://github.com/falcosecurity/charts/blob/40e45b1f1ef2f79787f1d35b5ba565b880f626de/falcosidekick/templates/deployment.yaml#L47-L58
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Many projects in CNCF expose
/healthzendpoint as health check endpoint. I picked some for example:As this project is one of cloud native projects, I think it's better to unify with the cloud native convention.