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(proxy) supports websockets #1827

Merged
merged 3 commits into from
Dec 3, 2016
Merged

feat(proxy) supports websockets #1827

merged 3 commits into from
Dec 3, 2016

Conversation

subnetmarco
Copy link
Member

Full changelog

  • Proxy support for Websockets.

Issues resolved

Fix #218.

@subnetmarco subnetmarco added this to the 0.10 RC milestone Nov 17, 2016
@subnetmarco subnetmarco force-pushed the feat/websockets branch 3 times, most recently from 4cb47fe to 6030b70 Compare November 17, 2016 05:42
return false, "Supported protocols are HTTP and HTTPS"
parsed_url.scheme = parsed_url.scheme:upper()
if not utils.table_contains(PROTOCOLS, parsed_url.scheme) then
return false, "Supported protocols are: "..table.concat(PROTOCOLS, ", ")
Copy link
Member

Choose a reason for hiding this comment

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

please refrain from using table_contains, it should not even exist. Just create a map.

local PROTOCOLS = { "HTTP", "HTTPS" }
for i, v in ipairs(PROTOCOLS) do PROTOCOLS[v] = i end

local function validate_upstream_url_protocol(value)
  local parsed_url = url.parse(value)
  if parsed_url.scheme and parsed_url.host then
    if not PROTOCOLS[parsed_url.scheme:upper()] then
      return false, "Supported protocols are: "..table.concat(PROTOCOLS, ", ")
    end
  end

  return true
end

Copy link
Member Author

Choose a reason for hiding this comment

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

What's wrong with table_contains or equivalent?

Copy link
Member

Choose a reason for hiding this comment

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

It's iterating over the table, and hence horribly slow. Just create a reverse indexed table for those lookups, so much faster...

@@ -94,6 +99,9 @@ server {
proxy_set_header X-Forwarded-Proto $scheme;
proxy_set_header Host $upstream_host;
proxy_pass_header Server;
proxy_http_version 1.1;
proxy_set_header Upgrade $http_upgrade;
proxy_set_header Connection $connection_upgrade;
Copy link
Member

Choose a reason for hiding this comment

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

Not familiar enough with this to see whether it would have any side effects... @thibaultcha ?

@@ -63,6 +63,11 @@ upstream kong_upstream {
keepalive ${{UPSTREAM_KEEPALIVE}};
}

map $http_upgrade $connection_upgrade {
default upgrade;
'' close;
Copy link
Member

@thibaultcha thibaultcha Nov 17, 2016

Choose a reason for hiding this comment

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

So if I understand correctly, this will set the headers to be sent to upstream as:

  • Connection: upgrade by default
  • Connection: close if the downstream Upgrade header is not set.

If so, this is a no-no. Switching to HTTP/1.1 is a good change (which we should have done a while ago but somehow never did. Properly managing the Connection header is also something that we somehow never implemented (very bad, proxies and gateways should never forward it to upstream services). But this is too rushed and not thought through.

In HTTP/1.1, connections are considered persistent by default. This means that unless a Connection: close header is sent from a client, the server will consider the connection as being persistent. Now, the problem here is that the defined behavior is to send Connection: close as soon as no Upgrade header is received from downstream, which means: no persistent connections, ever.

I believe a better solution could be:

map $http_upgrade $connection_upgrade {
    close close;
    upgrade  upgrade;
    default keep-alive;
}

To be tested of course.

@thibaultcha thibaultcha added the pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. label Nov 17, 2016
@subnetmarco
Copy link
Member Author

subnetmarco commented Nov 18, 2016

@thibaultcha Addressed your concerns with 4061513ff042540d21ec317f1ee1706f1bf895a9

Your proposed solution didn't work. So now it's keep-alive by default, unless it's an Upgrade request.

@subnetmarco subnetmarco added pr/status/needs review and removed pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. labels Nov 18, 2016

-- Websocket
local is_upgrade = ngx.var.http_connection and ngx.var.http_connection:lower() == "upgrade"
ngx.var.upstream_connection = is_upgrade and "upgrade" or "keep-alive"
Copy link
Member

Choose a reason for hiding this comment

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

The logic changed here: it is relying on the value of the Connection header, and not Upgrade anymore. It should rely on Upgrade.

Also, any particular reason for not implementing this in a map anymore?

Following those 2 rules, this could/should be:

map $http_upgrade $upstream_connection {
    default ''; # assumed keepalive connection by default since this is HTTP/1.1
    websocket upgrade; # set 'Connection: Upgrade' if 'Upgrade: WebSocket'
}

I think the upstream Upgrade header should also receive particular attention, being cleared by default, and only forwarded when its value is Upgrade: WebSocket

map $http_upgrade $upstream_upgrade {
    default '';
    websocket websocket;
}

@thibaultcha thibaultcha added pr/ready (but hold merge) No more concerns, but do not merge yet (probably a conflict of interest with another PR or release) and removed pr/status/needs review labels Dec 3, 2016
@thibaultcha thibaultcha merged commit a3c6e16 into next Dec 3, 2016
@thibaultcha thibaultcha deleted the feat/websockets branch December 3, 2016 01:21
@Tieske
Copy link
Member

Tieske commented Dec 3, 2016

@thefosk nice!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/ready (but hold merge) No more concerns, but do not merge yet (probably a conflict of interest with another PR or release)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants