feat(dns): Support alt domains

[akshayganeshen]

Alternate DNS domains are supplied with the -alt-domain flag or alt_domains configuration parameter.

The DNS resolver will attempt to parse and match each domain during resolution.

See #4165 for use-case.

[freddygv]

Thanks for the PR @akshayganeshen!

At first glance it seems like one of the new tests is consistently failing in CI: TestDNS_AltDomains_Overlap

Is this test passing locally?

[akshayganeshen]

Hey @freddygv, that test does not pass locally. It's an example of the sort of edge-cases that my implementation does not handle:

# alt_domains = [ "dc.consul.", "consul." ]
dig @127.0.0.1 -p 8600 web.service.dc.consul
# is the domain `consul`, and the datacenter `dc`?
# or is the domain `dc.consul`?

Without that test, I think it fulfills the desired use-case, but this edge-case should have a well-defined expectation. I believe it can be resolved in two ways:

1. Simply do not allow these ambiguous alt_domains. Raise a validation error in agent/config/builder.go when such a scenario is detected. (In my opinion this allows the greatest flexibility to change/define the behaviour later on.)
2. Prioritize domain matching in some way to break ties between ambiguous domain matches. For example, prefer matching domains that result in queries without a datacenter. (That would resolve the scenario I have in the test, but there are other possible edge-cases depending on the prioritization method).

I was hoping to get some feedback before assuming which was the desired solution, or if there's another solution entirely that I'm missing.

[akshayganeshen]

I've updated the implementation to use only a single alt_domain. However, I kept the test cases, which includes a failing case, even with the updated trimDomain. (Of course, I had to change my input slightly to demonstrate the problem I initially observed.)

Did I miss something there? Or is this as expected?

[hanshasselberg]

Thanks for making the changes! This PR looks pretty good now! I found another small thing and it would be also great if you could add the flag and the config option to website/source/docs/agent/options.html.md. 🙏 👍

We are still discussing internally what to do with the issue, when the datacenter is part of the domain.

[clly]

@akshayganeshen: Thank you so much for working on this.

My personal thought for ambiguous domains is that the primary domain as opposed to the alternative one should be used. I had not thought of this case when originally writing my branch.

A second option is to keep a list of the available datacenters to check a potential datacenter.

I can see a risk where not preferring the previous behavior can cause easily missed issues where a service suddenly fails to discover services that it expected to find in a remote datacenter.

[hanshasselberg]

@akshayganeshen @clly I think we found a solution for the ambiguous domains. I suggest we are validating the altDomain in config/builder.go like I did in this playground: https://play.golang.org/p/pWJO35B3pV-. That should hopefully resolve the problems. We discovered that not only the datacenter, but also other keywords could potentially cause troubles.

This validation should also make TestDNS_AltDomains_Overlap pass because the altDomain used in this test fails the checks.

Thanks again, for bearing with us!

[akshayganeshen]

@i0rek @clly Thanks for the tips! I'll do the validation in config/builder.go, since that's more in line with what I was thinking originally.

(I'll also update the docs appropriately.)

[akshayganeshen]

I've updated the implementation and the tests pass!

I'm still updating the documentation though. I'll also add a test case for the invalid config.

[hanshasselberg]

Great work, everything seems to work perfectly! Thanks for bearing with us!

[hanshasselberg]

oops, the test failures are relevant: https://travis-ci.org/hashicorp/consul/jobs/546001060#L1294. You have to add DNSAltDomain in these places: https://github.com/akshayganeshen/consul/blob/d7bf494f042800cdcec60d5dcbb6574c4a4a9595/agent/config/runtime_test.go#L5153 and https://github.com/akshayganeshen/consul/blob/d7bf494f042800cdcec60d5dcbb6574c4a4a9595/agent/config/runtime_test.go#L2950.

[akshayganeshen]

Good catch. Will fix.

[freddygv]

@akshayganeshen given that we have a release coming up shortly, I went ahead and made the remaining finishing touches to your PR.

I hope you don't mind us carrying the PR across the finish line. We appreciate your contribution!

[hanshasselberg]

Great work!

[akshayganeshen]

@freddygv @i0rek thank you both! 🙏

Sorry I couldn't get around to this sooner.