Only allow 1 CNAME when querying for a service. #4328
Conversation
This just makes sure that if multiple services are registered with unique service addresses that we don’t blast back multiple CNAMEs for the same service DNS name and keeps us within the DNS specs.
agent/dns.go
Outdated
| // only allow at most 1 CNAME record | ||
| switch records[0].(type) { | ||
| case *dns.CNAME: | ||
| if haveCNAME || !allowCNAME { |
There was a problem hiding this comment.
My recommendation would be to add some detailed comments here. It took me awhile reading the code to figure out what was going on and I'm questioning whether I even understood it correctly. It'd help to have a 2nd source helping me here.
There was a problem hiding this comment.
I just refactored it a lot and it should be much more straight forward. I also added a comment but hopefully this is much easier to follow.
Now we hold onto the first CNAME RR (+ associated RRs) until the end of the loop. If we have nothing in the Answer section then we add the CNAME answer.
Hopefully this a bit clearer as to the reasoning
agent/dns.go
Outdated
| switch records[0].(type) { | ||
| case *dns.CNAME: | ||
| // keep track of the first CNAME + associated RRs but don't add to the resp.Answer yet | ||
| // this will only be added if no non-CNAME RRs are found |
There was a problem hiding this comment.
Is the first one always correct? For example could this result in CNAMEs always resolving to a single instance of a service and so sending all load there versus the expected random order to spread the load?
There was a problem hiding this comment.
The node list gets shuffled in the serviceLookup function prior to actually formatting so it will be randomized which CNAME gets used.
This is kind of the best we can always do when mapping a single service name with fqdns for the service address.
One other approach I thought of would be to check if we knew the A/AAAA RR for the name pointed to by the CNAME and instead propagate those records up and hide the CNAME all together. This would work when the service address is a name known to Consul (either it is authoritative for it like with being a node name or Consul has recursors configured and can lookup the name that way).
However because this wouldn't work in a general case like the one that prompted this change I didn't think that was the route we should go.
This just makes sure that if multiple services are registered with unique service addresses that we don’t blast back multiple CNAMEs for the same service DNS name and keeps us within the DNS specs.
The general rule in place now: