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

tcp_proxy: support command operators in tunneling_config.hostname #21067

Merged
Show file tree
Hide file tree
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
16 changes: 16 additions & 0 deletions api/envoy/extensions/filters/network/tcp_proxy/v3/tcp_proxy.proto
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,22 @@ message TcpProxy {
"envoy.config.filter.network.tcp_proxy.v2.TcpProxy.TunnelingConfig";

// The hostname to send in the synthesized CONNECT headers to the upstream proxy.
// This field evaluates command operators if set, otherwise returns hostname as is.
//
// Example: dynamically set hostname using downstream SNI
//
// .. code-block:: yaml
//
// tunneling_config:
// hostname: "%REQUESTED_SERVER_NAME%:443"
//
// Example: dynamically set hostname using dynamic metadata
//
// .. code-block: yaml
//
// tunneling_config:
// hostname: "%DYNAMIC_METADATA(tunnel:address)%"
//
string hostname = 1 [(validate.rules).string = {min_len: 1}];

// Use POST method instead of CONNECT method to tunnel the TCP stream.
Expand Down
3 changes: 3 additions & 0 deletions changelogs/current.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ behavior_changes:
Fixed metric tag extraction so that :ref:`stat_prefix <envoy_v3_api_field_extensions.filters.network.redis_proxy.v3.RedisProxy.stat_prefix>`
is properly extracted. This changes the Prometheus name from
envoy_redis_myprefix_command_pttl_latency_sum{} to envoy_redis_command_pttl_latency_sum{envoy_redis_prefix="myprefix"}.
- area: tcp_proxy
change: |
added support for command operators in :ref:`TunnelingConfig hostname <envoy_v3_api_field_extensions.filters.network.tcp_proxy.v3.TcpProxy.TunnelingConfig.hostname>` to dynamically set upstream hostname.
minor_behavior_changes:
- area: thrift
Expand Down
4 changes: 3 additions & 1 deletion envoy/tcp/upstream.h
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,10 @@ class GenericUpstream;
class TunnelingConfigHelper {
public:
virtual ~TunnelingConfigHelper() = default;

// The host name of the tunneling upstream HTTP request.
virtual const std::string& hostname() const PURE;
// This function evaluates command operators if specified. Otherwise it returns host name as is.
virtual std::string host(const StreamInfo::StreamInfo& stream_info) const PURE;

// The method of the upstream HTTP request. True if using POST method, CONNECT otherwise.
virtual bool usePost() const PURE;
Expand Down
2 changes: 2 additions & 0 deletions source/common/tcp_proxy/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ envoy_cc_library(
"//source/common/common:empty_string",
"//source/common/common:macros",
"//source/common/common:minimal_logger_lib",
"//source/common/formatter:substitution_format_string_lib",
"//source/common/http:codec_client_lib",
"//source/common/network:application_protocol_lib",
"//source/common/network:cidr_range_lib",
Expand All @@ -71,6 +72,7 @@ envoy_cc_library(
"//source/common/network:upstream_server_name_lib",
"//source/common/network:upstream_socket_options_filter_state_lib",
"//source/common/network:utility_lib",
"//source/common/protobuf:utility_lib",
"//source/common/router:metadatamatchcriteria_lib",
"//source/common/stream_info:stream_info_lib",
"//source/common/upstream:load_balancer_lib",
Expand Down
23 changes: 22 additions & 1 deletion source/common/tcp_proxy/tcp_proxy.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -78,7 +79,7 @@ Config::SharedConfig::SharedConfig(
}
if (config.has_tunneling_config()) {
tunneling_config_helper_ =
std::make_unique<TunnelingConfigHelperImpl>(config.tunneling_config());
std::make_unique<TunnelingConfigHelperImpl>(config.tunneling_config(), context);
}
if (config.has_max_downstream_connection_duration()) {
const uint64_t connection_duration =
Expand Down Expand Up @@ -539,6 +540,26 @@ const Router::MetadataMatchCriteria* Filter::metadataMatchCriteria() {
}
}

TunnelingConfigHelperImpl::TunnelingConfigHelperImpl(
const envoy::extensions::filters::network::tcp_proxy::v3::TcpProxy_TunnelingConfig&
config_message,
Server::Configuration::FactoryContext& context)
: use_post_(config_message.use_post()),
header_parser_(Envoy::Router::HeaderParser::configure(config_message.headers_to_add())) {
envoy::config::core::v3::SubstitutionFormatString substitution_format_config;
substitution_format_config.mutable_text_format_source()->set_inline_string(
config_message.hostname());
hostname_fmt_ = Formatter::SubstitutionFormatStringUtils::fromProtoConfig(
substitution_format_config, context);
}

std::string TunnelingConfigHelperImpl::host(const StreamInfo::StreamInfo& stream_info) const {
return hostname_fmt_->format(*Http::StaticEmptyHeaders::get().request_headers,
*Http::StaticEmptyHeaders::get().response_headers,
*Http::StaticEmptyHeaders::get().response_trailers, stream_info,
absl::string_view());
}

void Filter::onConnectTimeout() {
ENVOY_CONN_LOG(debug, "connect timeout", read_callbacks_->connection());
read_callbacks_->upstreamHost()->outlierDetector().putResult(
Expand Down
11 changes: 6 additions & 5 deletions source/common/tcp_proxy/tcp_proxy.h
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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,
Copy link
Contributor

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. also this-> is unusual, but not a big deal.

Copy link
Contributor Author

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.

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_;
Copy link
Contributor

Choose a reason for hiding this comment

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

Make this const Formatter::FormatterPtr.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't, because fromProtoConfig does not return const.

Copy link
Member

Choose a reason for hiding this comment

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

You can if you initialize this in initialization list and extract

envoy::config::core::v3::SubstitutionFormatString substitution_format_config;
  substitution_format_config.mutable_text_format_source()->set_inline_string(
      config_message.hostname());
  return Formatter::SubstitutionFormatStringUtils::fromProtoConfig(
      substitution_format_config, context);

to a helper function.

};

/**
Expand Down
4 changes: 2 additions & 2 deletions source/common/tcp_proxy/upstream.cc
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,7 @@ void Http2Upstream::setRequestEncoder(Http::RequestEncoder& request_encoder, boo
is_ssl ? Http::Headers::get().SchemeValues.Https : Http::Headers::get().SchemeValues.Http;
auto headers = Http::createHeaderMap<Http::RequestHeaderMapImpl>({
{Http::Headers::get().Method, config_.usePost() ? "POST" : "CONNECT"},
{Http::Headers::get().Host, config_.hostname()},
{Http::Headers::get().Host, config_.host(downstream_info_)},
});

if (config_.usePost()) {
Expand Down Expand Up @@ -309,7 +309,7 @@ void Http1Upstream::setRequestEncoder(Http::RequestEncoder& request_encoder, boo

auto headers = Http::createHeaderMap<Http::RequestHeaderMapImpl>({
{Http::Headers::get().Method, config_.usePost() ? "POST" : "CONNECT"},
{Http::Headers::get().Host, config_.hostname()},
{Http::Headers::get().Host, config_.host(downstream_info_)},
});

if (config_.usePost()) {
Expand Down
1 change: 1 addition & 0 deletions test/common/tcp_proxy/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ envoy_cc_test(
deps = [
"//source/common/tcp_proxy",
"//test/mocks/http:http_mocks",
"//test/mocks/server:factory_context_mocks",
"//test/mocks/tcp:tcp_mocks",
"//test/mocks/upstream:cluster_manager_mocks",
"//test/test_common:test_runtime_lib",
Expand Down
78 changes: 69 additions & 9 deletions test/common/tcp_proxy/upstream_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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_;
Expand All @@ -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_) {
Expand All @@ -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));
Expand All @@ -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, "/"},
});

Expand Down Expand Up @@ -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");
Expand Down Expand Up @@ -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");
Expand Down Expand Up @@ -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");
Expand All @@ -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");
Copy link
Contributor

Choose a reason for hiding this comment

The 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 :authority today it seems like this should be feasible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change aims to allow setting command operators in the tunneling_config.hostname field to avoid setting empty or dummy hostname and then overriding header "Host" or ":authority" which are not allowed to override. But as @kyessenov requested, I'm refactoring this implementation to rely on substitution formatter which supports %DYNAMIC_METADATA%, so this change should provide what you need.

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) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc: @lambdai

Copy link
Contributor

Choose a reason for hiding this comment

The 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);
}
Expand Down
1 change: 1 addition & 0 deletions test/extensions/upstreams/tcp/generic/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ envoy_cc_test(
deps = [
"//source/common/tcp_proxy",
"//source/extensions/upstreams/tcp/generic:config",
"//test/mocks/server:factory_context_mocks",
"//test/mocks/upstream:upstream_mocks",
],
)
8 changes: 5 additions & 3 deletions test/extensions/upstreams/tcp/generic/config_test.cc
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#include "source/common/tcp_proxy/tcp_proxy.h"
#include "source/extensions/upstreams/tcp/generic/config.h"

#include "test/mocks/server/factory_context.h"
#include "test/mocks/tcp/mocks.h"
#include "test/mocks/upstream/cluster_manager.h"
#include "test/mocks/upstream/load_balancer_context.h"
Expand Down Expand Up @@ -31,12 +32,13 @@ class TcpConnPoolTest : public ::testing::Test {
NiceMock<StreamInfo::MockStreamInfo> downstream_stream_info_;
NiceMock<Network::MockConnection> connection_;
Upstream::MockLoadBalancerContext lb_context_;
NiceMock<Server::Configuration::MockFactoryContext> context_;
};

TEST_F(TcpConnPoolTest, TestNoConnPool) {
envoy::extensions::filters::network::tcp_proxy::v3::TcpProxy_TunnelingConfig config_proto;
config_proto.set_hostname("host");
const TcpProxy::TunnelingConfigHelperImpl config(config_proto);
const TcpProxy::TunnelingConfigHelperImpl config(config_proto, context_);
EXPECT_CALL(thread_local_cluster_, httpConnPool(_, _, _)).WillOnce(Return(absl::nullopt));
EXPECT_EQ(nullptr, factory_.createGenericConnPool(
thread_local_cluster_, TcpProxy::TunnelingConfigHelperOptConstRef(config),
Expand All @@ -49,7 +51,7 @@ TEST_F(TcpConnPoolTest, Http2Config) {
EXPECT_CALL(thread_local_cluster_, info).WillOnce(Return(info));
envoy::extensions::filters::network::tcp_proxy::v3::TcpProxy_TunnelingConfig config_proto;
config_proto.set_hostname("host");
const TcpProxy::TunnelingConfigHelperImpl config(config_proto);
const TcpProxy::TunnelingConfigHelperImpl config(config_proto, context_);

EXPECT_CALL(thread_local_cluster_, httpConnPool(_, _, _)).WillOnce(Return(absl::nullopt));
EXPECT_EQ(nullptr, factory_.createGenericConnPool(
Expand All @@ -65,7 +67,7 @@ TEST_F(TcpConnPoolTest, Http3Config) {
EXPECT_CALL(thread_local_cluster_, info).Times(AnyNumber()).WillRepeatedly(Return(info));
envoy::extensions::filters::network::tcp_proxy::v3::TcpProxy_TunnelingConfig config_proto;
config_proto.set_hostname("host");
const TcpProxy::TunnelingConfigHelperImpl config(config_proto);
const TcpProxy::TunnelingConfigHelperImpl config(config_proto, context_);
EXPECT_CALL(thread_local_cluster_, httpConnPool(_, _, _)).WillOnce(Return(absl::nullopt));
EXPECT_EQ(nullptr, factory_.createGenericConnPool(
thread_local_cluster_, TcpProxy::TunnelingConfigHelperOptConstRef(config),
Expand Down