Skip to content

Add filter state receive_before_connect to tcp_proxy#36581

Closed
akshita31 wants to merge 8 commits intoenvoyproxy:mainfrom
akshita31:akshita-tcp-proxy
Closed

Add filter state receive_before_connect to tcp_proxy#36581
akshita31 wants to merge 8 commits intoenvoyproxy:mainfrom
akshita31:akshita-tcp-proxy

Conversation

@akshita31
Copy link
Copy Markdown
Contributor

Commit Message:
This PR picks up the work done in #25804.

  1. It adds a receive_before_connect filter state which can be read by TCP_PROXY and if set TCP_PROXY will not disable downstream reads until the upstream connection is established. This can hence be used by filters before TCP_PROXY to set the filter state in initializeReadFilterCallbacks, and then StopIteration in onNewConnection and onData, until they have read the required amount of data before upstream connection establishment.
  2. Based on feedback in tcp_proxy: Add filter state envoy.tcp_proxy.receive_before_connect #25804 , this PR adds a flow control to prevent early_data_buffer from overflowing, by disabling downstream reads on early data until the upstream connection establishment is finished.
  3. Adds integration and unit tests to cover the newly added functionality.

Additional Description:
Risk Level: Low
Testing: Added integration and unit tests
Docs Changes: Done
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

@repokitteh-read-only
Copy link
Copy Markdown

Hi @akshita31, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #36581 was opened by akshita31.

see: more, trace.

@repokitteh-read-only
Copy link
Copy Markdown

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: #36581 was opened by akshita31.

see: more, trace.

@akshita31 akshita31 force-pushed the akshita-tcp-proxy branch 5 times, most recently from 5f83a94 to 5c171de Compare October 14, 2024 20:18
@akshita31 akshita31 force-pushed the akshita-tcp-proxy branch 2 times, most recently from 38f5d35 to f82ccc7 Compare October 21, 2024 19:13
ASSERT(0 == early_data_buffer_.length());

// Read enable the connection now that early data has been flushed to the upstream.
read_callbacks_->connection().readDisable(false);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should we in that case check connection state and assert that its actually readDisabled before enabling? Or maybe we don't care?

ASSERT_FALSE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection));

ASSERT_TRUE(tcp_client->write("world"));
test_server_->waitForCounterEq("tcp.tcpproxy_stats.early_data_received_count_total", 1);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Do we have a metric for how much data is received in early data. That would be good to have a metric on as well.

@akshita31 akshita31 force-pushed the akshita-tcp-proxy branch 3 times, most recently from cacf44e to cdcb41c Compare October 21, 2024 22:14
ASSERT(0 == early_data_buffer_.length());

// Read enable the connection now that early data has been flushed to the upstream.
read_callbacks_->connection().readDisable(false);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Ack.

jrajahalme and others added 7 commits October 24, 2024 12:18
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>
Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
Signed-off-by: Akshita Agarwal <akshita.agarwal@airbnb.com>
Signed-off-by: Akshita Agarwal <akshita.agarwal@airbnb.com>
Signed-off-by: Akshita Agarwal <akshita.agarwal@airbnb.com>
Signed-off-by: Akshita Agarwal <akshita.agarwal@airbnb.com>
Signed-off-by: Akshita Agarwal <akshita.agarwal@airbnb.com>
Copy link
Copy Markdown

@dwj300 dwj300 left a comment

Choose a reason for hiding this comment

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

LGTM

@github-actions
Copy link
Copy Markdown

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 github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Nov 28, 2024
@github-actions
Copy link
Copy Markdown

github-actions bot commented Dec 5, 2024

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stale stalebot believes this issue/PR has not been touched recently

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants