tcp_proxy: extend tunneling_config with auto_sni #20230

jewertow · 2022-03-05T18:37:29Z

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

This pull request is not yet ready for code review.

Commit Message: Extend tcp_proxy.tunneling_config with auto_sni
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Fixes #19612]

repokitteh-read-only · 2022-03-05T18:37:32Z

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: #20230 was opened by jewertow.

see: more, trace.

repokitteh-read-only · 2022-03-05T18:37:37Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @htuch
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #20230 was opened by jewertow.

see: more, trace.

api/envoy/extensions/filters/network/tcp_proxy/v3/tcp_proxy.proto

htuch · 2022-03-06T04:08:06Z

api/envoy/extensions/filters/network/tcp_proxy/v3/tcp_proxy.proto

+      option (validate.required) = true;
+
+      // Static hostname used in request-target of CONNECT request
+      string hostname = 1;


Unfortunately we can't upgrade an existing field to oneof per https://github.com/envoyproxy/envoy/blob/main/api/API_VERSIONING.md

Ok, I understand. So the only way is to deprecate existing field hostname and add another one, let's say static_hostname, and wrap it together with auto_sni in oneof? Take a look at this comment.
Then the schema would be:

message TunnelingConfig { string hostname = 1 [deprecated = true]; oneof hostname_config { string static_hostname = 4; AutoSni auto_sni = 5; } }

We could also go with the relaxed validation and mutual exclusion in the second part of #19612 (comment). Either of these is valid.

sni is indeed a reasonable source of host name header.

Can we relax the hostname value to %EXPR%?
I have the use case that populating the value from %FILTER_STATE(KEY:F):Z% and %DYNAMIC_METADATA(NAMESPACE:KEY*):Z%
It could be done in another PR but I'd like to point out SNI is not only source of required value.

At the beginning I suggested a similar idea, but we decided to add another config option to the API. For more context please read the discussion in the issue.

This api schema enables the original proposal and I like to invest after auto_sni.
Could you update the PR to move "hostname =1" out of one_of per Harvey's comment?
I can reference the new schema in the issue

Could you update the PR to move "hostname =1" out of one_of

Done

lambdai · 2022-03-16T16:46:29Z

api/envoy/extensions/filters/network/tcp_proxy/v3/tcp_proxy.proto

+    message AutoSni {
+      // Optional port used to create the request-target for all CONNECT request, i.e. CONNECT <hostname_from_sni>:<default_port>
+      // If default_port is not specified, 443 is used.
+      uint32 port = 1;


uint32 is not an optional type. Do you want to use Uint32 or clarify the default_port means value 0?

Yes, you're right. I used uint32 intentionally and by "optional" I meant that 0 value will be handled as 443. I will fix this description.

lambdai · 2022-03-16T16:53:09Z

api/envoy/extensions/filters/network/tcp_proxy/v3/tcp_proxy.proto

+      option (validate.required) = true;
+
+      // Static hostname used in request-target of CONNECT request
+      string hostname = 1;


This api schema enables the original proposal and I like to invest after auto_sni.
Could you update the PR to move "hostname =1" out of one_of per Harvey's comment?
I can reference the new schema in the issue

github-actions · 2022-04-15T20:01:20Z

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

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

jewertow · 2022-04-21T21:05:50Z

Don't close this PR, I work on it.

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

I am not convinced about this API though, since we can't wrap hostname and auto_sni in oneof. Without oneof the API will not be intuitive. I decided to open an alternative PR: #21067, which allows to interpolate %REQUESTED_SERVER_NAME% in the tunneling_config.hostname. I think it's better than overwriting headers and more flexible than aut_sni, because allows to support also other values in the future.

htuch · 2022-05-02T04:42:34Z

The alternative looks quite reasonable to me.

github-actions · 2022-06-01T08:02:46Z

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

github-actions · 2022-06-08T12:01:33Z

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

repokitteh-read-only bot added the api label Mar 5, 2022

repokitteh-read-only bot assigned htuch Mar 5, 2022

htuch reviewed Mar 6, 2022

View reviewed changes

lambdai reviewed Mar 16, 2022

View reviewed changes

jewertow mentioned this pull request Mar 29, 2022

Tunneling outbound traffic istio/istio#37968

Merged

github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Apr 15, 2022

jewertow force-pushed the tcp-proxy-tunneling-config-auto-sni branch from ea377fb to a4d4f7c Compare April 20, 2022 21:23

jewertow added 4 commits April 20, 2022 23:23

Extend TcpProxy.TunnelingConfig with auto_sni

1937607

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

Set hostname automatically based on SNI

8b55898

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

Remove length validation of hostname in tunneling_config.

46f6fca

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

Remove comment

a4d4f7c

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

github-actions bot removed the stale stalebot believes this issue/PR has not been touched recently label Apr 22, 2022

jewertow mentioned this pull request Apr 28, 2022

tcp_proxy: support command operators in tunneling_config.hostname #21067

Merged

github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Jun 1, 2022

github-actions bot closed this Jun 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tcp_proxy: extend tunneling_config with auto_sni #20230

tcp_proxy: extend tunneling_config with auto_sni #20230

jewertow commented Mar 5, 2022

repokitteh-read-only bot commented Mar 5, 2022

repokitteh-read-only bot commented Mar 5, 2022

htuch Mar 6, 2022

jewertow Mar 6, 2022

htuch Mar 9, 2022

lambdai Mar 16, 2022

jewertow Mar 16, 2022

lambdai Mar 16, 2022

jewertow Mar 16, 2022 •

edited

Loading

lambdai Mar 16, 2022

jewertow Mar 16, 2022

jewertow Mar 16, 2022

lambdai Mar 16, 2022

github-actions bot commented Apr 15, 2022

jewertow commented Apr 21, 2022

jewertow commented Apr 28, 2022

htuch commented May 2, 2022

github-actions bot commented Jun 1, 2022

github-actions bot commented Jun 8, 2022

tcp_proxy: extend tunneling_config with auto_sni #20230

tcp_proxy: extend tunneling_config with auto_sni #20230

Conversation

jewertow commented Mar 5, 2022

repokitteh-read-only bot commented Mar 5, 2022

repokitteh-read-only bot commented Mar 5, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jewertow Mar 16, 2022 • edited Loading

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

github-actions bot commented Apr 15, 2022

jewertow commented Apr 21, 2022

jewertow commented Apr 28, 2022

htuch commented May 2, 2022

github-actions bot commented Jun 1, 2022

github-actions bot commented Jun 8, 2022

jewertow Mar 16, 2022 •

edited

Loading