-
Notifications
You must be signed in to change notification settings - Fork 592
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(httproute): handle URLRewrite filter's hostname rewrite #5952
Conversation
45546f1
to
75adb90
Compare
73ee4ab
to
0896a31
Compare
794ddfc
to
7090f7b
Compare
0896a31
to
a4fcb91
Compare
a4fcb91
to
6f0ac96
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5952 +/- ##
=====================================
Coverage 72.2% 72.3%
=====================================
Files 180 180
Lines 18425 18470 +45
=====================================
+ Hits 13314 13356 +42
- Misses 4117 4122 +5
+ Partials 994 992 -2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit surprised this does apparently work with conformance as a pseudo-integration test. AFAIK using the request transformer to modify Host
usually doesn't quite work as expected. There's no link between the header value and Kong's notion of what the actual upstream host is, and the "proper" host is used for other purposes beyond setting the upstream host header.
Can you run a manual attempt to configure this on an HTTPS HTTPRoute? Does it behave oddly? I expect that the test isn't being run over HTTPS (not sure, but didn't see otherwise on a brief review of upstream code), and that we'll actually hit an issue here because the plugin doesn't change TLS SNI.
Upstream docs don't really touch on SNI handling, but in general you shouldn't ever send requests where the TLS SNI and HTTP Host
header don't match, which is what I think this will do.
In KIC we only support HTTPRoutes with Gateways configured with
I've verified that if we modify the Please note
Manifests: apiVersion: v1
kind: Service
metadata:
name: echo
annotations:
konghq.com/protocol: https
spec:
ports:
- protocol: TCP
name: https
port: 443
targetPort: https
selector:
app: echo
---
apiVersion: apps/v1
kind: Deployment
metadata:
labels:
app: echo
name: echo
spec:
replicas: 1
selector:
matchLabels:
app: echo
template:
metadata:
labels:
app: echo
spec:
containers:
- name: echo
image: mendhak/http-https-echo
ports:
- containerPort: 8443
name: https
resources:
requests:
cpu: 10m
---
apiVersion: gateway.networking.k8s.io/v1
kind: GatewayClass
metadata:
name: kong
annotations:
konghq.com/gatewayclass-unmanaged: "true"
spec:
controllerName: konghq.com/kic-gateway-controller
---
apiVersion: gateway.networking.k8s.io/v1
kind: Gateway
metadata:
name: kong
spec:
gatewayClassName: kong
listeners:
- name: http
protocol: HTTP
port: 80
---
apiVersion: gateway.networking.k8s.io/v1
kind: HTTPRoute
metadata:
name: rewrite-host
spec:
parentRefs:
- name: kong
rules:
- matches:
- path:
type: PathPrefix
value: /host-rewrite
filters:
- type: URLRewrite
urlRewrite:
hostname: "modified.host"
backendRefs:
- name: echo
port: 443 |
f1c2b99
to
83644e9
Compare
54e8fa9
to
74bbdba
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In KIC we only support HTTPRoutes with Gateways configured with TLSModeTerminate. That's the only mode Gateway API documents as supported. By saying HTTPS HTTPRoute - what do you mean exactly?
I mean an HTTPRoute you access over HTTPS.
Terminate doesn't mean we necessarily send HTTP upstream, just that we complete the TLS handshake downstream, rather than sniffing SNI for routing and sending the unmodified TLS downstream to upstream. If the upstream of our Terminate'd route is an HTTPS Service, we initiate a new TLS connection upstream and will need to provide our own SNI.
I understand that's handled by Kong's HTTP client automatically based on the prepared request.
Historically it wouldn't have been just by using request-transformer
, it'd need to be set as part of the Kong service or upstream. So if
I've verified that if we modify the Host header using the plugin as in this implementation, HTTPs Services are called by Kong with a properly modified SNI server name, matching the Host header. I understand that's handled by Kong's HTTP client automatically based on the prepared request.
Not sure what changed there 🤔
I kinda wanna double-check that but don't think that FUD is enough to hold the PR, given that GWAPI docs are kinda murky around handling this in TLS cases anyway. ✔️ this and will dig into the weeds after.
What this PR does / why we need it:
Extends support for
URLRewrite
filter with theHostname
field. Enables Gateway API conformance tests covering this feature.Which issue this PR fixes:
Fixes #3685.
Special notes for your reviewer:
PR Readiness Checklist:
Complete these before marking the PR as
ready to review
:CHANGELOG.md
release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR