-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Default disable port migration and allow it to be turned on in Envoy Mobile API #32112
Conversation
Signed-off-by: Renjie Tang <renjietang@google.com>
Signed-off-by: Renjie Tang <renjietang@google.com>
Signed-off-by: Renjie Tang <renjietang@google.com>
Signed-off-by: Renjie Tang <renjietang@google.com>
Signed-off-by: Renjie Tang <renjietang@google.com>
Signed-off-by: Renjie Tang <renjietang@google.com>
/assign @abeyad |
Signed-off-by: Renjie Tang <renjietang@google.com>
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 for doing this @RenjieTang ! Also, do we want to add this API for the other languages (java/kotlin/swift/objective-c)?
->mutable_http3_protocol_options() | ||
->mutable_quic_protocol_options() | ||
->mutable_num_timeouts_to_trigger_port_migration() | ||
->set_value(4); |
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.
do we want to make this value configurable too?
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.
4 is the number that we found out to be the best in previous experiment.
I'd want EM users to simply enable port migration and not worry about the number of timeouts for now.
That said, if the need to experiment with port migration rises again, I'll be happy to add it.
I also intentionally added C++ API only because I want to verify the crash is fixed first.
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.
SGTM, maybe add a comment then about 4 being the best value from previous experimentation
@@ -252,7 +252,7 @@ void EnvoyQuicClientSession::setHttp3Options( | |||
} | |||
static_cast<EnvoyQuicClientConnection*>(connection()) | |||
->setNumPtosForPortMigration(PROTOBUF_GET_WRAPPED_OR_DEFAULT( | |||
http3_options.quic_protocol_options(), num_timeouts_to_trigger_port_migration, 4)); | |||
http3_options.quic_protocol_options(), num_timeouts_to_trigger_port_migration, 0)); |
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 just realized this would turn off port migration for everyone else... I'm assuming non-mobile users aren't using it, but are we sure? we probably need to at least have a release note about this?
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'm not entirely sure that this code doesn't execute anywhere else. But since it's prone to UAF issues, turning it off does more good than evil.
done adding a change log.
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 are the UAF issues? Should it be forcefully disabled (unconfigurable) until that is resolved?
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 potential issue was fixed #31702. The issue manifested in Envoy Mobile clients but not on Envoy in production. We want to default disable it but allow it to be turned on to verify the fix.
Signed-off-by: Renjie Tang <renjietang@google.com>
added @ggreenway for cross-company review, thanks! |
changelogs/current.yaml
Outdated
@@ -8,6 +8,9 @@ minor_behavior_changes: | |||
- area: adaptive concurrency filter stats | |||
change: | | |||
Multiply the gradient value stat by 1000 to make it more granular (values will range between 500 and 2000). | |||
- area: QUIC | |||
change: | | |||
Port migration is default turned off. |
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.
Can you add more details to this, such as that this affects QUIC clients, what the implications may be, and link to the config knob for overriding the default?
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
Signed-off-by: Renjie Tang <renjietang@google.com>
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.
Will await @ggreenway approval and merge.
@ggreenway friendly ping |
@RenjieTang fyi you'll need to resolve the merge conflict |
Signed-off-by: Renjie Tang <renjietang@google.com>
Signed-off-by: RenjieTang <renjietang@google.com>
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.
/wait
changelogs/current.yaml
Outdated
- area: QUIC | ||
change: | | ||
Port migration is default turned off. QUIC client connections will no longer attempt to migrate to a new port when connections | ||
is degrading. Can be manually turned on via num_timeouts_to_trigger_port_migration proto. |
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: can you make this a link to the proto field?
Signed-off-by: RenjieTang <renjietang27@gmail.com>
Signed-off-by: Renjie Tang <renjietang@google.com>
Signed-off-by: Renjie Tang <renjietang@google.com>
Signed-off-by: Renjie Tang <renjietang@google.com>
Commit Message: Default disable port migration and allow it to be turned on in Envoy Mobile API
Additional Description: Currently it can only be turned on via the C++ API. More platforms will be covered once port migration is proven to work well.
Risk Level: Low
Testing: unit tests
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: mobile