Skip to content
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

Merged
merged 3 commits into from
May 10, 2024

Conversation

czeslavo
Copy link
Contributor

@czeslavo czeslavo commented Apr 29, 2024

What this PR does / why we need it:

Extends support for URLRewrite filter with the Hostname 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:

  • the CHANGELOG.md release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR

@czeslavo czeslavo self-assigned this Apr 29, 2024
@czeslavo czeslavo added area/gateway-api Relating to upstream Kubernetes SIG Networking Gateway API and removed size/L labels Apr 29, 2024
@czeslavo czeslavo force-pushed the feat/url-rewrite-host branch 3 times, most recently from 73ee4ab to 0896a31 Compare April 29, 2024 16:13
@czeslavo czeslavo force-pushed the feat/url-path-rewrite-expressions branch 3 times, most recently from 794ddfc to 7090f7b Compare May 6, 2024 14:31
Base automatically changed from feat/url-path-rewrite-expressions to main May 6, 2024 20:18
Copy link

codecov bot commented May 7, 2024

Codecov Report

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

Project coverage is 72.3%. Comparing base (641c73c) to head (74bbdba).
Report is 7 commits behind head on main.

Files Patch % Lines
...al/dataplane/translator/subtranslator/httproute.go 91.3% 7 Missing and 3 partials ⚠️
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.
📢 Have feedback on the report? Share it here.

@czeslavo czeslavo marked this pull request as ready for review May 7, 2024 12:09
@czeslavo czeslavo requested a review from a team as a code owner May 7, 2024 12:09
Copy link
Contributor

@rainest rainest left a 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.

internal/dataplane/translator/subtranslator/httproute.go Outdated Show resolved Hide resolved
internal/dataplane/translator/subtranslator/httproute.go Outdated Show resolved Hide resolved
@czeslavo
Copy link
Contributor Author

czeslavo commented May 8, 2024

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.

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?

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.

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.

Please note connection.servername matches the modified host header - connection.servername is how mendhak/docker-http-https-echo indicates SNI server name: https://github.com/mendhak/docker-http-https-echo/pull/2/files):

$ http localhost:8000/host-rewrite
HTTP/1.1 200 OK
Connection: keep-alive
Content-Length: 815
Content-Type: application/json; charset=utf-8
Date: Wed, 08 May 2024 13:04:30 GMT
ETag: W/"32f-3CuRJNRNzt+PJ42KBRr9SMbAfrA"
Via: kong/3.6.0
X-Kong-Proxy-Latency: 1
X-Kong-Request-Id: f24af438844d9e51f9db306ba2e98f49
X-Kong-Upstream-Latency: 47
X-Powered-By: Express

{
    "body": "",
    "clientCertificate": {},
    "connection": {
        "servername": "modified.host"
    },
    "fresh": false,
    "headers": {
        "accept": "*/*",
        "accept-encoding": "gzip, deflate",
        "connection": "keep-alive",
        "host": "modified.host",
        "user-agent": "HTTPie/3.2.2",
        "x-forwarded-for": "127.0.0.1",
        "x-forwarded-host": "localhost",
        "x-forwarded-path": "/host-rewrite",
        "x-forwarded-port": "80",
        "x-forwarded-proto": "http",
        "x-kong-request-id": "f24af438844d9e51f9db306ba2e98f49",
        "x-real-ip": "127.0.0.1"
    },
    "hostname": "localhost",
    "ip": "127.0.0.1",
    "ips": [
        "127.0.0.1"
    ],
    "method": "GET",
    "os": {
        "hostname": "echo-59875b796d-mppfc"
    },
    "path": "/host-rewrite",
    "protocol": "http",
    "query": {},
    "subdomains": [],
    "xhr": false
}

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

Copy link
Contributor

@rainest rainest left a 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.

@czeslavo czeslavo merged commit ce7b038 into main May 10, 2024
38 checks passed
@czeslavo czeslavo deleted the feat/url-rewrite-host branch May 10, 2024 07:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/gateway-api Relating to upstream Kubernetes SIG Networking Gateway API size/XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

experimental conformance - HTTPRouteHostRewrite
2 participants