Support CNAMEs, including out-of-zone

[fuhry]

Add support for CNAMEs both inside and outside of the zones set by this plugin.

CNAME support is robust against excessive stack depth and loops.

Includes tests for all changes.

Signed-off-by: Dan Fuhry [email protected]

[fuhry]

$ go test
2023/03/08 13:20:57 [ERROR] plugin/records: detected loop in CNAME chain, name [cnameloop1.example.org.] already processed
2023/03/08 13:20:57 [ERROR] plugin/records: maximum CNAME stack depth of 10 exceeded
PASS
ok      github.com/coredns/records      0.004s

[chrisohaver]

Can you move the ANY query support to a separate PR?

[fuhry]

Can you move the ANY query support to a separate PR?

That functionality touches the same line as CNAME support does:

if r.Header().Rrtype == state.QType() || r.Header().Rrtype == dns.TypeCNAME || state.QType() == dns.TypeANY {

The part of this change that implements ANY support is fairly trivial, it's the || state.QType() == dns.TypeANY on this line. It will cause a merge conflict if I separate it into another PR with current master as the base branch. In light of that, do you still want me to split it off?

[chrisohaver]

The part of this change that implements ANY support is fairly trivial, it's the || state.QType() == dns.TypeANY on this line. It will cause a merge conflict if I separate it into another PR with current master as the base branch. In light of that, do you still want me to split it off?

Of course the merge conflict would also be trivial. Regardless, I'd prefer to keep unrelated features in separate commits, which means separate PRs. But if you don't want to, thats fine - I'll review both in one PR.

I will need to research ANY behavior to make sure it's compliant. It's kind of an old DNS feature that has been widely deprecated. I'm not as familiar with it is as CNAME, and I think it has some peculiar edge case behavior.

This PR also has me thinking that perhaps instead of extending the records plugin as is, we should follow the advice in the README, and refactor records to leverage the file plugin package ...

The lookup algorithm is pretty basic. Future enhancements could leverage the code from the file plugin to make more compliant with the DNS specification.

Doing so may be harder now, but easier down the road, as the natural trend would be to try to bring records further into functional parity with file.

[fuhry]

@chrisohaver I've addressed all comments, can you re-review? Thanks!

[fuhry]

A few more fixes this morning. I realized that matching on the plugin's origins is not ideal behavior when the origins are set to .. This will be a pretty common use case when overriding more than one name.

At first I tried simply removing the zone match:

if plugin.Zones(re.origins).Matches(qname) == "" {

This means the plugin would go upstream unconditionally. This works under normal circumstances, but it breaks the loop detection and recursion depth, which are both tracked in local state. So I refactored such that when a CNAME is encountered, the plugin first tries to resolve the CNAME locally within its own records. Only if that doesn't work will it go upstream.

In combination with #6 which adds fallthrough support, this means we can use a configuration like:

records {
    $OPTION fallthrough
    foo.example.com.   300 IN CNAME other.example.net.
}

forward . tls://8.8.8.8 tls://8.8.4.4

If records comes before forward in the directives or plugin.cfg, this means foo.example.com would be the only name overridden, and there's no need for except foo.example.com in the forward block, which meets our requirement of not breaking resolution of sibling or child records for an overridden name.

[chrisohaver]

Thanks for contributing. I need to check on SERVFAIL response for too-long chains, but looks good otherwise.

It does irk me a bit the amount of duplicated logic between records and file. And I cant help but wonder if efforts would be better spent adjusting the file plugin meet the needs to both records and file users, and deprecating records.

[chrisohaver]

Not sure if servfail is right answer here. It could be, but I think a response with a dangling cname might be preferable to a servfail. Not sure, need to check RFCs/other implementations.