Default to upstream to self (coredns#2436)

* Default to upstream to self This is a backwards incompatible change. This is a massive (cleanup) PR where we default to resolving external names by the coredns process itself, instead of directly forwarding them to some upstream. This ignores any arguments `upstream` may have had and makes it depend on proxy/forward configuration in the Corefile. This allows resolved upstream names to be cached and we have better healthchecking of the upstreams. It also means there is only one way to resolve names, by either using the proxy or forward plugin. The proxy/forward lookup.go functions have been removed. This also lessen the dependency on proxy, meaning deprecating proxy will become easier. Some tests have been removed as well, or moved to the top-level test directory as they now require a full coredns process instead of just the plugin. For the etcd plugin, the entire StubZone resolving is *dropped*! This was a hacky (but working) solution to say the least. If someone cares deeply it can be brought back (maybe)? The pkg/upstream is now very small and almost does nothing. Also the New() function was changed to return a pointer to upstream.Upstream. It also returns only one parameter, so any stragglers using it will encounter a compile error. All documentation has been adapted. This affected the following plugins: * etcd * file * auto * secondary * federation * template * route53 A followup PR will make any upstream directives with arguments an error, right now they are ignored. Signed-off-by: Miek Gieben <[email protected]> * Fix etcd build - probably still fails unit test Signed-off-by: Miek Gieben <[email protected]> * Slightly smarter lookup check in upstream Signed-off-by: Miek Gieben <[email protected]> * Compilez Signed-off-by: Miek Gieben <[email protected]>
diogenxs · Jan 13, 2019 · 9c16ed1 · 9c16ed1
1 parent 6b56a9c
commit 9c16ed1
Show file tree

Hide file tree

Showing 55 changed files with 183 additions and 1,348 deletions.
diff --git a/plugin/auto/README.md b/plugin/auto/README.md
@@ -18,7 +18,7 @@ auto [ZONES...] {
     directory DIR [REGEXP ORIGIN_TEMPLATE [TIMEOUT]]
     reload DURATION
     no_reload
-    upstream [ADDRESS...]
+    upstream
 }
 ~~~
 
@@ -37,9 +37,7 @@ are used.
   and reloads zone when serial changes.
 * `no_reload` deprecated. Sets reload to 0.
 * `upstream` defines upstream resolvers to be used resolve external names found (think CNAMEs)
-  pointing to external names. **ADDRESS** can be an IP address, an IP:port or a string pointing to
-  a file that is structured as /etc/resolv.conf. If no **ADDRESS** is given, CoreDNS will resolve CNAMEs
-  against itself.
+  pointing to external names. CoreDNS will resolve CNAMEs against itself.
 
 All directives from the *file* plugin are supported. Note that *auto* will load all zones found,
 even though the directive might only receive queries for a specific zone. I.e:

diff --git a/plugin/auto/auto.go b/plugin/auto/auto.go
@@ -33,7 +33,7 @@ type (
 		// In the future this should be something like ZoneMeta that contains all this stuff.
 		transferTo     []string
 		ReloadInterval time.Duration
-		upstream       upstream.Upstream // Upstream for looking up names during the resolution process.
+		upstream       *upstream.Upstream // Upstream for looking up names during the resolution process.
 
 		duration time.Duration
 	}

diff --git a/plugin/auto/setup.go b/plugin/auto/setup.go
@@ -155,15 +155,8 @@ func autoParse(c *caddy.Controller) (Auto, error) {
 				a.loader.ReloadInterval = 0
 
 			case "upstream":
-				args := c.RemainingArgs()
-				if len(args) == 0 {
-					return a, c.ArgErr()
-				}
-				var err error
-				a.loader.upstream, err = upstream.New(args)
-				if err != nil {
-					return a, err
-				}
+				c.RemainingArgs() // eat remaining args
+				a.loader.upstream = upstream.New()
 
 			default:
 				t, _, e := parse.Transfer(c, false)

diff --git a/plugin/etcd/README.md b/plugin/etcd/README.md
@@ -28,28 +28,22 @@ If you want to `round robin` A and AAAA responses look at the `loadbalance` plug
 
 ~~~
 etcd [ZONES...] {
-    stubzones
     fallthrough [ZONES...]
     path PATH
     endpoint ENDPOINT...
-    upstream [ADDRESS...]
+    upstream
     tls CERT KEY CACERT
 }
 ~~~
 
-* `stubzones` enables the stub zones feature. The stubzone is *only* done in the etcd tree located
-    under the *first* zone specified.
 * `fallthrough` If zone matches but no record can be generated, pass request to the next plugin.
   If **[ZONES...]** is omitted, then fallthrough happens for all zones for which the plugin
   is authoritative. If specific zones are listed (for example `in-addr.arpa` and `ip6.arpa`), then only
   queries for those zones will be subject to fallthrough.
 * **PATH** the path inside etcd. Defaults to "/skydns".
 * **ENDPOINT** the etcd endpoints. Defaults to "http://localhost:2379".
-* `upstream` upstream resolvers to be used resolve external names found in etcd (think CNAMEs)
-  pointing to external names. If you want CoreDNS to act as a proxy for clients, you'll need to add
-  the proxy plugin. If no **ADDRESS** is given, CoreDNS will resolve CNAMEs against itself.
-  **ADDRESS** can be an IP address, and IP:port or a string pointing to a file that is structured
-  as /etc/resolv.conf.
+* `upstream` resolve names found in etcd (think CNAMEs) If you want CoreDNS to act as a proxy for clients,
+  you'll need to add the forward plugin. CoreDNS will resolve CNAMEs against itself.
 * `tls` followed by:
 
     * no arguments, if the server certificate is signed by a system-installed CA and no client cert is needed
@@ -79,10 +73,9 @@ This is the default SkyDNS setup, with everything specified in full:
 ~~~ corefile
 . {
     etcd skydns.local {
-        stubzones
         path /skydns
         endpoint http://localhost:2379
-        upstream 8.8.8.8:53 8.8.4.4:53
+        upstream
     }
     prometheus
     cache 160 skydns.local
@@ -98,7 +91,7 @@ when resolving external pointing CNAMEs.
 . {
     etcd skydns.local {
         path /skydns
-        upstream /etc/resolv.conf
+        upstream
     }
     cache 160 skydns.local
     proxy . /etc/resolv.conf
@@ -125,7 +118,6 @@ need to add the zone `0.0.10.in-addr.arpa` to the list of zones. Showing a snipp
 
 ~~~
 etcd skydns.local 10.0.0.0/24 {
-    stubzones
 ...
 ~~~
 

diff --git a/plugin/etcd/etcd.go b/plugin/etcd/etcd.go
@@ -12,7 +12,6 @@ import (
 	"github.com/coredns/coredns/plugin"
 	"github.com/coredns/coredns/plugin/etcd/msg"
 	"github.com/coredns/coredns/plugin/pkg/fall"
-	"github.com/coredns/coredns/plugin/proxy"
 	"github.com/coredns/coredns/request"
 
 	"github.com/coredns/coredns/plugin/pkg/upstream"
@@ -35,10 +34,9 @@ type Etcd struct {
 	Fall       fall.F
 	Zones      []string
 	PathPrefix string
-	Upstream   upstream.Upstream // Proxy for looking up names during the resolution process
+	Upstream   *upstream.Upstream
 	Client     *etcdcv3.Client
 	Ctx        context.Context
-	Stubmap    *map[string]proxy.Proxy // list of proxies for stub resolving.
 
 	endpoints []string // Stored here as well, to aid in testing.
 }

diff --git a/plugin/etcd/handler.go b/plugin/etcd/handler.go
@@ -14,20 +14,6 @@ func (e *Etcd) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg) (
 	opt := plugin.Options{}
 	state := request.Request{W: w, Req: r, Context: ctx}
 
-	name := state.Name()
-
-	// We need to check stubzones first, because we may get a request for a zone we
-	// are not auth. for *but* do have a stubzone forward for. If we do the stubzone
-	// handler will handle the request.
-	if e.Stubmap != nil && len(*e.Stubmap) > 0 {
-		for zone := range *e.Stubmap {
-			if plugin.Name(zone).Matches(name) {
-				stub := Stub{Etcd: e, Zone: zone}
-				return stub.ServeDNS(ctx, w, r)
-			}
-		}
-	}
-
 	zone := plugin.Zones(e.Zones).Matches(state.Name())
 	if zone == "" {
 		return plugin.NextOrFailure(e.Name(), e.Next, ctx, w, r)

diff --git a/plugin/etcd/lookup_test.go b/plugin/etcd/lookup_test.go
@@ -12,7 +12,6 @@ import (
 	"github.com/coredns/coredns/plugin/pkg/dnstest"
 	"github.com/coredns/coredns/plugin/pkg/tls"
 	"github.com/coredns/coredns/plugin/pkg/upstream"
-	"github.com/coredns/coredns/plugin/proxy"
 	"github.com/coredns/coredns/plugin/test"
 
 	"github.com/miekg/dns"
@@ -283,9 +282,8 @@ func newEtcdPlugin() *Etcd {
 	tlsc, _ := tls.NewTLSConfigFromArgs()
 	client, _ := newEtcdClient(endpoints, tlsc)
 
-	p := proxy.NewLookup([]string{"8.8.8.8:53"})
 	return &Etcd{
-		Upstream:   upstream.Upstream{Forward: &p},
+		Upstream:   upstream.New(),
 		PathPrefix: "skydns",
 		Ctx:        context.Background(),
 		Zones:      []string{"skydns.test.", "skydns_extra.test.", "skydns_zonea.test.", "skydns_zoneb.test.", "skydns_zonec.test.", "skydns_zoned.test.", "in-addr.arpa."},

diff --git a/plugin/etcd/setup.go b/plugin/etcd/setup.go
@@ -9,7 +9,6 @@ import (
 	clog "github.com/coredns/coredns/plugin/pkg/log"
 	mwtls "github.com/coredns/coredns/plugin/pkg/tls"
 	"github.com/coredns/coredns/plugin/pkg/upstream"
-	"github.com/coredns/coredns/plugin/proxy"
 
 	etcdcv3 "github.com/coreos/etcd/clientv3"
 	"github.com/mholt/caddy"
@@ -25,18 +24,11 @@ func init() {
 }
 
 func setup(c *caddy.Controller) error {
-	e, stubzones, err := etcdParse(c)
+	e, err := etcdParse(c)
 	if err != nil {
 		return plugin.Error("etcd", err)
 	}
 
-	if stubzones {
-		c.OnStartup(func() error {
-			e.UpdateStubZones()
-			return nil
-		})
-	}
-
 	dnsserver.GetConfig(c).AddPlugin(func(next plugin.Handler) plugin.Handler {
 		e.Next = next
 		return e
@@ -45,20 +37,17 @@ func setup(c *caddy.Controller) error {
 	return nil
 }
 
-func etcdParse(c *caddy.Controller) (*Etcd, bool, error) {
-	stub := make(map[string]proxy.Proxy)
+func etcdParse(c *caddy.Controller) (*Etcd, error) {
 	etc := Etcd{
 		// Don't default to a proxy for lookups.
 		//		Proxy:      proxy.NewLookup([]string{"8.8.8.8:53", "8.8.4.4:53"}),
 		PathPrefix: "skydns",
 		Ctx:        context.Background(),
-		Stubmap:    &stub,
 	}
 	var (
 		tlsConfig *tls.Config
 		err       error
 		endpoints = []string{defaultEndpoint}
-		stubzones = false
 	)
 	for c.Next() {
 		etc.Zones = c.RemainingArgs()
@@ -74,38 +63,35 @@ func etcdParse(c *caddy.Controller) (*Etcd, bool, error) {
 			for {
 				switch c.Val() {
 				case "stubzones":
-					stubzones = true
+					// ignored, remove later.
 				case "fallthrough":
 					etc.Fall.SetZonesFromArgs(c.RemainingArgs())
 				case "debug":
 					/* it is a noop now */
 				case "path":
 					if !c.NextArg() {
-						return &Etcd{}, false, c.ArgErr()
+						return &Etcd{}, c.ArgErr()
 					}
 					etc.PathPrefix = c.Val()
 				case "endpoint":
 					args := c.RemainingArgs()
 					if len(args) == 0 {
-						return &Etcd{}, false, c.ArgErr()
+						return &Etcd{}, c.ArgErr()
 					}
 					endpoints = args
 				case "upstream":
-					args := c.RemainingArgs()
-					u, err := upstream.New(args)
-					if err != nil {
-						return nil, false, err
-					}
-					etc.Upstream = u
+					// check args != 0 and error in the future
+					c.RemainingArgs() // clear buffer
+					etc.Upstream = upstream.New()
 				case "tls": // cert key cacertfile
 					args := c.RemainingArgs()
 					tlsConfig, err = mwtls.NewTLSConfigFromArgs(args...)
 					if err != nil {
-						return &Etcd{}, false, err
+						return &Etcd{}, err
 					}
 				default:
 					if c.Val() != "}" {
-						return &Etcd{}, false, c.Errf("unknown property '%s'", c.Val())
+						return &Etcd{}, c.Errf("unknown property '%s'", c.Val())
 					}
 				}
 
@@ -117,14 +103,14 @@ func etcdParse(c *caddy.Controller) (*Etcd, bool, error) {
 		}
 		client, err := newEtcdClient(endpoints, tlsConfig)
 		if err != nil {
-			return &Etcd{}, false, err
+			return &Etcd{}, err
 		}
 		etc.Client = client
 		etc.endpoints = endpoints
 
-		return &etc, stubzones, nil
+		return &etc, nil
 	}
-	return &Etcd{}, false, nil
+	return &Etcd{}, nil
 }
 
 func newEtcdClient(endpoints []string, cc *tls.Config) (*etcdcv3.Client, error) {

diff --git a/plugin/etcd/setup_test.go b/plugin/etcd/setup_test.go
@@ -56,7 +56,7 @@ func TestSetupEtcd(t *testing.T) {
 
 	for i, test := range tests {
 		c := caddy.NewTestController("dns", test.input)
-		etcd, _ /*stubzones*/, err := etcdParse(c)
+		etcd, err := etcdParse(c)
 
 		if test.shouldErr && err == nil {
 			t.Errorf("Test %d: Expected error but found %s for input %s", i, err, test.input)

diff --git a/plugin/etcd/stub.go b/plugin/etcd/stub.go