disallow exec probes for liveness and readiness checks

[joshrider]

Proposed Changes

• remove the allowance for ExecActions as Readiness and Liveness probes

The runtime contract states that If specified, liveness and readiness probes are REQUIRED to be of the httpGet or tcpSocket types

Release Note

NONE

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: joshrider
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: mattmoor

If they are not already assigned, you can assign the PR to them by writing /assign @mattmoor in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• pkg/apis/OWNERS
• test/OWNERS

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

[knative-prow-robot]

@joshrider: 0 warnings.

Details

In response to this:

Proposed Changes

• remove the allowance for ExecActions as Readiness and Liveness probes

The runtime contract states that If specified, liveness and readiness probes are REQUIRED to be of the httpGet or tcpSocket types

Release Note

NONE

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[knative-metrics-robot]

The following is the coverage report on pkg/.
Say /test pull-knative-serving-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/serving/fieldmask.go	99.5%	99.5%	-0.0
pkg/apis/serving/k8s_validation.go	98.6%	99.3%	0.8

[dgerd]

/hold

I have a PR currently out that looks to relax this requirement. https://github.com/knative/serving/pull/4035/files#diff-e5a9114cbfb7f985c0da2dfe04221c26R230

ExecProbes are unfortunately the only way to effectively probe many gRPC and HTTP/2.0 workloads as of right now. Taking this away has the potential to break existing workloads. We do need a better story here as the exec probes do not go through the queue-proxy container, are easy to implement incorrectly, and may not work in all environments. However, I don't think we can remove this at this time.

See https://kubernetes.io/blog/2018/10/01/health-checking-grpc-servers-on-kubernetes/ for more details.

/cc @ahmetb

[dprotaso]

exec probes do not go through the queue-proxy container

Do you envision using sharing process namespaces between the pod's containers (beta in k8s 1.14) as a way to enable this?

[mattmoor]

I'm wondering whether the combination of the exec probe and a readiness probe through the queue-proxy that TCP probes the user-container are (taken together) enough. This extends a layer further when we also incorporate probes on the mesh's proxy. 🐢

[alexellis]

Are httpProbes compatible with Istio under mTLS now, too? In Kubernetes / OpenFaaS we had to use execProbes to get around mTLS blocking the http traffic.

[mattmoor]

@alexellis Yes in 1.1. Closing this.