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

New oxy with gorilla for websocket with integration tests #1896

Merged
merged 1 commit into from Jul 27, 2017

Conversation

juliens
Copy link
Member

@juliens juliens commented Jul 25, 2017

  • Use the new websocket implementation in oxy, based on gorilla/websocket
  • Add integration tests for verify websocket

fixes #1858

In order to reproduce the issue:

docker run -d --name="home-assistant" \
    --restart=always \
    -v $PWD/config:/config \
    -v /etc/localtime:/etc/localtime:ro \
    -p 8123:8123 \
    --label traefik.port=8123 \
    --label traefik.frontend.rule="Host:127.0.0.1" \
    homeassistant/home-assistant
./traefik --docker=true

Then just try to view the homepage (http://127.0.0.1)

Copy link
Member

@ldez ldez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👏 👏 👏

Copy link
Member

@emilevauge emilevauge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks ☺️
LGTM

Copy link
Contributor

@dtomcej dtomcej left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

:shipit:

@juliens juliens merged commit 888e6dc into traefik:v1.3 Jul 27, 2017
@ldez ldez mentioned this pull request Jul 28, 2017
@juliens juliens deleted the fix-issue-1858 branch July 31, 2017 08:20
@@ -335,6 +303,7 @@ func (f *websocketForwarder) copyRequest(req *http.Request, u *url.URL) (outReq

outReq.URL = utils.CopyURL(req.URL)
outReq.URL.Scheme = u.Scheme
outReq.URL.Path = outReq.RequestURI
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, after upgrading to latest traefik (1.3.5) i noticed that query parameter sent in websock requests ends up URL-encoded on the backend server. Had a quick look and this line looks suspicious.

Websocket request:

curl -H "Connection: Upgrade" -H "Upgrade: websocket" -H "Sec-WebSocket-Version: 13" -H "Sec-WebSocket-Key: dummy" http://host/events?token=123

I see a requests to the backend that looks like this: (dumping with tcpdump -A)

GET /events%3Ftoken=123 HTTP/1.1     <-- notice the encoded "?"
Host: host
User-Agent: curl/7.54.0
Accept: */*
Connection: Upgrade
Sec-WebSocket-Key: 92tmVWFXOLFebYZc98UaAw==
Sec-WebSocket-Version: 13
Upgrade: websocket
X-Traefik-Reqid: 82

A normal GET request:

curl http://host/events?token=123

I see a request to the backend that looks like this:

GET /events?token=123 HTTP/1.1
Host: host
User-Agent: curl/7.54.0
Accept: */*
X-Forwarded-For: 1.2.3.4
X-Forwarded-Host: host
X-Forwarded-Proto: https
X-Forwarded-Server: 6ea428775acc
X-Traefik-Reqid: 87
Accept-Encoding: gzip

Now I also noticed that a lot of X-Forwarded headers are missing for the websocket request.

Copy link
Member

@ldez ldez Aug 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this line fixed the encoding pb. wrong PR 😄

Copy link
Member Author

@juliens juliens Aug 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx for your report, the problem is already fix, just need to be merge and release
containous/oxy#22

For the x-forwarded, feel free to propose a PR (on github.com/containous/oxy).
Thanks

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@juliens Ok! thanks for fix and info!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants