Set host for http health checker explicitly to avoid using the cluster name as host header for http health checking request. by lemonlinger · Pull Request #3057 · envoyproxy/gateway

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 <lemonlinger@gmail.com>

Signed-off-by: lemonlinger <lemonlinger@gmail.com>

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

/retest

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

internal/gatewayapi/backendtrafficpolicy.go

 					}
+
+					// 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 <lemonlinger@gmail.com>

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

internal/ir/xds.go

 }

+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 <lemonlinger@gmail.com> * fix broken tests Signed-off-by: lemonlinger <lemonlinger@gmail.com> * fix health-check test case in xds translation Signed-off-by: lemonlinger <lemonlinger@gmail.com> * Simplify code and concise comments Signed-off-by: lemonlinger <lemonlinger@gmail.com> --------- Signed-off-by: lemonlinger <lemonlinger@gmail.com> (cherry picked from commit 8f450a9) Signed-off-by: Arko Dasgupta <arko@tetrate.io>

* Run certgen when upgrading (#2934) run certgen when upgrading Signed-off-by: huabing zhao <zhaohuabing@gmail.com> (cherry picked from commit 62ecf15) Signed-off-by: Arko Dasgupta <arko@tetrate.io> * Fix: nil secret in resourceversiontable (#2982) * fix nil secret in resourceversiontable Signed-off-by: huabing zhao <zhaohuabing@gmail.com> * check secrets in the xds result Signed-off-by: huabing zhao <zhaohuabing@gmail.com> --------- Signed-off-by: huabing zhao <zhaohuabing@gmail.com> (cherry picked from commit e880439) Signed-off-by: Arko Dasgupta <arko@tetrate.io> * 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 <zhaohuabing@gmail.com> * refactor Signed-off-by: huabing zhao <zhaohuabing@gmail.com> * fix lint Signed-off-by: huabing zhao <zhaohuabing@gmail.com> * add comments Signed-off-by: huabing zhao <zhaohuabing@gmail.com> * remove refactor Signed-off-by: huabing zhao <zhaohuabing@gmail.com> * remove refactor Signed-off-by: huabing zhao <zhaohuabing@gmail.com> * fix gen Signed-off-by: huabing zhao <zhaohuabing@gmail.com> * fix lint Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> --------- Signed-off-by: huabing zhao <zhaohuabing@gmail.com> Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> (cherry picked from commit f699edf) Signed-off-by: Arko Dasgupta <arko@tetrate.io> * fix: allow websockets in url rewrite (#3022) allow websockets in url rewrite Signed-off-by: Jesse Haka <haka.jesse@gmail.com> Co-authored-by: zirain <zirain2009@gmail.com> (cherry picked from commit 3d51933) Signed-off-by: Arko Dasgupta <arko@tetrate.io> * 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 <lemonlinger@gmail.com> * fix broken tests Signed-off-by: lemonlinger <lemonlinger@gmail.com> * fix health-check test case in xds translation Signed-off-by: lemonlinger <lemonlinger@gmail.com> * Simplify code and concise comments Signed-off-by: lemonlinger <lemonlinger@gmail.com> --------- Signed-off-by: lemonlinger <lemonlinger@gmail.com> (cherry picked from commit 8f450a9) Signed-off-by: Arko Dasgupta <arko@tetrate.io> * 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 <karol.szwaj@gmail.com> * remove empty line Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com> * skip infra creation on empty listeners and log it Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com> --------- Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com> (cherry picked from commit 36d7141) Signed-off-by: Arko Dasgupta <arko@tetrate.io> * Fix: double slashes in redirect URL (#2998) * fix: double trailing splashs in redirect URL Signed-off-by: huabing zhao <zhaohuabing@gmail.com> * add e2e tests Signed-off-by: huabing zhao <zhaohuabing@gmail.com> * fix lint Signed-off-by: huabing zhao <zhaohuabing@gmail.com> * fix test Signed-off-by: huabing zhao <zhaohuabing@gmail.com> * fix test Signed-off-by: huabing zhao <zhaohuabing@gmail.com> * fix test Signed-off-by: huabing zhao <zhaohuabing@gmail.com> * fix test Signed-off-by: huabing zhao <zhaohuabing@gmail.com> * add e2e tests Signed-off-by: huabing zhao <zhaohuabing@gmail.com> * fix test Signed-off-by: huabing zhao <zhaohuabing@gmail.com> * revert Signed-off-by: huabing zhao <zhaohuabing@gmail.com> * use regex rewrite to generate the redirect url Signed-off-by: huabing zhao <zhaohuabing@gmail.com> * use regex rewrite to generate the redirect url Signed-off-by: huabing zhao <zhaohuabing@gmail.com> * use regex rewrite to generate the redirect url Signed-off-by: huabing zhao <zhaohuabing@gmail.com> * remove comments Signed-off-by: huabing zhao <zhaohuabing@gmail.com> * extract method Signed-off-by: huabing zhao <zhaohuabing@gmail.com> * address comments Signed-off-by: huabing zhao <zhaohuabing@gmail.com> --------- Signed-off-by: huabing zhao <zhaohuabing@gmail.com> (cherry picked from commit ceb697f) Signed-off-by: Arko Dasgupta <arko@tetrate.io> * fix: Allow Policy to attach to multiple http listeners (#2967) * Fixing the clienttrafficpolicy validation. Signed-off-by: Lior Okman <lior.okman@sap.com> * Make SecurityPolicy validate correctly. Signed-off-by: Lior Okman <lior.okman@sap.com> * Reverted the SecurityPolicy validation - handled differently via another feature. Signed-off-by: Lior Okman <lior.okman@sap.com> * Updated the tests to reflect that this validation isn't required for SecurityPolicy Signed-off-by: Lior Okman <lior.okman@sap.com> * Added some comments to explain the validation being performed. Signed-off-by: Lior Okman <lior.okman@sap.com> * Updated the error message as requested in the review. Signed-off-by: Lior Okman <lior.okman@sap.com> --------- Signed-off-by: Lior Okman <lior.okman@sap.com> (cherry picked from commit f9409e4) Signed-off-by: Arko Dasgupta <arko@tetrate.io> * fix: set path prefix for http ext auth service (#3018) Signed-off-by: huabing zhao <zhaohuabing@gmail.com> (cherry picked from commit 2882b7c) Signed-off-by: Arko Dasgupta <arko@tetrate.io> * 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 <stephane.cottin@vixns.com> (cherry picked from commit 11f56fd) Signed-off-by: Arko Dasgupta <arko@tetrate.io> * fix: infraIR duplicate port translation for merged gateways (#3061) * fix: duplicate port translation for merged gateways Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com> * refactor to map Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com> * rename map Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com> * add seperate testcase Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com> --------- Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com> (cherry picked from commit 29946b0) Signed-off-by: Arko Dasgupta <arko@tetrate.io> * translator: set SpawnUpstreamSpan to true (#3102) * translator: set SpawnUpstreamSpan to true Signed-off-by: zirain <zirain2009@gmail.com> * update Signed-off-by: zirain <zirain2009@gmail.com> --------- Signed-off-by: zirain <zirain2009@gmail.com> (cherry picked from commit 635ebfc) Signed-off-by: Arko Dasgupta <arko@tetrate.io> * 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 <zhaohuabing@gmail.com> * add e2e test for rate limit on multiple listeners Signed-off-by: huabing zhao <zhaohuabing@gmail.com> * address comments Signed-off-by: huabing zhao <zhaohuabing@gmail.com> --------- Signed-off-by: huabing zhao <zhaohuabing@gmail.com> Co-authored-by: Xunzhuo <bitliu@tencent.com> (cherry picked from commit a5bedbc) Signed-off-by: Arko Dasgupta <arko@tetrate.io> * rerun make testdata Signed-off-by: Arko Dasgupta <arko@tetrate.io> --------- Signed-off-by: huabing zhao <zhaohuabing@gmail.com> Signed-off-by: Arko Dasgupta <arko@tetrate.io> Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> Signed-off-by: Jesse Haka <haka.jesse@gmail.com> Signed-off-by: lemonlinger <lemonlinger@gmail.com> Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com> Signed-off-by: Lior Okman <lior.okman@sap.com> Signed-off-by: Stéphane Cottin <stephane.cottin@vixns.com> Signed-off-by: zirain <zirain2009@gmail.com> Co-authored-by: Huabing Zhao <zhaohuabing@gmail.com> Co-authored-by: Jesse Haka <haka.jesse@gmail.com> Co-authored-by: zirain <zirain2009@gmail.com> Co-authored-by: Meng <lemonlinger@gmail.com> Co-authored-by: Karol Szwaj <karol.szwaj@gmail.com> Co-authored-by: Lior Okman <lior.okman@sap.com> Co-authored-by: vixns <stephane.cottin@vixns.com> Co-authored-by: Xunzhuo <bitliu@tencent.com>

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 <lemonlinger@gmail.com>

fix broken tests

760e2fe

Signed-off-by: lemonlinger <lemonlinger@gmail.com>

fix health-check test case in xds translation

55dcc9a

Signed-off-by: lemonlinger <lemonlinger@gmail.com>

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 <lemonlinger@gmail.com>

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

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
zirain merged 4 commits intoenvoyproxy:mainfrom
lemonlinger:fix/invalid-hostname-in-health-check

lemonlinger commented Mar 29, 2024

Uh oh!

codecov bot commented Mar 30, 2024 •

edited

Loading

Uh oh!

lemonlinger commented Apr 1, 2024

Uh oh!

arkodg Apr 1, 2024

Uh oh!

lemonlinger Apr 1, 2024 •

edited

Loading

Uh oh!

arkodg Apr 1, 2024

Uh oh!

arkodg Apr 1, 2024

Uh oh!

lemonlinger Apr 1, 2024

Uh oh!

arkodg Apr 1, 2024

Uh oh!

lemonlinger Apr 1, 2024

Uh oh!

arkodg left a comment

Uh oh!

zirain commented Apr 1, 2024

Uh oh!

lemonlinger commented Apr 1, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

lemonlinger commented Mar 29, 2024

Uh oh!

codecov bot commented Mar 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

lemonlinger commented Apr 1, 2024

Uh oh!

arkodg Apr 1, 2024

Choose a reason for hiding this comment

Uh oh!

lemonlinger Apr 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arkodg Apr 1, 2024

Choose a reason for hiding this comment

Uh oh!

arkodg Apr 1, 2024

Choose a reason for hiding this comment

Uh oh!

lemonlinger Apr 1, 2024

Choose a reason for hiding this comment

Uh oh!

arkodg Apr 1, 2024

Choose a reason for hiding this comment

Uh oh!

lemonlinger Apr 1, 2024

Choose a reason for hiding this comment

Uh oh!

arkodg left a comment

Choose a reason for hiding this comment

Uh oh!

zirain commented Apr 1, 2024

Uh oh!

lemonlinger commented Apr 1, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Mar 30, 2024 •

edited

Loading

lemonlinger Apr 1, 2024 •

edited

Loading