feat: introduce EndpointHostname in BTP to set hostname field in Endpoint

[zhuoyang]

What type of PR is this?

Introduce EndpointHostname in BTP, to set hostname field in Endpoint

What this PR does / why we need it:
Currently HTTPURLRewriteFilter hostname.type: Backend doesn't work for backend pointing to k8s service, this PR will introduce the functionality to add k8s service FQDN to endpoint's hostname field.

Which issue(s) this PR fixes:

Fixes #7984

Release Notes: Yes

[netlify]

✅ Deploy Preview for cerulean-figolla-1f9435 ready!

Name	Link
🔨 Latest commit	5593590
🔍 Latest deploy log	https://app.netlify.com/projects/cerulean-figolla-1f9435/deploys/6a4ca9914e65720008704806
😎 Deploy Preview	https://deploy-preview-8606--cerulean-figolla-1f9435.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[zhuoyang]

@guydc can you please take a look at this PR?

[codecov]

Codecov Report

❌ Patch coverage is 92.30769% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.38%. Comparing base (c7e21fa) to head (d85af66).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/gatewayapi/route.go	94.28%	1 Missing and 1 partial ⚠️
internal/gatewayapi/runner/runner.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8606      +/-   ##
==========================================
+ Coverage   74.33%   74.38%   +0.05%     
==========================================
  Files         244      244              
  Lines       38774    38808      +34     
==========================================
+ Hits        28823    28868      +45     
+ Misses       7953     7944       -9     
+ Partials     1998     1996       -2     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[zhuoyang]

/retest

[zhuoyang]

can someone approve my test run again?

[zhuoyang]

i think the test is okay now, just some flaky e2e test (?)

[guydc]

Hi @zhuoyang ! Thanks for picking this up.
My main comments are:

• There's a possible behavior change hidden here. I'm concerned that this hostname would now be propagated into active healthchecks if no explicit HC hostname override is configured. I'm not sure that the URL rewrite filter for regular traffic justifies an implicit change for HCs as well. In the Backend resource, users essentially opt-in to use a specific hostname for that backend, here it becomes implicit. Maybe we need some knob to control that. cc @envoyproxy/gateway-maintainers .
• Prefer to see the hostname being built in the same codepath where it's being built for backends, meaning NewDestEndpoint rather than post endpoint creation.
• Prefer to avoid reliance on resource metadata to built the service's FQDN. Metadata should not be used, ideally, as input to translation.
• Prefer to avoid the EG restart mid-test. Can't we just rely on a default cluster.local domain here? Alternatively, set a. default value in the e2e test EG config?

[zhuoyang]

Thanks for the feedback @guydc!

Maybe we need some knob to control that.

Since this is more of a HTTPRoute/Envoy Cluster level change rather than rule level change, maybe we should expose this under a different settings? Currently the hostname will be added to the endpoint if one of the rule has hostname.type set to Backend

Prefer to see the hostname being built in the same codepath where it's being built for backends, meaning NewDestEndpoint rather than post endpoint creation.

If we were to use NewDestEndpoint to build the hostname, we would need to add a new parameter in processServiceDestinationSetting and getIREndpointsFromEndpointSlices which will introduce changes in few other places, is that acceptable?

[guydc]

Since this is more of a HTTPRoute/Envoy Cluster level change rather than rule level change, maybe we should expose this under a different settings? Currently the hostname will be added to the endpoint if one of the rule has hostname.type set to Backend

Good point. Since the scope of the change is indeed wider than just traffic routed through, maybe this warrants a knob in in BTP that determine the "hostname" value:

• default (backwards-compatible): No hostname
• K8s Service name
• Static value provided by user
  Need some other maintainers to chime in here. Will need to update docs to reflect the existence of this default and its impact on various features.

If we were to use NewDestEndpoint to build the hostname, we would need to add a new parameter in processServiceDestinationSetting and getIREndpointsFromEndpointSlices which will introduce changes in few other places, is that acceptable?

Maybe we can add a flag to IR like "useHostname", always true for backend with hostname, only true for service in some cases (maybe relates to previous point). XDS would only consume the hostname in some cases.

[zhuoyang]

alternatively, I think we could just not utilize auto_host_rewrite for this case, and just rewrite the host manually
this would introduce less change and side effect while retaining the same behavior

[guydc]

alternatively, I think we could just not utilize auto_host_rewrite for this case, and just rewrite the host manually
this would introduce less change and side effect while retaining the same behavior

Would that work in case there are multiple backendrefs to different services? In that case, only the final endpoint selected by the cluster would hold the correct hostname.

[zhuoyang]

Would that work in case there are multiple backendrefs to different services?

I didn't think of this case, yeah I don't think it would work.

[jukie]

Should this stay scoped to route rewrite instead of mutating DestinationEndpoint.Hostname?

[zhuoyang]

@guydc @jukie I've made changes to the PR to introduce the knob in BTP, let me know what you folks think.
I've not opted for adding the flag in IR, because it seems like it will introduce more changes.

[zhuoyang]

gentle bump on this

[github-actions]

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. Please feel free to give a status update now, ping for review, when it's ready. Thank you for your contributions!

[zhuoyang]

@guydc @jukie can we reopen this?

[arkodg]

doesnt the HTTPRouteFilter or Backend resource solve this use case