Fallback plugin #1420
Fallback plugin #1420
Conversation
|
This uses the proxy upstream parsing. It supports everything the proxy does. It should work like this: for example. I haven't tried that yet though. |
|
at this point exactly ONE |
plugin/fallback/setup.go
Outdated
| return plugin.Error("fallback", err) | ||
| } | ||
|
|
||
| f.rules[rc] = u |
There was a problem hiding this comment.
Hum.. for what I understand, this setup is called once per word 'fallback' found in the config file.
Therefore, the way the syntax of this stanza is designed, you can register only ONE rcode per 'fallback'.
in other word this map named rules could be replaced by a couple (int, Upstream).
Am I correct ?
There was a problem hiding this comment.
yes, one rcode per fallback directive. See the comment above - I changed the syntax from the original, so it would be easy to re-use the proxy code.
There is only one fallback plugin, not one per directive. So, this one plugin processes all fallback directives, and so they end up in this map.
There was a problem hiding this comment.
maybe I misread the setup call ...
Ok. I double checked when is called the setup .. and it says once per key for each server block.
Very good. - exactly what is needed :)
/ SetupFunc is used to set up a plugin, or in other words,
// execute a directive. It will be called once per key for
// each server block it appears in.
type SetupFunc func(c *Controller) error
johnbelamaric
force-pushed
the
fallback
branch
from
d552f71
to
35dc8be
Compare
Codecov Report
@@ Coverage Diff @@
## master #1420 +/- ##
==========================================
+ Coverage 52.02% 52.15% +0.12%
==========================================
Files 175 177 +2
Lines 8530 8573 +43
==========================================
+ Hits 4438 4471 +33
- Misses 3753 3760 +7
- Partials 339 342 +3
Continue to review full report at Codecov.
|
Remove upstreamMapper code. Other minor fixes.
Fallback ko
|
Ko added some tests and now this is ready for review. |
| @@ -15,6 +15,7 @@ import ( | |||
| _ "github.com/coredns/coredns/plugin/erratic" | |||
| _ "github.com/coredns/coredns/plugin/errors" | |||
| _ "github.com/coredns/coredns/plugin/etcd" | |||
| _ "github.com/coredns/coredns/plugin/fallback" | |||
There was a problem hiding this comment.
no reason to put it here? Can bump it to its own repo.
There was a problem hiding this comment.
yeah, we could do that. @yuewko I will create a separate repo for it in CoreDNS org.
| } | ||
| ``` | ||
|
|
||
| * `RCODE` is the string representation of the error response code. The set of valid error |
There was a problem hiding this comment.
Nit: **RCODE** same for PROXY_PARAMS
| ``` | ||
|
|
||
| * `RCODE` is the string representation of the error response code. The set of valid error | ||
| strings are defined as `RcodeToString` in <https://github.com/miekg/dns/blob/master/msg.go> |
There was a problem hiding this comment.
I would list them here? Instead of pointing to a (large) file
There was a problem hiding this comment.
Yeah... but RcodeToString is a rather long list.
How about including a handful of typical example rcodes here, and refer to that link for the complete list?
| * `RCODE` is the string representation of the error response code. The set of valid error | ||
| strings are defined as `RcodeToString` in <https://github.com/miekg/dns/blob/master/msg.go> | ||
| * `PROXY_SPECS` accepts the same parameters as the *proxy* plugin | ||
| <https://github.com/coredns/coredns/tree/master/plugin/proxy>. |
| import ( | ||
| "context" | ||
|
|
||
| "github.com/miekg/dns" |
| "github.com/coredns/coredns/request" | ||
| ) | ||
|
|
||
| // stubNextHandler is used to simulate a proxy plugin. |
There was a problem hiding this comment.
the testing code has more comments on the non-testing code
| c.called++ | ||
|
|
||
| // Ensure that it is called with the expected Upstream | ||
| if c.expectedUpstream != nil && c.expectedUpstream != upstream { |
There was a problem hiding this comment.
why do you need to guard against this in a test that you control?
There was a problem hiding this comment.
These checks are not meant to guard against error conditions. They are there to check against expected behavior as per test case.
|
|
||
| // Ensure that it is called with the expected Upstream | ||
| if c.expectedUpstream != nil && c.expectedUpstream != upstream { | ||
| c.t.Fatalf("Expected upstream passed to proxyCreator is '%v', but got '%v'", |
There was a problem hiding this comment.
calling t.Fatalf in a non test function? Should return an err
There was a problem hiding this comment.
As per comment above, the check here is indeed part of the test.
Having said that, I do think it is more appropriate to use t.Errorf (instead of t.Fatalf). Will fix.
| return &dns.Msg{}, nil | ||
| } | ||
|
|
||
| func (e dummyExchanger) Protocol() string { |
There was a problem hiding this comment.
These functions call all be put on 1 line
| } | ||
| } | ||
|
|
||
| func TestFallbackNotCalled(t *testing.T) { |
There was a problem hiding this comment.
Why all this mocking and not just checking if the impl does the right thing if an upstream returns nxdomain (i.e. dnstest.Server should make this relatively painless)
There was a problem hiding this comment.
I must confess that I'm not familiar with dnstest. I just took a quick look and it seems, IMHO, that it looks more akin to integration test.
How about adding a couple of tests using dnstest to supplement?
|
[ Quoting <notifications@github.com> in "Re: [coredns/coredns] Fallback plug..." ]
yuewko commented on this pull request.
> +}
+
+// testProxyCreator implements the proxyCreator interface.
+// Used by unit test to verify that the proxyCreator.Create() method is called as expected
+type testProxyCreator struct {
+ expectedUpstream proxy.Upstream
+ called int
+ t *testing.T
+}
+
+func (c *testProxyCreator) Create(trace plugin.Handler, upstream proxy.Upstream) plugin.Handler {
+ c.called++
+
+ // Ensure that it is called with the expected Upstream
+ if c.expectedUpstream != nil && c.expectedUpstream != upstream {
+ c.t.Fatalf("Expected upstream passed to proxyCreator is '%v', but got '%v'",
As per comment above, the check here is indeed part of the test.
Having said that, I do think it is more appropriate to use t.Errorf (instead of t.Fatalf). Will fix.
No you should return the error can not call fatalf here
|
|
[ Quoting <notifications@github.com> in "Re: [coredns/coredns] Fallback plug..." ]
Yeah... but RcodeToString is a rather long list.
How about including a handful of typical example rcodes here, and refer to that link for the complete list?
Ah, true. Putting a couple here wont hurt, I guess.
|
|
re: #1420 (comment) re: #1420 (comment) Thanks!! |
|
Sounds good!
--
Yue Ko
Staff Software Engineer, Cloud and Network Automation
Infoblox | Control Your Network
(T) +1-443-603-0249 (M) +1-410-991-6293
yko@infoblox.com<mailto:yko@infoblox.com> | www.infoblox.com<http://www.infoblox.com/>
From: John Belamaric <notifications@github.com>
Reply-To: coredns/coredns <reply@reply.github.com>
Date: Monday, February 5, 2018 at 11:23 AM
To: coredns/coredns <coredns@noreply.github.com>
Cc: Yue Ko <yko@infoblox.com>, Mention <mention@noreply.github.com>
Subject: Re: [coredns/coredns] Fallback plugin (#1420)
@johnbelamaric commented on this pull request.
In core/plugin/zplugin.go<#1420 (comment)>:
@@ -15,6 +15,7 @@ import (
_ "github.com/coredns/coredns/plugin/erratic"
_ "github.com/coredns/coredns/plugin/errors"
_ "github.com/coredns/coredns/plugin/etcd"
+ _ "github.com/coredns/coredns/plugin/fallback"
yeah, we could do that. @yuewko<https://github.com/yuewko> I will create a separate repo for it in CoreDNS org.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#1420 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AIyxNxMj69Gxe0eBEAlnwblXfLiYc1MTks5tRyrIgaJpZM4RqQy5>.
|
|
moved to github.com/coredns/fallback, coredns.io/explugins also updated. |
1. What does this pull request do?
Implements the
fallbackplugin.2. Which issues (if any) are related?
#1382
3. Which documentation changes (if any) need to be made?
It will need its own README.