-
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 7 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 |
---|---|---|
|
@@ -9,6 +9,7 @@ | |
#include "envoy/event/dispatcher.h" | ||
#include "envoy/event/timer.h" | ||
#include "envoy/extensions/filters/network/tcp_proxy/v3/tcp_proxy.pb.h" | ||
#include "envoy/extensions/filters/network/tcp_proxy/v3/tcp_proxy.pb.validate.h" | ||
#include "envoy/stats/scope.h" | ||
#include "envoy/upstream/cluster_manager.h" | ||
#include "envoy/upstream/upstream.h" | ||
|
@@ -22,6 +23,7 @@ | |
#include "source/common/common/utility.h" | ||
#include "source/common/config/utility.h" | ||
#include "source/common/config/well_known_names.h" | ||
#include "source/common/formatter/substitution_format_string.h" | ||
#include "source/common/network/application_protocol.h" | ||
#include "source/common/network/proxy_protocol_filter_state.h" | ||
#include "source/common/network/socket_option_factory.h" | ||
|
@@ -77,8 +79,14 @@ Config::SharedConfig::SharedConfig( | |
idle_timeout_ = std::chrono::hours(1); | ||
} | ||
if (config.has_tunneling_config()) { | ||
tunneling_config_helper_ = | ||
std::make_unique<TunnelingConfigHelperImpl>(config.tunneling_config()); | ||
envoy::config::core::v3::SubstitutionFormatString substitution_format_config; | ||
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 think this code could be made part of 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. Good idea, I did it. |
||
substitution_format_config.mutable_text_format_source()->set_inline_string( | ||
config.tunneling_config().hostname()); | ||
Formatter::FormatterPtr hostname_fmt = | ||
Formatter::SubstitutionFormatStringUtils::fromProtoConfig(substitution_format_config, | ||
context); | ||
tunneling_config_helper_ = std::make_unique<TunnelingConfigHelperImpl>( | ||
config.tunneling_config(), std::move(hostname_fmt)); | ||
} | ||
if (config.has_max_downstream_connection_duration()) { | ||
const uint64_t connection_duration = | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,7 @@ | |
#include "envoy/upstream/upstream.h" | ||
|
||
#include "source/common/common/logger.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 +115,25 @@ class TunnelingConfigHelperImpl : public TunnelingConfigHelper { | |
public: | ||
TunnelingConfigHelperImpl( | ||
const envoy::extensions::filters::network::tcp_proxy::v3::TcpProxy_TunnelingConfig& | ||
config_message) | ||
config_message, | ||
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. nit: It's better to move this implementation code to 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. Ok, I moved constructor and |
||
Formatter::FormatterPtr hostname_fmt) | ||
: 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_; } | ||
header_parser_(Envoy::Router::HeaderParser::configure(config_message.headers_to_add())), | ||
hostname_fmt_(std::move(hostname_fmt)) {} | ||
std::string host(const StreamInfo::StreamInfo& stream_info) const override { | ||
return hostname_fmt_->format(*Http::StaticEmptyHeaders::get().request_headers, | ||
*Http::StaticEmptyHeaders::get().response_headers, | ||
*Http::StaticEmptyHeaders::get().response_trailers, stream_info, | ||
absl::string_view()); | ||
} | ||
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 |
---|---|---|
@@ -1,11 +1,13 @@ | ||
#include <memory> | ||
|
||
#include "source/common/formatter/substitution_format_string.h" | ||
#include "source/common/tcp_proxy/tcp_proxy.h" | ||
#include "source/common/tcp_proxy/upstream.h" | ||
|
||
#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 +42,25 @@ template <typename T> class HttpUpstreamTest : public testing::Test { | |
} | ||
|
||
void setupUpstream() { | ||
config_ = std::make_unique<TunnelingConfigHelperImpl>(config_message_); | ||
envoy::config::core::v3::SubstitutionFormatString substitution_format_config; | ||
substitution_format_config.mutable_text_format_source()->set_inline_string( | ||
config_message_.hostname()); | ||
Formatter::FormatterPtr hostname_fmt = | ||
Formatter::SubstitutionFormatStringUtils::fromProtoConfig(substitution_format_config, | ||
context_); | ||
config_ = std::make_unique<TunnelingConfigHelperImpl>(config_message_, std::move(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. If you moved formatter construction into helper you wouldn't have to duplicate this logic several times. 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. Done |
||
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 +217,29 @@ template <typename T> class HttpUpstreamRequestEncoderTest : public testing::Tes | |
} | ||
|
||
void setupUpstream() { | ||
config_ = std::make_unique<TunnelingConfigHelperImpl>(config_message_); | ||
envoy::config::core::v3::SubstitutionFormatString substitution_format_config; | ||
substitution_format_config.mutable_text_format_source()->set_inline_string( | ||
config_message_.hostname()); | ||
Formatter::FormatterPtr hostname_fmt = | ||
Formatter::SubstitutionFormatStringUtils::fromProtoConfig(substitution_format_config, | ||
context_); | ||
config_ = std::make_unique<TunnelingConfigHelperImpl>(config_message_, std::move(hostname_fmt)); | ||
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) { | ||
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. nit: these could be 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. Done |
||
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 +257,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 +277,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 +290,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 +325,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 +353,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 +396,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 +408,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.
I think the label should be:
envoy_v3_api_field_extensions.filters.network.tcp_proxy.v3.TcpProxy.TunnelingConfig.hostname
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.
Thanks, I fixed it.