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

API proxy strips URL query params when proxying websocket connections #77142

Open
thediveo opened this issue Apr 26, 2019 · 52 comments
Open

API proxy strips URL query params when proxying websocket connections #77142

thediveo opened this issue Apr 26, 2019 · 52 comments
Assignees
Labels
sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery.

Comments

@thediveo
Copy link

thediveo commented Apr 26, 2019

Tested on Kubernetes 1.14 (minikube 1.0). To reproduce:

  1. deploy pod with a websocket server running:

    kubectl run wsserver --generator=run-pod/v1 --rm -i --tty --image ubuntu:disco -- bash -c \
      "apt-get update && apt-get install -y wget && \
      wget https://github.com/vi/websocat/releases/download/v1.4.0/websocat_1.4.0_ssl1.1_amd64.deb && \
      dpkg -i webso*.deb && \
      websocat -vv -s 0.0.0.0:8000"
  2. in host shell outside minikube run the following websocat command to connect to the websocket server in your wsserver pod:

    websocat --binary --ws-c-uri=wss://192.168.99.100:8443/api/v1/namespaces/default/pods/wsserver:8000/proxy/test?foo=what - ws-c:cmd:'socat - ssl:192.168.99.100:8443,verify=0,cafile=$HOME/.minikube/ca.crt,cert=$HOME/.minikube/client.crt,key=$HOME/.minikube/client.key'
  3. check the output of the server-side websocat and notice that the query params are missing: [DEBUG websocat::ws_server_peer] Incoming { version: Http11, subject: (Get, AbsolutePath("/test")), headers: ...

  4. exec into your pod with the websocket server, and issue a local websocket connect.

    kubectl exec wsserver websocat ws://localhost:8000/test?foo=what
  5. check output of the service-side websocat, now the query params are present as to be expected: [DEBUG websocat::ws_server_peer] Incoming { version: Http11, subject: (Get, AbsolutePath("/test?foo=what")), headers: ...

(note: updated instructions to be much simpler now)

@k8s-ci-robot k8s-ci-robot added the needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. label Apr 26, 2019
@thediveo
Copy link
Author

/sig api-machinery

@k8s-ci-robot k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Apr 26, 2019
@fedebongio
Copy link
Contributor

/assign @cheftako

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 28, 2019
@fejta-bot
Copy link

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Aug 27, 2019
@thediveo
Copy link
Author

/remove-lifecycle rotten

@k8s-ci-robot k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Aug 28, 2019
@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 26, 2019
@thediveo
Copy link
Author

/remove-lifecycle stale

This is still an issue even with 1.16.

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 26, 2019
@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 24, 2020
@thediveo
Copy link
Author

/remove-lifecycle stale

Still an issue.

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 24, 2020
@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 24, 2020
@thediveo
Copy link
Author

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 25, 2020
@tarokkk
Copy link

tarokkk commented Jun 11, 2020

Is there anything we can help about this issue?

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 9, 2020
@thediveo
Copy link
Author

thediveo commented Sep 9, 2020

/remove-lifecycle stale

@thediveo
Copy link
Author

To reproduce with a recent version of Minikube/k8s/Ubuntu, here the updated instructions:

kubectl run wsserver --rm -i --tty --image ubuntu:jammy -- bash -c   "apt-get update && apt-get install -y wget && \
  wget https://github.com/vi/websocat/releases/download/v1.9.0/websocat_linux64 && \
  chmod u+x ./websocat_linux64 && ./websocat_linux64 -vv -s 0.0.0.0:8000"

See how the socat ws server doesn't get the query params passed:

./websocat --binary --ws-c-uri=wss://192.168.49.2:8443/api/v1/namespaces/default/pods/wsserver:8000/proxy/test?foo=what - ws-c:cmd:'socat - ssl:192.168.49.2:8443,verify=0,cafile=$HOME/.minikube/ca.crt,cert=$HOME/.minikube/profiles/minikube/client.crt,key=$HOME/.minikube/profiles/minikube/client.key'

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 22, 2022
@thediveo
Copy link
Author

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 22, 2022
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue as fresh with /remove-lifecycle stale
  • Close this issue with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 22, 2023
@thediveo
Copy link
Author

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 22, 2023
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue as fresh with /remove-lifecycle stale
  • Close this issue with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 20, 2023
@thediveo
Copy link
Author

/remove-lifecycle stale

seriously, this bug is not going to puff off into thin air because some bot bots around.

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 20, 2023
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue as fresh with /remove-lifecycle stale
  • Close this issue with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 23, 2024
@thediveo
Copy link
Author

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 23, 2024
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue as fresh with /remove-lifecycle stale
  • Close this issue with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 22, 2024
@thediveo
Copy link
Author

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 22, 2024
@aojea
Copy link
Member

aojea commented Apr 22, 2024

one naive question, why is the query params relevant if AFAIK they are not used in any place?

@thediveo
Copy link
Author

Finally I can give more background information since Siemens opened its Edgeshark project for capturing container traffic from stand-alone container hosts as well as k8s nodes.

Captured packets are streamed via a websocket pcapng stream. In order to establish the correct stream, some parameters need to be passed to the our packet capture streaming service ("packetflix") on the host where the target container is located. In case of k8s the plugin connecting Wireshark with the node's streaming service goes through the k8s API, using its built-in proxy functionality.

And as we found out some years ago and reported here, the k8s API proxy verb strips URL query params instead of correctly forwarding them to the service.

@aojea
Copy link
Member

aojea commented Apr 22, 2024

Ok, so this only happens with websockets urls 🤔 , normal request are working ok

kubectl run webapp --command --image=registry.k8s.io/e2e-test-images/agnhost:2.47 -- /agnhost netexec --http-port=8080

kubectl get --raw http://1/api/v1/namespaces/default/pods/webapp:8080/proxy/echo?msg=echoed_msg -v7
I0422 13:17:39.333422 3767678 loader.go:373] Config loaded from file:  /usr/local/google/home/aojea/.kube/config
I0422 13:17:39.333989 3767678 round_trippers.go:463] GET https://127.0.0.1:34563/api/v1/namespaces/default/pods/webapp:8080/proxy/echo?msg=echoed_msg
I0422 13:17:39.334006 3767678 round_trippers.go:469] Request Headers:
I0422 13:17:39.334015 3767678 round_trippers.go:473]     Accept: application/json, */*
I0422 13:17:39.334023 3767678 round_trippers.go:473]     User-Agent: kubectl/v1.26.15 (linux/amd64) kubernetes/aae4c29
I0422 13:17:39.336562 3767678 round_trippers.go:574] Response Status: 200 OK in 2 milliseconds
echoed_msg

based on this #120290 it seems it get lost during the upgrade

Is the apiserver doing the upgrade to websockets or is passing it through?

cc: @liggitt @seans3

@liggitt
Copy link
Member

liggitt commented Apr 30, 2024

The proxy handlers are set up here:

  • pod
    location.Path = net.JoinPreservingTrailingSlash(location.Path, proxyOpts.Path)
    // Return a proxy handler that uses the desired transport, wrapped with additional proxy handling (to get URL rewriting, X-Forwarded-* headers, etc)
    return newThrottledUpgradeAwareProxyHandler(location, transport, true, false, responder), nil
  • service
    location.Path = net.JoinPreservingTrailingSlash(location.Path, proxyOpts.Path)
    // Return a proxy handler that uses the desired transport, wrapped with additional proxy handling (to get URL rewriting, X-Forwarded-* headers, etc)
    return newThrottledUpgradeAwareProxyHandler(location, transport, true, false, responder), nil
  • node
    location.Path = net.JoinPreservingTrailingSlash(location.Path, proxyOpts.Path)
    // Return a proxy handler that uses the desired transport, wrapped with additional proxy handling (to get URL rewriting, X-Forwarded-* headers, etc)
    return newThrottledUpgradeAwareProxyHandler(location, transport, true, false, responder), nil

Those only preserve the path from the incoming request, construct the target location within the kubelet handler, and construct a UpgradeAwareHandler with AppendLocationPath and UseRequestLocation set to false.

Handling for non-upgrade requests looks like this and explicitly copies the RawQuery from the request:

Handling for upgrade requests looks like this and does not use any part of the request location unless UseRequestLocation is true:

  • location := *h.Location
    if h.UseRequestLocation {
    location = *req.URL
    location.Scheme = h.Location.Scheme
    location.Host = h.Location.Host
    if h.AppendLocationPath {
    location.Path = singleJoiningSlash(h.Location.Path, location.Path)
    }
    }
    clone := utilnet.CloneRequest(req)
    // Only append X-Forwarded-For in the upgrade path, since httputil.NewSingleHostReverseProxy
    // handles this in the non-upgrade path.
    utilnet.AppendForwardedForHeader(clone)
    klog.V(6).Infof("Connecting to backend proxy (direct dial) %s\n Headers: %v", &location, clone.Header)
    if h.UseLocationHost {
    clone.Host = h.Location.Host
    }
    clone.URL = &location

That's why query params get dropped for upgraded requests when proxying through the API server.

@liggitt
Copy link
Member

liggitt commented Apr 30, 2024

So... now that we know why the issue occurs ... what can we do about it?

It would be easy enough to just modify the upgradeaware.go upgrade-handling path to copy over RawQuery the same way, but we'd have to be exceptionally careful about the following things:

  • are there query params already being set in h.location by the upgradeaware proxy constructor that will be stomped by whatever is in the request?
  • is using parts of the incoming request even when UseRequestLocation is false breaking expectations of the caller? does that expose the backend to manipulation by the caller getting control over query params?

Another possibility would be to copy over the query from the request to the location before constructing the upgradeawareproxy (in the core//rest/ handlers). That appeals to me more, since we can reason about those and know none of them are setting query params, and we'd avoid changing low-level upgradeaware.go behavior for all callers.

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue as fresh with /remove-lifecycle stale
  • Close this issue with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 29, 2024
@thediveo
Copy link
Author

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery.
Projects
None yet
Development

No branches or pull requests