Skip to content

quic: upstream sends early data requests#20167

Merged
alyssawilk merged 200 commits intoenvoyproxy:mainfrom
danzh2010:pooluse0rtt
May 23, 2022
Merged

quic: upstream sends early data requests#20167
alyssawilk merged 200 commits intoenvoyproxy:mainfrom
danzh2010:pooluse0rtt

Conversation

@danzh2010
Copy link
Copy Markdown
Contributor

@danzh2010 danzh2010 commented Mar 1, 2022

Commit Message: make HTTP/3 upstream sends 0-RTT (early data) requests if it has cached 0-RTT credentials. Add a config knob in RouteAction to specify which request can be sent over early data, which by default are HTTP safe methods.

Risk Level: high, changes to conn pool behavior though should only take effect for h3 pool
Testing: added h3 upstream integration tests.
Docs Changes: N/A
Release Notes: changes to docs/root/version_history/current.rst
Platform Specific Features: N/A
Runtime guard: envoy.reloadable_features.http3_sends_early_data
Fixes #18715, #19542

Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
@alyssawilk
Copy link
Copy Markdown
Contributor

looks like CI bounced. is this ready for another pass?
/wait-any

Signed-off-by: Dan Zhang <danzh@google.com>
@danzh2010
Copy link
Copy Markdown
Contributor Author

looks like CI bounced. is this ready for another pass? /wait-any

Yes, I addressed all the comments. But I haven't checked the test coverage due to the CI failure.

static Factory* getAndCheckFactory(const ProtoMessage& message, bool is_optional) {
Factory* factory = Utility::getFactoryByType<Factory>(message.typed_config());
if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.no_extension_lookup_by_name")) {
std::cerr
Copy link
Copy Markdown
Contributor

@alyssawilk alyssawilk May 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove cerr here and below

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Copy Markdown
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm, I think you still had some debug code here?
/wait

Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
@danzh2010
Copy link
Copy Markdown
Contributor Author

/retest

@repokitteh-read-only
Copy link
Copy Markdown

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #20167 (comment) was created by @danzh2010.

see: more, trace.

Copy link
Copy Markdown
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM modulo coverage checks (which are re-running)
Passing to ggreenway for !google pass and another close look at the conn pool changes

Copy link
Copy Markdown
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alyssawilk
alyssawilk previously approved these changes May 18, 2022
Copy link
Copy Markdown
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on second look at coverage it looks like new lines are covered - I'm baffled by the coverage line count being lower but that's not your problem

LGTM but please main mereg

Signed-off-by: Dan Zhang <danzh@google.com>
@danzh2010
Copy link
Copy Markdown
Contributor Author

ping @ggreenway

Copy link
Copy Markdown
Member

@ggreenway ggreenway left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

I'd say this is probably high risk; the connection pool code and it's interactions are complicated. But everything looks good; I don't see any issues.

@danzh2010
Copy link
Copy Markdown
Contributor Author

/retest

@repokitteh-read-only
Copy link
Copy Markdown

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #20167 (comment) was created by @danzh2010.

see: more, trace.

Signed-off-by: Dan Zhang <danzh@google.com>
@alyssawilk
Copy link
Copy Markdown
Contributor

@markdroth I think we need another API stamp?

@alyssawilk alyssawilk merged commit 8ce13d7 into envoyproxy:main May 23, 2022
ravenblackx pushed a commit to ravenblackx/envoy that referenced this pull request Jun 8, 2022
Commit Message: make HTTP/3 upstream sends 0-RTT (early data) requests if it has cached 0-RTT credentials. Add a config knob in RouteAction to specify which request can be sent over early data, which by default are HTTP safe methods.

Risk Level: high, changes to conn pool behavior though should only take effect for h3 pool
Testing: added h3 upstream integration tests.
Docs Changes: N/A
Release Notes: changes to docs/root/version_history/current.rst
Platform Specific Features: N/A
Runtime guard: envoy.reloadable_features.http3_sends_early_data
Fixes envoyproxy#18715, envoyproxy#19542
Signed-off-by: Dan Zhang <danzh@google.com>


Signed-off-by: Dan Zhang <danzh@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

upstream HTTP/3 0-RTT should work, and be restricted to safe methods

9 participants