fallthrough support for forward plugin #2789

achinthagunasekara · 2019-04-17T06:44:28Z

fallthrough support for forward plugin

1. Why is this pull request needed and what does it do?

test {
    forward . 127.0.0.1:8600 {
        force_tcp
    }
    cache 300
}

In this situation, a plugin handles the query, but the reply it got from its backend (i.e. maybe it got NXDOMAIN) is such that it wants other plugins in the chain to take a look as well. If fallthrough is provided (and enabled!), the next plugin is called. A plugin that works like this is the hosts plugin. First, a lookup in the host table (/etc/hosts) is attempted, if it finds an answer, it returns that. If not, it will fallthrough to the next one in the hope that other plugins may return something to the client.

However looking at forward plugin, it doesn't seem to have support for fallthrough - https://coredns.io/plugins/forward/

This PR adds fallthrough support to forward plugin.

2. Which issues (if any) are related?

Not an issue. New feature

3. Which documentation changes (if any) need to be made?

forward plugin documentation

4. Does this introduce a backward incompatible change or deprecation?

Yes

corbot · 2019-04-17T06:44:34Z

Thank you for your contribution. I've just checked the OWNERS files to find a suitable reviewer. This search was successful and I've asked grobie (via plugin/forward/OWNERS) for a review.

If you have questions or suggestions for this bot, please file an issue against the miekg/dreck repository.

The bot understands the commands that are listed here.

codecov-io · 2019-04-17T06:51:31Z

Codecov Report

Merging #2789 into master will increase coverage by 0.02%.
The diff coverage is 71.42%.

@@            Coverage Diff             @@
##           master    #2789      +/-   ##
==========================================
+ Coverage   55.26%   55.28%   +0.02%     
==========================================
  Files         201      201              
  Lines       10202    10207       +5     
==========================================
+ Hits         5638     5643       +5     
- Misses       4147     4148       +1     
+ Partials      417      416       -1

Impacted Files	Coverage Δ
plugin/forward/setup.go	`58.9% <100%> (+0.57%)`	⬆️
plugin/forward/forward.go	`55.05% <60%> (-0.76%)`	⬇️
plugin/file/reload.go	`76.31% <0%> (+5.26%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5aafa98...cafd2ca. Read the comment docs.

miekg · 2019-04-17T06:52:51Z

The host plugin is special because it's can't actually reply with NXDOMAIN because that how /etc/hosts works (in conjunction with /etc/nswitch.conf). It needs fall through. No other plugin should have to do this and most certainly not the forward plugin. So NACK from me.

…

On Wed, 17 Apr 2019, 07:44 Achintha Gunasekara, ***@***.***> wrote: fallthrough support for forward plugin 1. Why is this pull request needed and what does it do? test { forward . 127.0.0.1:8600 { force_tcp } cache 300 } In this situation, a plugin handles the query, but the reply it got from its backend (i.e. maybe it got NXDOMAIN) is such that it wants other plugins in the chain to take a look as well. If fallthrough is provided (and enabled!), the next plugin is called. A plugin that works like this is the hosts plugin. First, a lookup in the host table (/etc/hosts) is attempted, if it finds an answer, it returns that. If not, it will fallthrough to the next one in the hope that other plugins may return something to the client. However looking at forward plugin, it doesn't seem to have support for fallthrough - https://coredns.io/plugins/forward/ This PR adds fallthrough support to forward plugin. 2. Which issues (if any) are related? Not an issue. New feature 3. Which documentation changes (if any) need to be made? forward plugin documentation 4. Does this introduce a backward incompatible change or deprecation? Yes ------------------------------ You can view, comment on, or merge this pull request online at: #2789 Commit Summary - fallthrough support for forward plugin File Changes - *M* plugin/forward/forward.go <https://github.com/coredns/coredns/pull/2789/files#diff-0> (16) - *M* plugin/forward/setup.go <https://github.com/coredns/coredns/pull/2789/files#diff-1> (2) - *M* plugin/forward/setup_test.go <https://github.com/coredns/coredns/pull/2789/files#diff-2> (2) Patch Links: - https://github.com/coredns/coredns/pull/2789.patch - https://github.com/coredns/coredns/pull/2789.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#2789>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAVkW_ugxt70zl_7lZY401vXS8p1M9Lhks5vhsLRgaJpZM4c0T2x> .

rdrozhdzh · 2019-04-17T08:58:35Z

consider using plugin alternate
https://github.com/coredns/alternate

achinthagunasekara · 2019-04-17T23:14:46Z

@miekg @rdrozhdzh
My current issue is, my upstreams sometimes return NXDOMAIN for valid queries. In this situation I'd like to get the result from the cache.

Something like,

test {
    forward . 127.0.0.1:8600 {
        force_tcp
        fallthrough
    }
    cache 300
}

Is there any other way to do this?

Thanks

chrisohaver · 2019-04-18T13:36:17Z

You may want to look into why your upstreams sometimes return NXDOMAIN for valid queries. Is this something under your control? Is it an intentional behavior? Perhaps there is an alternate solution that follows DNS standards.

That aside...

Taken literally, you could do what you are asking for by setting your cache up to not cache negative responses. Doing this means that if there is a cache entry, then it will not be an NXDOMAIN. In the event that there is no cache entry, or it has expired, then there is no cache entry to use.

achinthagunasekara · 2019-04-18T21:54:02Z

@chrisohaver

is cache plugin has any configuration that we can use to turn off caching of NXDOMAIN responses? I don't see anything like that in https://coredns.io/plugins/cache/

chrisohaver · 2019-04-18T21:56:56Z

cache {
    denial 0
}

achinthagunasekara · 2019-04-18T22:19:03Z

@chrisohaver What I'm trying to archive is,

Check the cache. If a non-expired response exists, return it.
If no non-expired response, ask upstream. If it returns a response, cache it and return it.
If no upstream response, check the cache for an expired response, return it.
If no expired response, return NXDOMAIN.

chrisohaver · 2019-04-18T22:40:12Z

IMO, step 3, returning expired entries from cache, is too obscure of a behavior to add as an option to cache.

Why does your upstream “return NXDOMAIN sometimes for valid queries?” Is this something you control?

achinthagunasekara · 2019-04-28T23:22:00Z

@chrisohaver
Sorry about the delay getting back to you.
We don't have any control over the upstream, that was the reason we were looking into this solution.
Any suggestions on how I can proceed? :)

chrisohaver · 2019-04-29T18:02:04Z

Any suggestions on how I can proceed? :)

Maybe you could rewrite TTLs, making them really long so they stay in the cache for a really long time ... (see the rewrite plugin). It's definitely "hack" territory, but you're kind of already there if you're looking to serve expired entries from cache.

stp-ip · 2019-04-30T09:40:16Z

How about cache + prefetch + denial cache disabled. Currently the denial cache can't be disabled afaik. But this might be something that could be supported via "denial 0".
Thoughts?

chrisohaver · 2019-04-30T12:53:47Z

How about cache + prefetch + denial cache disabled.

I suggested this here - sans prefetch. I'm not seeing how prefetch helps solve the issue at hand though.

Currently the denial cache can't be disabled afaik. But this might be something that could be supported via "denial 0".
Thoughts?

I think negative cache can already be disabled like this. IIRC it was enabled a couple of releases ago.

The TTL rewrite suggestion is in addition to no negative caching. I.e. make the positive answers live in the cache for a very long time (but never put negatives in the cache).

stp-ip · 2019-04-30T13:02:08Z

With prefetch the idea is to extend the time a positive answer is within the cache. But maybe that's not perfect.

Ah yeah read through another issue today (just checked it was before the mentioned release) that mentioned setting denial to 0 doesn't.

chrisohaver · 2019-04-30T13:03:58Z

I think negative cache can already be disabled like this. IIRC it was enabled a couple of releases ago.

Hmm. Looking at code , I don't see where we check for zero, and we seem to enforce an undocumented minimum size cache. I keep tripping over this. I think we should allow it.

chrisohaver · 2019-04-30T13:08:00Z

Ah, This is why I thought it was done already...

#2588

... it stalled 2 months ago.

achinthagunasekara · 2019-05-01T04:01:37Z

@chrisohaver, @stp-ip

What if we change the config to something like below, we can set AMOUNT to fairly low and DURATION fairly high. This is not going to solve the problem entirely, but should help?

test {
    cache 300 {
        prefetch 2 60 50%
    }
    forward . 127.0.0.1:8600 {
        force_tcp
    }
}

miekg · 2019-05-23T20:15:21Z

closing this as it will not be merge and it seems to indicate some upstream problem

m-yosefpor · 2020-08-22T23:28:10Z

@achinthagunasekara I think this is what you are looking for https://coredns.io/explugins/fallback/

The fallback plugin allows an alternate set of upstreams be specified which will be used if the plugin chain returns specific error message

. {
    forward . 127.0.0.1:8600 {
        force_tcp
    }
    fallback NXDOMAIN . 8.8.8.8 {
        protocol dns force_tcp
    }
    cache 300
}

fallthrough support for forward plugin

92439df

corbot bot requested a review from grobie April 17, 2019 06:44

Docs update

9147891

Not falling through in an error case. Var name change.

cafd2ca

miekg closed this May 23, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fallthrough support for forward plugin #2789

fallthrough support for forward plugin #2789

achinthagunasekara commented Apr 17, 2019

corbot bot commented Apr 17, 2019

codecov-io commented Apr 17, 2019 •

edited

miekg commented Apr 17, 2019 via email

rdrozhdzh commented Apr 17, 2019

achinthagunasekara commented Apr 17, 2019

chrisohaver commented Apr 18, 2019

achinthagunasekara commented Apr 18, 2019

chrisohaver commented Apr 18, 2019

achinthagunasekara commented Apr 18, 2019

chrisohaver commented Apr 18, 2019

achinthagunasekara commented Apr 28, 2019

chrisohaver commented Apr 29, 2019

stp-ip commented Apr 30, 2019

chrisohaver commented Apr 30, 2019

stp-ip commented Apr 30, 2019

chrisohaver commented Apr 30, 2019

chrisohaver commented Apr 30, 2019

achinthagunasekara commented May 1, 2019 •

edited

miekg commented May 23, 2019

m-yosefpor commented Aug 22, 2020 •

edited

fallthrough support for forward plugin #2789

fallthrough support for forward plugin #2789

Conversation

achinthagunasekara commented Apr 17, 2019

1. Why is this pull request needed and what does it do?

2. Which issues (if any) are related?

3. Which documentation changes (if any) need to be made?

4. Does this introduce a backward incompatible change or deprecation?

corbot bot commented Apr 17, 2019

codecov-io commented Apr 17, 2019 • edited

Codecov Report

miekg commented Apr 17, 2019 via email

rdrozhdzh commented Apr 17, 2019

achinthagunasekara commented Apr 17, 2019

chrisohaver commented Apr 18, 2019

achinthagunasekara commented Apr 18, 2019

chrisohaver commented Apr 18, 2019

achinthagunasekara commented Apr 18, 2019

chrisohaver commented Apr 18, 2019

achinthagunasekara commented Apr 28, 2019

chrisohaver commented Apr 29, 2019

stp-ip commented Apr 30, 2019

chrisohaver commented Apr 30, 2019

stp-ip commented Apr 30, 2019

chrisohaver commented Apr 30, 2019

chrisohaver commented Apr 30, 2019

achinthagunasekara commented May 1, 2019 • edited

miekg commented May 23, 2019

m-yosefpor commented Aug 22, 2020 • edited

codecov-io commented Apr 17, 2019 •

edited

achinthagunasekara commented May 1, 2019 •

edited

m-yosefpor commented Aug 22, 2020 •

edited