Switch webhook liveness/readiness probes to use http ports

[ywluogg]

Changes

Switch webhook liveness/readiness probes to http ports and use a separate port from actual webhook HTTPS ports.
This fixes misleading errors mentioned in #3303

Webhook logs:

kubectl -n tekton-pipelines logs $(kubectl -n tekton-pipelines get pods -l app=tekton-pipelines-webhook -o name)
2020/10/06 23:57:31 maxprocs: Leaving GOMAXPROCS=2: CPU quota undefined
2020/10/06 23:57:31 Server listening on port 8080

This solves the issue because it doesn't send simple TCP request to a HTTPS port, which will cause TLS handshake to fail.

Other considerations will be 1. continue using the same HTTPS port but establish HTTPS handshake. This is not done because using the same port as actual webhook port for probes is not ideal for traffic and responsibility separation.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

• Includes tests (if functionality changed/added)
• Includes docs (if user facing)
• Commit messages follow commit message best practices
• Release notes block has been filled in or deleted (only if no user facing changes)

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

• If you are adding a new binary/image to the cmd dir, please update
  the release Task to build and release this image.

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.

Release Notes

[jerop]

/kind bug

please consider adding more detail in the commit message as described in https://github.com/tektoncd/community/blob/master/standards.md#commit-messages

[ywluogg]

/retest

[afrittoli]

Thanks for this!
Eventually it would be nice to have probes that actually verify that the service is running - however this is definitely a step forward!
It would be great to do the same for the controller (in a different PR) if you'd like.
/lgtm

[ywluogg]

Sorry I was distracted by something else for a bit and now was able to come back to this. @afrittoli I think @imjasonh is still not convinced to add the probes in controller and I also think it will be reasonable to follow the comment you added in #3111. I will keep track of the health checks they've added to knative package and take a look at that to make probes available in controller.

@mattmoor @pmorie I found knative/pkg#1048 but it's not clear to me then whether it is available yet or not. If not we could perhaps add a "shallow" /healthz for now, that always reports "ok" (as @imjasonh suggested) and switch to the knative/pkg one once it becomes available.

[imjasonh]

/lgtm

@sbwsg do you have any other comments?

[ghost]

Thanks @ywluogg !

/approve

[tekton-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbwsg

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 [sbwsg]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[qu1queee]

@ywluogg quick question, would be ok to have this same implementation for the missing probes in https://github.com/tektoncd/pipeline/blob/master/config/controller.yaml ?

[ywluogg]

@ywluogg quick question, would be ok to have this same implementation for the missing probes in https://github.com/tektoncd/pipeline/blob/master/config/controller.yaml ?

The issue was addressed in #3111. I will add the probes in controller once the health checks mentioned in #3111 is available.

[afrittoli]

The approach in knative for the webhook is actually to use the existing HTTPs endpoint like used to do, but with HTTPs instead of TCP, and passing a special k-kubelet-probe header:

https://github.com/knative/serving/blob/1d259fb640b5ba9748a430a0cfa90d451d5826d2/config/core/deployments/webhook.yaml#L95-L102

I guess we could do the same for Tekton and avoid the need for the extra server, but it would require doing TLS handshakes, so perhaps its good to stick with this solution :)

For the controller there is no solution yet, so one way forward might be to have a dedicated small HTTP server until we have a solution from knative/pkg. knative/pkg#1048 is back open

[mattmoor]

You don't need the k-kubelet-probe header, this is sort of a vestige of a time when Istio incorrectly rewrote K8s probes to drop the header it sends. Maybe it's a special User-Agent? I think we have an IsKubeletProbe function somewhere that checks for these things.

[ywluogg]

The approach in knative for the webhook is actually to use the existing HTTPs endpoint like used to do, but with HTTPs instead of TCP, and passing a special k-kubelet-probe header:

https://github.com/knative/serving/blob/1d259fb640b5ba9748a430a0cfa90d451d5826d2/config/core/deployments/webhook.yaml#L95-L102

I guess we could do the same for Tekton and avoid the need for the extra server, but it would require doing TLS handshakes, so perhaps its good to stick with this solution :)

For the controller there is no solution yet, so one way forward might be to have a dedicated small HTTP server until we have a solution from knative/pkg. knative/pkg#1048 is back open

Pinging @imjasonh again for any suggestion for adding a temporary http server in controller. I think currently we may still need for info from other users about why they need this probe in the controller.

[afrittoli]

The probe needed is mostly the liveness one.
When running multiple replicas through leader election, the probe helps detecting a faulty controller and releasing its bucket of resources to other controllers.

[ywluogg]

It sounds necessary to add at least the liveness probe in controller based on this situation. I don't think there is another simpler way to check on liveness under this situation. But I'm waiting for @imjasonh con re-assurance.

[imjasonh]

I agree, it sounds like HA controller will benefit from having a liveness probe, which we can implement now with a simple dummy HTTP server, and later improve with real health checks.

[ywluogg]

Sounds good will add it as soon as possible.

[xiujuan95]

@ywluogg Go through above discussions, are you only enable liveness probe for controller, now? Could you pls also add readiness probe for controller? It's necessary for me. Thanks in advance!

[ywluogg]

@xiujuan95 do you mind also provide some details about the situation you plan to use it? Thanks

[xiujuan95]

@ywluogg Sure, I can. Actually, It's a requirement from my manager team. It requires all pods need to enable liveness and readiness check. Besides, I think readiness can tell us if all containers within this pod are ready or not. It also makes sense! So pls also help add it! Thanks in advance!

[ywluogg]

Sounds good I will add them too.

[xiujuan95]

@ywluogg Thanks a lot!