-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
tcp_proxy: support command operators in tunneling_config.hostname #21067
Changes from all commits
d306a05
09e4eea
2a0d637
c1b6143
64a6242
6627550
c0d27c1
1825288
844473a
72b000d
a107138
66011c4
6f9fed7
5117b94
200cc44
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,8 @@ | |
#include "envoy/upstream/upstream.h" | ||
|
||
#include "source/common/common/logger.h" | ||
#include "source/common/formatter/substitution_format_string.h" | ||
#include "source/common/http/header_map_impl.h" | ||
#include "source/common/network/cidr_range.h" | ||
#include "source/common/network/filter_impl.h" | ||
#include "source/common/network/hash_policy.h" | ||
|
@@ -114,17 +116,16 @@ class TunnelingConfigHelperImpl : public TunnelingConfigHelper { | |
public: | ||
TunnelingConfigHelperImpl( | ||
const envoy::extensions::filters::network::tcp_proxy::v3::TcpProxy_TunnelingConfig& | ||
config_message) | ||
: hostname_(config_message.hostname()), use_post_(config_message.use_post()), | ||
header_parser_(Envoy::Router::HeaderParser::configure(config_message.headers_to_add())) {} | ||
const std::string& hostname() const override { return hostname_; } | ||
config_message, | ||
Server::Configuration::FactoryContext& context); | ||
std::string host(const StreamInfo::StreamInfo& stream_info) const override; | ||
bool usePost() const override { return use_post_; } | ||
Envoy::Http::HeaderEvaluator& headerEvaluator() const override { return *header_parser_; } | ||
|
||
private: | ||
const std::string hostname_; | ||
const bool use_post_; | ||
std::unique_ptr<Envoy::Router::HeaderParser> header_parser_; | ||
Formatter::FormatterPtr hostname_fmt_; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Make this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can't, because There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can if you initialize this in initialization list and extract
to a helper function. |
||
}; | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,7 @@ | |
#include "test/mocks/buffer/mocks.h" | ||
#include "test/mocks/http/mocks.h" | ||
#include "test/mocks/http/stream_encoder.h" | ||
#include "test/mocks/server/factory_context.h" | ||
#include "test/mocks/tcp/mocks.h" | ||
#include "test/test_common/environment.h" | ||
#include "test/test_common/network_utility.h" | ||
|
@@ -40,17 +41,19 @@ template <typename T> class HttpUpstreamTest : public testing::Test { | |
} | ||
|
||
void setupUpstream() { | ||
config_ = std::make_unique<TunnelingConfigHelperImpl>(config_message_); | ||
config_ = std::make_unique<TunnelingConfigHelperImpl>(config_message_, context_); | ||
upstream_ = std::make_unique<T>(callbacks_, *this->config_, downstream_stream_info_); | ||
upstream_->setRequestEncoder(encoder_, true); | ||
} | ||
|
||
NiceMock<StreamInfo::MockStreamInfo> downstream_stream_info_; | ||
Http::MockRequestEncoder encoder_; | ||
Http::MockHttp1StreamEncoderOptions stream_encoder_options_; | ||
NiceMock<Tcp::ConnectionPool::MockUpstreamCallbacks> callbacks_; | ||
TcpProxy_TunnelingConfig config_message_; | ||
std::unique_ptr<TunnelingConfigHelper> config_; | ||
std::unique_ptr<HttpUpstream> upstream_; | ||
NiceMock<Server::Configuration::MockFactoryContext> context_; | ||
}; | ||
|
||
using testing::Types; | ||
|
@@ -207,14 +210,23 @@ template <typename T> class HttpUpstreamRequestEncoderTest : public testing::Tes | |
} | ||
|
||
void setupUpstream() { | ||
config_ = std::make_unique<TunnelingConfigHelperImpl>(config_message_); | ||
config_ = std::make_unique<TunnelingConfigHelperImpl>(config_message_, context_); | ||
upstream_ = std::make_unique<T>(callbacks_, *this->config_, this->downstream_stream_info_); | ||
} | ||
|
||
void populateMetadata(envoy::config::core::v3::Metadata& metadata, const std::string& ns, | ||
const std::string& key, const std::string& value) { | ||
ProtobufWkt::Struct struct_obj; | ||
auto& fields_map = *struct_obj.mutable_fields(); | ||
fields_map[key] = ValueUtil::stringValue(value); | ||
(*metadata.mutable_filter_metadata())[ns] = struct_obj; | ||
} | ||
|
||
NiceMock<StreamInfo::MockStreamInfo> downstream_stream_info_; | ||
Http::MockRequestEncoder encoder_; | ||
Http::MockHttp1StreamEncoderOptions stream_encoder_options_; | ||
NiceMock<Tcp::ConnectionPool::MockUpstreamCallbacks> callbacks_; | ||
NiceMock<Server::Configuration::MockFactoryContext> context_; | ||
|
||
std::unique_ptr<HttpUpstream> upstream_; | ||
TcpProxy_TunnelingConfig config_message_; | ||
|
@@ -232,7 +244,7 @@ TYPED_TEST(HttpUpstreamRequestEncoderTest, RequestEncoderOld) { | |
std::unique_ptr<Http::RequestHeaderMapImpl> expected_headers; | ||
expected_headers = Http::createHeaderMap<Http::RequestHeaderMapImpl>({ | ||
{Http::Headers::get().Method, "CONNECT"}, | ||
{Http::Headers::get().Host, this->config_->hostname()}, | ||
{Http::Headers::get().Host, this->config_->host(this->downstream_stream_info_)}, | ||
}); | ||
|
||
if (this->is_http2_) { | ||
|
@@ -252,7 +264,7 @@ TYPED_TEST(HttpUpstreamRequestEncoderTest, RequestEncoder) { | |
std::unique_ptr<Http::RequestHeaderMapImpl> expected_headers; | ||
expected_headers = Http::createHeaderMap<Http::RequestHeaderMapImpl>({ | ||
{Http::Headers::get().Method, "CONNECT"}, | ||
{Http::Headers::get().Host, this->config_->hostname()}, | ||
{Http::Headers::get().Host, this->config_->host(this->downstream_stream_info_)}, | ||
}); | ||
|
||
EXPECT_CALL(this->encoder_, encodeHeaders(HeaderMapEqualRef(expected_headers.get()), false)); | ||
|
@@ -265,7 +277,7 @@ TYPED_TEST(HttpUpstreamRequestEncoderTest, RequestEncoderUsePost) { | |
std::unique_ptr<Http::RequestHeaderMapImpl> expected_headers; | ||
expected_headers = Http::createHeaderMap<Http::RequestHeaderMapImpl>({ | ||
{Http::Headers::get().Method, "POST"}, | ||
{Http::Headers::get().Host, this->config_->hostname()}, | ||
{Http::Headers::get().Host, this->config_->host(this->downstream_stream_info_)}, | ||
{Http::Headers::get().Path, "/"}, | ||
}); | ||
|
||
|
@@ -300,7 +312,7 @@ TYPED_TEST(HttpUpstreamRequestEncoderTest, RequestEncoderHeaders) { | |
std::unique_ptr<Http::RequestHeaderMapImpl> expected_headers; | ||
expected_headers = Http::createHeaderMap<Http::RequestHeaderMapImpl>({ | ||
{Http::Headers::get().Method, "CONNECT"}, | ||
{Http::Headers::get().Host, this->config_->hostname()}, | ||
{Http::Headers::get().Host, this->config_->host(this->downstream_stream_info_)}, | ||
}); | ||
|
||
expected_headers->setCopy(Http::LowerCaseString("header0"), "value0"); | ||
|
@@ -328,7 +340,7 @@ TYPED_TEST(HttpUpstreamRequestEncoderTest, ConfigReuse) { | |
std::unique_ptr<Http::RequestHeaderMapImpl> expected_headers; | ||
expected_headers = Http::createHeaderMap<Http::RequestHeaderMapImpl>({ | ||
{Http::Headers::get().Method, "CONNECT"}, | ||
{Http::Headers::get().Host, this->config_->hostname()}, | ||
{Http::Headers::get().Host, this->config_->host(this->downstream_stream_info_)}, | ||
}); | ||
|
||
expected_headers->setCopy(Http::LowerCaseString("key"), "value1"); | ||
|
@@ -371,7 +383,7 @@ TYPED_TEST(HttpUpstreamRequestEncoderTest, RequestEncoderHeadersWithDownstreamIn | |
std::unique_ptr<Http::RequestHeaderMapImpl> expected_headers; | ||
expected_headers = Http::createHeaderMap<Http::RequestHeaderMapImpl>({ | ||
{Http::Headers::get().Method, "CONNECT"}, | ||
{Http::Headers::get().Host, this->config_->hostname()}, | ||
{Http::Headers::get().Host, this->config_->host(this->downstream_stream_info_)}, | ||
}); | ||
|
||
expected_headers->setCopy(Http::LowerCaseString("header0"), "value0"); | ||
|
@@ -383,7 +395,55 @@ TYPED_TEST(HttpUpstreamRequestEncoderTest, RequestEncoderHeadersWithDownstreamIn | |
*Network::Test::getCanonicalLoopbackAddress(ip_versions[0]), 80); | ||
Network::ConnectionInfoSetterImpl connection_info(ip_port, ip_port); | ||
EXPECT_CALL(this->downstream_stream_info_, downstreamAddressProvider) | ||
.WillOnce(testing::ReturnRef(connection_info)); | ||
.WillRepeatedly(testing::ReturnRef(connection_info)); | ||
EXPECT_CALL(this->encoder_, encodeHeaders(HeaderMapEqualRef(expected_headers.get()), false)); | ||
this->upstream_->setRequestEncoder(this->encoder_, false); | ||
} | ||
|
||
TYPED_TEST(HttpUpstreamRequestEncoderTest, | ||
RequestEncoderHostnameWithDownstreamInfoRequestedServerName) { | ||
this->config_message_.set_hostname("%REQUESTED_SERVER_NAME%:443"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we support arbitrary header additions? See #21804. This is a major blocker for us after some recent changes landed. Given we can add arbitrary metadata for all headers except There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change aims to allow setting command operators in the |
||
this->setupUpstream(); | ||
|
||
std::unique_ptr<Http::RequestHeaderMapImpl> expected_headers; | ||
expected_headers = Http::createHeaderMap<Http::RequestHeaderMapImpl>({ | ||
{Http::Headers::get().Method, "CONNECT"}, | ||
{Http::Headers::get().Host, "www.google.com:443"}, | ||
}); | ||
|
||
auto ip_versions = TestEnvironment::getIpVersionsForTest(); | ||
ASSERT_FALSE(ip_versions.empty()); | ||
|
||
auto ip_port = Network::Utility::getAddressWithPort( | ||
*Network::Test::getCanonicalLoopbackAddress(ip_versions[0]), 80); | ||
Network::ConnectionInfoSetterImpl connection_info(ip_port, ip_port); | ||
connection_info.setRequestedServerName("www.google.com"); | ||
EXPECT_CALL(this->downstream_stream_info_, downstreamAddressProvider) | ||
.Times(2) | ||
.WillRepeatedly(testing::ReturnRef(connection_info)); | ||
EXPECT_CALL(this->encoder_, encodeHeaders(HeaderMapEqualRef(expected_headers.get()), false)); | ||
this->upstream_->setRequestEncoder(this->encoder_, false); | ||
} | ||
|
||
TYPED_TEST(HttpUpstreamRequestEncoderTest, | ||
RequestEncoderHostnameWithDownstreamInfoDynamicMetadata) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @howardjohn please take a look at this test. I think it covers your case. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cc: @lambdai There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. awesome, thanks! |
||
this->config_message_.set_hostname("%DYNAMIC_METADATA(tunnel:address)%:443"); | ||
this->setupUpstream(); | ||
|
||
std::unique_ptr<Http::RequestHeaderMapImpl> expected_headers; | ||
expected_headers = Http::createHeaderMap<Http::RequestHeaderMapImpl>({ | ||
{Http::Headers::get().Method, "CONNECT"}, | ||
{Http::Headers::get().Host, "www.google.com:443"}, | ||
}); | ||
|
||
auto ip_versions = TestEnvironment::getIpVersionsForTest(); | ||
ASSERT_FALSE(ip_versions.empty()); | ||
|
||
envoy::config::core::v3::Metadata metadata; | ||
this->populateMetadata(metadata, "tunnel", "address", "www.google.com"); | ||
|
||
EXPECT_CALL(testing::Const(this->downstream_stream_info_), dynamicMetadata()) | ||
.WillRepeatedly(testing::ReturnRef(metadata)); | ||
EXPECT_CALL(this->encoder_, encodeHeaders(HeaderMapEqualRef(expected_headers.get()), false)); | ||
this->upstream_->setRequestEncoder(this->encoder_, false); | ||
} | ||
|
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.
nit: It's better to move this implementation code to
.cc
file to prevent header pollution. alsothis->
is unusual, but not a big deal.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.
Ok, I moved constructor and
host
method to .cc file.