Default to upstream to self (#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 <miek@miek.nl>

* Fix etcd build - probably still fails unit test

Signed-off-by: Miek Gieben <miek@miek.nl>

* Slightly smarter lookup check in upstream

Signed-off-by: Miek Gieben <miek@miek.nl>

* Compilez

Signed-off-by: Miek Gieben <miek@miek.nl>

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
 ...
 ~~~
 

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.
 }

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)

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."},

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) {

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)

plugin/etcd/stub.go

@@ -1,81 +0,0 @@
-package etcd
-
-import (
-	"net"
-	"strconv"
-	"time"
-
-	"github.com/coredns/coredns/plugin/etcd/msg"
-	"github.com/coredns/coredns/plugin/pkg/dnsutil"
-	"github.com/coredns/coredns/plugin/proxy"
-	"github.com/coredns/coredns/request"
-
-	"github.com/miekg/dns"
-)
-
-// UpdateStubZones checks etcd for an update on the stubzones.
-func (e *Etcd) UpdateStubZones() {
-	go func() {
-		for {
-			e.updateStubZones()
-			time.Sleep(15 * time.Second)
-		}
-	}()
-}
-
-// Look in .../dns/stub/<zone>/xx for msg.Services. Loop through them
-// extract <zone> and add them as forwarders (ip:port-combos) for
-// the stub zones. Only numeric (i.e. IP address) hosts are used.
-// Only the first zone configured on e is used for the lookup.
-func (e *Etcd) updateStubZones() {
-	zone := e.Zones[0]
-
-	fakeState := request.Request{W: nil, Req: new(dns.Msg)}
-	fakeState.Req.SetQuestion(stubDomain+"."+zone, dns.TypeA)
-
-	services, err := e.Records(fakeState, false)
-	if err != nil {
-		return
-	}
-
-	stubmap := make(map[string]proxy.Proxy)
-	// track the nameservers on a per domain basis, but allow a list on the domain.
-	nameservers := map[string][]string{}
-
-Services:
-	for _, serv := range services {
-		if serv.Port == 0 {
-			serv.Port = 53
-		}
-		ip := net.ParseIP(serv.Host)
-		if ip == nil {
-			log.Warningf("Non IP address stub nameserver: %s", serv.Host)
-			continue
-		}
-
-		domain := msg.Domain(serv.Key)
-		labels := dns.SplitDomainName(domain)
-
-		// If the remaining name equals any of the zones we have, we ignore it.
-		for _, z := range e.Zones {
-			// Chop of left most label, because that is used as the nameserver place holder
-			// and drop the right most labels that belong to zone.
-			// We must *also* chop of dns.stub. which means cutting two more labels.
-			domain = dnsutil.Join(labels[1 : len(labels)-dns.CountLabel(z)-2]...)
-			if domain == z {
-				log.Warningf("Skipping nameserver for domain we are authoritative for: %s", domain)
-				continue Services
-			}
-		}
-		nameservers[domain] = append(nameservers[domain], net.JoinHostPort(serv.Host, strconv.Itoa(serv.Port)))
-	}
-
-	for domain, nss := range nameservers {
-		stubmap[domain] = proxy.NewLookup(nss)
-	}
-	// atomic swap (at least that's what we hope it is)
-	if len(stubmap) > 0 {
-		e.Stubmap = &stubmap
-	}
-	return
-}

plugin/etcd/stub_handler.go

@@ -1,83 +0,0 @@
-package etcd
-
-import (
-	"context"
-	"errors"
-
-	"github.com/coredns/coredns/request"
-
-	"github.com/miekg/dns"
-)
-
-// Stub wraps an Etcd. We have this type so that it can have a ServeDNS method.
-type Stub struct {
-	*Etcd
-	Zone string // for what zone (and thus what nameservers are we called)
-}
-
-// ServeDNS implements the plugin.Handler interface.
-func (s Stub) ServeDNS(ctx context.Context, w dns.ResponseWriter, req *dns.Msg) (int, error) {
-	if hasStubEdns0(req) {
-		log.Warningf("Forwarding cycle detected, refusing msg: %s", req.Question[0].Name)
-		return dns.RcodeRefused, errors.New("stub forward cycle")
-	}
-	req = addStubEdns0(req)
-	proxy, ok := (*s.Etcd.Stubmap)[s.Zone]
-	if !ok { // somebody made a mistake..
-		return dns.RcodeServerFailure, nil
-	}
-
-	state := request.Request{W: w, Req: req}
-	m, e := proxy.Forward(state)
-	if e != nil {
-		return dns.RcodeServerFailure, e
-	}
-	w.WriteMsg(m)
-	return dns.RcodeSuccess, nil
-}
-
-// hasStubEdns0 checks if the message is carrying our special edns0 zero option.
-func hasStubEdns0(m *dns.Msg) bool {
-	option := m.IsEdns0()
-	if option == nil {
-		return false
-	}
-	for _, o := range option.Option {
-		if o.Option() == ednsStubCode && len(o.(*dns.EDNS0_LOCAL).Data) == 1 &&
-			o.(*dns.EDNS0_LOCAL).Data[0] == 1 {
-			return true
-		}
-	}
-	return false
-}
-
-// addStubEdns0 adds our special option to the message's OPT record.
-func addStubEdns0(m *dns.Msg) *dns.Msg {
-	option := m.IsEdns0()
-	// Add a custom EDNS0 option to the packet, so we can detect loops when 2 stubs are forwarding to each other.
-	if option != nil {
-		option.Option = append(option.Option, &dns.EDNS0_LOCAL{Code: ednsStubCode, Data: []byte{1}})
-		return m
-	}
-
-	m.Extra = append(m.Extra, ednsStub)
-	return m
-}
-
-const (
-	ednsStubCode = dns.EDNS0LOCALSTART + 10
-	stubDomain   = "stub.dns"
-)
-
-var ednsStub = func() *dns.OPT {
-	o := new(dns.OPT)
-	o.Hdr.Name = "."
-	o.Hdr.Rrtype = dns.TypeOPT
-	o.SetUDPSize(4096)
-
-	e := new(dns.EDNS0_LOCAL)
-	e.Code = ednsStubCode
-	e.Data = []byte{1}
-	o.Option = append(o.Option, e)
-	return o
-}()

plugin/etcd/stub_test.go

@@ -1,87 +0,0 @@
-// +build etcd
-
-package etcd
-
-import (
-	"net"
-	"strconv"
-	"testing"
-
-	"github.com/coredns/coredns/plugin/etcd/msg"
-	"github.com/coredns/coredns/plugin/pkg/dnstest"
-	"github.com/coredns/coredns/plugin/test"
-
-	"github.com/miekg/dns"
-)
-
-func fakeStubServerExampleNet(t *testing.T) (*dns.Server, string) {
-	server, addr, err := test.UDPServer("127.0.0.1:0")
-	if err != nil {
-		t.Fatalf("Failed to create a UDP server: %s", err)
-	}
-	// add handler for example.net
-	dns.HandleFunc("example.net.", func(w dns.ResponseWriter, r *dns.Msg) {
-		m := new(dns.Msg)
-		m.SetReply(r)
-		m.Answer = []dns.RR{test.A("example.net.	86400	IN	A	93.184.216.34")}
-		w.WriteMsg(m)
-	})
-
-	return server, addr
-}
-
-func TestStubLookup(t *testing.T) {
-	server, addr := fakeStubServerExampleNet(t)
-	defer server.Shutdown()
-
-	host, p, _ := net.SplitHostPort(addr)
-	port, _ := strconv.Atoi(p)
-	exampleNetStub := &msg.Service{Host: host, Port: port, Key: "a.example.net.stub.dns.skydns.test."}
-	servicesStub = append(servicesStub, exampleNetStub)
-
-	etc := newEtcdPlugin()
-
-	for _, serv := range servicesStub {
-		set(t, etc, serv.Key, 0, serv)
-		defer delete(t, etc, serv.Key)
-	}
-
-	etc.updateStubZones()
-
-	for _, tc := range dnsTestCasesStub {
-		m := tc.Msg()
-
-		rec := dnstest.NewRecorder(&test.ResponseWriter{})
-		_, err := etc.ServeDNS(ctxt, rec, m)
-		if err != nil && m.Question[0].Name == "example.org." {
-			// This is OK, we expect this backend to *not* work.
-			continue
-		}
-		if err != nil {
-			t.Errorf("Expected no error, got %v for %s\n", err, m.Question[0].Name)
-		}
-		resp := rec.Msg
-		if resp == nil {
-			// etcd not running?
-			continue
-		}
-
-		test.SortAndCheck(t, resp, tc)
-	}
-}
-
-var servicesStub = []*msg.Service{
-	// Two tests, ask a question that should return servfail because remote it no accessible
-	// and one with edns0 option added, that should return refused.
-	{Host: "127.0.0.1", Port: 666, Key: "b.example.org.stub.dns.skydns.test."},
-}
-
-var dnsTestCasesStub = []test.Case{
-	{
-		Qname: "example.org.", Qtype: dns.TypeA, Rcode: dns.RcodeServerFailure,
-	},
-	{
-		Qname: "example.net.", Qtype: dns.TypeA,
-		Answer: []dns.RR{test.A("example.net.	86400	IN	A	93.184.216.34")},
-	},
-}