network dns: fix AUTO settings to properly fallback to v4 lookup when v6 returns only cname entries by glicht · Pull Request #2641 · envoyproxy/envoy

glicht · 2018-02-19T13:55:47Z

network dns: fix AUTO settings to properly fallback to v4 lookup when v6 returns only cname entries

Description:

Fixes #2634

Fixed dns_impl.cc to assume completed only when actual ips are returned from a v6 lookup.

Risk Level: Low

Testing:

Added unit tests to test the CNAME scenario where a v6 lookup returns only cnames.
Manual testing as described at DNS lookup AUTO settings doesn't fallback to V4 when a single CNAME entry is returned #2634

Release Notes: N/A

…returns only cname entries Fixes envoyproxy#2634 Added unit tests to test the CNAME scenario where a v6 lookup retuns only cnames. Fixed dns_impl.cc to assume completed only when actuall ips are returned from a v6 lookup. Signed-off-by: Guy Lichtman <guy@lichtman.io>

alyssawilk · 2018-02-26T14:10:29Z

@junr03 you up for doing first pass?

junr03

sorry for the delay @glicht. Expect a much faster turn around going forward

junr03 · 2018-02-27T17:33:23Z

test/common/network/dns_impl_test.cc


  ~TestDnsServerQuery() { connection_->close(ConnectionCloseType::NoFlush); }

+  // Utility to encode a dns string in the rfc format. Example: \004some\004good\006domain


link to RFC in the comment

junr03 · 2018-02-27T17:50:16Z

test/common/network/dns_impl_test.cc

+        write_buffer_.add(&response_size_n, sizeof(response_size_n));
+        write_buffer_.add(response_base, response_base_len);
+
+        // create a resource record for cname if we have


if we have a cname

junr03 · 2018-02-27T17:52:47Z

test/common/network/dns_impl_test.cc

+          hostLookup = cname.c_str();
+          encodedCname = TestDnsServerQuery::encodeDnsName(cname);
+          ip_question = reinterpret_cast<const unsigned char*>(encodedCname.c_str());
+          ip_name_len = encodedCname.size() + 1;


sorry if I am being dense, but why do we have +1 here and below in the response_cname_len calculation? It might be worthwhile to sprinkle some clarifying comments in order to ease a new reader into faster understanding

+1, as the encoded cname format always ends with a null (0) char which also needs to be written out. Will add this in a comment.

junr03 · 2018-02-27T18:00:51Z

test/common/network/dns_impl_test.cc

+        }
+        size_t response_cname_len =
+            encodedCname.size() > 0 ? name_len + RRFIXEDSZ + encodedCname.size() + 1 : 0;
+        // Send response to client.


This comment makes this section a little confusing.

Previously, we computed all the elements of the response and then wrote them all to the client.

In this PR we have moved to computing and writing in lockstep on each piece of the request.

I think we should either refactor to how it was previously done (compute then write all), or change the comments (the one this review comment hangs off of, add a comment above line 171 size_t response_ip_rest_len, etc)

I agree the cname flow is a bit confusing. First there is need to see if a domain name resolves to a cname entry. Then there is need to lookup the cname record to see if it resolves to an ip or multiple ips. We still need to compute the total response size before starting to write each response element. This is computed with the variable: response_size_n. After writing out the header response, then we need to write out all response elements which include the cname entry and the ips. I'll add a comment to better explain this logic.

Signed-off-by: Guy Lichtman <guy@lichtman.io>

glicht · 2018-03-01T07:33:23Z

@junr03, I've added some more comments following your code review. Let me know if you see need for more changes.

junr03 · 2018-03-01T18:14:57Z

@alyssawilk looks fine to me. Want to give a pass?

alyssawilk

Thanks for the fix and the comprehensive testing! Only a few minor nits on my end.

alyssawilk · 2018-03-01T18:38:36Z

test/common/network/dns_impl_test.cc

+    auto name_split = StringUtil::splitToken(input, ".");
+    std::string res;
+    for (const auto& it : name_split) {
+      res += static_cast<char>(it.size());


Can you toss in an assert on the total size, just to make it super clear it won't work with illegally long DNS names?

alyssawilk · 2018-03-01T18:49:02Z

test/common/network/dns_impl_test.cc

+            encodedCname.size() > 0 ? name_len + RRFIXEDSZ + encodedCname.size() + 1 : 0;
+        const uint16_t response_size_n =
+            htons(response_base_len + response_ip_rest_len + response_cname_len);
+        Buffer::OwnedImpl write_buffer_;


If this is local scope you don't need the trailing _ - that's only for member variables

eyes of a falcon, totally missed it

The trailing "_" was present before my changes, so I didn't really give this thought. Anyway, I'll add a commit to change the name of the variable.

alyssawilk · 2018-03-01T18:49:43Z

test/common/network/dns_impl_test.cc

+        DNS_HEADER_SET_ANCOUNT(response_base, answer_size);
        DNS_HEADER_SET_NSCOUNT(response_base, 0);
        DNS_HEADER_SET_ARCOUNT(response_base, 0);
+        // Total response size will be computed according to cname response size + ip response sizes


Optional - this function is getting reaaally long. If there's anything else you can easily factor into a utility helper that'd be nice. Optional because it's test code and you've done a great job with comments and variable names.

I'll pass on this. I agree the function is getting long. But, I am not too sure that refactoring will make this easier to understand.

Signed-off-by: Guy Lichtman <guy@lichtman.io>

glicht · 2018-03-02T12:11:55Z

@alyssawilk, I've added a commit following your code review. Let me know if you see need for more changes.

alyssawilk

Sorry for the delay - I'm out Fridays. LGTM!

alyssawilk · 2018-03-05T15:47:22Z

@junr03 LGTY? I can only force-merge while github has you flagged as having requested changes :-/

…kup when v6 returns only cname entries (envoyproxy#2641)" This reverts commit f194fa2.

Disable flaky TestConfig.StringAccessors util the cause of flakiness is addressed. #2641 Risk Level: Low Testing: N/A Docs Changes: N/A Release Notes: N/A Signed-off-by: Ryan Hamilton rch@google.com Signed-off-by: JP Simard <jp@jpsim.com>

glicht changed the title ~~network: fix AUTO settings to properly fallback to v4 lookup when v6 returns only cname entries~~ network dns: fix AUTO settings to properly fallback to v4 lookup when v6 returns only cname entries Feb 19, 2018

danielhochman assigned junr03 and alyssawilk Feb 20, 2018

junr03 suggested changes Feb 27, 2018

View reviewed changes

improved comments following code review by @junr03

4ca311f

Signed-off-by: Guy Lichtman <guy@lichtman.io>

alyssawilk suggested changes Mar 1, 2018

View reviewed changes

improvemnets following code review by @alyssawilk

4e80e63

Signed-off-by: Guy Lichtman <guy@lichtman.io>

alyssawilk approved these changes Mar 5, 2018

View reviewed changes

junr03 approved these changes Mar 5, 2018

View reviewed changes

alyssawilk merged commit f194fa2 into envoyproxy:master Mar 5, 2018

lita pushed a commit to lita/envoy that referenced this pull request Mar 5, 2018

Revert "network dns: fix AUTO settings to properly fallback to v4 loo…

f0c2c4a

…kup when v6 returns only cname entries (envoyproxy#2641)" This reverts commit f194fa2.


		~TestDnsServerQuery() { connection_->close(ConnectionCloseType::NoFlush); }

		// Utility to encode a dns string in the rfc format. Example: \004some\004good\006domain

Conversation

glicht commented Feb 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alyssawilk commented Feb 26, 2018

Uh oh!

junr03 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glicht Feb 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glicht commented Mar 1, 2018

Uh oh!

junr03 commented Mar 1, 2018

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glicht commented Mar 2, 2018

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

alyssawilk commented Mar 5, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

glicht commented Feb 19, 2018 •

edited

Loading

glicht Feb 27, 2018 •

edited

Loading