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.
Envoy core changes for reverse connections #37368
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
base: main
Are you sure you want to change the base?
Envoy core changes for reverse connections #37368
Changes from 13 commits
457e1e9
6ce719e
3d33358
3331617
4104495
edd3b17
553fada
5401791
7c270c2
05ee7d6
841dfea
ba4e0bf
ae3e00b
bc15de5
0e26d53
d10a90a
d54a8d0
9c953ae
d478775
2b365bf
611e8ba
86fe208
13d6ba9
b4c6fe1
6f5de2b
ce9edf6
1919394
7492552
09c7245
a1dddbb
55e4ab4
24ef333
4592851
f2de792
8f7ea97
b7d7049
2064812
f82cce1
5a6086b
a1b112b
5152af6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Actually a runtime guard has a 6 month deletion policy, and it is usually used to guard the data plane behavior change. I think here you want a parameter to tell if this is going to be a local reply on a reverse connection. Could you point me where you detect this use case, and I assume it is in your HTTP filter extension.
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.
Yes, this is related to the http filter. Basically, the reverse connection changes are dependent on the guard http_filter_avoid_reentrant_local_reply, which is now deprecated. Without it, when the responder accepts the reverse connection and calls sendLocalReply() to ACK the connection, the filter manager prepares a local reply to be sent after the filter execution returns. This results in a segfault as the connection is closed. To allow the reverse conn filter to send a local reply similar to http_filter_avoid_reentrant_local_reply, I added the runtime guard. Would you suggest adding a flag instead?
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 really need the direct
sendLocalReply()
behavior, we should not use the runtime guard here, the original introduce of this runtime flag is to fix the reentrant issue, and all other normal users will not see the difference. We need to make this code path only work for the reverse connection with some flag like you did forconnection
to indicate this is thefilter_manager
that is used on reverse connection.