feat(proxy) supports websockets

[subnetmarco]

Full changelog

• Proxy support for Websockets.

Issues resolved

Fix #218.

[Tieske]

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

[subnetmarco]

What's wrong with table_contains or equivalent?

[Tieske]

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

[Tieske]

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

[thibaultcha]

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.

[subnetmarco]

@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.

[thibaultcha]

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;
}

[Tieske]

@thefosk nice!