cloudfoundry / gorouter

CF Router
Apache License 2.0
441 stars 226 forks source link

Remove obsolete request handler #403

Closed domdom82 closed 6 months ago

domdom82 commented 8 months ago

OK, I know. It's quite a handful, this pull request. But hear me out:

While I was working on the missing session affinity for web sockets where I wanted to add a proper middleware for handling session cookies, I realized that this wouldn't work for web sockets. Why? The main reason is that the request handler does not play by the rules of Negroni and does not call the next function, instead it just returns after handling the web socket.

This made it impossible to call any other middleware after request handler, which also meant that I couldn't use it to set session cookies.

While digging into the history of the request handler, I realized that Go itself already handles web sockets in httputil.ReverseProxy since Go 1.12. So there was no real reason for keeping the web socket handling in Gorouter.

There is a second issue with handling web sockets (or any protocol for that matter) outside the stdlib: RFC compliance. While refactoring, I stumbled across a test that violates the web socket RFC: If the server does not respond with a 101 status on upgrade, the proxy is supposed to treat the response as regular HTTP, not as an error. However, request handler does and thus breaks the protocol compliance. It is one instance where implementing a standard protocol yourself can backfire.

Another weirdness that was refactored in this PR is the Hop-By-Hop header filtering which is a generic HTTP feature that should belong in its own middleware, not a specific handler for web sockets. So I moved that piece into its proper handler, further improving the code structure.

The long-term benefit I am trying to achieve with this PR:

This PR lays the ground work for a follow-up PR that enables moving session affinity handling into a proper middleware, enabling it to work for both HTTP and web sockets requests, thus solving the missing session affinity for web sockets

This is a major refactoring effort. The behavior should be unchanged, except where RFC 6455 was violated by request handler before.

domdom82 commented 8 months ago

side note about routing-release:

domdom82 commented 7 months ago

@geofffranks @ameowlia any updates / blockers?

domdom82 commented 6 months ago

hi @ameowlia @geofffranks I've just stumbled across this code piece in routeservice.go because a customer was complaining they need to rate-limit their WS app using a route-service which is not supported atm. Do you recall why we don't support WebSockets over route-services? I didn't double-check but it may be to do with how we implemented WebSockets prior to this PR - so it may be another benefit if we were able to remove that limitation as a follow-up.