Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

file plugin does not answer "ANY" requests #3405

Closed
jpicht opened this issue Oct 27, 2019 · 14 comments
Closed

file plugin does not answer "ANY" requests #3405

jpicht opened this issue Oct 27, 2019 · 14 comments

Comments

@jpicht
Copy link

jpicht commented Oct 27, 2019

Version: CoreDNS-1.6.4 (b139ba3-dirty) via docker.io/coredns/coredns:latest with Corefile

When I set up the file plugin (or the auto plugin) queries for "ANY" records are answered by SOA only. I have no idea if this is by design. When I use the same zone files with bind9, I get the result I would expect: all records matching the name.

I put together a small testcase (using docker) in a git repo.

I dug into the sources and found this line in the lookup code, that explains the behavior: only records matching the "ANY" type would be returned. This does not seem very intentional.

Maybe it could be fixed by changing the lookup like this

diff --git a/plugin/file/lookup.go b/plugin/file/lookup.go
index 3d8d899d..319ca556 100644
--- a/plugin/file/lookup.go
+++ b/plugin/file/lookup.go
@@ -153,11 +153,16 @@ func (z *Zone) Lookup(ctx context.Context, state request.Request, qname string)
        // Found entire name.
        if found && shot {
 
-               if rrs := elem.Type(dns.TypeCNAME); len(rrs) > 0 && qtype != dns.TypeCNAME {
+               if rrs := elem.Type(dns.TypeCNAME); len(rrs) > 0 && qtype != dns.TypeCNAME && qtype != dns.TypeANY {
                        return z.externalLookup(ctx, state, elem, rrs)
                }
 
-               rrs := elem.Type(qtype)
+               var rrs []dns.RR
+               if qtype == dns.TypeANY {
+                       rrs = elem.All()
+               } else {
+                       rrs = elem.Type(qtype)
+               }
 
                // NODATA
                if len(rrs) == 0 {

testcase

diff --git a/plugin/file/lookup_test.go b/plugin/file/lookup_test.go
index 71004397..8dba6bbf 100644
--- a/plugin/file/lookup_test.go
+++ b/plugin/file/lookup_test.go
@@ -12,6 +12,13 @@ import (
 )
 
 var dnsTestCases = []test.Case{
+       {
+               Qname: "www.miek.nl.", Qtype: dns.TypeANY,
+               Answer: []dns.RR{
+                       test.CNAME("www.miek.nl.        1800    IN      CNAME   a.miek.nl."),
+               },
+               Ns: miekAuth,
+       },
        {
                Qname: "www.miek.nl.", Qtype: dns.TypeA,
                Answer: []dns.RR{

I only looked at the straight forward code path, not at wildcards or anything, so there is probably a bit more to do, if the desired behavior is to return the same data bind9 does.

@miekg
Copy link
Member

miekg commented Oct 28, 2019 via email

@corbot corbot bot added plugin/file dns DNS protolol labels Oct 28, 2019
@jpicht
Copy link
Author

jpicht commented Oct 28, 2019

First: I personally agree.

But I have a problem with this behavior: the VMWare SDDC installer verifies that the DNS is set up "correctly" by querying forward and reverse. It uses ANY requests for both 😲

I consider that to be a bug on their end, but they seem to be content with "it works fine with windows" 😫

I was hoping to just enable ANY requests on our CoreDNS server and be done with it, but now I ran into this behavior. I think it should either be documented, that ANY is not implemented in file/auto/hosts/maybe more, or be implemented. The presence of the "any" filter plugin suggests to me that it is implemented in the other plugins.

@miekg
Copy link
Member

miekg commented Oct 29, 2019 via email

@johnbelamaric
Copy link
Member

johnbelamaric commented Oct 29, 2019 via email

@chrisohaver
Copy link
Member

... or if the response needs to be something more specific, the template plugin might work.

@chrisohaver
Copy link
Member

Nevermind - apparently the template plugin uses the word "ANY" to be a wildcard - so you cannot create a template for type == ANY ...

@miekg
Copy link
Member

miekg commented Nov 6, 2019

@jpicht can you propose your fix as a PR (with a test) ?. Thanks!

@miekg miekg added the bug label Nov 6, 2019
@jpicht
Copy link
Author

jpicht commented Nov 20, 2019

@jpicht can you propose your fix as a PR (with a test) ?. Thanks!

Will do, but may take a few more days still. I'm really busy at the moment.

@jpicht
Copy link
Author

jpicht commented Nov 26, 2019

I've implemented it, including a test case. While testing it, I became aware that this change introduces a 10% performance hit on simple lookups.

While investigating what caused this regression, I found that a few simple changes to https://github.com/miekg/dns could shave nearly 50% off the lookup process, so making it 10% slower doesn't seem like a big deal?

Should I try to improve the performance in-place or just create two pull requests, one for this change and one for the dns library?

@jpicht
Copy link
Author

jpicht commented Nov 26, 2019

Afterthought: I could add a compile-time switch to restore the previous behavior, thus creating a possibility to gain back those 10% if necessary.

@miekg
Copy link
Member

miekg commented Nov 29, 2019 via email

@jpicht
Copy link
Author

jpicht commented Nov 29, 2019

I've opened miekg/dns#1039 for the DNS library.

I had the idea, that this ANY implementation could be separate from the actual backend-plugin and created this proof of concept plugin code: https://github.com/jpicht/coredns/tree/anyplex/plugin/anyplex

It just rewrites ANY-queries to configurable other query types and then merges the results. The code does need more work, tests, documentation etc. also the name is questionable, but this approach would only introduce query latency for people who actually need that feature.

@miekg
Copy link
Member

miekg commented Nov 30, 2019 via email

@miekg
Copy link
Member

miekg commented Dec 6, 2019

/duplicate #3445

@corbot corbot bot added the duplicate label Dec 6, 2019
@corbot corbot bot closed this as completed Dec 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants