fix: Ensure readiness probe relies on translated config - #10542
fix: Ensure readiness probe relies on translated config#10542davidjumani wants to merge 66 commits into
Conversation
|
Issues linked to changelog: |
1 similar comment
|
Issues linked to changelog: |
|
Visit the preview URL for this PR (updated for commit b074b14): https://gloo-edge--pr10542-fix-readiness-probe-omzmzbzp.web.app (expires Thu, 09 Jan 2025 09:03:12 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 77c2b86e287749579b7ff9cadb81e099042ef677 |
davidjumani
changed the title
| @@ -1,4 +1,4 @@ | |||
| package httplisteneroptions | |||
| package httplisteneroptions_test | |||
There was a problem hiding this comment.
Changes made to avoid import cycles
| reporter, | ||
| ) | ||
|
|
||
| listeners = listener.AppendHealthCheckListener(listeners) |
There was a problem hiding this comment.
So, previously, the readiness listener is gated by $gateway.readinessProbe helm option, now that's not possible anymore? If that's the case, should any documentation be updated?
Also, the envoy admin port /ready route is also removed from the static config, is that intentional?
There was a problem hiding this comment.
The helm option still generates the probe, however the HTTP endpoint is configured via translation and not the static bootstrap config
This is an internal change and user should not notice any difference. The ready route was removed since it was not used anywhere nor mentioned in the docs
There was a problem hiding this comment.
Can we be sure no customer is using the envoy admin /ready endpoint directly? If you already discussed this with Sam or Nathan, I am ok with that.
There was a problem hiding this comment.
Customers use the customhealthcheck with /ready if thats the question
There was a problem hiding this comment.
Thanks @nfuden. Looking at the code, I don't see a way to have a route action point to an envoy cluster (I've tried clusterHeader with a request transform / header addition). Any thoughts on how to get around this or can we have the health check on a different port ?
sheidkamp
left a comment
sheidkamp
left a comment
There was a problem hiding this comment.
One question, overall looks good.
| } | ||
|
|
||
| // GenerateHealthCheckListener returns a readiness listener with the health check plugin defined on it | ||
| func GenerateHealthCheckListener() *v1.Listener { |
There was a problem hiding this comment.
Do we have a way of validating that this generates the same config as the removed Helm? Or are the passing tests enough validation?
There was a problem hiding this comment.
The passing tests should do. Additionally the changes to the test files (https://github.com/solo-io/gloo/pull/10542/files#diff-1b1edb892a960bc5d498be228066a8c5eaa997b92dfb62a9c297e03c6b0fe07e) show the envoy listener generated
4 participants
Description
When the readiness listener is defined in the bootstrap config, the pod can be considered ready before it receives any config over XDS that can lead to downtime during an upgrade. This fixes the issue by defining the readiness listener during translation, ensuring that the envoy pod will be ready only when it receives the XDS config
API changes
N/A
Context
This fixes the flaky Zero Downtime tests
Testing steps
Checklist: