DNS: Add support for reverse lookup of services

[42wim]

This adds support for reverse lookup of service ip addresses.

[pierresouchay]

The service request is probably not at the right place

[pierresouchay]

I don't get what you are doing there... does the Catalog.NodeServices should not be when the node has been found instead ?

In my understanding, you are creating LOTS of requests to the Consul server for nothing

I don't get why you created the label Outer neither...

[42wim]

The code i'madding is to find the service(name) of an ip address, eg 2001:db8:85a3::1, so I have to check the ip address of all the registered services using Catalog.NodeServices.

If we find the address, we create the response and break out of the 2nd level for loop using the Outer label.

[pierresouchay]

this is gonna be very slow on 5k nodes with ~3-4 services per host

More than 5k request just to perform a reverse DNS???

I would rather: keep code unchanged since original loop is a single loop.

It will already address all people not using a specific address for service (I mean address of service is address of node)

If none found AND option is set do what you want to do, but probably by searching, not by performing x requests (where x is the number of nodes).

As it is, it probably won't work and will do a timeout on a significant infrastructure

[42wim]

How exactly do you propose the searching should happen then?

[pierresouchay]

Probably server side by indexing adresses in the database

[42wim]

I've changed the code, should be performant enough now.
Uses an external dependency (https://github.com/coredns/coredns/tree/master/plugin/pkg/dnsutil), don't know if it's better to copy the code or vendor it in

[mkeeler]

@42wim What is the use case for using multiple PTR records? There is some debate in various places on the internet whether multiple PTR records is good practice.

While the DNS specs don't specifically disallow multiple PTR records for an IP it is generally assumed that the PTR record holds the canonical name of the node. Having multiple PTR records breaks this assumption.

I know from personal experience that responding with multiple PTR records can break or cause undefined behavior with some applications. Most of the issues I have seen are around inconsistent ordering of results for PTR queries. In our case the PTR RR for the node will always be the first RR so it may not be as big of an issue here as most things I have seen will only look at the first RR as they are only expecting 1.

With all that being said, I am sure you have a particular scenario where this would be beneficial and I would love to here it.

Lastly an issue I just found with the code as is, is that we could get into trouble with regards to DNS message sizes. Before with only ever responding with a single PTR RR we would always be within the 512 byte minimum supported message size but now we will need to do response trimming like we do for handling other queries.

[42wim]

@mkeeler There shouldn't be multiple PTR records, it breaks the loop if it finds the requested address. https://github.com/42wim/consul/blob/5c04864b289337c2394aa9e54cf2ed2670ba9bcb/agent/dns.go#L231

The ServiceAddressNodes function returns them all, could be useful for something in the future, but I can change this to only return one.

The only way it returns 2 PTR is if the node ip is the same as the service IP. In our setup this is never the case, but I guess you're hinting to this? I could add a check so that if the reverse resolves to a node address we keep this and do not look into the services.

Is there anything else you want me to do for this PR ?

[mkeeler]

@42wim This makes a lot more sense now. You don't want multiple PTRs you just need a PTR even when you have no node with that IP but you still have registered services.

In that case I dont see any issues with returning PTR rrs from the service lookup. I do think that adding a check to see if the node lookup was successful before issuing the service lookup RPC would be good. In many cases we can completely avoid that second RPC.

The only other two things to do for this PR would be to add some tests into dns_test.go and to vendor that dnsutil lib: govendor fetch github.com/coredns/coredns/plugin/pkg/dnsutil@v1.1.2

[42wim]

@mkeeler ok, done.

I've based the tests on the existing reverse lookup tests and added an extra TestDNS_ServiceReverseLookupNodeAddress which tests that if the node and service have the same address the one of the node will be returned.

[mkeeler]

@42wim Thanks for this PR. It all looks good to me.