tcp_proxy: Add filter state envoy.tcp_proxy.receive_before_connect by jrajahalme · Pull Request #25804 · envoyproxy/envoy

jrajahalme · 2023-02-27T22:42:47Z

Commit Message:

Add filter state object envoy.tcp_proxy.receive_before_connect that TcpProxy
filter checks for and when set to 'true' will not set readDisable=true on the
downstream connection before upstream connection is established.

proxy_receive_before_connect needs to be set a read filter's
initializeReadFilterCallbacks() as tcp_proxy checks for this in its
initializeReadFilterCallbacks() that is called when the filter chain is created,
but after all the other filters as tcp_proxy is the last filter in the chain.

This change allows advanced cases where a network read filter needs to
receive (and possibly send) downstream data that may change the
destination of the upstream TCP connection, e.g. via metadata set based
on the received data. If this is the case, then the read filter preceding
the tcp_proxy filter must return StopIteration from its onNewConnection()
call, causing tcp_proxy filter to postpone its upstream connection
establishment until onData() returns Continue.

Any data reaching the tcp_proxy filter before the upstream connection is
established is buffered so that the downstream filters do not see the
same data again which would be the case if it would remaining in the buffer
and more data is received. This also allows downstream filters inject
data before the upstream connection is established; such injected data
would be lost if tcp_proxy would not buffer it while connection
establishment is still ongoing.

An existing dynamic metadata integration test is modified to use
proxy_receive_before_connect=true. This makes the use case less
reliant on the tcp_proxy internal detail, such as balancing and timing of
the readDisable() calls on the downstream connection.

Additional Description:
Risk Level: Low
Testing: Existing test is enhanced to cover the new code
Docs Changes:
Release Notes: Tcp proxy filter can now be configured to receive data from downstream connection before the upstream connection is established with the new receive_before_connect configuration option.
Platform Specific Features:

repokitteh-read-only · 2023-02-27T22:42:56Z

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

🐱

Caused by: #25804 was opened by jrajahalme.

see: more, trace.

jrajahalme · 2023-02-28T11:21:19Z

Force-pushed to fix format.

jrajahalme · 2023-03-01T07:50:22Z

Force pushed to fix spelling

botengyao

Thanks! Overall reasonable for me, but we need @ggreenway's input as well I think.

botengyao · 2023-03-03T23:24:30Z

test/integration/tcp_proxy_integration_test.cc

This is not the early data buffer you create in Tcp proxy, right? It is a little bit confusing for the comment, like we stop the filter chain iteration at InjectDynamicMetadata::onData() when just receives pri. Tcp proxy cannot get the onData call before the InjectDynamicMetadata continues the filter chain.

And writing a new set of integration tests maybe more clear here to make sure the original behavior is also tested. We should also add some situations that trigger early_data_buffer_ when connection is not created, but seems not easy to test.

@jrajahalme please address this.

botengyao · 2023-03-03T23:43:41Z

test/integration/tcp_proxy_integration_test.cc

Actually we can read-enable here, seems this PR will only buy some time back between the initialization and onNewConnection?

This PR does two things:

asks the (TCP) proxy to not read-disable in the first place, so that other filters do not need to know the exact spots where to read-enable and disable again. This enables filters receiving data before and while the upstream connection setup takes place.

adds early data buffering to the tcp_proxy filter, so that filters can write (early) data after tcp_proxy has started connect() system call, but the connection has not yet been established. As of now tcp_proxy will assert-fail if it receives data before the connection has been established. This allows filters that read data enabled by point (1) to pass it on without buffering and waiting for connection setup to complete.

botengyao · 2023-03-03T23:46:55Z

/assign @ggreenway

ggreenway

This is basically a compatibility problem between different filters. In my opinion, it would be nicer to do this mechanism via some communication between the filters (maybe a FilterState), not via a config knob. The user shouldn't have to keep track of whether their other network filter requires this tcp_proxy setting. And accidentally setting this when the other network filters aren't handling the data will result in buggy behavior.

/wait

jrajahalme · 2023-03-13T12:02:56Z

This is basically a compatibility problem between different filters. In my opinion, it would be nicer to do this mechanism via some communication between the filters (maybe a FilterState), not via a config knob. The user shouldn't have to keep track of whether their other network filter requires this tcp_proxy setting. And accidentally setting this when the other network filters aren't handling the data will result in buggy behavior.

Changed to introduce a new filter state instead as suggested here. Since reverting API changes in a new commit could have been confusing, I force pushed the code with the new approach, hope this is OK.

jrajahalme · 2023-03-14T13:27:19Z

@ggreenway Care to take another look?

ggreenway

/wait

ggreenway · 2023-03-14T16:35:46Z

source/common/network/proxy_receive_before_connect.h

I don't think we need a new class for this; you can use StreamInfo::BoolAccessor. Also, the definition of the key belongs in source/common/tcp_proxy/tcp_proxy.h.

Related example that was recently added: constexpr absl::string_view DisableTunnelingFilterStateKey = "envoy.tcp_proxy.disable_tunneling";.

Thanks for the pointers, changed!

ggreenway · 2023-03-14T16:48:27Z

source/common/tcp_proxy/tcp_proxy.cc

Should there be a limit on how much data can be buffered here? Or do you depend on the earlier network filter to readDisable() if there's too much data?

Did not consider this, so maybe the latter? Of should I change the boolean metadata to be an integral maximum buffer size instead?

You could probably use one of the existing connection buffer size settings for the value. But if you want the other filter to specify how much data to buffer, that's fine too.

ggreenway · 2023-03-14T16:49:28Z

source/common/tcp_proxy/tcp_proxy.cc

Can you add a test that causes a high watermark on the upstream here, to make sure that any callbacks that re-renter this class have the correct state?

Could you point to a test that is close enough to use for inspiration to what you mean here?

kyessenov · 2023-03-14T20:38:44Z

Side-note - could we use this to solve #9023, assuming we have some way to detect the end of the downstream TLS handshake.

jrajahalme · 2023-04-05T22:08:33Z

@ggreenway Sorry for the delay, addressed your comment on use of BoolAccessor & asked for more context on the rest.

mattklein123 · 2023-04-14T14:39:03Z

Friendly ping @ggreenway

jrajahalme · 2023-04-15T01:04:28Z

Added docs.

jrajahalme · 2023-04-15T01:07:11Z

Rebased to resolve conflict on changelogs.

jrajahalme · 2023-04-15T01:22:47Z

fixed doc format

Add filter state object bool proxy_receive_before_connect that tcp_proxy filter checks for and when set to 'true' will not set readDisable on the downstream connection before upstream connection is established. This change allows advanced cases where a network read filter needs to receive (and possibly send) downstream data that may change the destination of the upstream TCP connection, e.g. via metadata set based on the received data. If this is the case, then the read filter preceding the tcp_proxy filter must return StopIteration from its onNewConnection() call, causing tcp_proxy filter to postpone its upstream connection establishment until onData() returns Continue. Any data reaching the tcp_proxy filter before the upstream connection is established is buffered so that the downstream filters do not see the same data again which would be the case if it would remaining in the buffer and more data is received. This also allows downstream filters inject data before the upstream connection is established; such injected data would be lost if tcp_proxy would not buffer it while connection establishment is still ongoing. An existing dynamic metadata integration test is modified to use proxy_receive_before_connect=true. This makes the use case less reliant on the tcp_proxy internal detail, such as balancing and timing of the readDisable() calls on the downstream connection. Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>

jrajahalme · 2023-04-19T17:41:32Z

Rebased to fix conflict on changelogs/current.yaml

jrajahalme · 2023-04-24T06:28:06Z

@ggreenway Do I need to be rebasing this to resolve the conflict on changelogs/current.yaml or will that be handled automatically on merge?

ggreenway

/wait

ggreenway · 2023-04-25T16:45:33Z

test/integration/tcp_proxy_integration_test.cc

@jrajahalme please address this.

ggreenway · 2023-04-25T16:48:40Z

source/common/tcp_proxy/tcp_proxy.cc

  }
-  return Network::FilterStatus::StopIteration;
+  // Allow OnData() to receive data before connect if so configured
+  return receive_before_connect_ ? Network::FilterStatus::Continue


In the case of receive_before_connect_, now that earlier filters have seen whatever data they need and allowed the filter chain to continue, should tcp_proxy readDisable(true) to prevent additional data from getting buffered while the upstream connection is established?

If not, then I think there needs to be some limit to the data enforced. One option is to leave it in the Connection buffer so that flow control can be triggered.

ggreenway · 2023-04-25T16:49:41Z

@ggreenway Do I need to be rebasing this to resolve the conflict on changelogs/current.yaml or will that be handled automatically on merge?

There's a merge conflict, so merge main into your PR branch and resolve conflict there.

…n-to-read-before-connect

jrajahalme · 2023-05-11T13:57:22Z

@ggreenway merged main, conflicts resolved.

jrajahalme · 2023-05-19T10:39:18Z

@ggreenway Can you re-trigger presubmit checks? The fail seems to be in examples/ext_authz which is not even using tcp_proxy filter, so it is likely unrelated to this PR.

…n-to-read-before-connect

jrajahalme · 2023-05-19T10:40:30Z

Pushed another main merge to rerun tests.

ggreenway

This still needs some buffer-limit/flow-control added to it.

Also, need to address https://github.com/envoyproxy/envoy/pull/25804/files#r1125129552.

/wait

ggreenway · 2023-05-22T17:27:13Z

source/common/tcp_proxy/tcp_proxy.cc

You could probably use one of the existing connection buffer size settings for the value. But if you want the other filter to specify how much data to buffer, that's fine too.

github-actions · 2023-06-21T20:01:11Z

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 · 2023-06-28T20:01:14Z

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!

akshita31 · 2024-06-14T17:47:37Z

Hi @jrajahalme would you be able to re-work this PR ? Our team also has a use case where we have RBAC before TCP_Proxy and TCP_Proxy is opening connection to upstream before RBAC is performed which is not ideal for us. So having this filter state will be very useful for our case as well.

repokitteh-read-only bot added the api label Feb 27, 2023

repokitteh-read-only bot assigned markdroth Feb 27, 2023

jrajahalme force-pushed the tcp-proxy-add-option-to-read-before-connect branch from 7b5077b to 3d4a268 Compare February 28, 2023 11:20

jrajahalme force-pushed the tcp-proxy-add-option-to-read-before-connect branch from 3d4a268 to aa1e991 Compare March 1, 2023 07:50

botengyao reviewed Mar 3, 2023

View reviewed changes

repokitteh-read-only bot assigned ggreenway Mar 3, 2023

ggreenway requested changes Mar 6, 2023

View reviewed changes

repokitteh-read-only bot added the waiting label Mar 6, 2023

jrajahalme force-pushed the tcp-proxy-add-option-to-read-before-connect branch from aa1e991 to b5d1460 Compare March 13, 2023 11:55

repokitteh-read-only bot removed the waiting label Mar 13, 2023

jrajahalme changed the title ~~tcp_proxy: Add option to read before connect~~ tcp_proxy: Add filter state proxy_read_before_connect Mar 13, 2023

jrajahalme force-pushed the tcp-proxy-add-option-to-read-before-connect branch from b5d1460 to 9d3d455 Compare March 13, 2023 13:57

jrajahalme requested a review from ggreenway March 13, 2023 13:58

ggreenway requested changes Mar 14, 2023

View reviewed changes

repokitteh-read-only bot added the waiting label Mar 14, 2023

repokitteh-read-only bot removed the waiting label Apr 5, 2023

jrajahalme requested a review from ggreenway April 5, 2023 22:11

jrajahalme force-pushed the tcp-proxy-add-option-to-read-before-connect branch from c9b3227 to 5ac3dd3 Compare April 15, 2023 01:06

jrajahalme closed this Apr 15, 2023

jrajahalme reopened this Apr 15, 2023

jrajahalme changed the title ~~tcp_proxy: Add filter state proxy_read_before_connect~~ tcp_proxy: Add filter state envoy.tcp_proxy.receive_before_connect Apr 15, 2023

jrajahalme added 3 commits April 19, 2023 10:37

tcp_proxy: Use StreamInfo::BoolAccessor

79bc1fb

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>

add docs

fff172e

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>

jrajahalme force-pushed the tcp-proxy-add-option-to-read-before-connect branch from 8ebec98 to fff172e Compare April 19, 2023 17:40

ggreenway requested changes Apr 25, 2023

View reviewed changes

repokitteh-read-only bot added the waiting label Apr 25, 2023

Merge remote-tracking branch 'upstream/main' into tcp-proxy-add-optio…

6f52664

…n-to-read-before-connect

jrajahalme requested a review from ggreenway May 11, 2023 14:23

Merge remote-tracking branch 'upstream/main' into tcp-proxy-add-optio…

df45a3c

…n-to-read-before-connect

repokitteh-read-only bot removed the waiting label May 19, 2023

ggreenway requested changes May 22, 2023

View reviewed changes

repokitteh-read-only bot added the waiting label May 22, 2023

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

github-actions bot closed this Jun 28, 2023

This was referenced Oct 14, 2024

[Do not review] [WIP] Add filter state receive_before_connect to tcp_proxy #36543

Closed

Add filter state receive_before_connect to tcp_proxy #36580

Closed

Add filter state receive_before_connect to tcp_proxy #36581

Closed

akshita31 mentioned this pull request Jan 25, 2025

Add filter state receive_before_connect to tcp_proxy #38189

Merged

Conversation

jrajahalme commented Feb 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Feb 27, 2023

Uh oh!

jrajahalme commented Feb 28, 2023

Uh oh!

jrajahalme commented Mar 1, 2023

Uh oh!

botengyao left a comment

Choose a reason for hiding this comment

Uh oh!

botengyao Mar 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

botengyao commented Mar 3, 2023

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

jrajahalme commented Mar 13, 2023

Uh oh!

jrajahalme commented Mar 14, 2023

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kyessenov commented Mar 14, 2023

Uh oh!

jrajahalme commented Apr 5, 2023

Uh oh!

mattklein123 commented Apr 14, 2023

Uh oh!

jrajahalme commented Apr 15, 2023

Uh oh!

jrajahalme commented Apr 15, 2023

Uh oh!

jrajahalme commented Apr 15, 2023

Uh oh!

jrajahalme commented Apr 19, 2023

Uh oh!

jrajahalme commented Apr 24, 2023

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ggreenway commented Apr 25, 2023

Uh oh!

jrajahalme commented May 11, 2023

Uh oh!

jrajahalme commented May 19, 2023

Uh oh!

jrajahalme commented May 19, 2023

Uh oh!

jrajahalme commented Feb 27, 2023 •

edited

Loading

botengyao Mar 3, 2023 •

edited

Loading