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

internal/dag: add websocket support to ingressroute #600

Merged
merged 1 commit into from
Aug 8, 2018
Merged

internal/dag: add websocket support to ingressroute #600

merged 1 commit into from
Aug 8, 2018

Conversation

alexbrand
Copy link
Contributor

Adds a new field to the Routes object inside the IngressRoute CRD called EnableWebsockets, which controls whether a client can upgrade to a websocket connection on the given route.

Fixes #595

@stevesloka
Copy link
Member

Overall this LGTM with a couple comments:

  1. Is there any reason to not default this on for all routes?
  2. It might be nice to as well have a default like we do for HealthChecks / LBAlgo to set it at the root and let it cascade down (probably as a new issue).

@alexbrand
Copy link
Contributor Author

Thanks @stevesloka. You raise a good question in number 1. My assumption is that we default websockets to be disabled because Envoy does so. I did some research and found some context around this in the envoy repo: (envoyproxy/envoy#319 (comment)):

We should explicitly specify in the route configuration whether a route is a WS route or not. Meaning, we will not support attempting to detect if it is WS all the way back to the backend. If we receive a WS upgrade over HTTP/1.1, we will check the route table to see if the matched route is WS, and if not, fail the request with 400 or some other code.

I still don't quite understand the reasoning behind having to mark a route as WS though. Curious if @dfc has any thoughts here.

Signed-off-by: Alexander Brand <alexbrand09@gmail.com>
Copy link
Contributor

@davecheney davecheney left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you.

@davecheney davecheney merged commit 844dd38 into projectcontour:master Aug 8, 2018
@davecheney davecheney added this to the 0.6.0-beta.3 milestone Aug 9, 2018
@alexbrand alexbrand deleted the ingressroute-ws branch August 9, 2018 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants