android DNS: add ability to use fallback nameservers #1953
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 5 commits
November 30, 2021 10:23
Signed-off-by: Jose Nino <jnino@lyft.com>
Closed
added 3 commits
November 30, 2021 10:30
Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
This reverts commit b41131b.
goaway
reviewed
Dec 1, 2021
added 3 commits
December 2, 2021 15:27
goaway
reviewed
Dec 3, 2021
| @@ -267,6 +268,9 @@ R"( | |||
| base_interval: *dns_fail_base_interval | |||
| max_interval: *dns_fail_max_interval | |||
| dns_query_timeout: *dns_query_timeout | |||
| typed_dns_resolver_config: | |||
| name: *dns_resolver_name | |||
There was a problem hiding this comment.
Need a default for this. I think it makes sense to just specify c-ares as the default above (along with dns_resolver_config, and iOS can still always override it with the apple dns resolves.
There was a problem hiding this comment.
@goaway I was wondering if this was the case. I make overrides for it on both the java and objc definitions so I saw no point in putting in a default in the common template given there is no common default.
There was a problem hiding this comment.
Right now the default (header + config) with no overrides is a valid config. I'd like to keep it that way.
One reason is that we can start to use it in tests, instead of the canned configs some tests use (that have tendency to drift).
|
Looks good overall, left one comment. Thanks for updating! |
added 2 commits
December 3, 2021 13:20
Signed-off-by: Jose Nino <jnino@lyft.com>
goaway
previously approved these changes
Dec 3, 2021
This wrapper allows developers to quickly get off the ground without having to install bazel or bazelisk manually. Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
Description: This is a followup to #1937. Risk Level: I believe these extra parameters are safe to ignore if we don't need to propagate them to the Objective-C interface, but would like someone else to confirm that. (Confirmed.) Testing: Example apps still work, and downstream (closed source) consumers now compile again. Signed-off-by: JP Simard <jp@jpsim.com>
Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
Description: Fix NSString to envoy_data conversion for non-ascii strings such as '台灣大哥大'. -length is the number of UTF-16 code units in the receiver whereas -lengthOfBytesUsingEncoding: is the number of UTF8 bytes. Using the latter is necessary since we're using this value to determine how many bytes to copy from the string. Risk Level: Low. Testing: Added an Objective-C unit test, and enabled running on CI. Signed-off-by: JP Simard <jp@jpsim.com>
Description: Both v4_interfaces and v6_interfaces were using V4 interfaces likely due to a typo. This is a follow-up to #1897. Risk Level: Low, just fixing a log. Signed-off-by: JP Simard <jp@jpsim.com>
Changes: envoyproxy/envoy@23e5fc2...70a5f29 Signed-off-by: JP Simard <jp@jpsim.com>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Jose Nino <jnino@lyft.com>
junr03
force-pushed
the
working-config
branch
from
539cf31
to
2da3985
Compare
Signed-off-by: Jose Nino <jnino@lyft.com>
goaway
reviewed
Dec 15, 2021
| @@ -100,6 +100,10 @@ std::string EngineBuilder::generateConfigStr() { | |||
| {"dns_preresolve_hostnames", this->dns_preresolve_hostnames_}, | |||
| {"dns_refresh_rate", fmt::format("{}s", this->dns_refresh_seconds_)}, | |||
| {"dns_query_timeout", fmt::format("{}s", this->dns_query_timeout_seconds_)}, | |||
| {"dns_resolver_name", "envoy.network.dns_resolver.cares"}, | |||
| {"dns_resolver_config", | |||
| "{\"@type\":\"type.googleapis.com/" | |||
There was a problem hiding this comment.
nit: I'd suggest a string quoting approach that doesn't require a bunch of inline escaping
goaway
approved these changes
Dec 15, 2021
|
Thanks @junr03! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description: this PR uses config templating to introduce fallback DNS servers for c-ares in Android. Subsequent PR will add EngineBuilder configurability.
Risk Level: low -- the current config is a no-op.
Testing: e2e testing with the Lyft App