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
quiche: implement Envoy Quic stream and connection #7721
quiche: implement Envoy Quic stream and connection #7721
Changes from all commits
3253b11
9914fb4
2ea58dd
a8aa262
6b5315b
02e4fd9
6f8c2e4
58f3842
8fec99d
15e5e33
72c2f84
25998ab
2fd6df0
aacc588
f68e5dd
12ef3c7
6029aeb
14f1e79
2fccff1
ef7cf28
5419162
117244f
1719fd0
732b3c8
f94f424
0d56de4
bacb5c5
f04a469
9ba2754
05daf8d
6eca9c6
b8fc520
f5e0c11
b881ae8
3db6847
1e55f68
ab474d5
5f7dd41
fcfe160
9dc44be
7a2395f
3e4388b
2397898
0641fba
30d976b
a50fc1e
ed15996
8b297e4
adbdee0
57144ed
d278167
1b8117b
8375028
6361860
4e51861
44e262f
f2fc291
97b7d90
7361ca0
10e85f6
5f55a97
a4050dc
81e5f9a
109e9a9
9d545c5
4e91aae
7c863ef
c6f0763
bca0450
42b5df3
8891a14
9a5468e
1d23053
817c08d
3405bfc
548f8f5
b218f5a
be37cb1
d7c551f
bb1e435
61ecd12
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.
hm, so I'd imagined that the HCM onData would be passed to the quic codec Dispatch. Do we pass directly to have the per-packet information persisted?
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.
De-multiplexing is handled by QUIC stack, so HCM onData should never be called, so is dispatch() here.
What do you mean? HCM should only see request header, body and trailer, rather than UDP payload.
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.
sadly these don't quite work, and so add to our uncovered lines.
It really bites when we get close to 97% coverage because if you add some NOT_REACHED_GCOVR_EXCL_LINE in a small CL you then have to write unit tests for unrelated code.
Let's instead add quiche to COVERAGE_IGNORE_REGEX in test/run_envoy_bazel_coverage.sh and have a tracking item in one of the QUICHE bugs to fix up coverage before we land.
I would like you to take a look at coverge results and see if any of the new code not explicitly NOT_REACHED makes sense to unit test, because I suspect much of it does and it's easier to land in line than do it all at once. If you don't know how to dig through the CI -> coverage -> artifacts I can run you through it.
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.
done
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.
Q for my own understanding: Would we actually do this? Is HTTP/3 different from HTTP/2 in any substantial way at the app layer? I'm mainly wondering if it's worth it to make Envoy aware of HTTP/3 above the codec.
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.
Distinguish Http3 from Http2 traffic is more for monitoring. These two works the same at the app layer. The distinction should only exists in codec
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.
unclear why it wasn't before?
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.
Because self_address is not initialized till this point.
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.
It should be possible to make self_address available to the connection earlier, as a parameter of QuicDispatcher::CreateQuicSession(), that way we don't need to do initialization work at here.
Maybe add a TODO for that?
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.
Client connection is not possible. I would like to keep both side use same logic.
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.
To capture our offline discussion: Client connection in Enovy should also know how to initialize the filter chain even without knowing the self address. It'd be nice if we could initialize the filter chan in a more straightforward place, rather than after a packet is received. A TODO to do it later is totally fine.
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 added a TODO to investigate the possibility of this approach
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.
are we validating somewhere that the only valid config is HCM, etc?
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.
Not right now, but I think it's easier to validate the config during filter factory creation in addListener()
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.
nit: no need to have empty_filter_chain.
if (!listener_config_....) {}
should read just fine.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.
The meaning of the return value is not intuitive from the function name, so I added a temporary variable for readability.
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.
Sure, sounds good.
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.
this is an example of something we might want to unit test?
https://273543-65214191-gh.circle-artifacts.com/0/coverage/coverage/proc/self/cwd/source/extensions/quic_listeners/quiche/envoy_quic_connection.cc.html
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.
added in envoy_quic_dispatcher_test.cc
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.
Could this class work as a client connection? Could you add a comment for it?
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. Commented.
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.
Thanks!