Add support for SRV queries in DNSResolverImpl

[0x1997]

This adds basic SRV query support to the DNS resolver. #125

[mattklein123]

@0x1997 thanks for the contribution.

@htuch do you mind taking a first pass over this once this passes tests?

cc @lyft/network-team

[htuch]

This interface needs some documenting. I think it might be a little confusing to the reader. In general, an A record is independent of port, so I'm not sure it makes sense to expect the caller to supply a port in the non-SRV case, at least conceptually.

Is the rationale that we're eventually providing an Address::Ipv4Instance which is qualified with port? If this is the case, I think it might make sense to have a variant of the Address class that is also independent of port, similar to the C level distinction between in_addr and sockaddr_in structs. What do you think @mattklein123? I think @jamessynge was also after something like this.

[mattklein123]

I think there are a few things here:

1. The Address interface having a port is somewhat orthogonal IMO to this issue. Right now we spit out Address from DNS that effectively have no port, and then we make a new address with the port merged in.
2. I do agree that this interface and how one might use/document it is confusing. I guess I could be convinced that passing 0 as the DNS address port in the config engages SRV support, but IMO that is too much magic and we will get a lot of questions about it.

I can go either way on having port passed into the DNS interfaces. On one hand, I agree that conceptually it's strange because A records don't have ports. On the other hand, I think it does make the calling code simpler, especially when you start adding in SRV support.

I guess my vote (a weak vote) would be to spit port out of the DNS interfaces only in the case of SRV, but not engage SRV magically via passing a 0 port. I would add a new function, resolveSrv() which does not take port as an argument (but addresses that come out will have a port). Then, calling code, based on config, can either call resolve() or resolveSrv(). In the case of standard resolve() they can merge the port in after the fact like they do now. Otherwise they get a port in the address that comes out. (The alternative would be to have a new interface for Address that does not have port, but then we need a whole new set of callbacks for SRV resolving that spits out the ones with ports).

The main question IMO is how to actually engage SRV in the config and then document it. I think there are a few options here:

1. Actually do use port 0 and document it. E.g., "tcp://foo.service:0"
2. Have some extra config element like "dns_use_srv: true" which overrides port
3. Do something special in the "URL" parsing like "tcp://foo.service:srv"

I actually kind of like 3, but I'm open to suggestions.

Anyway, I can't say that I have a very strong opinion here. I think we could go a bunch of different ways.

[0x1997]

I agree. A new interface for SRV will be needed anyway if we are going to utilize the priority and weight info from SRV query results.

[mattklein123]

@0x1997 before implementing, can you propose the interfaces and configuration method (how the config will look). We can agree on that and then you can implement. That will be more efficient.

[0x1997]

@mattklein123

Currently, the Host interface requires weights to be in the range 1-100. I'm not sure how to handle out of range weights in the case of logical DNS clusters. Maybe more SRV config options will be needed. So I think the config may look like this,

"hosts": [{"url": "tcp://foo.service"}],
"srv_config": {
   "some_option": "some_value"
}

Then

• Change Utility::resolveUrl Utility::hostFromTcpUrl and Utility::portFromTcpUrl to handle this case.
• Add a wrapper type for an Address::Instance with priority and weight. Though I'm not sure about the name.
• Change the ResolveCb type accordingly.
• Add a resolveSrv method.
• Use this new address type to construct HostImpls.

What do you think?

PS: I might not be able to continue working on this as the current implementation already met our needs. Maybe someone can start from here.

[mattklein123]

I think I would favor a new method resolveSrv(...). Along with this, a new ResolveSrvCb, which returns a new SrvAddress interface which contains an Address, along with other info like weight, priority, etc. This could then be mapped into a HostImpl construction by the caller where appropriate.

I'm not sure that we really need "srv_options" right now vs. just doing something like I said above such as "tcp://foo.service:srv" or variant, but I don't feel super strongly.

[mattklein123]

@0x1997 I'm going to go ahead and close this PR for now as I don't have anyone that is going to actively working on this. I linked this PR from the tracking issue. If you want to reopen this and work on it feel free. If someone else wants to pick it up they can comment here or in the linked issue.