search

istio-ecosystem / authservice

Move OIDC token acquisition out of your app code and into the Istio mesh
Apache License 2.0
217 stars 63 forks source link

implement healthcheck endpoint for active JWKs existence #174

Closed Shikugawa closed 3 years ago

Shikugawa commented 3 years ago

This PR makes it as a gRPC service for the checking. Is that necessary? Kubernetes supports HTTP probes only. https://kubernetes.io/docs/tasks/configure-pod-container/configure-liveness-readiness-startup-probes/

I thought we can open a port and serve the health check status there. And the k8s deployment YAML will specify the health check roughly like

readinessProbe:
  httpGet:
    path: /healthz
    port: 8080 # or something else.

It is because we don't have to add rest endpoint only for healthcheck. We can utilize grpc-health-probe here. https://github.com/grpc-ecosystem/grpc-health-probe

incfly commented 3 years ago

Are you suggesting that we can add this grpc_health_probe binary into the authservice docker image, and then configure with command type health check? https://kubernetes.io/docs/tasks/configure-pod-container/configure-liveness-readiness-startup-probes/#define-a-liveness-command

In comparision, adding a HTTP REST point does not require adding new dependencies into our docker images(consider the CVEs the burden to update the binary), and semantics (response code, headers, timeout, etc) is wider understood.

because of the above, I would prefer to have HTTP instead then.

istio-testing commented 3 years ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: incfly, Shikugawa

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: - ~~[OWNERS](https://github.com/istio-ecosystem/authservice/blob/master/OWNERS)~~ [incfly] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
incfly commented 3 years ago

Some test seems has failed.

incfly commented 3 years ago

Oops, I didn't realize that the bot would merge the PR when there's a test failure. @Shikugawa could you take a look and send a fix? https://github.com/istio-ecosystem/authservice/runs/3972302821?check_suite_focus=true