network: implement initial heuristic for binding alternate interface #1858

goaway · 2021-10-04T12:41:53Z

Description: Introduces an initial heuristic for attempting a connection using a socket with a bound interface. The heuristic is entirely contained within reportNetworkUsage(...), and as such is straightforward to iterate and experiment upon. In support of the heuristic, current network/configuration state is identified with a configuration_key that prevents stale information from influencing internal tracking. ~~Current network state is stored and updated automatically.~~ This allows static accessors to begin to report OS updates during Envoy's startup period, and ensures OS updates have a timely effect on new requests.

The initial heuristic is as follows:

For both WiFi and cellular network types, there is a default socket configuration that does not use a bound socket, and an alternative "override" configuration that will attempt to use an interface associated with the opposite network type (a cellular-bound socket when the preferred network type is WiFi, and a WiFi-bound socket when network type is cellular).

A "network fault" is defined as a request that terminates in error while having received no upstream bytes.
If a connection has never handled a request without a fault, it is allowed only one fault before the alternative will be tried.
If a connection has ever handled a request without a fault, it is allowed up to three consecutive faults before the alternative will be tried.

The above behavior is disabled by default, and may be enabled by settings enableInterfaceBinding(true) on the public EngineBuilders.

Risk Level: Moderate
Testing: Manual and new/updated coverage

Signed-off-by: Mike Schore mike.schore@gmail.com

Signed-off-by: Mike Schore <mike.schore@gmail.com>

goaway · 2021-10-12T11:36:29Z

Marking ready for review, but with some caveats:

Additional testing is needed; either here or in a follow-up.
The AuxStreamInfo should probably switch to one of the built-in metadata mechanisms in StreamInfo, which I missed when looking for someplace to put these values initially.

jpsim · 2021-10-12T19:24:48Z

Is there somewhere I could read up on what issue this change is attempting to address/improve?

library/common/extensions/filters/http/network_configuration/filter.cc

Signed-off-by: Mike Schore <mike.schore@gmail.com>

library/common/extensions/retry/options/network_configuration/predicate.cc

library/common/network/configurator.cc

goaway · 2021-10-13T21:25:26Z

cc @Reflejo

mattklein123

Very nice to see this coming together and excited to see this land. Flushing out my first pass of comments, thanks.

library/common/extensions/filters/http/network_configuration/filter.cc

library/common/extensions/retry/options/network_configuration/predicate.cc

library/common/network/configurator.cc

library/common/network/configurator.h

Signed-off-by: Mike Schore <mike.schore@gmail.com>

library/common/extensions/retry/options/network_configuration/predicate.cc

library/common/extensions/filters/http/network_configuration/filter.h

library/common/extensions/retry/options/network_configuration/predicate.cc

Signed-off-by: Mike Schore <mike.schore@gmail.com>

goaway · 2021-10-15T18:12:42Z

@mattklein123 I resolved your comments as I addressed them (hopefully you don't mind), since I like to use the GH facility as a checklist of sorts during large updates. Feel free to re-open any of them though if you feel they need further discussion/work.

mattklein123

Thanks overall looks great. Flushing some more comments from another pass.

library/common/extensions/filters/http/network_configuration/filter.cc

library/common/extensions/retry/options/network_configuration/predicate.cc

mattklein123 · 2021-10-15T20:44:38Z

library/common/extensions/retry/options/network_configuration/predicate.cc

+    return Upstream::RetryOptionsPredicate::UpdateOptionsReturn{absl::nullopt};
+  }
+
+  bool fault = !stream_info.firstUpstreamRxByteReceived().has_value();


Perhaps move this and the filter fault calculation into a shared static utility so it's a bit easier to comment above why we are doing various things?

I added comments to the two locations where we carry out this check (one in the filter, and one in the predicate), but opted not to factor into a shared utility just yet. If this check evolves to become more complicated, and/or we end up calling it from a third place, I'd definitely switch to a shared utility.

library/common/extensions/retry/options/network_configuration/predicate.cc

mattklein123 · 2021-10-15T20:47:52Z

library/common/network/configurator.h

+ *   toggle the boolean. For the purpose of this heuristic, a fault is a request that terminates
+ *   having received 0 transmitted bytes, and a success is anything else.


nit: move this comment to wherever you put the shared calculation logic per my other comments.

library/common/network/configurator.h

library/common/network/configurator.cc

Signed-off-by: Mike Schore <mike.schore@gmail.com>

goaway · 2021-10-15T23:25:34Z

@mattklein123 thanks for the feedback! Updated again.

Signed-off-by: Mike Schore <mike.schore@gmail.com>

mattklein123

Awesome LGTM to ship and iterate!

buildbreaker · 2021-10-16T17:36:14Z

Thank you @mattklein123

* origin/main: [Apple] Guess string encoding when creating an NSString with UTF8 fails (envoyproxy#1891) Link android dev document in a doctree (envoyproxy#1892) bazel: Remove rules_jvm_external dep on JAVA_HOME (envoyproxy#1890) release: 0.4.3.20211020 (envoyproxy#1887) Add debug instructions and sample bazelproject (envoyproxy#1888) bazel: Use hermetic JDK 11 (envoyproxy#1863) envoy: bump upstream to c687308 (envoyproxy#1886) docs: how to test with local envoy (envoyproxy#1876) network: implement initial heuristic for binding alternate interface (envoyproxy#1858) Assign an int to each log level (envoyproxy#1885) envoy: bump upstream to a5b3af2 (envoyproxy#1884) android: stub out jni logging by default (envoyproxy#1879) CI: Add local JDK to asan/tsan builds (envoyproxy#1878) Make JniBridgeUtility public (envoyproxy#1880) swift: Fix Swift version in podspec (envoyproxy#1875) Signed-off-by: JP Simard <jp@jpsim.com>

goaway force-pushed the ms/interface-binding-4 branch from b4b79e5 to 4df0c17 Compare October 4, 2021 18:48

goaway force-pushed the ms/interface-binding-3 branch from 0d2add8 to e77667d Compare October 7, 2021 05:36

goaway force-pushed the ms/interface-binding-4 branch from 4df0c17 to 42da036 Compare October 7, 2021 05:37

Base automatically changed from ms/interface-binding-3 to main October 11, 2021 19:31

network: implement initial heuristic for binding alternate interface

e3fee16

Signed-off-by: Mike Schore <mike.schore@gmail.com>

goaway force-pushed the ms/interface-binding-4 branch from 42da036 to e3fee16 Compare October 12, 2021 11:34

goaway marked this pull request as ready for review October 12, 2021 11:35

jpsim reviewed Oct 12, 2021

View reviewed changes

library/common/extensions/filters/http/network_configuration/filter.cc Show resolved Hide resolved

goaway added 6 commits October 13, 2021 04:49

format

310c742

Signed-off-by: Mike Schore <mike.schore@gmail.com>

use filter state to share configuration key

7fa491b

Signed-off-by: Mike Schore <mike.schore@gmail.com>

format

78479d7

Signed-off-by: Mike Schore <mike.schore@gmail.com>

remove todo

ae67120

Signed-off-by: Mike Schore <mike.schore@gmail.com>

fix test

6909299

Signed-off-by: Mike Schore <mike.schore@gmail.com>

test fix

e0a6ef5

Signed-off-by: Mike Schore <mike.schore@gmail.com>

goaway commented Oct 13, 2021

View reviewed changes

library/common/extensions/retry/options/network_configuration/predicate.cc Show resolved Hide resolved

goaway commented Oct 13, 2021

View reviewed changes

library/common/network/configurator.cc Outdated Show resolved Hide resolved

goaway commented Oct 13, 2021

View reviewed changes

library/common/network/configurator.cc Outdated Show resolved Hide resolved

goaway requested a review from mattklein123 October 13, 2021 21:22

mattklein123 self-assigned this Oct 13, 2021

mattklein123 requested changes Oct 13, 2021

View reviewed changes

goaway added 2 commits October 14, 2021 19:10

pr feedback: filter cleanup

9bfcdfc

Signed-off-by: Mike Schore <mike.schore@gmail.com>

add TODO to predicate

7d31070

Signed-off-by: Mike Schore <mike.schore@gmail.com>

goaway commented Oct 14, 2021

View reviewed changes

library/common/extensions/retry/options/network_configuration/predicate.cc Show resolved Hide resolved

mattklein123 reviewed Oct 14, 2021

View reviewed changes

library/common/extensions/filters/http/network_configuration/filter.h Outdated Show resolved Hide resolved

library/common/extensions/retry/options/network_configuration/predicate.cc Outdated Show resolved Hide resolved

goaway added 3 commits October 15, 2021 17:34

wip updates

2059798

Signed-off-by: Mike Schore <mike.schore@gmail.com>

Merge branch 'main' into ms/interface-binding-4

f619c48

Signed-off-by: Mike Schore <mike.schore@gmail.com>

further feedback

6775bf3

Signed-off-by: Mike Schore <mike.schore@gmail.com>

coverage

80d0aaf

Signed-off-by: Mike Schore <mike.schore@gmail.com>

goaway requested a review from mattklein123 October 15, 2021 18:13

mattklein123 requested changes Oct 15, 2021

View reviewed changes

goaway added 4 commits October 16, 2021 05:38

further document filter and predicate

d608148

Signed-off-by: Mike Schore <mike.schore@gmail.com>

update overridden_ to socket_mode_

c9d3ff6

Signed-off-by: Mike Schore <mike.schore@gmail.com>

add ABSL guards

21251aa

Signed-off-by: Mike Schore <mike.schore@gmail.com>

more feedback addressed

352dcd3

Signed-off-by: Mike Schore <mike.schore@gmail.com>

goaway requested a review from mattklein123 October 15, 2021 23:25

spelling

067b043

Signed-off-by: Mike Schore <mike.schore@gmail.com>

mattklein123 approved these changes Oct 16, 2021

View reviewed changes

buildbreaker merged commit 44586eb into main Oct 16, 2021

buildbreaker deleted the ms/interface-binding-4 branch October 16, 2021 17:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

network: implement initial heuristic for binding alternate interface #1858

network: implement initial heuristic for binding alternate interface #1858

goaway commented Oct 4, 2021 •

edited

Loading

goaway commented Oct 12, 2021

jpsim commented Oct 12, 2021

goaway commented Oct 13, 2021

mattklein123 left a comment

goaway commented Oct 15, 2021

mattklein123 left a comment

mattklein123 Oct 15, 2021

goaway Oct 15, 2021

mattklein123 Oct 15, 2021

goaway commented Oct 15, 2021

mattklein123 left a comment

buildbreaker commented Oct 16, 2021

		* toggle the boolean. For the purpose of this heuristic, a fault is a request that terminates
		* having received 0 transmitted bytes, and a success is anything else.

network: implement initial heuristic for binding alternate interface #1858

network: implement initial heuristic for binding alternate interface #1858

Conversation

goaway commented Oct 4, 2021 • edited Loading

goaway commented Oct 12, 2021

jpsim commented Oct 12, 2021

goaway commented Oct 13, 2021

mattklein123 left a comment

Choose a reason for hiding this comment

goaway commented Oct 15, 2021

mattklein123 left a comment

Choose a reason for hiding this comment

mattklein123 Oct 15, 2021

Choose a reason for hiding this comment

goaway Oct 15, 2021

Choose a reason for hiding this comment

mattklein123 Oct 15, 2021

Choose a reason for hiding this comment

goaway commented Oct 15, 2021

mattklein123 left a comment

Choose a reason for hiding this comment

buildbreaker commented Oct 16, 2021

goaway commented Oct 4, 2021 •

edited

Loading