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.
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
quic: add QUIC downstream connection close error stats. #16584
quic: add QUIC downstream connection close error stats. #16584
Changes from 12 commits
b7b52e1
a704f7d
e77ca28
1b9275b
a47dc19
c4394c5
71f28c3
a71c23c
79e200b
8957117
5c83f5f
6cd0d83
4fe3fc5
ac867ce
d009105
832b098
8b7047a
0cc03f4
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.
If we're doccing up listener stats, won't direction always be downstream?
And actually I hate to catch this now, but probably rather than self/peer it should be tx/rx for consistency with other Envoy stats. I find self and peer more intuitive (maybe 'cause that's what I'm used to) but I think most of the other stats (like resets tx_reset/rx_reset) are documented that way with tx for things sent to the peer and rx being codes received from the peer.
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.
You are right. The listener stats should be all downstreams.
I haven't figured out the details on upstream stats. So I will update the doc again when I add them.
Done changing names to tx/rx.
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.
From a quick read through it's not clear to me that this won't create an unbounded number of stats controlled by downstream (for exampled if the name somehow has the code number in it and the client controls the codes). This is probably not the case, but can you add more comments about why this is safe for use with uncontrolled peers?
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.
Great point!
I initially had an assertion in QuicStatNames::connectionCloseStatName() to make sure we don't create unlimited amount of stats.
But after some investigation, I found that if the connection close is initiated from peers, the error_code is parsed from the wire and no enum range checking is done. So an assertion here is too strong and malicious clients might be able to attack Envoy by sending out-of-range error codes.
Instead, I now added a check in QuicStatNames::chargeQuicConnectionCloseStats() to ignore out-of-range error_codes and log a warning.
I will follow up to dig deeper into the quiche code and see if bad error code handling can be done earlier.