Overwrite k8s HTTP probes when transparent proxy is enabled. #517
Conversation
When this is enabled, the mutating webhook will also overwrite the HTTP probes of the application container to point them to Envoy, while saving the original ports of the probes as annotations. The endpoints controller will set Expose configuration for the proxy, setting the LocalPathPort to the port saved in the annotation. - Add a new flag -transparent-proxy-default-overwrite-probes (default to true) to allow overwriting k8s probes. - Allow setting this on a per-pod basis via the "consul.hashicorp.com/transparent-proxy-overwrite-probes" annotation - Add new annotations to allow for choosing custom ports for Envoy listeners to expose readiness and liveness probes
ishustava
added
type/enhancement
ishustava
requested review from
a team,
lkysow and
thisisnotashwin
and removed request for
a team
There was a problem hiding this comment.
This looks very excellent.
I had a broad suggestion. I think the handler.go method overwriteProbes() should be able to set the desired values for the Port on both the probes as it should be able to read the annotations of the pod. This was the endpoint controller does not have to check the annotations to determine what the value of the port should be and it can just read it off the pod spec.
| return nil, nil, err | ||
| } | ||
| if overwriteProbes { | ||
| if cs := pod.Spec.Containers; len(cs) > 0 { |
There was a problem hiding this comment.
I know for service name and service port we would look at the first container name/port in the past, but those were also overrideable by configuration. In this case, there's no configuration to pick which container is the app container to rewrite the healthcheck. Is it alright to assume here that it should be the first container?
I think yes because we don't support multi-port services and because it's not a limiting requirement to the app dev that the first container is their main app since there isn't an order the containers will run in. But just wanted to voice it out loud in case you thought of something along these lines as well.
There was a problem hiding this comment.
Yep, that's a really good point. Thanks for bringing this up!
You're right that with a port you could still choose a port from any container, but now we will always overwrite probes for the first container. Thinking about it, I think we could make it work and use the same container to overwrite probes as the container that has the port from the provided port annotation. That will probably be the most accurate, but I'll do it a separate PR. WDYT?
There was a problem hiding this comment.
Yes I think that makes sense-- if the port annotation exists, look for the container with a port that matches the annotation, else use the first container? Separate PR totally makes sense as this PR is valuable on its own!
When this is enabled, the mutating webhook will also overwrite the HTTP probes
of the application container to point them to Envoy, while saving the original
ports of the probes as annotations. The endpoints controller will set Expose
configuration for the proxy, setting the LocalPathPort to the port saved
in the annotation.
to allow overwriting k8s probes.
annotation
readiness and liveness probes
-enable-transparent-proxyto-default-enable-transparent-proxyHow I've tested this PR:
Acceptance test and the helm PR here: hashicorp/consul-helm#953
How I expect reviewers to test this PR:
code review
Checklist: