-
Notifications
You must be signed in to change notification settings - Fork 5.4k
tcp proxy: add new option to let tcp proxy check the drain close status #44567
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
Merged
Merged
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
cc90be1
tcp proxy: add new option to let tcp proxy check the drain close status
wbpcode b5d3597
Merge branch 'main' of ssh://ssh.github.com:443/envoyproxy/envoy into…
wbpcode 89681fb
update comment
wbpcode 801544d
add document of stats
wbpcode e3fa7cb
fix format
wbpcode 75bc67f
Merge branch 'main' of ssh://ssh.github.com:443/envoyproxy/envoy into…
wbpcode 39b1259
Merge branch 'main' of ssh://ssh.github.com:443/envoyproxy/envoy into…
wbpcode 7f7c7e5
try improve coverage
wbpcode 07169e2
address some comments
wbpcode ddd5f8e
Merge branch 'main' of ssh://ssh.github.com:443/envoyproxy/envoy into…
wbpcode File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -59,6 +59,7 @@ constexpr absl::string_view ReceiveBeforeConnectKey = "envoy.tcp_proxy.receive_b | |
| * All tcp proxy stats. @see stats_macros.h | ||
| */ | ||
| #define ALL_TCP_PROXY_STATS(COUNTER, GAUGE) \ | ||
| COUNTER(downstream_cx_drain_close) \ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this counter described in a doc?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's me take a check. |
||
| COUNTER(downstream_cx_no_route) \ | ||
| COUNTER(downstream_cx_rx_bytes_total) \ | ||
| COUNTER(downstream_cx_total) \ | ||
|
|
@@ -377,6 +378,9 @@ class Config { | |
| } | ||
|
|
||
| const absl::optional<uint32_t>& maxEarlyDataBytes() const { return max_early_data_bytes_; } | ||
| bool checkDrainClose() const { return check_drain_close_; } | ||
| const Network::DrainDecision& drainDecision() const { return drain_decision_; } | ||
| Network::DrainDirection drainCloseScope() const { return drain_close_scope_; } | ||
|
|
||
| private: | ||
| struct SimpleRouteImpl : public Route { | ||
|
|
@@ -433,6 +437,9 @@ class Config { | |
| envoy::extensions::filters::network::tcp_proxy::v3::UpstreamConnectMode upstream_connect_mode_{ | ||
| envoy::extensions::filters::network::tcp_proxy::v3::IMMEDIATE}; | ||
| absl::optional<uint32_t> max_early_data_bytes_; | ||
| const Network::DrainDecision& drain_decision_; | ||
| const Network::DrainDirection drain_close_scope_{}; | ||
| const bool check_drain_close_{false}; | ||
| }; | ||
|
|
||
| using ConfigSharedPtr = std::shared_ptr<Config>; | ||
|
|
@@ -690,6 +697,7 @@ class Filter : public Network::ReadFilter, | |
| void onDownstreamEvent(Network::ConnectionEvent event); | ||
| void onUpstreamData(Buffer::Instance& data, bool end_stream); | ||
| void onUpstreamEvent(Network::ConnectionEvent event); | ||
| void maybeCloseDownstreamForDrainClose(); | ||
| void onUpstreamConnection(); | ||
| void onIdleTimeout(); | ||
| void resetIdleTimer(); | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we enable by default and leave a runtime setting to temporarily change the default. It seems like much better behavior to have this enabled.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if there is a case that the users want to keep the existing TCP connections as long time as possible by setting a long drain duration. Different with HTTP, we have no way to close the connection gracefully.
So, I slightly inclined to keep an option to control it. But I can change the bool to wrapped value so it's would be easier to change the default value in the future. WDYT?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you strongly prefer to use runtime guard and enable it by default, I am also fine to that. Feel free to let me know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't feel strongly, it was just a thought. I'm ok leaving it as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you used a message here, you could have put a timer-based check as well in the future. I don't think we see TCP keep-alives in Envoy, unlike HTTP, so a connection can be stuck without drain or data without a timer.