Set host for http health checker explicitly to avoid using the cluster name as host header for http health checking request. #3057

lemonlinger · 2024-03-29T08:57:53Z

codecov · 2024-03-30T09:15:24Z

Codecov Report

Attention: Patch coverage is 66.66667% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 58.06%. Comparing base (7bed6fa) to head (1d1b369).
Report is 5 commits behind head on main.

Files	Patch %	Lines
internal/ir/xds.go	42.85%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3057      +/-   ##
==========================================
- Coverage   58.88%   58.06%   -0.83%     
==========================================
  Files         162      165       +3     
  Lines       27083    27470     +387     
==========================================
+ Hits        15948    15950       +2     
- Misses      10182    10565     +383     
- Partials      953      955       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

… name as host header for http health checking request Signed-off-by: lemonlinger <[email protected]>

Signed-off-by: lemonlinger <[email protected]>

lemonlinger · 2024-04-01T07:14:39Z

/retest

arkodg · 2024-04-01T07:17:01Z

internal/gatewayapi/backendtrafficpolicy.go

@@ -387,6 +387,11 @@ func (t *Translator) translateBackendTrafficPolicyForRoute(policy *egv1a1.Backen
 						}
 						r.Timeout = to
 					}
+
+					// See issue #3033.
+					if r.HealthCheck != nil {


doesnt look right, why isn't this part of buildHealthCheck

Because the Host in HTTP Health check is derived from the Hostname of a HTTP Route. We don't know which routes will be bound when buildHealthCheck is called.

I'm not sure I understand, we have access to the route IR here

nvm I understood your point, in this case, I'd suggest doing this by adding a single line in

gateway/internal/gatewayapi/backendtrafficpolicy.go

Line 356 in 55dcc9a

tcp.HealthCheck = hc

with a comment saying - Update the Host field in HealthCheck, now that we have access to the Route Hostname

Signed-off-by: lemonlinger <[email protected]>

arkodg · 2024-04-01T09:32:14Z

internal/ir/xds.go

@@ -1551,6 +1552,12 @@ type ActiveHealthCheck struct {
 	TCP *TCPHealthChecker `json:"tcp,omitempty" yaml:"tcp,omitempty"`
 }

+func (h *HealthCheck) SetHTTPHostIfAbsent(host string) {
+	if h != nil && h.Active != nil && h.Active.HTTP != nil && h.Active.HTTP.Host == "" {


do we need a check for h.Active.HTTP.Host == "" ?

To avoid overriding the user-specified host in http health check if defined by users one day.

arkodg

one non blocking nit, LGTM !

zirain · 2024-04-01T12:30:44Z

/retest

lemonlinger · 2024-04-01T14:14:43Z

/retest

…r name as host header for http health checking request. (envoyproxy#3057) * Set host for http health checker explictly to avoid using the cluster name as host header for http health checking request Signed-off-by: lemonlinger <[email protected]> * fix broken tests Signed-off-by: lemonlinger <[email protected]> * fix health-check test case in xds translation Signed-off-by: lemonlinger <[email protected]> * Simplify code and concise comments Signed-off-by: lemonlinger <[email protected]> --------- Signed-off-by: lemonlinger <[email protected]> (cherry picked from commit 8f450a9) Signed-off-by: Arko Dasgupta <[email protected]>

* Run certgen when upgrading (#2934) run certgen when upgrading Signed-off-by: huabing zhao <[email protected]> (cherry picked from commit 62ecf15) Signed-off-by: Arko Dasgupta <[email protected]> * Fix: nil secret in resourceversiontable (#2982) * fix nil secret in resourceversiontable Signed-off-by: huabing zhao <[email protected]> * check secrets in the xds result Signed-off-by: huabing zhao <[email protected]> --------- Signed-off-by: huabing zhao <[email protected]> (cherry picked from commit e880439) Signed-off-by: Arko Dasgupta <[email protected]> * fix: add missing http filters to the http filter chain (#2970) * fix: add missing http filters to the http filter chain Signed-off-by: huabing zhao <[email protected]> * refactor Signed-off-by: huabing zhao <[email protected]> * fix lint Signed-off-by: huabing zhao <[email protected]> * add comments Signed-off-by: huabing zhao <[email protected]> * remove refactor Signed-off-by: huabing zhao <[email protected]> * remove refactor Signed-off-by: huabing zhao <[email protected]> * fix gen Signed-off-by: huabing zhao <[email protected]> * fix lint Signed-off-by: Huabing Zhao <[email protected]> --------- Signed-off-by: huabing zhao <[email protected]> Signed-off-by: Huabing Zhao <[email protected]> (cherry picked from commit f699edf) Signed-off-by: Arko Dasgupta <[email protected]> * fix: allow websockets in url rewrite (#3022) allow websockets in url rewrite Signed-off-by: Jesse Haka <[email protected]> Co-authored-by: zirain <[email protected]> (cherry picked from commit 3d51933) Signed-off-by: Arko Dasgupta <[email protected]> * Set host for http health checker explicitly to avoid using the cluster name as host header for http health checking request. (#3057) * Set host for http health checker explictly to avoid using the cluster name as host header for http health checking request Signed-off-by: lemonlinger <[email protected]> * fix broken tests Signed-off-by: lemonlinger <[email protected]> * fix health-check test case in xds translation Signed-off-by: lemonlinger <[email protected]> * Simplify code and concise comments Signed-off-by: lemonlinger <[email protected]> --------- Signed-off-by: lemonlinger <[email protected]> (cherry picked from commit 8f450a9) Signed-off-by: Arko Dasgupta <[email protected]> * fix: do not create infra resources when missing translated listeners (#3043) * fix: do not create infra resources when missing translated listeners Signed-off-by: Karol Szwaj <[email protected]> * remove empty line Signed-off-by: Karol Szwaj <[email protected]> * skip infra creation on empty listeners and log it Signed-off-by: Karol Szwaj <[email protected]> --------- Signed-off-by: Karol Szwaj <[email protected]> (cherry picked from commit 36d7141) Signed-off-by: Arko Dasgupta <[email protected]> * Fix: double slashes in redirect URL (#2998) * fix: double trailing splashs in redirect URL Signed-off-by: huabing zhao <[email protected]> * add e2e tests Signed-off-by: huabing zhao <[email protected]> * fix lint Signed-off-by: huabing zhao <[email protected]> * fix test Signed-off-by: huabing zhao <[email protected]> * fix test Signed-off-by: huabing zhao <[email protected]> * fix test Signed-off-by: huabing zhao <[email protected]> * fix test Signed-off-by: huabing zhao <[email protected]> * add e2e tests Signed-off-by: huabing zhao <[email protected]> * fix test Signed-off-by: huabing zhao <[email protected]> * revert Signed-off-by: huabing zhao <[email protected]> * use regex rewrite to generate the redirect url Signed-off-by: huabing zhao <[email protected]> * use regex rewrite to generate the redirect url Signed-off-by: huabing zhao <[email protected]> * use regex rewrite to generate the redirect url Signed-off-by: huabing zhao <[email protected]> * remove comments Signed-off-by: huabing zhao <[email protected]> * extract method Signed-off-by: huabing zhao <[email protected]> * address comments Signed-off-by: huabing zhao <[email protected]> --------- Signed-off-by: huabing zhao <[email protected]> (cherry picked from commit ceb697f) Signed-off-by: Arko Dasgupta <[email protected]> * fix: Allow Policy to attach to multiple http listeners (#2967) * Fixing the clienttrafficpolicy validation. Signed-off-by: Lior Okman <[email protected]> * Make SecurityPolicy validate correctly. Signed-off-by: Lior Okman <[email protected]> * Reverted the SecurityPolicy validation - handled differently via another feature. Signed-off-by: Lior Okman <[email protected]> * Updated the tests to reflect that this validation isn't required for SecurityPolicy Signed-off-by: Lior Okman <[email protected]> * Added some comments to explain the validation being performed. Signed-off-by: Lior Okman <[email protected]> * Updated the error message as requested in the review. Signed-off-by: Lior Okman <[email protected]> --------- Signed-off-by: Lior Okman <[email protected]> (cherry picked from commit f9409e4) Signed-off-by: Arko Dasgupta <[email protected]> * fix: set path prefix for http ext auth service (#3018) Signed-off-by: huabing zhao <[email protected]> (cherry picked from commit 2882b7c) Signed-off-by: Arko Dasgupta <[email protected]> * Change route sorting order to Exact > RegularExpression > PathPrefix (#2579) * Change route sorting order to Exact > RegularExpression > PathPrefix kubernetes-sigs/gateway-api#1770 kubernetes-sigs/gateway-api#1855 Signed-off-by: Stéphane Cottin <[email protected]> (cherry picked from commit 11f56fd) Signed-off-by: Arko Dasgupta <[email protected]> * fix: infraIR duplicate port translation for merged gateways (#3061) * fix: duplicate port translation for merged gateways Signed-off-by: Karol Szwaj <[email protected]> * refactor to map Signed-off-by: Karol Szwaj <[email protected]> * rename map Signed-off-by: Karol Szwaj <[email protected]> * add seperate testcase Signed-off-by: Karol Szwaj <[email protected]> --------- Signed-off-by: Karol Szwaj <[email protected]> (cherry picked from commit 29946b0) Signed-off-by: Arko Dasgupta <[email protected]> * translator: set SpawnUpstreamSpan to true (#3102) * translator: set SpawnUpstreamSpan to true Signed-off-by: zirain <[email protected]> * update Signed-off-by: zirain <[email protected]> --------- Signed-off-by: zirain <[email protected]> (cherry picked from commit 635ebfc) Signed-off-by: Arko Dasgupta <[email protected]> * fix: rate limit doesn't work with two(and more) listeners (#3085) * fix: rate limit doesn't work with two listeners Signed-off-by: huabing zhao <[email protected]> * add e2e test for rate limit on multiple listeners Signed-off-by: huabing zhao <[email protected]> * address comments Signed-off-by: huabing zhao <[email protected]> --------- Signed-off-by: huabing zhao <[email protected]> Co-authored-by: Xunzhuo <[email protected]> (cherry picked from commit a5bedbc) Signed-off-by: Arko Dasgupta <[email protected]> * rerun make testdata Signed-off-by: Arko Dasgupta <[email protected]> --------- Signed-off-by: huabing zhao <[email protected]> Signed-off-by: Arko Dasgupta <[email protected]> Signed-off-by: Huabing Zhao <[email protected]> Signed-off-by: Jesse Haka <[email protected]> Signed-off-by: lemonlinger <[email protected]> Signed-off-by: Karol Szwaj <[email protected]> Signed-off-by: Lior Okman <[email protected]> Signed-off-by: Stéphane Cottin <[email protected]> Signed-off-by: zirain <[email protected]> Co-authored-by: Huabing Zhao <[email protected]> Co-authored-by: Jesse Haka <[email protected]> Co-authored-by: zirain <[email protected]> Co-authored-by: Meng <[email protected]> Co-authored-by: Karol Szwaj <[email protected]> Co-authored-by: Lior Okman <[email protected]> Co-authored-by: vixns <[email protected]> Co-authored-by: Xunzhuo <[email protected]>

lemonlinger requested a review from a team as a code owner March 29, 2024 08:57

lemonlinger added 3 commits April 1, 2024 14:22

Set host for http health checker explictly to avoid using the cluster…

a7b4696

… name as host header for http health checking request Signed-off-by: lemonlinger <[email protected]>

fix broken tests

760e2fe

Signed-off-by: lemonlinger <[email protected]>

fix health-check test case in xds translation

55dcc9a

Signed-off-by: lemonlinger <[email protected]>

lemonlinger force-pushed the fix/invalid-hostname-in-health-check branch from 7018733 to 55dcc9a Compare April 1, 2024 06:22

arkodg reviewed Apr 1, 2024

View reviewed changes

Simplify code and concise comments

1d1b369

Signed-off-by: lemonlinger <[email protected]>

arkodg reviewed Apr 1, 2024

View reviewed changes

arkodg approved these changes Apr 1, 2024

View reviewed changes

arkodg added the cherrypick/release-v1.0 label Apr 1, 2024

arkodg requested review from a team April 1, 2024 18:42

zirain approved these changes Apr 2, 2024

View reviewed changes

zirain merged commit 8f450a9 into envoyproxy:main Apr 2, 2024
21 checks passed

lemonlinger deleted the fix/invalid-hostname-in-health-check branch April 2, 2024 02:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set host for http health checker explicitly to avoid using the cluster name as host header for http health checking request. #3057

Set host for http health checker explicitly to avoid using the cluster name as host header for http health checking request. #3057

lemonlinger commented Mar 29, 2024

codecov bot commented Mar 30, 2024 •

edited

Loading

lemonlinger commented Apr 1, 2024

arkodg Apr 1, 2024

lemonlinger Apr 1, 2024 •

edited

Loading

arkodg Apr 1, 2024

arkodg Apr 1, 2024

lemonlinger Apr 1, 2024

arkodg Apr 1, 2024

lemonlinger Apr 1, 2024

arkodg left a comment

zirain commented Apr 1, 2024

lemonlinger commented Apr 1, 2024

Set host for http health checker explicitly to avoid using the cluster name as host header for http health checking request. #3057

Set host for http health checker explicitly to avoid using the cluster name as host header for http health checking request. #3057

Conversation

lemonlinger commented Mar 29, 2024

codecov bot commented Mar 30, 2024 • edited Loading

Codecov Report

lemonlinger commented Apr 1, 2024

arkodg Apr 1, 2024

Choose a reason for hiding this comment

lemonlinger Apr 1, 2024 • edited Loading

Choose a reason for hiding this comment

arkodg Apr 1, 2024

Choose a reason for hiding this comment

arkodg Apr 1, 2024

Choose a reason for hiding this comment

lemonlinger Apr 1, 2024

Choose a reason for hiding this comment

arkodg Apr 1, 2024

Choose a reason for hiding this comment

lemonlinger Apr 1, 2024

Choose a reason for hiding this comment

arkodg left a comment

Choose a reason for hiding this comment

zirain commented Apr 1, 2024

lemonlinger commented Apr 1, 2024

codecov bot commented Mar 30, 2024 •

edited

Loading

lemonlinger Apr 1, 2024 •

edited

Loading