[WIP] WebSocket Proxy Support #581

Merged
merged 5 commits into from Nov 24, 2015

Conversation

Projects
None yet
2 participants
@zlm2012
Contributor

zlm2012 commented Nov 7, 2015

This pull request is for WebSocket proxy support for H2O. Still work in progress. Now just simply implemented a skeleton for this support. Currently there's no timeout after the WebSocket proxy fully established.

@kazuho

This comment has been minimized.

Show comment
Hide comment
@kazuho

kazuho Nov 9, 2015

Member

👍 Looking forward to getting the work merged. Please let me know when you need a review.

Member

kazuho commented Nov 9, 2015

👍 Looking forward to getting the work merged. Please let me know when you need a review.

@zlm2012

This comment has been minimized.

Show comment
Hide comment
@zlm2012

zlm2012 Nov 9, 2015

Contributor

Thanks. I think you could review now since I'm not so confident about my implementation. It can work on a simple WebSocket echo test and it's now the simplest implementation. So if there's something wrong I can fix it easily.

Contributor

zlm2012 commented Nov 9, 2015

Thanks. I think you could review now since I'm not so confident about my implementation. It can work on a simple WebSocket echo test and it's now the simplest implementation. So if there's something wrong I can fix it easily.

@kazuho

This comment has been minimized.

Show comment
Hide comment
@kazuho

kazuho Nov 9, 2015

Member

The code looks beautiful. I'll add some comments inline.

EDIT. the three in-line comments are all the nit-picks I could find :-) great work!

Member

kazuho commented Nov 9, 2015

The code looks beautiful. I'll add some comments inline.

EDIT. the three in-line comments are all the nit-picks I could find :-) great work!

lib/core/proxy.c
+ }
+ buf.base[offset++] = ' ';
+ APPEND(req->input.authority.base, req->input.authority.len);
+ APPEND_STRLIT("\r\n");

This comment has been minimized.

@kazuho

kazuho Nov 9, 2015

Member

please include all headers regardless of whether if the request is a websocket handshake. Application servers may use the information provided by the headers (e.g. x-forwarded-for) to determine if it should allow opening a websocket connection.

@kazuho

kazuho Nov 9, 2015

Member

please include all headers regardless of whether if the request is a websocket handshake. Application servers may use the information provided by the headers (e.g. x-forwarded-for) to determine if it should allow opening a websocket connection.

lib/websocket_proxy.c
@@ -0,0 +1,123 @@
+#include <stdio.h>

This comment has been minimized.

@kazuho

kazuho Nov 9, 2015

Member

Let's rename this file (and websocket_proxy.h) to tunnel.c (and .h), and let them concentrate on tunneling the data.
IMO it can be used for other purposes as well.

@kazuho

kazuho Nov 9, 2015

Member

Let's rename this file (and websocket_proxy.h) to tunnel.c (and .h), and let them concentrate on tunneling the data.
IMO it can be used for other purposes as well.

lib/websocket_proxy.c
+}
+
+static void on_upgrade_complete(void *_socket_info, h2o_socket_t *sock, size_t reqsize)
+{

This comment has been minimized.

@kazuho

kazuho Nov 9, 2015

Member

As part of changing the purpose of the file to handle L4 tunneling (instead of websocket proxying), please move this function (and h2o_websocket_proxy_hs_success) to lib/core/proxy.c, and instead expose a function that starts tunneling between two sockets.

@kazuho

kazuho Nov 9, 2015

Member

As part of changing the purpose of the file to handle L4 tunneling (instead of websocket proxying), please move this function (and h2o_websocket_proxy_hs_success) to lib/core/proxy.c, and instead expose a function that starts tunneling between two sockets.

@zlm2012

This comment has been minimized.

Show comment
Hide comment
@zlm2012

zlm2012 Nov 9, 2015

Contributor

Now changed code to make a general L4 tunnel & added a function to break down the tunnel. Is there some other features needed to add into this implementation?

Contributor

zlm2012 commented Nov 9, 2015

Now changed code to make a general L4 tunnel & added a function to break down the tunnel. Is there some other features needed to add into this implementation?

@kazuho

This comment has been minimized.

Show comment
Hide comment
@kazuho

kazuho Nov 9, 2015

Member

@zlm2012 Wow! so fast. I expected this whole websocket work to take weeks!

Is there some other features needed to add into this implementation?

Please add two configuration variables:

  • a configuration directive for disabling upgrade to websocket
  • a configuration directive for setting timeout that closes the tunnel when it is left idle for more than specified milliseconds
    • I do not have a concrete idea for the default: does 300 seconds sound OK?

I will also run a test to see if there are any other issues.

Member

kazuho commented Nov 9, 2015

@zlm2012 Wow! so fast. I expected this whole websocket work to take weeks!

Is there some other features needed to add into this implementation?

Please add two configuration variables:

  • a configuration directive for disabling upgrade to websocket
  • a configuration directive for setting timeout that closes the tunnel when it is left idle for more than specified milliseconds
    • I do not have a concrete idea for the default: does 300 seconds sound OK?

I will also run a test to see if there are any other issues.

@zlm2012

This comment has been minimized.

Show comment
Hide comment
@zlm2012

zlm2012 Nov 10, 2015

Contributor

May I add those variables into proxy configuration list?

Contributor

zlm2012 commented Nov 10, 2015

May I add those variables into proxy configuration list?

@kazuho

This comment has been minimized.

Show comment
Hide comment
@kazuho

kazuho Nov 10, 2015

Member

May I add those variables into proxy configuration list?

Yes please.

Member

kazuho commented Nov 10, 2015

May I add those variables into proxy configuration list?

Yes please.

@zlm2012

This comment has been minimized.

Show comment
Hide comment
@zlm2012

zlm2012 Nov 10, 2015

Contributor

And about the two variables mentioned above, how should I pass them to lib/core/proxy.c? Extend h2o_req_t vs extend the argument list of h2o_reprocess_request, which one is better?

EDIT: Maybe I can pass them by extending h2o_req_overrides_t

Contributor

zlm2012 commented Nov 10, 2015

And about the two variables mentioned above, how should I pass them to lib/core/proxy.c? Extend h2o_req_t vs extend the argument list of h2o_reprocess_request, which one is better?

EDIT: Maybe I can pass them by extending h2o_req_overrides_t

@zlm2012

This comment has been minimized.

Show comment
Hide comment
@zlm2012

zlm2012 Nov 10, 2015

Contributor

Forgot to check if overrides is NULL before referencing it... Now all tests should pass...

Contributor

zlm2012 commented Nov 10, 2015

Forgot to check if overrides is NULL before referencing it... Now all tests should pass...

@kazuho kazuho added this to the v1.6 milestone Nov 18, 2015

@kazuho kazuho merged commit 49ed913 into h2o:master Nov 24, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

kazuho added a commit that referenced this pull request Nov 24, 2015

@kazuho

This comment has been minimized.

Show comment
Hide comment
@kazuho

kazuho Nov 24, 2015

Member

Thank you for the patience. Merged to master.

At the moment the feature is considered experimental and turned off by default. Hopefully in 1.8 we can declare it production-level and enable it by default.

Member

kazuho commented Nov 24, 2015

Thank you for the patience. Merged to master.

At the moment the feature is considered experimental and turned off by default. Hopefully in 1.8 we can declare it production-level and enable it by default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment