Merge Kubernetes.DNS implementations

[seivan]

merges #36 & #99 into a single Module, but keeps the tests for both around.

And also fixes #113 since I noticed namespace was missing in the docs. Now it will also default to default if it's missing.

But there are also pending issues related to handling wrong or missing configurations.
Right now either implementations do both crash by using Keyword.fetch!/2 but at the same uses predicates to log if configs are missing.

Which approach would you prefer? I defaulted to returning errors (Keyword.get/2) instead.

[bitwalker]

I think we need to keep the definition of the old module, but redirect its start_link to the merged module (modifying the topology config as necessary), and mark it as deprecated. Once we've gone through a deprecation cycle, we can remove the old module entirely; but as a number of deployments likely use it, we can't just remove it out from under people.

[seivan]

@bitwalker Ah you're absolutely right.
Sorta what I did like the unit tests.

but redirect its start_link to the merged module
Yeah that's actually a good idea.

I think it could just redirect and set its settings to use :srv, if you're ok with that change, I'll add it.

[bitwalker]

Sounds good!

[seivan]

@bitwalker So what approach did you want for tackling the bad paths (errors)? I resorted to logging instead of crashing, but I am not so sure that's the best approach.

[seivan]

There is another issue at play here, and it is the :srv implementation hardcodes the service with some assumptions, like the address will be .svc.cluster.local, this is not necessarily always true.
I didn't tackle this when moving things over to keep it as is, but since we're going forward I am thinking of changing that.

In general it's also unnecessary to hardcode ".#{namespace}.svc.cluster.local."
In the event of custom domains or different namespaces, the search list should change.
Not only is that address hardcoded, but the namespace is as well if the user doesn't supply one.

What I mean is that in a scenario where the namespace is app-namespace theetc/resolv.conf would already be populated a search list to tackle that:

nameserver 10.96.0.10
search app-namespace.svc.cluster.local svc.cluster.local cluster.local
options ndots:5

So the service name (dig srv +search busybox-service) should suffice.

This allows the user to define a full domain, or even part of it if necessary and let the search list do its thing.
That means these will work on their own:
dig a +search busybox-service
dig a +search busybox-service.app-namespace
dig a +search busybox-service.app-namespace.svc.custom.domain

Today the DNS module on its own will work fine even without supplying a namespace as it's relying on the search list created by k8, but the DNSSRV will not, making it incomplete.

My suggestion is to prune the config and make things less error prone, rely on K8 to tackle these things and optionally accept a partially qualified domain name that could include the namespace, but not limited to.

Example:
domain: app-namespace
domain: app-namespace.svc.custom.domain
This will be used in the combination with the suppled and not optional service name.

In theory, passing in the namespace on its own is unnecessary since the default ndots is 5, the resolver will be using each component of the search path in turn until a match is found.

[bitwalker]

Hey, sorry for the long absence on this one @seivan

For configuration issues, I prefer to crash hard - but rather than raising on Keyword.fetch!, we could instead raise an exception of our own that properly explains the issue. If a configuration issue occurs on an optional configuration value, then those should just log warnings rather than raise.

Your proposed changes in your last comment sound appropriate to me. It seems like we may have another issue with latest versions of k8s that use kube-dns, see #123. I'd like for the changes here to factor that in as well, but we don't have to hold this PR up for that unless you think it makes sense to do it all in one shot.

[seivan]

@bitwalker Yeah, I am hesitant to merging them now.
Actually I'll close this PR and open a new one for fixing the Keyword lookups and raising, but I'd advice whoever is maintaining DNSSRV to look into this as it's problem waiting to happen, unless it's already happened now with the other PR you linked.

[bitwalker]

I agree, unfortunately I'm not sure anyone is actively contributing towards it right now, so I'm leaning heavily towards deprecating it to avoid giving people headaches - or at least removing it from prominence in the documentation.