Fix PROXY protocol out with tls #9698
Merged
Conversation
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
maskit
reviewed
May 10, 2023
lzx404243
commented
May 11, 2023
| > POST / HTTP/1.1 | ||
| `` | ||
| < HTTP/1.1 502 Connection refused | ||
| < HTTP/1.1 502 Broken pipe |
Collaborator
Author
There was a problem hiding this comment.
Note to reviewers:
The post_slow_server_max_requests_in was failing with my changes, as it expects a 502 Connection refused, but gets a 502 Broken pipe.
The expectation changed from 502 Broken pipe->502 Connection refused as part of the #9366. Discussed with @bneradt and we believe we should update/revert the expectation to the original. After this PR, certain things seem to be more aligned to how they worked pre H2ToOrigin.
bneradt
requested changes
May 11, 2023
Contributor
bneradt
left a comment
bneradt
left a comment
There was a problem hiding this comment.
Looks great. Thanks for fixing and updating the tests.
Member
|
[approve ci centos] |
cmcfarlen
approved these changes
May 22, 2023
bneradt
approved these changes
May 22, 2023
cmcfarlen
pushed a commit
to cmcfarlen/trafficserver
that referenced
this pull request
Jun 3, 2024
* asf/master: Fix PROXY protocol out with tls (apache#9698) Add automatic detection of ccache for cmake (apache#9720) Fix cqpv log field value on H3 connections (apache#9719) system_stats: fix buffer overflow caught by asan (apache#9723)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
While updating the PROXY protocol tests, I observe the https connections to the origin would hang when PROXY protocol out is enabled in ATS. This PR addresses this.
root cause
After the #9366 changes, the logic of kicking off state machine is in
ConnectingEntryand is triggered by callbacks upon one ofVC_EVENT_READ_COMPLETE,VC_EVENT_WRITE_READYorVC_EVENT_WRITE_COMPLETEevents. The root cause of the hang has to do with none of these events is triggered:VC_EVENT_WRITE_READYis triggered and the connection flows. However when PROXY protocol out is enabled, as PROXY protocol data is sent before ssl handshake, the accounting ofvio.nbytesandvio.donechanged , leading toVC_EVENT_WRITE_READYnot being triggered.VC_EVENT_READ_COMPLETEis not triggered becausevioread is not enabled. Prior to Http2 to origin #9366,attach_server_session()enables vio read and is called prior to the handshake. After Http2 to origin #9366, this is called after the handshake so thevioread is disabled during the handshake. As read is disabled, theVC_EVENT_READ_COMPLETEcan't be signaled to the handler.This PR adds a 0-byte read so the
vioread is enabled andConnectingEntry::state_http_server_open()handler can be signaled and called back with theVC_EVENT_READ_COMPLETEevent.