fix: HealthCheck should respect endpoint hostname#8854
fix: HealthCheck should respect endpoint hostname#8854zirain wants to merge 10 commits intoenvoyproxy:mainfrom
Conversation
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8854 +/- ##
==========================================
- Coverage 74.73% 74.72% -0.02%
==========================================
Files 251 251
Lines 40360 40379 +19
==========================================
+ Hits 30165 30175 +10
- Misses 8130 8135 +5
- Partials 2065 2069 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…ostFrom test - Remove auto-derive of DestinationEndpoint.Hostname from FQDN in route.go (changed IR output for all FQDN backends unnecessarily) - Include health check testdata from envoyproxy#8854 (by @zirain) - Add backendtrafficpolicy-with-healthcheck-auto-host-rewrite test exercising HostFrom=Endpoint with hostname.type: Backend Signed-off-by: asalvador <asalvador@newrelic.com>
f31f65c to
f2d4d6a
Compare
Signed-off-by: asalvador <asalvador@newrelic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f2d4d6a8a3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
albsga4
left a comment
There was a problem hiding this comment.
Nice approach — passing the URLRewrite directly to the xDS translator is cleaner than adding a new IR field. Tested locally with a gatewayapi golden test for hostname.type: Backend (auto-host-rewrite via HTTPRouteFilter ExtensionRef) and it works correctly on your branch.
The unit test in cluster_test.go covers the xDS level well. One suggestion: a gatewayapi integration test would also cover the full flow (HTTPRouteFilter → IR → xDS). Feel free to grab the test input from my PR: backendtrafficpolicy-with-healthcheck-auto-host-rewrite.in.yaml
It has two routes:
- Auto-host-rewrite + no explicit
http.hostname→ health check usesBackend.endpoint.hostname - Auto-host-rewrite + explicit
http.hostname→ explicit value wins
I'll close #8851 in favor of this PR. Thanks for taking this on!
|
@copilot resolve the merge conflicts in this pull request |
|
imo irrespective of URL rewrite being enabled or not, the endpoint hostname should be used if healthCheck host is unset |
I do believe we should use Endpoint hostname by default if we don't care aboud backward compatibility. |
albsga4
left a comment
There was a problem hiding this comment.
Tested hostname.type: Backend with two FQDN backends without the explicit hostname field on the Backend endpoint. Routing works (Envoy uses the FQDN address for STRICT_DNS auto_host_rewrite), but health checks get nothing because ep.Hostname is nil.
Suggested a fallback to ep.Host (the FQDN address) so users don't need to duplicate the FQDN.
albsga4
left a comment
There was a problem hiding this comment.
Requesting changes: hostname.type: Backend with FQDN backends that don't set the explicit hostname field results in no per-endpoint health check hostname. See suggestion for fallback to ep.Host.
b90975e to
9f2c250
Compare
albsga4
left a comment
There was a problem hiding this comment.
The hostname field on BackendEndpoint was added in #6503 / #6280 specifically for IP-based backends where there is no DNS name for auto_host_rewrite to use. For FQDN backends, the FQDN address IS the hostname — forcing users to duplicate it doesn't add value.
Verified on a live cluster: auto_host_rewrite on STRICT_DNS correctly rewrites Host to the FQDN address for each endpoint, even without the explicit hostname field. But health checks get nothing because ep.Hostname is nil.
| Hostname: ptr.Deref(irEp.Hostname, ""), | ||
| Address: buildAddress(irEp), | ||
| HealthCheckConfig: buildHealthCheckConfig(hc, irEp), | ||
| HealthCheckConfig: buildHealthCheckConfig(hc, rewrite, irEp), |
There was a problem hiding this comment.
imo we shouldnt be using route rewrite info which is for user requests for health check request semantics
There was a problem hiding this comment.
Correct me if I'm wrong, when you rewrited the URL that mostly because the upstream may only accept the rewrited host(no matter it's from per-route or per-backend).
There was a problem hiding this comment.
Rewriting applies to user requests initiated by downstream
Health check requests are initiated by envoy with specific info of backend ( host, dns/ip)
There was a problem hiding this comment.
Removed it now, we could add it back if needed in the future.
06d6519 to
dbf8f63
Compare
| - matches: | ||
| - path: | ||
| value: "/v6-rewrite" | ||
| # Hostname rewrite filter applied, should use example2.com when doing health check. |
There was a problem hiding this comment.
can the title & examples be updated to not rely on URLRewrite for heathcheck semantics
Signed-off-by: zirain <zirain2009@gmail.com>
Signed-off-by: zirain <zirain2009@gmail.com>
Signed-off-by: zirain <zirain2009@gmail.com>
Co-authored-by: albsga4 <asalvador@newrelic.com> Signed-off-by: zirain <zirain2009@gmail.com>
Signed-off-by: zirain <zirain2009@gmail.com>
Signed-off-by: zirain <zirain2009@gmail.com>
aa81ed1 to
48ff586
Compare
Signed-off-by: zirain <zirain2009@gmail.com>
fix: #8848