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

Remove obsolete request handler #403

Merged
merged 4 commits into from
Apr 29, 2024

Conversation

domdom82
Copy link
Contributor

  • A short explanation of the proposed change:

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:

  • Consolidation of the code base (remove large chunks of legacy code, including the entire handler package)
  • Better standards compliance by relying on the Go stdlib wherever possible
  • Streamline handling of requests (everything goes through ProxyRoundTripper now)
  • Make it easier to add new functionality such as session handling via middleware
  • An explanation of the use cases your change solves

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

  • Instructions to functionally test the behavior change using operator interfaces (BOSH manifest, logs, curl, and metrics)

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

  • Expected result after the change
  • All tests should pass
  • Current result before the change
  • All tests should pass
  • Links to any other associated PRs

  • I have viewed signed and have submitted the Contributor License Agreement

  • I have made this pull request to the main branch

  • I have run all the unit tests.

  • (Optional) I have run Routing Acceptance Tests and Routing Smoke Tests

  • (Optional) I have run CF Acceptance Tests

@domdom82 domdom82 requested a review from a team as a code owner March 16, 2024 09:14
@domdom82
Copy link
Contributor Author

side note about routing-release:

  • once this PR is merged, routing-release needs to remove this line else the gosub spec tests will fail

handlers/hop_by_hop.go Show resolved Hide resolved
proxy/proxy_test.go Show resolved Hide resolved
@maxmoehl maxmoehl requested a review from a team March 21, 2024 07:50
handlers/hop_by_hop.go Outdated Show resolved Hide resolved
Copy link
Contributor

@geofffranks geofffranks left a comment

Choose a reason for hiding this comment

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

Added a couple questions + items to fix.

Also it would probably be best to discuss the potential risks + benefits of this at the WG meeting in April, to see what others think. I'm in favor of making this change though.

@domdom82
Copy link
Contributor Author

@geofffranks @ameowlia any updates / blockers?

@domdom82
Copy link
Contributor Author

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.

@geofffranks geofffranks merged commit 35ae37e into cloudfoundry:main Apr 29, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants