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.

Sign up for GitHub

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

Jump to bottom

Websocket support: Do not remove websocket upgrade headers #1247

Merged
merged 4 commits into from
Jul 14, 2017
Merged

Websocket support: Do not remove websocket upgrade headers #1247

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b98ad8b
do not remove websocket upgrade headers
Jul 12, 2017
cf6089d
nits
Jul 12, 2017
438b1d9
Merge branch 'master' into allow-websocket-upgrade-headers
Jul 12, 2017
bb9d67f
eliminate header lookups for H2
Jul 13, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 7 additions & 2 deletions source/common/http/conn_manager_utility.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,17 @@ void ConnectionManagerUtility::mutateRequestHeaders(Http::HeaderMap& request_hea
Runtime::Loader& runtime,
const LocalInfo::LocalInfo& local_info) {
// Clean proxy headers.
request_headers.removeConnection();
request_headers.removeEnvoyInternalRequest();
request_headers.removeKeepAlive();
request_headers.removeProxyConnection();
request_headers.removeTransferEncoding();
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.
Copy link
Member

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.

Copy link
Member Author

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.

if (!Utility::isWebSocketUpgradeRequest(request_headers)) {
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member Author

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)

Copy link
Member

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.

Copy link
Member

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.

request_headers.removeConnection();
request_headers.removeUpgrade();
}

// If we are "using remote address" this means that we create/append to XFF with our immediate
// peer. Cases where we don't "use remote address" include trusted double proxy where we expect
Expand Down
5 changes: 5 additions & 0 deletions source/common/http/headers.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,13 @@ class HeaderValues {

struct {
const std::string Close{"close"};
const std::string Upgrade{"upgrade"};
} ConnectionValues;

struct {
const std::string WebSocket{"websocket"};
} UpgradeValues;

struct {
const std::string Text{"text/plain"};
const std::string Grpc{"application/grpc"};
Expand Down
10 changes: 10 additions & 0 deletions source/common/http/utility.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,16 @@ bool Utility::isInternalRequest(const HeaderMap& headers) {
return Network::Utility::isInternalAddress(forwarded_for->value().c_str());
}

bool Utility::isWebSocketUpgradeRequest(const HeaderMap& headers) {
return (headers.Connection() && headers.Upgrade() &&
(0 == StringUtil::caseInsensitiveCompare(
headers.Connection()->value().c_str(),
Http::Headers::get().ConnectionValues.Upgrade.c_str())) &&
(0 == StringUtil::caseInsensitiveCompare(
headers.Upgrade()->value().c_str(),
Http::Headers::get().UpgradeValues.WebSocket.c_str())));
}

Http2Settings Utility::parseHttp2Settings(const Json::Object& config) {
Http2Settings ret;

Expand Down
8 changes: 8 additions & 0 deletions source/common/http/utility.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,14 @@ class Utility {
*/
static bool isInternalRequest(const HeaderMap& headers);

/**
* Determine whether this is a WebSocket Upgrade request.
* This function returns true if the following HTTP headers and values are present:
* - Connection: Upgrade
* - Upgrade: websocket
*/
static bool isWebSocketUpgradeRequest(const HeaderMap& headers);

/**
* @return Http2Settings An Http2Settings populated from the "http_codec_options" and
* "http2_settings" JSON fields.
Expand Down
18 changes: 18 additions & 0 deletions test/common/http/conn_manager_utility_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -371,6 +371,24 @@ TEST_F(ConnectionManagerUtilityTest, ExternalAddressInternalRequestUseRemote) {
EXPECT_TRUE(headers.has("x-envoy-internal"));
}

TEST_F(ConnectionManagerUtilityTest, DoNotRemoveConnectionUpgradeForWebSocketRequests) {
TestHeaderMapImpl headers{{"connection", "upgrade"}, {"upgrade", "websocket"}};

ConnectionManagerUtility::mutateRequestHeaders(headers, connection_, config_, route_config_,
random_, runtime_, local_info_);
EXPECT_EQ("upgrade", headers.get_("connection"));
EXPECT_EQ("websocket", headers.get_("upgrade"));
}

TEST_F(ConnectionManagerUtilityTest, RemoveConnectionUpgradeForNonWebSocketRequests) {
TestHeaderMapImpl headers{{"connection", "close"}, {"upgrade", "websocket"}};

ConnectionManagerUtility::mutateRequestHeaders(headers, connection_, config_, route_config_,
random_, runtime_, local_info_);
EXPECT_FALSE(headers.has("connection"));
EXPECT_FALSE(headers.has("upgrade"));
}

TEST_F(ConnectionManagerUtilityTest, MutateResponseHeaders) {
route_config_.response_headers_to_remove_.push_back(LowerCaseString("custom_header"));
route_config_.response_headers_to_add_.push_back({LowerCaseString("to_add"), "foo"});
Expand Down
9 changes: 9 additions & 0 deletions test/common/http/utility_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,15 @@ TEST(HttpUtility, isInternalRequest) {
EXPECT_TRUE(Utility::isInternalRequest(TestHeaderMapImpl{{"x-forwarded-for", "127.0.0.1"}}));
}

TEST(HttpUtility, isWebSocketUpgradeRequest) {
EXPECT_FALSE(Utility::isWebSocketUpgradeRequest(TestHeaderMapImpl{}));
EXPECT_FALSE(Utility::isWebSocketUpgradeRequest(TestHeaderMapImpl{{"connection", "upgrade"}}));
EXPECT_FALSE(Utility::isWebSocketUpgradeRequest(TestHeaderMapImpl{{"upgrade", "websocket"}}));
Copy link
Member

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.

Copy link
Member Author

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed


EXPECT_TRUE(Utility::isWebSocketUpgradeRequest(
TestHeaderMapImpl{{"connection", "upgrade"}, {"upgrade", "websocket"}}));
}

TEST(HttpUtility, appendXff) {
{
TestHeaderMapImpl headers;
Expand Down