Exercise etcd connectivity in the /healthz/ping endpoint

[bjhaid]

Is this a BUG REPORT or FEATURE REQUEST?:
/kind feature

What happened:

We fenced off an api-server from connecting to etcd, however the /healthz/ping endpoint was returning 200

What you expected to happen:

The /healthz/ping endpoint to exercise connectivity to etcd and return a 500 type response code if it can't connect to etcd since the apiserver is useless without etcd

[k8s-github-robot]

@bjhaid There are no sig labels on this issue. Please add a sig label by:
(1) mentioning a sig: @kubernetes/sig-<team-name>-misc
e.g., @kubernetes/sig-api-machinery-* for API Machinery
(2) specifying the label manually: /sig <label>
e.g., /sig scalability for sig/scalability

Note: method (1) will trigger a notification to the team. You can find the team list here and label list here

[xiangpengzhao]

/sig api-machinery

[bjhaid]

thanks @xiangpengzhao

[deads2k]

Checking to see if you can reach etcd makes sense. I don't think it should be part of ping though. How about a separate health check which is conditional on having a storage a config which speaks to etcd?

[bjhaid]

@deads2k

I don't think it should be part of ping though.
Fair

How about a separate health check which is conditional on having a storage a config which speaks to etcd?

It would be better to have an health check that exercises whatever storage backend is in use, however if that's too complex maybe having something etcd specific would be okay.

[deads2k]

It would be better to have an health check that exercises whatever storage backend is in use, however if that's too complex maybe having something etcd specific would be okay.

I think those would be different health checks. It is possible to have multiple backends for a single server. Consider the arguments around events we're having now.

[bjhaid]

@deads2k how about aggregating health checks for multiple backends into a single endpoint?

[deads2k]

@deads2k how about aggregating health checks for multiple backends into a single endpoint?

/healthz is already a summary of all health checks. I'd prefer to keep separate health checks separate and have one level of aggregation.

[bjhaid]

/healthz is already a summary of all health checks. I'd prefer to keep separate health checks separate and have one level of aggregation.

/healthz returns just bare ok on 1.6.1 I can't find any summary information there, is that expected? Or maybe I do not understand what you mean by:

summary of all health checks

[deads2k]

/healthz returns just bare ok on 1.6.1 I can't find any summary information there, is that expected? Or maybe I do not understand what you mean by:

if any constituent health check (see kubectl get --raw / for the list) fails, it shows you the failing name, but not details (avoids leaking information). Given the name, you can lookup the specific health check failures. There are maybe a half dozen or so checks on a normal apiserver.

[bjhaid]

ah makes perfect sense, so how will this be turned into a work stream? ;) I also don't mind taking a stab at implementing it with some guidance

[bjhaid]

@deads2k I got something working here:

bjhaid@3d685f1

It's doesn't have tests, but I have tested it from the command line and it works as expected, I can maybe add tests and move the logic to a separate function if you think this makes sense

[deads2k]

bjhaid/kubernetes@3d685f1

It's doesn't have tests, but I have tested it from the command line and it works as expected, I can maybe add tests and move the logic to a separate function if you think this makes sense

Yeah, it looks close enough to be worth a pull and some tests

[bjhaid]

Thanks ❤️ I'll clean it up and send in a PR

[lavalamp]

/assign @jpbetz

[k8s-ci-robot]

@lavalamp: GitHub didn't allow me to assign the following users: jpbetz.

Note that only kubernetes members can be assigned.

In response to this:

/assign @jpbetz

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.

[bjhaid]

@lavalamp I actually have a WIP for this and was going to PR this tomorrow

[jpbetz]

Loosely related to this- Etcd's own /health endpoint will start correctly reporting as unhealthy when any alarms are triggered: etcd-io/etcd#8272. This won't be available till etcd 3.3.0

[bjhaid]

@jpbetz that will not solve the motivation for this issue, the apiserver was fenced off from having a network communication with etcd and the healthz endpoints still reported healthy, even though all critical api requests were failing