dns: introduce ResolutionStatus for ResolveCb and fix #9927 #10137

junr03 · 2020-02-21T23:33:14Z

Description: this PR introduces ResolutionStatus to the ResolveCb. This status can be used to determine if DnsResolution was successful or not, and lets the callback target adjust accordingly. It is used to fix #9927. Additionally, it can be used to fix
envoyproxy/envoy-mobile#673 in a subsequent PR.
Risk Level: med. This PR changes the behavior of Strict Dns Cluster. The introduced argument is not used anywhere, so all behavior stays the same.
Testing: Updated tests to pass the new argument to the callback. But behavior stays the same.
Docs Changes: updated config docs and discovery docs.
Release Notes: added a release note.

Fixes #9927

Signed-off-by: Jose Nino jnino@lyft.com

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 · 2020-02-21T23:35:52Z

@mattklein123 I finally got time to come back to this. I started by just introducing the new argument for the callback, but leaving all behavior unchanged. I didn't want to conflate this with the two independent changes (don't update hosts on failure, allow different timeouts on failure) needed for the linked issues.

Let me know if you'd rather look at it all together.

Signed-off-by: Jose Nino <jnino@lyft.com>

include/envoy/network/dns.h

Signed-off-by: Jose Nino <jnino@lyft.com>

mattklein123

Thanks this is great. Flushing out a few comments from my first pass. Thank you!

/wait

mattklein123 · 2020-02-24T17:43:34Z

source/common/network/dns_impl.cc

@@ -89,7 +89,7 @@ void DnsResolverImpl::PendingResolution::onAresGetAddrInfoCallback(int status, i
    // callback_ target _should_ still be around. In that case, raise the callback_ so the target
    // can be done with this query and initiate a new one.
    if (!cancelled_) {
-      callback_({});
+      callback_(ResolutionStatus::Failure, {});


Should you have another status like cancelled/destructing/etc.? This is not quite a failure? I'm not sure it matters to higher layer code but it might?

I thought about this when I first introduced the destruction behavior. My first opinion is that higher layers only pivot behavior based on success/failure so I was going to keep it at that level of complexity for this change, and the subsequent PR where I fix #9927.

Then I was going to explore the need to add a Destruction value that allow us to set tighter timers than on "regular failure" when fixing for envoyproxy/envoy-mobile#673. But I am not sure if we need different behavior based on what particular failure scenario the resolver experienced. By the same token, people might want even more granularity in the future?

In short: I did not want to commit to more granular status in this initial PR, and I don't think it is going to be that bad to add more granularity, if desired, in subsequent PRs.

test/common/network/dns_impl_test.cc

test/common/upstream/logical_dns_cluster_test.cc

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 · 2020-02-25T00:14:32Z

@mattklein123 updated. I have updated dns_impl_test for expectations around status in different conditions. I also updated higher level constructs to use status, but I didn't change any behavior yet based on this new information (other than adding more relevant stats where possible). I plan on addressing #10137 (comment) and hence #9927 in a subsequent PR. Let me know if you'd rather see it here.

Edit: fixing #9927 here.

Signed-off-by: Jose Nino <jnino@lyft.com>

repokitteh-read-only · 2020-02-26T01:44:25Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #10137 was synchronize by junr03.

see: more, trace.

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 · 2020-02-26T01:52:44Z

source/common/upstream/logical_dns_cluster.cc

          // TODO(mattklein123): Move port handling into the DNS interface.
          ASSERT(response.front().address_ != nullptr);
          Network::Address::InstanceConstSharedPtr new_address =
              Network::Utility::getAddressWithPort(*(response.front().address_),
                                                   Network::Utility::portFromTcpUrl(dns_url_));

-          if (respect_dns_ttl_ && response.front().ttl_ != std::chrono::seconds(0)) {


moved this for consistency with the strict dns cluster code. But happy to move back up.

junr03 · 2020-02-26T01:53:54Z

test/common/upstream/logical_dns_cluster_test.cc

+  tls_.shutdownThread();
+}
+
+TEST_F(LogicalDnsParamTest, TtlAsDnsRefreshRate) {


Adding coverage that did not exist.

Signed-off-by: Jose Nino <jnino@lyft.com>

mattklein123

Nice work. Just a test comment. Thank you!

/wait

test/common/network/dns_impl_test.cc

mattklein123 · 2020-02-26T19:42:48Z

test/common/network/dns_impl_test.cc

+                                        [&](DnsResolver::ResolutionStatus,
+                                            std::list<DnsResponse>&& results) -> void {


I'm not sure what the best way would be but is there some way to simplify all of thee calls to make sure have a success/failure expectation on all of them? Perhaps some helper function/object that does the resolve, does the expectations, and stores the resolution list?

folded everything into resolveWithExpectations

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 · 2020-02-27T00:09:09Z

test/common/network/dns_impl_test.cc

-  EXPECT_TRUE(hasAddress(address_list, "201.134.56.7"));
-}
-
-TEST_P(DnsImplTest, DnsIpAddressVersion) {


moved above

Signed-off-by: Jose Nino <jnino@lyft.com>

mattklein123

Thanks for the awesome test cleanup. 1 question. Thank you!

/wait-any

mattklein123 · 2020-02-27T16:42:30Z

test/common/network/dns_impl_test.cc

-                         [&](const std::list<DnsResponse> &
-                             /*results*/) -> void { throw EnvoyException("Envoy exception"); }));
+  EXPECT_EQ(nullptr,
+            resolver_->resolve(


qq: Can't we use the new resolve with expectations everywhere?

yes! I thought it might be overkill given that there were far fewer call sites. But if I'm cleaning up something why not go the entire way :). Now everything (for real) is folded into helper functions.

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 · 2020-02-27T22:00:11Z

Looks like the circle machines have a funny hosts file?

[----------] 1 test from IpVersions/DnsImplTest
[ RUN      ] IpVersions/DnsImplTest.LocalLookup/IPv4
test/common/network/dns_impl_test.cc:470: Failure
Expected equality of these values:
  expected_results
    Which is: { "127.0.0.1" }
  address_as_string_list
    Which is: { "127.0.0.1", "127.0.0.1" }

mattklein123 · 2020-02-27T22:07:46Z

Thanks for the fix. If that's the CI error you are seeing it seems like it must have to do with your change?

junr03 · 2020-02-28T14:01:40Z

Yep, definitely has to do with my change. I just wanted to make sure that I was surfacing something latent in circle ci and not an unintended regression. Previously the LocalLookup test only asserted that 127.0.0.1 was contained in the resolved address list, not exact match like now. I opened #10197 to verify. It looks like our circle CI setup ends up with a hosts file with a double entry for localhost. https://app.circleci.com/jobs/github/envoyproxy/envoy/324116.

Signed-off-by: Jose Nino <jnino@lyft.com>

mattklein123

Thanks!

Description: updates envoy ref to include envoyproxy/envoy#10137. Exposes client builder function to add dns failure refresh rate. Risk Level: low, new configuration option Testing: local apps, CI Docs Changes: created #715 to create a section in the docs for all the builder options. Fixes #673

Description: updates envoy ref to include envoyproxy/envoy#10137. Exposes client builder function to add dns failure refresh rate. Risk Level: low, new configuration option Testing: local apps, CI Docs Changes: created #715 to create a section in the docs for all the builder options. Fixes #673 Signed-off-by: Michael Rebello <me@michaelrebello.com>

Description: updates envoy ref to include #10137. Exposes client builder function to add dns failure refresh rate. Risk Level: low, new configuration option Testing: local apps, CI Docs Changes: created #715 to create a section in the docs for all the builder options. Fixes #673 Signed-off-by: JP Simard <jp@jpsim.com>

introduce status to the callback and maintain prior functionality

8b16860

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 requested review from alyssawilk and mattklein123 as code owners February 21, 2020 23:33

junr03 changed the title ~~introduce status to the callback and maintain prior functionality~~ dns: introduce status to the callback and maintain prior functionality Feb 21, 2020

junr03 assigned mattklein123 Feb 21, 2020

update validation resolver

7f160b2

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 changed the title ~~dns: introduce status to the callback and maintain prior functionality~~ dns: introduce ResolutionStatus for ResolveCb Feb 21, 2020

junr03 commented Feb 22, 2020

View reviewed changes

include/envoy/network/dns.h Show resolved Hide resolved

Jose Nino added 2 commits February 21, 2020 16:19

update dns impl test

6bc89aa

Signed-off-by: Jose Nino <jnino@lyft.com>

update upstream impl test

7a90217

Signed-off-by: Jose Nino <jnino@lyft.com>

mattklein123 requested changes Feb 24, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Feb 24, 2020

update

98f2205

Signed-off-by: Jose Nino <jnino@lyft.com>

repokitteh-read-only bot removed the waiting label Feb 25, 2020

missing commit

a2385e4

Signed-off-by: Jose Nino <jnino@lyft.com>

mattklein123 added the waiting label Feb 25, 2020

update and fix #9927

6cce569

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 changed the title ~~dns: introduce ResolutionStatus for ResolveCb~~ dns: introduce ResolutionStatus for ResolveCb and fix #9927 Feb 26, 2020

repokitteh-read-only bot removed the waiting label Feb 26, 2020

repokitteh-read-only bot added the api label Feb 26, 2020

Jose Nino added 2 commits February 25, 2020 17:45

release notes

af99ef8

Signed-off-by: Jose Nino <jnino@lyft.com>

Merge branch 'master' into dns-status

3f9e574

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 commented Feb 26, 2020

View reviewed changes

fmt

4628947

Signed-off-by: Jose Nino <jnino@lyft.com>

mattklein123 requested changes Feb 26, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Feb 26, 2020

update test

93b3180

Signed-off-by: Jose Nino <jnino@lyft.com>

repokitteh-read-only bot removed the waiting label Feb 27, 2020

junr03 commented Feb 27, 2020

View reviewed changes

move line

c7ee391

Signed-off-by: Jose Nino <jnino@lyft.com>

mattklein123 reviewed Feb 27, 2020

View reviewed changes

repokitteh-read-only bot added the waiting:any label Feb 27, 2020

ask and you shall receive

a8a140e

Signed-off-by: Jose Nino <jnino@lyft.com>

repokitteh-read-only bot removed the waiting:any label Feb 27, 2020

Merge branch 'master' into dns-status

a44add4

Signed-off-by: Jose Nino <jnino@lyft.com>

mattklein123 added the waiting label Feb 28, 2020

circle ci

3d35981

Signed-off-by: Jose Nino <jnino@lyft.com>

repokitteh-read-only bot removed the waiting label Feb 28, 2020

mattklein123 approved these changes Feb 28, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Feb 28, 2020

mattklein123 merged commit 64d91b2 into envoyproxy:master Feb 28, 2020

junr03 mentioned this pull request Feb 29, 2020

dns: add ability to set dns failure refresh rate envoyproxy/envoy-mobile#714

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dns: introduce ResolutionStatus for ResolveCb and fix #9927 #10137

dns: introduce ResolutionStatus for ResolveCb and fix #9927 #10137

junr03 commented Feb 21, 2020 •

edited

Loading

junr03 commented Feb 21, 2020

mattklein123 left a comment

mattklein123 Feb 24, 2020

junr03 Feb 24, 2020

junr03 commented Feb 25, 2020 •

edited

Loading

repokitteh-read-only bot commented Feb 26, 2020

junr03 Feb 26, 2020

junr03 Feb 26, 2020

mattklein123 left a comment

mattklein123 Feb 26, 2020

junr03 Feb 27, 2020

junr03 Feb 27, 2020

mattklein123 left a comment

mattklein123 Feb 27, 2020

junr03 Feb 27, 2020

junr03 commented Feb 27, 2020

mattklein123 commented Feb 27, 2020

junr03 commented Feb 28, 2020

mattklein123 left a comment

		[&](DnsResolver::ResolutionStatus,
		std::list<DnsResponse>&& results) -> void {

dns: introduce ResolutionStatus for ResolveCb and fix #9927 #10137

dns: introduce ResolutionStatus for ResolveCb and fix #9927 #10137

Conversation

junr03 commented Feb 21, 2020 • edited Loading

junr03 commented Feb 21, 2020

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

junr03 commented Feb 25, 2020 • edited Loading

repokitteh-read-only bot commented Feb 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

junr03 commented Feb 27, 2020

mattklein123 commented Feb 27, 2020

junr03 commented Feb 28, 2020

mattklein123 left a comment

Choose a reason for hiding this comment

junr03 commented Feb 21, 2020 •

edited

Loading

junr03 commented Feb 25, 2020 •

edited

Loading