Websocket support #1274

rshriram · 2017-07-18T02:53:32Z

This PR adds WebSocket support to Envoy.
The main implementation is fairly complete. I have tested this code with a simple python based websocket client talking to echo.websocket.org (both 80 and 443) and it works :). I would appreciate any feedback on the code.

Tests are TODO. Finding it difficult, as this requires part HTTP mocks and part TCP mocks. Pointers appreciated.

Need to test interaction of WebSockets in the presence of RDS (cc @kyessenov @GregHanson @ijsnellf)

cc @mattklein123 @htuch @PiotrSikora

…ng test temporarily

Initial implementation of WsHandlerImpl without WsHandler interface. Still missing the initial header encode.

rshriram · 2017-08-02T20:23:52Z

@mattklein123 @dnoe addressed comments.

dnoe · 2017-08-03T13:23:27Z

Will take a look today, thanks.

dnoe

Looking good, thanks for the TCP proxy refactoring. This is a big PR and my eyes are glazing over by the end of it, I'll try and make one more pass for nits in a bit. Otherwise looks good to me pending @htuch and @mattklein123 approval for final review.

dnoe · 2017-08-03T18:32:34Z

source/common/http/conn_manager_impl.cc

+        connection_manager_.ws_connection_->initializeReadFilterCallbacks(
+            *connection_manager_.read_callbacks_);
+        if (connection_manager_.ws_connection_->onNewConnection() ==
+            Network::FilterStatus::Continue) {


Is there an error case here?

the onNewConnection will send the error response back to caller via the WsHandlerCallbacks.

mattklein123

@rshriram nice work this looks great. Few random comments then I think we are ready to go.

mattklein123 · 2017-08-04T00:47:26Z

source/common/filter/tcp_proxy.h

+  struct UpstreamHelperCallbacks {
+    virtual ~UpstreamHelperCallbacks() {}
+
+    virtual const std::string& getUpstreamCluster() PURE;


Is there a reason that we need to have a new object here that we allocate? Can these just be normal virtual functions that get overriden by the WsHandlerImpl derived class? That would save the extra allocation which AFAICT isn't needed?

mattklein123 · 2017-08-04T00:49:08Z

source/common/http/conn_manager_impl.cc

@@ -91,7 +93,11 @@ ConnectionManagerImpl::~ConnectionManagerImpl() {
    if (codec_->protocol() == Protocol::Http2) {
      stats_.named_.downstream_cx_http2_active_.dec();
    } else {
-      stats_.named_.downstream_cx_http1_active_.dec();
+      if (isWebSocketConnection()) {
+        stats_.named_.downstream_cx_websocket_active_.dec();


Optimally, you would verify this stat dance in your integration tests. Can you make sure that the gauges are correct at various points?

mattklein123 · 2017-08-04T00:49:53Z

source/common/http/conn_manager_impl.cc

+  // should return 404. The current returns no response if there is no router filter.
+  if ((protocol == Protocol::Http11) && cached_route_.value()) {
+    const Router::RouteEntry* route_entry = cached_route_.value()->routeEntry();
+    bool websocket_required = (route_entry != nullptr) && route_entry->useWebSocket();


const for websocket_required and websocket_requested

mattklein123 · 2017-08-04T00:56:10Z

source/common/http/conn_manager_impl.cc

+
+        connection_manager_.ws_connection_.reset(new WebSocket::WsHandlerImpl(
+            *request_headers_, *route_entry, *this, connection_manager_.cluster_manager_));
+        connection_manager_.ws_connection_->initializeReadFilterCallbacks(


nit: Can this be done as part of the WsHandlerImpl constructor?

mattklein123 · 2017-08-04T00:57:04Z

source/common/http/websocket/ws_handler_impl.h

+ */
+class WsHandlerImpl : public Filter::TcpProxy {
+public:
+  WsHandlerImpl(Http::HeaderMap& request_headers, const Router::RouteEntry& route_entry,


nit: Since you are in Http namespace, you shouldn't need Http:: in all/most places in this file and cc file. Can you remove?

mattklein123 · 2017-08-04T00:57:26Z

source/common/http/websocket/ws_handler_impl.h

+  }
+
+protected:
+  struct WsUpstreamHelperCallbacks : public UpstreamHelperCallbacks {


per other comment I think these can just be virtual overrides.

mattklein123 · 2017-08-04T00:57:42Z

source/common/http/websocket/ws_handler_impl.h

+private:
+  struct NullHttpConnectionCallbacks : public Http::ConnectionCallbacks {
+    // Http::ConnectionCallbacks
+    void onGoAway() override{};


nit: remove ';' at end of line

mattklein123 · 2017-08-04T00:59:26Z

source/common/http/websocket/ws_handler_impl.h

+public:
+  WsHandlerImpl(Http::HeaderMap& request_headers, const Router::RouteEntry& route_entry,
+                WsHandlerCallbacks& callbacks, Upstream::ClusterManager& cluster_manager);
+  ~WsHandlerImpl(){};


nit: del, not needed.

mattklein123

Small test request, otherwise very nice work and LGTM. @dnoe @htuch @alyssawilk any further comments?

mattklein123 · 2017-08-07T20:59:28Z

test/common/http/conn_manager_impl_test.cc

+  EXPECT_EQ(1U, stats_.named_.downstream_cx_websocket_active_.value());
+  EXPECT_EQ(1U, stats_.named_.downstream_cx_websocket_total_.value());
+  EXPECT_EQ(0U, stats_.named_.downstream_cx_http1_active_.value());
+}


if you reset() conn_manager_ here can you then verify the gauges are zero? Same below?

It isn't working. Getting assertion error from ActiveStream in tsan and asan.

I don't know what the problem is off the top of my head. As long as you have coverage in the integration tests which it looks like you added, that's fine.

mattklein123 · 2017-08-08T02:43:38Z

test/integration/server.h

@@ -199,6 +199,10 @@ class IntegrationTestServer : Logger::Loggable<Logger::Id::testing>,
    return TestUtility::findCounter(*stat_store_, name);
  }

+  Stats::GaugeSharedPtr gauge(const std::string& name) {
+    return TestUtility::findGauge(*stat_store_, name);


The same comment applies here as to counter() above. Can you add a similar comment or just have a comment that says something to the effect of "See counter() above for why this method exists."

mattklein123

Ready to ship IMO. @dnoe any further comments?

dnoe

Took a quick final pass, looks good to me. Many thanks for the hard work @rshriram

As per #1274 (comment), this patch is no longer required and as per offline discussion it doesn't seem effective. Signed-off-by: Ryan Hamilton <rch@google.com> Signed-off-by: JP Simard <jp@jpsim.com>

Shriram Rajagopalan and others added 30 commits July 8, 2017 18:26

reject websocket upgrades on non websocket routes

85e5549

moving websocket check to conn man - with segfaults

f6fd360

isolated segfault, but not resolved

58a9340

check websocket compatibility before scrubbing headers. disable cachi…

13934c5

…ng test temporarily

WsHandlerImpl (#1)

697af16

Initial implementation of WsHandlerImpl without WsHandler interface. Still missing the initial header encode.

forward upgrade headers to upstream

6c524c9

example client

8d5b908

bug fixes

39a6dc2

debugging

6383d40

bug fixes and working websocket implementation

a66e533

path/host rewrite

5a2f6c9

auto_host_rewrite

d32f373

Merge branch 'master' into envoy-websocket

89f21c0

do not remove websocket upgrade headers

b98ad8b

do not remove websocket upgrade headers

fd9586e

nits

cf6089d

Merge branch 'master' into allow-websocket-upgrade-headers

438b1d9

Merge branch 'allow-websocket-upgrade-headers' into envoy-websocket

1467bf4

remove commented code

b61a596

move route cache test before filter chain setup

266c2f6

remove dead code

cea96bc

eliminate header lookups for H2

033c1df

remove websocket option from route config

b613f12

do not upgrade with pending HTTP responses

ac5b91d

Merge branch 'master' into envoy-websocket

e190e26

cleanup debug statements

4275cf0

move websocket code to separate files

051c953

adding stats to track ws connections

1010030

streamline with tcpproxy interface

8a14a59

more comments and TODOs

01b5c7b

dnoe reviewed Aug 3, 2017

View reviewed changes

mattklein123 reviewed Aug 4, 2017

View reviewed changes

Shriram Rajagopalan added 4 commits August 4, 2017 12:21

nits

fc35ed5

some unit tests

1efd392

Merge branch 'master' into envoy-websocket

51aaa9b

fixing integration tests

70f3f3c

mattklein123 reviewed Aug 7, 2017

View reviewed changes

Shriram Rajagopalan added 2 commits August 7, 2017 18:57

gauge==0 and prefix rewrite unit test

a6341a6

check for gauge==0 in integration test

7ac856a

mattklein123 reviewed Aug 8, 2017

View reviewed changes

Shriram Rajagopalan added 5 commits August 8, 2017 00:14

fix comments and gauge in integration test

b0c5c7f

asan fix

d4d55d6

destroy fake_upstream to force stats update

21d6895

check gauge in unit tests

6553b9a

undo integration tests for gauge

505dadb

mattklein123 approved these changes Aug 8, 2017

View reviewed changes

dnoe approved these changes Aug 8, 2017

View reviewed changes

mattklein123 merged commit 8c42f16 into envoyproxy:master Aug 8, 2017

mattklein123 mentioned this pull request Aug 8, 2017

Websockets support #319

Closed

rshriram mentioned this pull request Aug 8, 2017

Expose websocket routes istio/old_pilot_repo#1006

Closed

mattklein123 mentioned this pull request Aug 10, 2017

grpc_json_transcoder broken in :latest docker container #1427

Closed

lizan mentioned this pull request Aug 10, 2017

grpc transcoding: need to refresh cached route after header changes #1434

Closed

rshriram deleted the envoy-websocket branch August 14, 2017 18:10

joestringer mentioned this pull request May 27, 2020

Websocket support in layer 7 NetworkPolicy cilium/cilium#11672

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Websocket support #1274

Websocket support #1274

rshriram commented Jul 18, 2017 •

edited

Loading

rshriram commented Aug 2, 2017

dnoe commented Aug 3, 2017

dnoe left a comment

dnoe Aug 3, 2017

rshriram Aug 3, 2017

mattklein123 left a comment

mattklein123 Aug 4, 2017

mattklein123 Aug 4, 2017

mattklein123 Aug 4, 2017

mattklein123 Aug 4, 2017

mattklein123 Aug 4, 2017

mattklein123 Aug 4, 2017

mattklein123 Aug 4, 2017

mattklein123 Aug 4, 2017

mattklein123 left a comment

mattklein123 Aug 7, 2017

rshriram Aug 8, 2017

mattklein123 Aug 8, 2017

mattklein123 Aug 8, 2017

mattklein123 left a comment

dnoe left a comment

Websocket support #1274

Websocket support #1274

Conversation

rshriram commented Jul 18, 2017 • edited Loading

rshriram commented Aug 2, 2017

dnoe commented Aug 3, 2017

dnoe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

dnoe left a comment

Choose a reason for hiding this comment

rshriram commented Jul 18, 2017 •

edited

Loading