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
Add support for client-side QUIC 0-RTT #16260
Add support for client-side QUIC 0-RTT #16260
Changes from 4 commits
6e631af
12e4a08
b295998
04e5ce1
22c4e3d
fe07df8
65c0883
8a59130
1ed0f28
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.
nits: if you make this a private member function, it can directly access system_time_ and no need to pass in system_time.
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.
There is no
system_time_
member. I think you might be confusing it withtime_source_
which is different - we want to avoid computing the time from the time source too often.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.
Right, I meant to access time_source_ directly.
time_source_.systemTime()
is always called before isSessionValid(). For readability, I think either renamingsystem_time
tonow
here or making this function a class member.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.
Makes sense. I've renamed
system_time
tonow
.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.
nits: making
other
const as well?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.
I was wondering why we compare the pointer of two different objects. But it seems to be only possible when both
state
andother
are nullptr? I slightly prefer to make it explicit 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.
Yes, this is needed to avoid a dereference when they're both nullptr. I personally prefer this style of comparison because it is correct for all inputs. The fact that callers won't ever send the same non-null pointer as both parameters is an implementation details that doesn't need to be visible in this function.
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 duplication between this and QuicClientSessionCache is a bit of a bummer.
Optionally, as a follow-up, maybe we can implement more of the base logic in the quiche class and either template away time or have pure virtuals for time bits. but that'd be a follow-up no matter what and should wait until we've sorted out any upstream scalability problems.
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.
What's QuicClientSessionCache?
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.
https://chromium.googlesource.com/chromium/src/+/master/net/quic/quic_client_session_cache.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.
Ah right, yes we're going to have duplication between Chrome and Envoy :)
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.
Yep, but modulo "can we refactor away the time abstraction" seems reasonable to stick this one in quiche at some 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.
drive by question - why is SSL_CTX unused here and in quiche? Should we clean up upstream?
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.
Some clients may want to use different CCL_CTX'es. We could remove it but it isn't causing any issues. Either way that would be a QUICHE CL.
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.
yeah, sorry, quiche == upstream in this context. Not something you'd do in situ but if it's not used anywhere might be worth adding our quiche clean up list.
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.
SG
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.
/s/save oldest/track the oldest session/ ?
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.
It's a bit confusing about the life time of application_state. Can you comment that it's now owned?
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 input parameter isn't owned. The fact that it's a const pointer makes the ownership model clear to me. Am I missing something?
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.
sorry, typo "not" for "now". If we create ApplicationState instance for each entry here, why does doApplicationStatesMatch() compare pointers?
And Lookup() also create ApplicationState for return value. Probably QUICHE somehow requires duplication this object.
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 don't think we need to comment that a const pointer is not owned, as that's very common usage.
doApplicationStatesMatch() dereferences the pointers and compares the values. It looks at pointers because they can be nullptr.
I'm not sure what you're asking? QUICHE passes us a const pointer and we need to make a copy that we own.
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.
we walk the entire list on insertion? Maybe make that clear in prune comments()
Also definitely worth a comment up by that 1024 that it should stay small and why. eek!
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.
O(1024) is negligible compared to the cryptographic operations required to create a new connection.
Added comments though
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.
ditto | here and below
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.
Retrieves and removes the latest session...?
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.
Why size of 2?
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 that's sufficient in practice, it's the value Chrome uses.
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.
Mind commenting about the reason? Otherwise a queue-like data structure might be more preferred.
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 a comment.
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.
mind adding a bit more detail for those of with less QUIC-savvy? is it that session information changes infrequently? If so why do we need two?
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.
Optionally
If after that we are still at the size limit, also remove ->
If all entries were valid but the cache is at its size limit, instead removes
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