-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Websocket support: Do not remove websocket upgrade headers #1247
Websocket support: Do not remove websocket upgrade headers #1247
Conversation
Here is the reference branch with the entire websocket code: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
request_headers.removeUpgrade(); | ||
|
||
// If this is a WebSocket Upgrade request, do not remove the Connection and Upgrade headers, | ||
// as we forward them verbatim to the client. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To the client or the backend in this case? It looks like this is on the request path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops.. Good catch..Yes, to the backend.
TEST(HttpUtility, isWebSocketUpgradeRequest) { | ||
EXPECT_FALSE(Utility::isWebSocketUpgradeRequest(TestHeaderMapImpl{})); | ||
EXPECT_FALSE(Utility::isWebSocketUpgradeRequest(TestHeaderMapImpl{{"connection", "upgrade"}})); | ||
EXPECT_FALSE(Utility::isWebSocketUpgradeRequest(TestHeaderMapImpl{{"upgrade", "websocket"}})); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe test mixed case and the negative case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I already test for the negative case in the two lines above. Its EXPECT_FALSE. While the one below is the +ve case. Do you have something else in mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks.
|
||
// If this is a WebSocket Upgrade request, do not remove the Connection and Upgrade headers, | ||
// as we forward them verbatim to the upstream hosts. | ||
if (!Utility::isWebSocketUpgradeRequest(request_headers)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about this more and I think we should only do this check if protocol is HTTP/1? It's not valid on h2 and we can avoid the perf hit of the extra header lookups and compares?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Let me look around for something that would tell me whether the request is HTTP1 or 2 and do this check.
On the performance issue, in the full websocket PR, the header lookup is done once again in the connection manager to kick off the websocket related code. I had thought about avoiding the double lookup for HTTP1, but it would require passing a "is_websocket" boolean parameter to this function. If you guys are okay with chaning the function signature and putting up with some clumsiness, then we can avoid the double header lookup for HTTP1.
See here master...amalgam8:envoy-websocket#diff-d70ea2d9706e0246700148b2e2bb63ceR501 for reference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
essentially
void ConnectionManagerUtility::mutateRequestHeaders(
Http::HeaderMap& request_headers,
Network::Connection& connection, ConnectionManagerConfig& config, const
Router::Config& route_config, Runtime::RandomGenerator& random, Runtime::Loader&
runtime, const LocalInfo::LocalInfo& local_info)
becomes
void ConnectionManagerUtility::mutateRequestHeaders(
Http::HeaderMap& request_headers, bool is_websocket,
Network::Connection& connection, ConnectionManagerConfig& config, const
Router::Config& route_config, Runtime::RandomGenerator& random, Runtime::Loader&
runtime, const LocalInfo::LocalInfo& local_info)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would there be other "cached" header analysis we would want to pass around like this? I'm a little hesitant to special case for websockets here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could pass protocol to this function and only do the check if protocol is h2. That seems reasonable to be, but it's not that big of a deal either way.
PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works for me if works for @htuch
Description: bazelisk is the default and preferred way to build to ensure consistency as much as possible. This change updates the project's docs to refer to bazelisk when providing example build commands. Signed-off-by: Mike Schore <mike.schore@gmail.com> Signed-off-by: JP Simard <jp@jpsim.com>
Description: bazelisk is the default and preferred way to build to ensure consistency as much as possible. This change updates the project's docs to refer to bazelisk when providing example build commands. Signed-off-by: Mike Schore <mike.schore@gmail.com> Signed-off-by: JP Simard <jp@jpsim.com>
This is the first of the PRs for enabling websocket support in Envoy.
Currently, we strip upgrade and connection headers. For websocket support, we need to allow these headers to pass through if
Connection: upgrade
andUpgrade: websocket
.c.f. #319