tcp_proxy: support command operators in tunneling_config.hostname #21067

jewertow · 2022-04-28T14:19:40Z

Signed-off-by: Jacek Ewertowski jacek.ewertowski1@gmail.com

Commit Message: tcp_proxy: support command operators in tunneling_config.hostname
Additional Description: This change enables dynamically setting tunneling_config.hostname with command operators.
Risk Level: Low
Testing: added unit tests
Docs Changes: done
Release Notes: done
Platform Specific Features: none
[Optional Fixes #19612 #21804]

This pull request is an alternative for auto_sni.

This change allows to configure TCP proxy as follows:

tunneling_config:
  hostname: %REQUESTED_SERVER_NAME%:443

repokitteh-read-only · 2022-04-28T14:19:44Z

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #21067 was opened by jewertow.

see: more, trace.

jewertow · 2022-04-28T14:21:39Z

@ramaraochavali @lambdai what do you think?

ramaraochavali

Thank you. LGTM

jewertow · 2022-05-11T20:03:57Z

@alyssawilk could you take a look at this pull request? I didn't add integration test yet, but you can review the general idea and implementation. This approach makes it possible to support also other command operators if make sense. The advantage of this approach in contrary to overwriting Host header is that the hostname is not empty nor set to a dummy value what would be confusing.

I gave up the idea of auto_sni, because it's not backward compatible to wrap hostname and auto_sni in oneof. So the idea does not make sense, because it would be required to define empty or dummy hostname and I wanted to avoid it.

source/common/tcp_proxy/tcp_proxy.h

kyessenov

Please make sure exceptions are not thrown on the data path, otherwise, looks reasonable.

source/common/tcp_proxy/tcp_proxy.h

kyessenov · 2022-05-26T16:01:48Z

/wait-any

jewertow · 2022-05-26T20:19:31Z

Regarding to your comment: I would like to complete this feature. Are you okay with that? I also would be interested to extend this feature to support dynamic metadata in subsequent pull request.

KBaichoo · 2022-06-01T14:40:16Z

@kyessenov I think this is waiting on your reply; PTAL

@jewertow seems like you need to merge main

kyessenov · 2022-06-01T14:58:36Z

I spoke with @jewertow offline, and made suggestions to use the formatter library. I think either that or fixing this version to not throw on data path still needs to be added.

kyessenov · 2022-06-01T14:58:49Z

/wait-any

lambdai · 2022-06-09T18:47:57Z

Ping @jewertow
Are you still working on it? It's an easy one to remove the exception.

I'd like to propose the richer formatter on top of this change. Let me know if you want to complete this one

lambdai · 2022-06-09T20:15:59Z

/wait-any restore the flag

Signed-off-by: Jacek Ewertowski <jacek.ewertowski1@gmail.com>

kyessenov

Generally looks good, but there are a few comments that could improve this I think.

kyessenov · 2022-06-28T21:48:27Z

changelogs/current.yaml

@@ -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_envoy.config.filter.network.tcp_proxy.v3.TcpProxy.TunnelingConfig.hostname>` to dynamically set upstream hostname.


I think the label should be:
envoy_v3_api_field_extensions.filters.network.tcp_proxy.v3.TcpProxy.TunnelingConfig.hostname

Thanks, I fixed it.

kyessenov · 2022-06-28T21:50:33Z

source/common/tcp_proxy/tcp_proxy.h

  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_;


Make this const Formatter::FormatterPtr.

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

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.

kyessenov · 2022-06-28T21:52:47Z

source/common/tcp_proxy/tcp_proxy.cc

@@ -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;


I think this code could be made part of TunnelingConfigHelperImpl. E.g. add a function there, and initialize const formatter field with a function call.

Good idea, I did it.

kyessenov · 2022-06-28T21:53:21Z

test/common/tcp_proxy/upstream_test.cc

+    Formatter::FormatterPtr hostname_fmt =
+        Formatter::SubstitutionFormatStringUtils::fromProtoConfig(substitution_format_config,
+                                                                  context_);
+    config_ = std::make_unique<TunnelingConfigHelperImpl>(config_message_, std::move(hostname_fmt));


If you moved formatter construction into helper you wouldn't have to duplicate this logic several times.

kyessenov · 2022-06-28T22:11:53Z

@jewertow Please don't force push. That makes it hard to track previous comments. Thanks.
/wait

Signed-off-by: Jacek Ewertowski <jacek.ewertowski1@gmail.com>

kyessenov · 2022-06-29T21:52:07Z

test/common/tcp_proxy/upstream_test.cc

    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) {


nit: these could be const std::string&

kyessenov · 2022-06-29T21:52:12Z

source/common/tcp_proxy/tcp_proxy.h

-      : 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,


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.

Ok, I moved constructor and host method to .cc file.

kyessenov

LGTM, need @envoyproxy/api-shepherds approval for proto change, and a couple of code nits.

Signed-off-by: Jacek Ewertowski <jacek.ewertowski1@gmail.com>

…ader file to cc file Signed-off-by: Jacek Ewertowski <jacek.ewertowski1@gmail.com>

jewertow · 2022-06-30T15:21:07Z

@htuch you are familiar with the issue, so could you review this PR?

jewertow · 2022-06-30T17:49:12Z

/retest

repokitteh-read-only · 2022-06-30T17:49:19Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #21067 (comment) was created by @jewertow.

see: more, trace.

lizan

/lgtm api

jewertow mentioned this pull request Apr 28, 2022

tcp_proxy: extend tunneling_config with auto_sni #20230

Closed

ramaraochavali reviewed May 5, 2022

View reviewed changes

jewertow force-pushed the tcp-proxy-tunneling-config-hostname-requested-server-name branch from a593ae9 to a1d758d Compare May 6, 2022 18:06

jewertow marked this pull request as ready for review May 11, 2022 19:53

kyessenov reviewed May 11, 2022

View reviewed changes

source/common/tcp_proxy/tcp_proxy.h Outdated Show resolved Hide resolved

repokitteh-read-only bot added waiting and removed waiting labels May 11, 2022

mattklein123 assigned kyessenov May 16, 2022

kyessenov reviewed May 16, 2022

View reviewed changes

source/common/tcp_proxy/tcp_proxy.h Outdated Show resolved Hide resolved

source/common/tcp_proxy/tcp_proxy.h Outdated Show resolved Hide resolved

kyessenov mentioned this pull request May 25, 2022

http: fixing up CONNECT to be RFC compliant #21440

Merged

repokitteh-read-only bot added waiting:any and removed waiting:any labels May 26, 2022

repokitteh-read-only bot added the waiting:any label Jun 1, 2022

repokitteh-read-only bot removed the waiting:any label Jun 9, 2022

repokitteh-read-only bot added the waiting:any label Jun 9, 2022

jewertow force-pushed the tcp-proxy-tunneling-config-hostname-requested-server-name branch from 940ec2a to 2a925c6 Compare June 21, 2022 15:40

repokitteh-read-only bot removed the waiting:any label Jun 21, 2022

jewertow force-pushed the tcp-proxy-tunneling-config-hostname-requested-server-name branch 2 times, most recently from bd5f4e4 to 96dd84c Compare June 21, 2022 21:41

jewertow changed the title ~~Allow to set %REQUESTED_SERVER_NAME% in tunneling_config.hostname~~ WIP: Allow to set %REQUESTED_SERVER_NAME% in tunneling_config.hostname Jun 21, 2022

Describe change in the current changelog

c0d27c1

Signed-off-by: Jacek Ewertowski <jacek.ewertowski1@gmail.com>

jewertow force-pushed the tcp-proxy-tunneling-config-hostname-requested-server-name branch from 6825d7b to c0d27c1 Compare June 28, 2022 19:00

kyessenov requested changes Jun 28, 2022

View reviewed changes

repokitteh-read-only bot added the waiting label Jun 28, 2022

jewertow added 3 commits June 29, 2022 17:47

Move creating hostname formatter to TunnelingConfigHelper constructor

1825288

Signed-off-by: Jacek Ewertowski <jacek.ewertowski1@gmail.com>

Remove unused field hostname_ from TunnelingConfigHelper

844473a

Signed-off-by: Jacek Ewertowski <jacek.ewertowski1@gmail.com>

Fix label in the documentation

72b000d

Signed-off-by: Jacek Ewertowski <jacek.ewertowski1@gmail.com>

repokitteh-read-only bot removed the waiting label Jun 29, 2022

jewertow added 3 commits June 29, 2022 18:03

Fix creating of TunnelingConfigHelper in config_test

a107138

Signed-off-by: Jacek Ewertowski <jacek.ewertowski1@gmail.com>

Remove unused header

66011c4

Signed-off-by: Jacek Ewertowski <jacek.ewertowski1@gmail.com>

Fix code format

6f9fed7

Signed-off-by: Jacek Ewertowski <jacek.ewertowski1@gmail.com>

jewertow requested a review from kyessenov June 29, 2022 17:10

kyessenov reviewed Jun 29, 2022

View reviewed changes

kyessenov previously approved these changes Jun 29, 2022

View reviewed changes

jewertow added 2 commits June 30, 2022 17:06

Use string reference instead of string in populateMetadata function

5117b94

Signed-off-by: Jacek Ewertowski <jacek.ewertowski1@gmail.com>

Move constructor and host method of TunnelingConfigHelperImpl from he…

200cc44

…ader file to cc file Signed-off-by: Jacek Ewertowski <jacek.ewertowski1@gmail.com>

jewertow dismissed kyessenov’s stale review via 200cc44 June 30, 2022 15:17

jewertow requested a review from kyessenov June 30, 2022 15:19

kyessenov approved these changes Jun 30, 2022

View reviewed changes

lizan reviewed Jul 1, 2022

View reviewed changes

repokitteh-read-only bot removed the api label Jul 1, 2022

kyessenov merged commit 764a2e9 into envoyproxy:main Jul 6, 2022

kyleli666 mentioned this pull request Dec 20, 2024

Dynamic setting value of spec.trafficPolicy.tunnel.targetHost/targetPort in DestinationRule istio/istio#54433

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tcp_proxy: support command operators in tunneling_config.hostname #21067

tcp_proxy: support command operators in tunneling_config.hostname #21067

jewertow commented Apr 28, 2022 •

edited

Loading

repokitteh-read-only bot commented Apr 28, 2022

jewertow commented Apr 28, 2022 •

edited

Loading

ramaraochavali left a comment

jewertow commented May 11, 2022

kyessenov left a comment

kyessenov commented May 26, 2022

jewertow commented May 26, 2022

KBaichoo commented Jun 1, 2022

kyessenov commented Jun 1, 2022

kyessenov commented Jun 1, 2022

lambdai commented Jun 9, 2022

lambdai commented Jun 9, 2022

kyessenov left a comment

kyessenov Jun 28, 2022

jewertow Jun 29, 2022

kyessenov Jun 28, 2022

jewertow Jun 29, 2022

lizan Jul 1, 2022

kyessenov Jun 28, 2022

jewertow Jun 29, 2022

kyessenov Jun 28, 2022

jewertow Jun 29, 2022

kyessenov commented Jun 28, 2022

kyessenov Jun 29, 2022

jewertow Jun 30, 2022

kyessenov Jun 29, 2022

jewertow Jun 30, 2022

kyessenov left a comment

jewertow commented Jun 30, 2022

jewertow commented Jun 30, 2022

repokitteh-read-only bot commented Jun 30, 2022

lizan left a comment

tcp_proxy: support command operators in tunneling_config.hostname #21067

tcp_proxy: support command operators in tunneling_config.hostname #21067

Conversation

jewertow commented Apr 28, 2022 • edited Loading

repokitteh-read-only bot commented Apr 28, 2022

jewertow commented Apr 28, 2022 • edited Loading

ramaraochavali left a comment

Choose a reason for hiding this comment

jewertow commented May 11, 2022

kyessenov left a comment

Choose a reason for hiding this comment

kyessenov commented May 26, 2022

jewertow commented May 26, 2022

KBaichoo commented Jun 1, 2022

kyessenov commented Jun 1, 2022

kyessenov commented Jun 1, 2022

lambdai commented Jun 9, 2022

lambdai commented Jun 9, 2022

kyessenov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyessenov commented Jun 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyessenov left a comment

Choose a reason for hiding this comment

jewertow commented Jun 30, 2022

jewertow commented Jun 30, 2022

repokitteh-read-only bot commented Jun 30, 2022

lizan left a comment

Choose a reason for hiding this comment

jewertow commented Apr 28, 2022 •

edited

Loading

jewertow commented Apr 28, 2022 •

edited

Loading