Skip to content
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.

Sign up for GitHub

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

Jump to bottom

add http1-safe max_connection_duration for http connection manager #35209

Merged
merged 27 commits into from
Aug 8, 2024
Merged

add http1-safe max_connection_duration for http connection manager #35209

merged 27 commits into from
Aug 8, 2024

Conversation

antoniovleonti
Copy link
Contributor

@antoniovleonti antoniovleonti commented Jul 16, 2024 •
edited
Loading

Commit Message: add http1-safe max_connection_duration for http connection manager

This PR adds an option to change the http connection manager's draining behavior for max_connection_duration for http1. The new behavior is that envoy will wait indefinitely for one more request, add connection:close to the response headers, then close the connection once the stream ends.

This is to avoid a networking race condition which results in the client not receiving a response to their last request if they send it right when envoy is closing the connection.

Fixes #34356 (check for context)

@repokitteh-read-only RepoKitteh - Read Only
Copy link

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

🐱

Caused by: #35209 was opened by antoniovleonti.

see: more, trace.

@antoniovleonti
Copy link
Contributor Author

/assign @KBaichoo

@antoniovleonti
fix doc link
cd18bdb 
Signed-off-by: antoniovleonti <[email protected]>
@antoniovleonti
try fix doc
aae00ea 
Signed-off-by: antoniovleonti <[email protected]>
@antoniovleonti
revert back to what I think should be correct
012510c 
Signed-off-by: antoniovleonti <[email protected]>
@antoniovleonti
try different prefix
a69e483 
Signed-off-by: antoniovleonti <[email protected]>
@antoniovleonti
fix mock
7f19c3b 
Signed-off-by: antoniovleonti <[email protected]>
@antoniovleonti
Copy link
Contributor Author

/retest

@antoniovleonti
fix formatting issue
75f5b33 
Signed-off-by: antoniovleonti <[email protected]>
@antoniovleonti
implement http1SafeMaxConnectionDuration in AdminImpl
ef94656 
Signed-off-by: antoniovleonti <[email protected]>
@antoniovleonti
fix fuzz test
3fc1ab5 
Signed-off-by: antoniovleonti <[email protected]>
@antoniovleonti
fix other use of setup()
d19c475 
Signed-off-by: antoniovleonti <[email protected]>
@antoniovleonti
Copy link
Contributor Author

/retest

@antoniovleonti
Copy link
Contributor Author

@KBaichoo CI errors fixed, PTAL

@KBaichoo
Copy link
Contributor

Thanks @antoniovleonti , will give a pass later today!

KBaichoo
KBaichoo reviewed Jul 18, 2024
View reviewed changes
Copy link
Contributor

@KBaichoo KBaichoo left a comment

Choose a reason for hiding this comment

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

/wait

Thank you for working on this

api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto Show resolved Hide resolved
api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto Outdated Show resolved Hide resolved
source/common/http/conn_manager_impl.cc Show resolved Hide resolved
source/server/admin/admin.h Outdated Show resolved Hide resolved
test/common/http/conn_manager_impl_test_2.cc Show resolved Hide resolved
test/common/http/conn_manager_impl_test_base.h Show resolved Hide resolved
KBaichoo
KBaichoo reviewed Aug 6, 2024
View reviewed changes
Copy link
Contributor

@KBaichoo KBaichoo left a comment

Choose a reason for hiding this comment

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

Otherwise lgtm. Can you also update the description (or the issue) to better capture the end behavior since we iterated on it? Thank you

source/common/http/conn_manager_config.h Show resolved Hide resolved
api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto Outdated Show resolved Hide resolved
@antoniovleonti
update proto doc comment
ebc40f5 
Signed-off-by: antoniovleonti <[email protected]>
@antoniovleonti
document new stat
fbe0104 
Signed-off-by: antoniovleonti <[email protected]>
@antoniovleonti
Copy link
Contributor Author

Otherwise lgtm. Can you also update the description (or the issue) to better capture the end behavior since we iterated on it? Thank you

The description has been updated. Thanks!

KBaichoo
KBaichoo previously approved these changes Aug 6, 2024
View reviewed changes
Copy link
Contributor

@KBaichoo KBaichoo left a comment

Choose a reason for hiding this comment

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

Thank you

@KBaichoo
Copy link
Contributor

KBaichoo commented Aug 6, 2024

/assign @yanavlasov

@yanavlasov
Copy link
Contributor

Needs main merge to fix coverage build error.

yanavlasov
yanavlasov reviewed Aug 8, 2024
View reviewed changes
Copy link
Contributor

@yanavlasov yanavlasov left a comment

Choose a reason for hiding this comment

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

LGTM,. See if you can replace the check for log message with the check for stat value.

/wait-any

changelogs/current.yaml Show resolved Hide resolved
test/common/http/conn_manager_impl_test_2.cc Outdated Show resolved Hide resolved
@antoniovleonti
fix bad merge
bba721c 
Signed-off-by: antoniovleonti <[email protected]>
@antoniovleonti
replace log check with stat check
0f0c6b0 
Signed-off-by: antoniovleonti <[email protected]>
@yanavlasov yanavlasov enabled auto-merge (squash) August 8, 2024 15:10
@yanavlasov yanavlasov merged commit 5b22dce into envoyproxy:main Aug 8, 2024
50 of 52 checks passed
martinduke pushed a commit to martinduke/envoy that referenced this pull request Aug 8, 2024
@antoniovleonti @martinduke
add http1-safe max_connection_duration for http connection manager (e…
0e58d6d 
…nvoyproxy#35209)

This PR adds an option to change the http connection manager's draining
behavior for max_connection_duration for http1. The new behavior is that
envoy will wait indefinitely for one more request, add connection:close
to the response headers, then close the connection once the stream ends.

This is to avoid a networking race condition which results in the client
not receiving a response to their last request if they send it right
when envoy is closing the connection.

Fixes envoyproxy#34356 (check for context)

---------

Signed-off-by: antoniovleonti <[email protected]>
Signed-off-by: Martin Duke <[email protected]>
@adisuissa adisuissa mentioned this pull request Aug 12, 2024
Merged
@antoniovleonti antoniovleonti deleted the soft-mcd branch August 12, 2024 17:06
RyanTheOptimist pushed a commit that referenced this pull request Aug 12, 2024
@adisuissa
test: update a broken test due to PRs race (#35671)
aed9ef2 
Commit Message: test: update a broken test due to PRs race
Additional Description:
PR #35568 used an old `setup()` method call which was modified by PR
#35209 that was merged just before it, causing a build failure.
This PR update the test and fixes the build failure.

Risk Level: low
Testing: N/A
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A

Signed-off-by: Adi Suissa-Peleg <[email protected]>
asingh-g pushed a commit to asingh-g/envoy that referenced this pull request Aug 20, 2024
@antoniovleonti @asingh-g
add http1-safe max_connection_duration for http connection manager (e…
1b957bc 
…nvoyproxy#35209)

This PR adds an option to change the http connection manager's draining
behavior for max_connection_duration for http1. The new behavior is that
envoy will wait indefinitely for one more request, add connection:close
to the response headers, then close the connection once the stream ends.

This is to avoid a networking race condition which results in the client
not receiving a response to their last request if they send it right
when envoy is closing the connection.

Fixes envoyproxy#34356 (check for context)

---------

Signed-off-by: antoniovleonti <[email protected]>
Signed-off-by: asingh-g <[email protected]>
asingh-g pushed a commit to asingh-g/envoy that referenced this pull request Aug 20, 2024
@adisuissa @asingh-g
test: update a broken test due to PRs race (envoyproxy#35671)
754c415 
Commit Message: test: update a broken test due to PRs race
Additional Description:
PR envoyproxy#35568 used an old `setup()` method call which was modified by PR
envoyproxy#35209 that was merged just before it, causing a build failure.
This PR update the test and fixes the build failure.

Risk Level: low
Testing: N/A
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A

Signed-off-by: Adi Suissa-Peleg <[email protected]>
Signed-off-by: asingh-g <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yanavlasov yanavlasov yanavlasov approved these changes

@KBaichoo KBaichoo KBaichoo left review comments

@alyssawilk alyssawilk Awaiting requested review from alyssawilk alyssawilk is a code owner

@mattklein123 mattklein123 Awaiting requested review from mattklein123 mattklein123 is a code owner

@tyxia tyxia Awaiting requested review from tyxia tyxia is a code owner

@htuch htuch Awaiting requested review from htuch

@abeyad abeyad Awaiting requested review from abeyad

Assignees

@abeyad abeyad

@KBaichoo KBaichoo

@yanavlasov yanavlasov

Labels
api
Projects
None yet
Milestone
No milestone
4 participants
@antoniovleonti @KBaichoo @yanavlasov @abeyad