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

Do not 503 on Upgrade: h2c instead remove the header and ignore. #95

Merged
merged 1 commit into from
Aug 23, 2019
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions source/common/common/utility.cc
Original file line number Diff line number Diff line change
@@ -338,6 +338,33 @@ std::vector<absl::string_view> StringUtil::splitToken(absl::string_view source,
return absl::StrSplit(source, absl::ByAnyChar(delimiters), absl::SkipEmpty());
}

std::string StringUtil::removeTokens(absl::string_view source, absl::string_view delimiters,
std::set<absl::string_view> tokens_to_remove,
absl::string_view joiner, bool trim_tokens, bool ignore_case) {
const auto values = Envoy::StringUtil::splitToken(source, delimiters);
std::string new_value;
for (auto& v : values) {
absl::string_view token_for_compare = v;
absl::string_view token_for_result = v;
if (trim_tokens) {
token_for_result = token_for_compare = StringUtil::trim(v);
}
std::string lower_value;
if (ignore_case) {
lower_value = StringUtil::toLower(token_for_compare);
token_for_compare = lower_value;
}
if (tokens_to_remove.count(token_for_compare) != 0) {
continue;
}
if (!new_value.empty()) {
new_value.append(joiner.data(), joiner.size());
}
new_value.append(token_for_result.data(), token_for_result.size());
}
return new_value;
}

uint32_t StringUtil::itoa(char* out, size_t buffer_size, uint64_t i) {
// The maximum size required for an unsigned 64-bit integer is 21 chars (including null).
if (buffer_size < 21) {
15 changes: 15 additions & 0 deletions source/common/common/utility.h
Original file line number Diff line number Diff line change
@@ -285,6 +285,21 @@ class StringUtil {
absl::string_view delimiters,
bool keep_empty_string = false);

/**
* Remove tokens from a delimiter-separated string view.
* @param source supplies the delimiter-separated string view.
* @param multi-delimiter supplies chars used to split the delimiter-separated string view.
* @param tokens_to_remove supplies a set of tokens which should not appear in the result.
* @param joiner contains a string used between tokens in the result.
* @param trim_tokens trims the the tokens before comparing to 'tokens_to_remove'.
* @param ignore_case compares the tokens in a case-insensitive way to tokens_to_remove'.
* @return string of the remaining joined tokens.
*/
static std::string removeTokens(absl::string_view source, absl::string_view delimiters,
std::set<absl::string_view> tokens_to_remove,
absl::string_view joiner, bool trim_tokens = true,
bool ignore_case = true);

/**
* Size-bounded string copying and concatenation
*/
3 changes: 3 additions & 0 deletions source/common/http/headers.h
Original file line number Diff line number Diff line change
@@ -76,6 +76,7 @@ class HeaderValues {
const LowerCaseString GrpcAcceptEncoding{"grpc-accept-encoding"};
const LowerCaseString Host{":authority"};
const LowerCaseString HostLegacy{"host"};
const LowerCaseString Http2Settings{"http2-settings"};
const LowerCaseString KeepAlive{"keep-alive"};
const LowerCaseString LastModified{"last-modified"};
const LowerCaseString Location{"location"};
@@ -104,11 +105,13 @@ class HeaderValues {

struct {
const std::string Close{"close"};
const std::string Http2Settings{"http2-settings"};
const std::string KeepAlive{"keep-alive"};
const std::string Upgrade{"upgrade"};
} ConnectionValues;

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

28 changes: 26 additions & 2 deletions source/common/http/http1/codec_impl.cc
Original file line number Diff line number Diff line change
@@ -4,6 +4,8 @@
#include <memory>
#include <string>

#include "absl/strings/match.h"

#include "envoy/buffer/buffer.h"
#include "envoy/http/header_map.h"
#include "envoy/network/connection.h"
@@ -447,8 +449,30 @@ int ConnectionImpl::onHeadersCompleteBase() {
protocol_ = Protocol::Http10;
}
if (Utility::isUpgrade(*current_header_map_)) {
ENVOY_CONN_LOG(trace, "codec entering upgrade mode.", connection_);
handling_upgrade_ = true;
// Ignore h2c upgrade requests until we support them.
// See https://github.com/envoyproxy/envoy/issues/7161 for details.
if (current_header_map_->Upgrade() &&
absl::EqualsIgnoreCase(current_header_map_->Upgrade()->value().getStringView(),
Http::Headers::get().UpgradeValues.H2c)) {
ENVOY_CONN_LOG(trace, "removing unsupported h2c upgrade headers.", connection_);
current_header_map_->removeUpgrade();
if (current_header_map_->Connection()) {
auto new_value = StringUtil::removeTokens(
current_header_map_->Connection()->value().getStringView(), ",",
{Http::Headers::get().ConnectionValues.Upgrade,
Http::Headers::get().ConnectionValues.Http2Settings},
",");
if (new_value.empty()) {
current_header_map_->removeConnection();
} else {
current_header_map_->Connection()->value(new_value);
}
}
current_header_map_->remove(Headers::get().Http2Settings);
} else {
ENVOY_CONN_LOG(trace, "codec entering upgrade mode.", connection_);
handling_upgrade_ = true;
}
}

int rc = onHeadersComplete(std::move(current_header_map_));
23 changes: 23 additions & 0 deletions test/common/common/utility_speed_test.cc
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Note: this should be run with --compilation_mode=opt, and would benefit from a
// quiescent system with disabled cstate power management.

#include <iostream>
#include <random>

#include "common/common/assert.h"
@@ -206,6 +207,28 @@ static void BM_FindTokenValueNoSplit(benchmark::State& state) {
}
BENCHMARK(BM_FindTokenValueNoSplit);

static void BM_RemoveTokensLong(benchmark::State& state) {
auto size = state.range(0);
std::string input(size, ',');
std::vector<std::string> to_remove;
std::set<absl::string_view> to_remove_set;
for (decltype(size) i = 0; i < size; i++) {
to_remove.push_back(std::to_string(i));
}
for (int i = 0; i < size; i++) {
if (i & 1) {
to_remove_set.insert(to_remove[i]);
}
input.append(",");
input.append(to_remove[i]);
}
for (auto _ : state) {
Envoy::StringUtil::removeTokens(input, ",", to_remove_set, ",");
state.SetBytesProcessed(static_cast<int64_t>(state.iterations()) * input.size());
}
}
BENCHMARK(BM_RemoveTokensLong)->Range(8, 8 << 10);

static void BM_IntervalSetInsert17(benchmark::State& state) {
for (auto _ : state) {
Envoy::IntervalSetImpl<size_t> interval_set;
25 changes: 25 additions & 0 deletions test/common/common/utility_test.cc
Original file line number Diff line number Diff line change
@@ -395,6 +395,31 @@ TEST(StringUtil, StringViewSplit) {
}
}

TEST(StringUtil, StringViewRemoveTokens) {
// Basic cases.
EXPECT_EQ(StringUtil::removeTokens("", ",", {"two"}, ","), "");
EXPECT_EQ(StringUtil::removeTokens("one", ",", {"two"}, ","), "one");
EXPECT_EQ(StringUtil::removeTokens("one,two ", ",", {"two"}, ","), "one");
EXPECT_EQ(StringUtil::removeTokens("one,two,three ", ",", {"two"}, ","), "one,three");
EXPECT_EQ(StringUtil::removeTokens(" one , two , three ", ",", {"two"}, ","), "one,three");
EXPECT_EQ(StringUtil::removeTokens(" one , two , three ", ",", {"three"}, ","), "one,two");
EXPECT_EQ(StringUtil::removeTokens(" one , two , three ", ",", {"three"}, ", "), "one, two");
EXPECT_EQ(StringUtil::removeTokens("one,two,three", ",", {"two", "three"}, ","), "one");
EXPECT_EQ(StringUtil::removeTokens("one,two,three,four", ",", {"two", "three"}, ","), "one,four");
// Ignore case.
EXPECT_EQ(StringUtil::removeTokens("One,Two,Three,Four", ",", {"two", "three"}, ","), "One,Four");
// Longer joiner.
EXPECT_EQ(StringUtil::removeTokens("one,two,three,four", ",", {"two", "three"}, " , "),
"one , four");
// Delimiters.
EXPECT_EQ(StringUtil::removeTokens("one,two;three ", ",;", {"two"}, ","), "one,three");
// No trimming
EXPECT_EQ(StringUtil::removeTokens("one, two, three ", ",", {" two"}, ",", false), "one, three ");
// No ignore case.
EXPECT_EQ(StringUtil::removeTokens("one, Two, three", ",", {"two"}, ",", true, false),
"one,Two,three");
}

TEST(StringUtil, removeCharacters) {
IntervalSetImpl<size_t> removals;
removals.insert(3, 5);
37 changes: 37 additions & 0 deletions test/common/http/http1/codec_impl_test.cc
Original file line number Diff line number Diff line change
@@ -632,6 +632,43 @@ TEST_F(Http1ServerConnectionImplTest, RequestWithTrailers) {
EXPECT_EQ(0U, buffer.length());
}

TEST_F(Http1ServerConnectionImplTest, IgnoreUpgradeH2c) {
initialize();

TestHeaderMapImpl expected_headers{
{":authority", "www.somewhere.com"}, {":path", "/"}, {":method", "GET"}};
Buffer::OwnedImpl buffer(
"GET http://www.somewhere.com/ HTTP/1.1\r\nConnection: "
"Upgrade, HTTP2-Settings\r\nUpgrade: h2c\r\nHTTP2-Settings: token64\r\nHost: bah\r\n\r\n");
expectHeadersTest(Protocol::Http11, true, buffer, expected_headers);
}

TEST_F(Http1ServerConnectionImplTest, IgnoreUpgradeH2cClose) {
initialize();

TestHeaderMapImpl expected_headers{{":authority", "www.somewhere.com"},
{":path", "/"},
{":method", "GET"},
{"connection", "Close"}};
Buffer::OwnedImpl buffer("GET http://www.somewhere.com/ HTTP/1.1\r\nConnection: "
"Upgrade, Close, HTTP2-Settings\r\nUpgrade: h2c\r\nHTTP2-Settings: "
"token64\r\nHost: bah\r\n\r\n");
expectHeadersTest(Protocol::Http11, true, buffer, expected_headers);
}

TEST_F(Http1ServerConnectionImplTest, IgnoreUpgradeH2cCloseEtc) {
initialize();

TestHeaderMapImpl expected_headers{{":authority", "www.somewhere.com"},
{":path", "/"},
{":method", "GET"},
{"connection", "Close,Etc"}};
Buffer::OwnedImpl buffer("GET http://www.somewhere.com/ HTTP/1.1\r\nConnection: "
"Upgrade, Close, HTTP2-Settings, Etc\r\nUpgrade: h2c\r\nHTTP2-Settings: "
"token64\r\nHost: bah\r\n\r\n");
expectHeadersTest(Protocol::Http11, true, buffer, expected_headers);
}

TEST_F(Http1ServerConnectionImplTest, UpgradeRequest) {
initialize();