network/config: Add IPv6 DNS support by hennna · Pull Request #1002 · envoyproxy/envoy

hennna · 2017-05-19T21:06:41Z

This PR adds IPv6 DNS support and adds a cluster config parameter "dns_lookup_ip_version" that specifies the DNS lookup IP address policy. The default policy if no parameter is specified is "v4_only".

Modifications were made to upstream_cluster and logical_dns_cluster to pass this parameter on when calling DnsResolver resolve. Modifications were made to dns_impl to add v6 support and also a fallback option when v4 dns lookup fails.

Fixes #978 .

hennna · 2017-05-19T21:07:00Z

@htuch @jamessynge for high level review and sanity check first

htuch · 2017-05-19T21:22:49Z

+dns_loopkup_ip_version
+  *(optional, string)* The dns IP address version lookup policy. The options are *v4_only*, *v6_only*,
+  and *auto*. If this setting is not specified, the value defaults to *v4_only*. When *v4_only* is selected,
+  the dns resolver will only perform a lookup for addresses in the IPv4 family. If *v6_only* is selected,


Nit: s/dns/DNS/ at least in documentation/comments.

htuch · 2017-05-21T22:30:59Z

 }

+void DnsResolverImpl::PendingResolution::getHostByName(int family) {
+  ares_gethostbyname(channel_, dns_name_.c_str(),


As discussed IRL last week, ares_gethostbyname() can implement the IPv6-IPv4 fallback with AF_UNSPEC family set (but is badly documented). However, I think your current implementation gets us closer to where we want to be in #978, since it will allow for the return of both IPv6 and IPv4 addresses in a list in a future PR, where we can support the caller deciding upon fallback as a result of connect() failure. So, I reckon it's good to keep the explicit fallback in this PR.

It also seems that AF_UNSPEC has unexpected behavior (not a straightforward IPv6 lookup followed by a IPv4 lookup). Once that and returning multiple address types is added to the c-ares library, we can switch over to using AF_UNSPEC.

dnoe · 2017-05-22T12:43:52Z


+.. _config_cluster_manager_cluster_dns_lookup_ip_version:
+
+dns_loopkup_ip_version


loopkup -> lookup

dnoe · 2017-05-22T12:52:47Z

   */
-  virtual ActiveDnsQuery* resolve(const std::string& dns_name, ResolveCb callback) PURE;
+  virtual ActiveDnsQuery* resolve(const std::string& dns_name,
+                                  const std::string& dns_lookup_ip_version,


Instead of passing the dns_lookup_ip_version as a string, consider resolving it into an enum when you process the configuration and use that enum here. This will be cheaper than doing the string comparisons every time you resolve.

hennna · 2017-05-22T19:55:17Z

Updated. PTAL.

htuch

The core ares code looks good, but there's a dangerous side condition to fix :)

htuch · 2017-05-22T21:38:27Z

  virtual void cancel() PURE;
 };

+enum class DnsLookupFamily { v4_only, v6_only, fallback };


Nit: CAPS for enums for consistency:

It seems there are enums with all caps (https://github.com/lyft/envoy/blob/998b8119ed57269e77d0a03b1b92e7a98c5d30d1/source/server/config/network/http_connection_manager.h#L114) but also many cases of not all caps (https://github.com/lyft/envoy/blob/998b8119ed57269e77d0a03b1b92e7a98c5d30d1/source/server/config/network/http_connection_manager.h#L24) Which is the convention?

This came up in another PR recently: https://github.com/lyft/envoy/pull/989/files/431c909dcd7d70686cc9986c9ef019bef8942ae2#r117533920

I think we are going with all-caps. Feel free to create a https://github.com/lyft/envoy/blob/master/STYLE.md to codify.

htuch · 2017-05-22T21:39:13Z

   */
-  virtual ActiveDnsQuery* resolve(const std::string& dns_name, ResolveCb callback) PURE;
+  virtual ActiveDnsQuery* resolve(const std::string& dns_name,
+                                  const DnsLookupFamily& dns_lookup_family,


I would just pass this by value, it might even be more efficient to do so (it's just a small integer).

htuch · 2017-05-22T21:39:44Z

      },
+      "dns_lookup_family" : {
+        "type" : "string",
+        "enum" : ["v4_only", "v6_only", "auto"]


Docs say fallback, but it's auto here.

I actually, like 'auto' for consistency with other settings (just fix the doc)

The problem with auto is that it's a keyword and can't be used when making the string to enum conversion.

Actually AUTO might be okay

htuch · 2017-05-22T21:43:33Z

+        address.sin6_family = AF_INET6;
+        address.sin6_port = 0;
+        address.sin6_addr = *reinterpret_cast<in6_addr*>(hostent->h_addr_list[i]);
+        address_list.emplace_back(new Address::Ipv6Instance(address));


Unrelated to your review, but it seems weird that the sockeadd[_in6] constructors for Ipv4Instance and Ipv6Instance differ in whether they take a pointer or reference.

htuch · 2017-05-22T21:45:19Z

+    getHostByName(AF_INET);
  }
-  if (!cancelled_) {
+  if (!cancelled_ && completed_) {


Nit: You can move both if clauses under a if (completed_) {.

htuch · 2017-05-22T21:47:33Z

+                                         ResolveCb callback) {
  std::unique_ptr<PendingResolution> pending_resolution(new PendingResolution());
  pending_resolution->callback_ = callback;
+  pending_resolution->channel_ = channel_;


As a cleanup to my earlier mess, you could make all of these constructor args and initialize them in the constructor's member initializor list, allowing them to be made const.

htuch · 2017-05-22T21:50:09Z

    }
+  } else if (fallback_if_failed) {
+    fallback_if_failed = false;
+    getHostByName(AF_INET);


I think there's a dangerous corner case here. If the first getHostByName() defers to asynch completion but then the fallback getHostByName() succeeds immediately (synchronously), it will invoke the callback, which will delete this;. This will then return to the caller method belonging to a now deleted object.

RomanDzhabarov · 2017-05-22T22:05:58Z

      },
+      "dns_lookup_family" : {
+        "type" : "string",
+        "enum" : ["v4_only", "v6_only", "auto"]


I actually, like 'auto' for consistency with other settings (just fix the doc)

RomanDzhabarov · 2017-05-22T22:17:30Z

+  ares_gethostbyname(channel_, dns_name_.c_str(),
+                     family, [](void* arg, int status, int timeouts, hostent* hostent) {
+                       static_cast<PendingResolution*>(arg)->onAresHostCallback(status, hostent);
+                       UNREFERENCED_PARAMETER(timeouts);


i think you should be able to (void arg, int status, int, hostent hostent) and remove UNREFERENCED_PARAMETER.

RomanDzhabarov · 2017-05-22T22:21:51Z

+    ares_channel channel_;
+    std::string dns_name_;
+
+    void getHostByName(int family);


nit: put this right after void onAresHostCallback(int status, hostent* hostent);

RomanDzhabarov · 2017-05-22T22:23:04Z

+  } else if (dns_lookup_family == "fallback") {
+    dns_lookup_family_ = Network::DnsLookupFamily::fallback;
+  } else {
+    dns_lookup_family_ = Network::DnsLookupFamily::v4_only;


for being on a defensive side can you assert here that dns_lookup_family is equal to "v4_only".

RomanDzhabarov · 2017-05-22T22:23:53Z

    throw EnvoyException("logical_dns clusters must have a single host");
  }

+  // Resolve string to enum.


up to you, but the comment does not add value, it's pretty straightforward what's happening here

RomanDzhabarov · 2017-05-22T22:27:13Z

+          // TODO(mattklein123): Move port handling into the DNS interface.
+          Network::Address::IpVersion version = address_list.front()->ip()->version();
+          Network::Address::InstanceConstSharedPtr new_address;
+          if (version == Network::Address::IpVersion::v4) {


can you do switch here, so we make sure that compiler will complain in case enum values changes (with low probability though :))

RomanDzhabarov · 2017-05-22T22:27:22Z

            // want to do better again later.
-            logical_host_.reset(new LogicalHost(
-                info_, hostname_, Network::Utility::resolveUrl("tcp://0.0.0.0:0"), *this));
+            if (version == Network::Address::IpVersion::v4) {


htuch

This is close to being ready to merge once the comments are solved. @RomanDzhabarov do you have any further comments?

htuch · 2017-05-24T13:52:21Z

  includes both const and non-const references.
 * Function names using camel case starting with a lower case letter (e.g., "doFoo()").
 * Struct/Class member variables have a '\_' postfix (e.g., "int foo\_;").
+* Enum values using all CAPS (e.g., "EXTERNAL_ONLY").


Thanks. Small nit: use back ticks to quote, e.g. EXTERNAL_ONLY.

hm, I'm not sure about this.
grep -ir 'enum class' gives the majority of the enum values in PascalCase.

Any particular reason we want to stick to the all CAPS?

It actually looks like #989 was referring to static const values. However, there are also cases of all CAPS enums in the code.

yeah, I'd do Pascal and create an issue to clean up all of the enum values which do not conform this rule (Those are mostly have HTTPX form)

@Henna @htuch what would you say?

htuch · 2017-05-24T13:53:53Z

    "dns_refresh_rate_ms": "...",
-    "outlier_detection": "{...}"
+    "dns_lookup_family": "...",
+    "outlier_detection": "..."


I think you undid #1010 here, can you revert the change to the "outlier_detection" line?

htuch · 2017-05-24T13:57:54Z

    bool cancelled_ = false;
+    // If dns_lookup_family is "fallback", fallback to v4 address if v6
+    // resolution failed.
+    bool fallback_if_failed = false;


fallback_if_failed_

htuch · 2017-05-24T13:58:47Z

+    fallback_if_failed = false;
+    getHostByName(AF_INET);
+    // Note: Nothing can follow this call to getHostByName due to deletion of this
+    // object upon synchronous resolution.


Maybe add an explicit return; after the comment to make this even clearer.

htuch · 2017-05-24T14:07:59Z

+          // TODO(mattklein123): Move port handling into the DNS interface.
+          Network::Address::IpVersion version = address_list.front()->ip()->version();
+          Network::Address::InstanceConstSharedPtr new_address;
+          switch (version) {


This stuff can probably be factored out. You can add to Network::Address::Instance a withPort(int port) method that returns a new address with the port replaced as done here.

htuch · 2017-05-24T14:08:32Z

+                parent_.info_, dns_address_,
+                Network::Address::InstanceConstSharedPtr{
+                    new Network::Address::Ipv6Instance(address->ip()->addressAsString(), port_)},
+                false, 1, ""));


As mentioned above.

htuch · 2017-05-24T14:10:53Z

+        EXPECT_CALL(*resolve_timer_, enableTimer(_));
+        cb(TestUtility::makeDnsResponse({"::1"}));
+        return nullptr;
+      }));


There might be an argument for using TEST_P here, but I'm not sure it's worth the effort and it's probably more readable the way you've done it.

htuch · 2017-05-24T14:13:52Z

+                               dispatcher);
+  cluster.setInitializedCb([&]() -> void { initialized.ready(); });
+  EXPECT_EQ(2UL, cluster.hosts().size());
+  EXPECT_EQ(2UL, cluster.healthyHosts().size());


Actually, I think for these bigger examples, TEST_P would make more sense. You can make the parameter a std::tuple of string for the dns_lookup_family, DNS family and the DNS response list.

htuch · 2017-05-24T14:15:14Z

+  EXPECT_EQ(2UL, cluster.healthyHosts().size());
+}
+
+TEST(StrictDnsClusterImplTest, ImmediateResolveV6Only) {


Is there also LogicalDnsClusterImplTest?

Yes, there's a logical_dns_cluster_test.cc

RomanDzhabarov · 2017-05-24T17:44:37Z

@htuch let me make the last pass also

htuch · 2017-05-24T18:14:41Z

  return any;
 }

+Address::InstanceConstSharedPtr Utility::getAddressUpdatePort(const Address::Instance& address,


Prefer addressWithPort.

htuch · 2017-05-24T18:31:44Z

I think mattklein123@ advocated for this in #989 (comment). I don't have a strong opinion as long as we are consistent with this and document.

…

On Wed, May 24, 2017 at 11:27 AM Roman Dzhabarov ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In STYLE.md <#1002 (comment)>: > @@ -17,6 +17,7 @@ includes both const and non-const references. * Function names using camel case starting with a lower case letter (e.g., "doFoo()"). * Struct/Class member variables have a '\_' postfix (e.g., "int foo\_;"). +* Enum values using all CAPS (e.g., "EXTERNAL_ONLY"). hm, I'm not sure about this. grep -ir 'enum class' gives the majority of the enum values in PascalCase. Any particular reason we want to stick to the all CAPS? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1002 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AKaLv0kMQ1fkCskjNJaOj9tpezgaX5EPks5r9HaigaJpZM4NhBDq> .

ccaraman · 2017-05-24T21:38:26Z

    }

-    // c-ares ares_gethostbyname() query callback.
+    /* c-ares ares_gethostbyname() query callback.


nit: add /** before this line

ccaraman · 2017-05-24T21:38:33Z

+     * @param hostent structure that stores information about a given host.
+     */
    void onAresHostCallback(int status, hostent* hostent);
+    /* wrapper function of call to ares_gethostbyname.


nit: add /** before this line

RomanDzhabarov · 2017-05-25T04:20:20Z

+1

Description: Adds invocation path for Android platform-bridged filters, covering request/response headers, data, and trailers. This change does not provide for state modification via the return path. Risk Level: Moderate Testing: Local and CI Signed-off-by: Mike Schore <mike.schore@gmail.com> Signed-off-by: JP Simard <jp@jpsim.com>

**Description** This removes the unused "schema" API from AIGatewayRoute as a preparation for transparent passthrough etc support for non-OpenAI SDK support as well as the prefix configuration #1002 **Related Issues/PRs (if applicable)** Follow up on #998 Necessary for #1002 --------- Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>

**Description** This commit makes the "prefix" for the supported endpoints configurable via global configurations. **Related Issues/PRs (if applicable)** Closes #1002 --------- Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>

Add Ipv6 dns support

b292f3f

Fix docs

41f290f

htuch mentioned this pull request May 21, 2017

DnsResolver v4/v6 selection #978

Closed

htuch reviewed May 21, 2017

View reviewed changes

dnoe reviewed May 22, 2017

View reviewed changes

hennna mentioned this pull request May 22, 2017

DNS IPv6 fallback support follow-up #1005

Closed

junr03 self-requested a review May 22, 2017 18:25

RomanDzhabarov assigned junr03 May 22, 2017

hennna added 2 commits May 22, 2017 15:48

Response to comments

c6ca161

fix

536a204

Fix

c17cd01

htuch reviewed May 22, 2017

View reviewed changes

RomanDzhabarov reviewed May 22, 2017

View reviewed changes

RomanDzhabarov self-assigned this May 22, 2017

hennna added 3 commits May 23, 2017 16:23

Response to comments

9ba1839

Merge remote-tracking branch 'upstream/master' into dns-ipv6-support

3ff5df7

Test not needed

5c45591

htuch reviewed May 24, 2017

View reviewed changes

Response to comments

5588ebe

htuch previously approved these changes May 24, 2017

View reviewed changes

Fix

759c5e4

hennna dismissed htuch’s stale review via 759c5e4 May 24, 2017 18:26

hennna added 2 commits May 24, 2017 15:05

Fix coverage test failure

2b24ec3

Fix enum style

554fb2e

htuch previously approved these changes May 24, 2017

View reviewed changes

ccaraman reviewed May 24, 2017

View reviewed changes

Fix

405fd79

hennna dismissed htuch’s stale review via 405fd79 May 24, 2017 21:54

htuch approved these changes May 25, 2017

View reviewed changes

htuch merged commit fd81bdd into envoyproxy:master May 25, 2017

hennna deleted the dns-ipv6-support branch May 31, 2017 16:26

mattklein123 unassigned junr03 and RomanDzhabarov Sep 14, 2017


		.. _config_cluster_manager_cluster_dns_lookup_ip_version:

		dns_loopkup_ip_version

Conversation

hennna commented May 19, 2017

Uh oh!

hennna commented May 19, 2017

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!

hennna commented May 22, 2017

Uh oh!

htuch 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!

hennna May 22, 2017 • 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!

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!

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!

htuch 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!

hennna May 24, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

hennna May 22, 2017 •

edited

Loading

hennna May 24, 2017 •

edited

Loading