[WIP] dns: add support for SRV records in DNS lookup #6379

venilnoronha · 2019-03-26T05:13:39Z

Description: This adds support for SRV records in DNS lookup by introducing a new SrvInstance type which holds a regular Address::Instance object ~~along with priority and weight information~~.
Risk Level: Med
Testing: Pending
Docs Changes: Pending
Release Notes: Pending
Fixes #125
Related #517

This PR is currently a WIP. Wiring, configuration, tests, documentation, etc. will be added once the implementation looks okay.

/cc @mattklein123 @htuch

htuch

At a high level, this PR and approach looks good, thanks for the contribution. What I'd like to see is some more refactoring in dns_impl.cc to avoid having code duplication around some of the admittedly sketchy code that is already there, e.g. the exception handling in cares, the self deletion, etc. The work on markForDestruction() that you've done already is a good example of this :)

/wait

include/envoy/network/address.h

stale · 2019-04-04T06:31:22Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

venilnoronha · 2019-04-04T21:50:36Z

WIP

stale · 2019-04-11T22:40:17Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

venilnoronha · 2019-04-12T19:21:43Z

Still WIP

stale · 2019-04-19T20:13:08Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

venilnoronha · 2019-04-24T20:19:33Z

.

venilnoronha · 2019-05-07T17:53:09Z

/wait

venilnoronha · 2019-05-10T19:38:57Z

/wait

stale · 2019-05-17T20:01:24Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

Signed-off-by: Venil Noronha <[email protected]>

This implements a DNS SRV resolver named envoy.srv and handles it's dynamic registration on server startup. Signed-off-by: Venil Noronha <[email protected]>

Signed-off-by: Venil Noronha <[email protected]>

htuch

A few more comments on the current PR. What's the status of this? Is it still WiP?
/wait

htuch · 2019-07-30T17:16:21Z

source/common/network/resolver_impl.cc

+  });
+  dns_timeout->enableTimer(std::chrono::seconds(5));
+
+  latch.Wait();


I think that what you're encountering here is the limitation that exists today in Envoy resolvers; they are synchronous. If you want, I think a separate PR to add asynch resolver plugin support would be welcomed.

htuch · 2019-07-30T17:17:08Z

include/envoy/network/dns.h

+   * @return if non-null, a handle that can be used to cancel the resolution.
+   *         This is only valid until the invocation of callback or ~DnsResolver().
+   */
+  virtual ActiveDnsQuery* resolveSrv(const std::string& dns_name, DnsLookupFamily dns_lookup_family,


I know the existing API does it this way, but I'd be interested if we could make ActiveDnsQuery RAII.

Would you be willing to explain what you mean here more? The ActiveDnsQuery abstract class only has one method and no data members. The PendingResolution struct is derived from ActiveDnsQuery and contains data, is that what you want to be RAII? Do you know why a struct was used instead of a class? Was it just to avoid another class definition?

I think I was suggesting that the returned ActiveDnsQuery be a unique ptr. The idea is that if this returned object is then destructed, it gives you automagic cancellation of the request.

@htuch Are you saying it's better to return a std::unique_ptr<ActiveDnsQuery> instead of ActiveDnsQuery*? I'm not quite sure how it actually cancels the pending dns request. Mind elaborating a bit more? (Sorry if the answer is obvious as I'm pretty new to C++ :) )

Right, return std::unique_ptr<ActiveDnsQuery>. The destructor for ActiveDnsQuery can then perform the cancellation.

htuch · 2019-07-30T17:23:22Z

source/common/network/dns_impl.cc

+void DnsResolverImpl::PendingSrvResolution::onAresSrvFinishCallback(
+    std::list<DnsResponse>&& srv_records) {
+  if (!srv_records.empty()) {
+    completed_ = true;


Isn't this true regardless of the length of the srv_records? I.e. the query is over, we're not waiting any longer?

htuch · 2019-07-30T17:27:50Z

source/common/network/dns_impl.cc

+              for (auto instance = response.begin(); instance != response.end(); ++instance) {
+                Address::InstanceConstSharedPtr inst_with_port(
+                    Utility::getAddressWithPort(*instance->address_, current_reply->port));
+                mutex->lock();


Why are we taking locks? Isn't everything thread local on this dispatcher?

Are threads and dispatchers always coupled? I think so, but I'm not 100% sure. So any operations happening through a particular dispatcher will operate on the same thread?

For the purpose of DNS resolution, this should be true (that they are coupled).

htuch · 2019-07-30T17:32:29Z

source/common/network/dns_impl.cc

+              }
+            });
+      }
+      replies_parsed = true;


Do we need an explicit replies_parsed variable here, or can the control logic be made more explicit?

stale · 2019-08-07T03:27:06Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

stale · 2019-08-15T10:10:14Z

This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

cottonbeckfield · 2019-08-15T23:19:29Z

Is this still actually a WIP or has it really closed out? Seems like there was just a drop off..

venilnoronha · 2019-08-15T23:26:44Z

I've been a little busy so couldn't get it to a finish. I may pick it up later once I have more time.

venilnoronha force-pushed the srv branch from 53c0840 to ae7d9b3 Compare March 26, 2019 16:09

mattklein123 assigned htuch Mar 26, 2019

htuch suggested changes Mar 28, 2019

View reviewed changes

include/envoy/network/address.h Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Mar 28, 2019

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Apr 4, 2019

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Apr 4, 2019

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Apr 11, 2019

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Apr 12, 2019

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Apr 19, 2019

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Apr 24, 2019

venilnoronha force-pushed the srv branch from ae7d9b3 to ee05fb7 Compare April 29, 2019 19:58

repokitteh-read-only bot removed the waiting label Apr 29, 2019

mattklein123 added the waiting label May 1, 2019

venilnoronha force-pushed the srv branch from 8d69601 to 6c23ed4 Compare May 7, 2019 17:52

repokitteh-read-only bot removed the waiting label May 7, 2019

repokitteh-read-only bot added the waiting label May 7, 2019

venilnoronha force-pushed the srv branch from 6c23ed4 to 696393b Compare May 10, 2019 19:10

repokitteh-read-only bot removed the waiting label May 10, 2019

repokitteh-read-only bot added the waiting label May 10, 2019

stale bot added the stale stalebot believes this issue/PR has not been touched recently label May 17, 2019

venilnoronha force-pushed the srv branch from 696393b to dcb831d Compare May 20, 2019 19:13

venilnoronha added 13 commits July 17, 2019 14:09

Fix tests

a302103

Signed-off-by: Venil Noronha <[email protected]>

Fix format

9da69fd

Signed-off-by: Venil Noronha <[email protected]>

Update ResolveSrvCb definition

7c5dcdd

Signed-off-by: Venil Noronha <[email protected]>

Fix compilation errors

8f937ff

Signed-off-by: Venil Noronha <[email protected]>

Implement a DNS SRV resolver

5c42041

This implements a DNS SRV resolver named envoy.srv and handles it's dynamic registration on server startup. Signed-off-by: Venil Noronha <[email protected]>

Disallow double registration of SrvResolver

303ccca

Signed-off-by: Venil Noronha <[email protected]>

Add a todo note in the Redis cluster impl

b0dc4da

Signed-off-by: Venil Noronha <[email protected]>

Generate appropriate SRV URLs in DNS impls

b4e9b37

Signed-off-by: Venil Noronha <[email protected]>

Add unit tests for Utility::urlFromSocketAddress

dbbdfbd

Signed-off-by: Venil Noronha <[email protected]>

Fix DNS cluster impl tests

601c1f6

Signed-off-by: Venil Noronha <[email protected]>

Add unit tests for SrvInstance

406eb1e

Signed-off-by: Venil Noronha <[email protected]>

Enhance utility test with specific error messages

b63a81c

Signed-off-by: Venil Noronha <[email protected]>

Wrap SrvInstanceConstSharedPtr in DnsSrvResponse

a8e2903

Signed-off-by: Venil Noronha <[email protected]>

venilnoronha force-pushed the srv branch from 6ee804f to a8e2903 Compare July 17, 2019 23:01

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Jul 17, 2019

repokitteh-read-only bot removed the waiting label Jul 17, 2019

Minimalize labmda captures

dc152d2

Signed-off-by: Venil Noronha <[email protected]>

htuch added the no stalebot Disables stalebot from closing an issue label Jul 18, 2019

venilnoronha added 2 commits July 18, 2019 17:33

Drop specialized types and use existing ones

ea167bc

Signed-off-by: Venil Noronha <[email protected]>

Enable safe thread and memory DNS operations

6faaddd

Signed-off-by: Venil Noronha <[email protected]>

mattklein123 removed the no stalebot Disables stalebot from closing an issue label Jul 28, 2019

htuch suggested changes Jul 30, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Jul 30, 2019

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Aug 7, 2019

stale bot closed this Aug 15, 2019

kylebevans mentioned this pull request Dec 24, 2020

SRV service discovery support #125

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] dns: add support for SRV records in DNS lookup #6379

[WIP] dns: add support for SRV records in DNS lookup #6379

venilnoronha commented Mar 26, 2019 •

edited

Loading

htuch left a comment

stale bot commented Apr 4, 2019

venilnoronha commented Apr 4, 2019

stale bot commented Apr 11, 2019

venilnoronha commented Apr 12, 2019

stale bot commented Apr 19, 2019

venilnoronha commented Apr 24, 2019

venilnoronha commented May 7, 2019

venilnoronha commented May 10, 2019

stale bot commented May 17, 2019

htuch left a comment

htuch Jul 30, 2019

htuch Jul 30, 2019

kylebevans Apr 17, 2020

htuch Apr 20, 2020

ntgsx92 May 7, 2021

htuch May 9, 2021

htuch Jul 30, 2019

htuch Jul 30, 2019

kylebevans Apr 17, 2020

htuch Apr 20, 2020

htuch Jul 30, 2019

stale bot commented Aug 7, 2019

stale bot commented Aug 15, 2019

cottonbeckfield commented Aug 15, 2019

venilnoronha commented Aug 15, 2019

[WIP] dns: add support for SRV records in DNS lookup #6379

[WIP] dns: add support for SRV records in DNS lookup #6379

Conversation

venilnoronha commented Mar 26, 2019 • edited Loading

htuch left a comment

Choose a reason for hiding this comment

stale bot commented Apr 4, 2019

venilnoronha commented Apr 4, 2019

stale bot commented Apr 11, 2019

venilnoronha commented Apr 12, 2019

stale bot commented Apr 19, 2019

venilnoronha commented Apr 24, 2019

venilnoronha commented May 7, 2019

venilnoronha commented May 10, 2019

stale bot commented May 17, 2019

htuch 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

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

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

stale bot commented Aug 7, 2019

stale bot commented Aug 15, 2019

cottonbeckfield commented Aug 15, 2019

venilnoronha commented Aug 15, 2019

venilnoronha commented Mar 26, 2019 •

edited

Loading