Simplify plugin/pkg/fall (#1358)

* Simplify plugin/pkg/fall

* Remove unused import

* Fix fall_test

* Get fall coverage to 100% just because

* gofmt. sigh.

plugin/etcd/etcd.go

@@ -21,7 +21,7 @@ import (
 // Etcd is a plugin talks to an etcd cluster.
 type Etcd struct {
 	Next       plugin.Handler
-	Fall       *fall.F
+	Fall       fall.F
 	Zones      []string
 	PathPrefix string
 	Proxy      proxy.Proxy // Proxy for looking up names during the resolution process

plugin/etcd/multi_test.go

@@ -7,7 +7,6 @@ import (
 
 	"github.com/coredns/coredns/plugin/etcd/msg"
 	"github.com/coredns/coredns/plugin/pkg/dnstest"
-	"github.com/coredns/coredns/plugin/pkg/fall"
 	"github.com/coredns/coredns/plugin/test"
 
 	"github.com/miekg/dns"
@@ -16,7 +15,6 @@ import (
 func TestMultiLookup(t *testing.T) {
 	etc := newEtcdPlugin()
 	etc.Zones = []string{"skydns.test.", "miek.nl."}
-	etc.Fall = fall.New()
 	etc.Next = test.ErrorHandler()
 
 	for _, serv := range servicesMulti {

plugin/etcd/setup.go

@@ -6,7 +6,6 @@ import (
 	"github.com/coredns/coredns/core/dnsserver"
 	"github.com/coredns/coredns/plugin"
 	"github.com/coredns/coredns/plugin/pkg/dnsutil"
-	"github.com/coredns/coredns/plugin/pkg/fall"
 	mwtls "github.com/coredns/coredns/plugin/pkg/tls"
 	"github.com/coredns/coredns/plugin/proxy"
 
@@ -74,8 +73,7 @@ func etcdParse(c *caddy.Controller) (*Etcd, bool, error) {
 				case "stubzones":
 					stubzones = true
 				case "fallthrough":
-					etc.Fall = fall.New()
-					etc.Fall.SetZones(c.RemainingArgs())
+					etc.Fall.SetZonesFromArgs(c.RemainingArgs())
 				case "debug":
 					/* it is a noop now */
 				case "path":

plugin/hosts/hosts.go

@@ -17,7 +17,7 @@ type Hosts struct {
 	Next plugin.Handler
 	*Hostsfile
 
-	Fall *fall.F
+	Fall fall.F
 }
 
 // ServeDNS implements the plugin.Handle interface.

plugin/hosts/setup.go

@@ -9,7 +9,6 @@ import (
 
 	"github.com/coredns/coredns/core/dnsserver"
 	"github.com/coredns/coredns/plugin"
-	"github.com/coredns/coredns/plugin/pkg/fall"
 
 	"github.com/mholt/caddy"
 )
@@ -106,10 +105,9 @@ func hostsParse(c *caddy.Controller) (Hosts, error) {
 		for c.NextBlock() {
 			switch c.Val() {
 			case "fallthrough":
-				h.Fall = fall.New()
-				h.Fall.SetZones(c.RemainingArgs())
+				h.Fall.SetZonesFromArgs(c.RemainingArgs())
 			default:
-				if h.Fall.IsNil() {
+				if len(h.Fall.Zones) == 0 {
 					line := strings.Join(append([]string{c.Val()}, c.RemainingArgs()...), " ")
 					inline = append(inline, line)
 					continue

plugin/hosts/setup_test.go

@@ -14,48 +14,48 @@ func TestHostsParse(t *testing.T) {
 		shouldErr           bool
 		expectedPath        string
 		expectedOrigins     []string
-		expectedFallthrough *fall.F
+		expectedFallthrough fall.F
 	}{
 		{
 			`hosts
 `,
-			false, "/etc/hosts", nil, nil,
+			false, "/etc/hosts", nil, fall.Zero(),
 		},
 		{
 			`hosts /tmp`,
-			false, "/tmp", nil, nil,
+			false, "/tmp", nil, fall.Zero(),
 		},
 		{
 			`hosts /etc/hosts miek.nl.`,
-			false, "/etc/hosts", []string{"miek.nl."}, nil,
+			false, "/etc/hosts", []string{"miek.nl."}, fall.Zero(),
 		},
 		{
 			`hosts /etc/hosts miek.nl. pun.gent.`,
-			false, "/etc/hosts", []string{"miek.nl.", "pun.gent."}, nil,
+			false, "/etc/hosts", []string{"miek.nl.", "pun.gent."}, fall.Zero(),
 		},
 		{
 			`hosts {
 				fallthrough
 			}`,
-			false, "/etc/hosts", nil, fall.Zero(),
+			false, "/etc/hosts", nil, fall.Root(),
 		},
 		{
 			`hosts /tmp {
 				fallthrough
 			}`,
-			false, "/tmp", nil, fall.Zero(),
+			false, "/tmp", nil, fall.Root(),
 		},
 		{
 			`hosts /etc/hosts miek.nl. {
 				fallthrough
 			}`,
-			false, "/etc/hosts", []string{"miek.nl."}, fall.Zero(),
+			false, "/etc/hosts", []string{"miek.nl."}, fall.Root(),
 		},
 		{
 			`hosts /etc/hosts miek.nl 10.0.0.9/8 {
 				fallthrough
 			}`,
-			false, "/etc/hosts", []string{"miek.nl.", "10.in-addr.arpa."}, fall.Zero(),
+			false, "/etc/hosts", []string{"miek.nl.", "10.in-addr.arpa."}, fall.Root(),
 		},
 	}
 
@@ -92,7 +92,7 @@ func TestHostsInlineParse(t *testing.T) {
 		inputFileRules      string
 		shouldErr           bool
 		expectedbyAddr      map[string][]string
-		expectedFallthrough *fall.F
+		expectedFallthrough fall.F
 	}{
 		{
 			`hosts highly_unlikely_to_exist_hosts_file example.org {
@@ -105,7 +105,7 @@ func TestHostsInlineParse(t *testing.T) {
 					`example.org.`,
 				},
 			},
-			fall.Zero(),
+			fall.Root(),
 		},
 		{
 			`hosts highly_unlikely_to_exist_hosts_file example.org {
@@ -117,7 +117,7 @@ func TestHostsInlineParse(t *testing.T) {
 					`example.org.`,
 				},
 			},
-			nil,
+			fall.Zero(),
 		},
 		{
 			`hosts highly_unlikely_to_exist_hosts_file example.org {
@@ -126,14 +126,13 @@ func TestHostsInlineParse(t *testing.T) {
 			                        }`,
 			true,
 			map[string][]string{},
-			fall.Zero(),
+			fall.Root(),
 		},
 	}
 
 	for i, test := range tests {
 		c := caddy.NewTestController("dns", test.inputFileRules)
 		h, err := hostsParse(c)
-
 		if err == nil && test.shouldErr {
 			t.Fatalf("Test %d expected errors, but got no error", i)
 		} else if err != nil && !test.shouldErr {

plugin/kubernetes/kubernetes.go

@@ -41,7 +41,7 @@ type Kubernetes struct {
 	Namespaces       map[string]bool
 	podMode          string
 	endpointNameMode bool
-	Fall             *fall.F
+	Fall             fall.F
 	ttl              uint32
 
 	primaryZoneIndex   int

plugin/kubernetes/setup.go

@@ -10,7 +10,6 @@ import (
 	"github.com/coredns/coredns/core/dnsserver"
 	"github.com/coredns/coredns/plugin"
 	"github.com/coredns/coredns/plugin/pkg/dnsutil"
-	"github.com/coredns/coredns/plugin/pkg/fall"
 	"github.com/coredns/coredns/plugin/proxy"
 
 	"github.com/mholt/caddy"
@@ -173,8 +172,7 @@ func kubernetesParse(c *caddy.Controller) (*Kubernetes, dnsControlOpts, error) {
 				}
 				return nil, opts, c.ArgErr()
 			case "fallthrough":
-				k8s.Fall = fall.New()
-				k8s.Fall.SetZones(c.RemainingArgs())
+				k8s.Fall.SetZonesFromArgs(c.RemainingArgs())
 			case "upstream":
 				args := c.RemainingArgs()
 				if len(args) == 0 {

plugin/kubernetes/setup_test.go

@@ -5,6 +5,8 @@ import (
 	"testing"
 	"time"
 
+	"github.com/coredns/coredns/plugin/pkg/fall"
+
 	"github.com/mholt/caddy"
 	meta "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
@@ -19,7 +21,7 @@ func TestKubernetesParse(t *testing.T) {
 		expectedResyncPeriod  time.Duration // expected resync period value
 		expectedLabelSelector string        // expected label selector value
 		expectedPodMode       string
-		expectedFallthrough   *[]string
+		expectedFallthrough   fall.F
 		expectedUpstreams     []string
 	}{
 		// positive
@@ -32,7 +34,7 @@ func TestKubernetesParse(t *testing.T) {
 			defaultResyncPeriod,
 			"",
 			podModeDisabled,
-			nil,
+			fall.Zero(),
 			nil,
 		},
 		{
@@ -44,7 +46,7 @@ func TestKubernetesParse(t *testing.T) {
 			defaultResyncPeriod,
 			"",
 			podModeDisabled,
-			nil,
+			fall.Zero(),
 			nil,
 		},
 		{
@@ -57,7 +59,7 @@ func TestKubernetesParse(t *testing.T) {
 			defaultResyncPeriod,
 			"",
 			podModeDisabled,
-			nil,
+			fall.Zero(),
 			nil,
 		},
 		{
@@ -71,7 +73,7 @@ func TestKubernetesParse(t *testing.T) {
 			defaultResyncPeriod,
 			"",
 			podModeDisabled,
-			nil,
+			fall.Zero(),
 			nil,
 		},
 		{
@@ -85,7 +87,7 @@ func TestKubernetesParse(t *testing.T) {
 			defaultResyncPeriod,
 			"",
 			podModeDisabled,
-			nil,
+			fall.Zero(),
 			nil,
 		},
 		{
@@ -99,7 +101,7 @@ func TestKubernetesParse(t *testing.T) {
 			defaultResyncPeriod,
 			"",
 			podModeDisabled,
-			nil,
+			fall.Zero(),
 			nil,
 		},
 		{
@@ -113,7 +115,7 @@ func TestKubernetesParse(t *testing.T) {
 			30 * time.Second,
 			"",
 			podModeDisabled,
-			nil,
+			fall.Zero(),
 			nil,
 		},
 		{
@@ -127,7 +129,7 @@ func TestKubernetesParse(t *testing.T) {
 			15 * time.Minute,
 			"",
 			podModeDisabled,
-			nil,
+			fall.Zero(),
 			nil,
 		},
 		{
@@ -141,7 +143,7 @@ func TestKubernetesParse(t *testing.T) {
 			defaultResyncPeriod,
 			"environment=prod",
 			podModeDisabled,
-			nil,
+			fall.Zero(),
 			nil,
 		},
 		{
@@ -155,7 +157,7 @@ func TestKubernetesParse(t *testing.T) {
 			defaultResyncPeriod,
 			"application=nginx,environment in (production,qa,staging)",
 			podModeDisabled,
-			nil,
+			fall.Zero(),
 			nil,
 		},
 		{
@@ -173,7 +175,7 @@ func TestKubernetesParse(t *testing.T) {
 			15 * time.Minute,
 			"application=nginx,environment in (production,qa,staging)",
 			podModeDisabled,
-			&[]string{},
+			fall.Root(),
 			nil,
 		},
 		// negative
@@ -188,7 +190,7 @@ func TestKubernetesParse(t *testing.T) {
 			defaultResyncPeriod,
 			"",
 			podModeDisabled,
-			nil,
+			fall.Zero(),
 			nil,
 		},
 		{
@@ -202,7 +204,7 @@ func TestKubernetesParse(t *testing.T) {
 			defaultResyncPeriod,
 			"",
 			podModeDisabled,
-			nil,
+			fall.Zero(),
 			nil,
 		},
 		{
@@ -216,7 +218,7 @@ func TestKubernetesParse(t *testing.T) {
 			0 * time.Minute,
 			"",
 			podModeDisabled,
-			nil,
+			fall.Zero(),
 			nil,
 		},
 		{
@@ -230,7 +232,7 @@ func TestKubernetesParse(t *testing.T) {
 			0 * time.Second,
 			"",
 			podModeDisabled,
-			nil,
+			fall.Zero(),
 			nil,
 		},
 		{
@@ -244,7 +246,7 @@ func TestKubernetesParse(t *testing.T) {
 			0 * time.Second,
 			"",
 			podModeDisabled,
-			nil,
+			fall.Zero(),
 			nil,
 		},
 		{
@@ -258,7 +260,7 @@ func TestKubernetesParse(t *testing.T) {
 			0 * time.Second,
 			"",
 			podModeDisabled,
-			nil,
+			fall.Zero(),
 			nil,
 		},
 		{
@@ -272,7 +274,7 @@ func TestKubernetesParse(t *testing.T) {
 			0 * time.Second,
 			"",
 			podModeDisabled,
-			nil,
+			fall.Zero(),
 			nil,
 		},
 		// pods disabled
@@ -287,7 +289,7 @@ func TestKubernetesParse(t *testing.T) {
 			defaultResyncPeriod,
 			"",
 			podModeDisabled,
-			nil,
+			fall.Zero(),
 			nil,
 		},
 		// pods insecure
@@ -302,7 +304,7 @@ func TestKubernetesParse(t *testing.T) {
 			defaultResyncPeriod,
 			"",
 			podModeInsecure,
-			nil,
+			fall.Zero(),
 			nil,
 		},
 		// pods verified
@@ -317,7 +319,7 @@ func TestKubernetesParse(t *testing.T) {
 			defaultResyncPeriod,
 			"",
 			podModeVerified,
-			nil,
+			fall.Zero(),
 			nil,
 		},
 		// pods invalid
@@ -332,7 +334,7 @@ func TestKubernetesParse(t *testing.T) {
 			defaultResyncPeriod,
 			"",
 			podModeVerified,
-			nil,
+			fall.Zero(),
 			nil,
 		},
 		// fallthrough with zones
@@ -347,7 +349,7 @@ func TestKubernetesParse(t *testing.T) {
 			defaultResyncPeriod,
 			"",
 			podModeDisabled,
-			&[]string{"ip6.arpa.", "inaddr.arpa.", "foo.com."},
+			fall.F{Zones: []string{"ip6.arpa.", "inaddr.arpa.", "foo.com."}},
 			nil,
 		},
 		// Valid upstream
@@ -362,7 +364,7 @@ func TestKubernetesParse(t *testing.T) {
 			defaultResyncPeriod,
 			"",
 			podModeDisabled,
-			nil,
+			fall.Zero(),
 			[]string{"13.14.15.16:53"},
 		},
 		// Invalid upstream
@@ -377,7 +379,7 @@ func TestKubernetesParse(t *testing.T) {
 			defaultResyncPeriod,
 			"",
 			podModeDisabled,
-			nil,
+			fall.Zero(),
 			nil,
 		},
 	}
@@ -443,23 +445,8 @@ func TestKubernetesParse(t *testing.T) {
 		}
 
 		// fallthrough
-		foundFallthrough := k8sController.Fall
-		if foundFallthrough != nil {
-			failed := false
-			if test.expectedFallthrough == nil {
-				failed = true
-			} else if len(*foundFallthrough) != len(*test.expectedFallthrough) {
-				failed = true
-			} else {
-				for i := range *foundFallthrough {
-					if (*foundFallthrough)[i] != (*test.expectedFallthrough)[i] {
-						failed = true
-					}
-				}
-			}
-			if failed {
-				t.Errorf("Test %d: Expected kubernetes controller to be initialized with fallthrough '%v'. Instead found fallthrough '%v' for input '%s'", i, test.expectedFallthrough, foundFallthrough, test.input)
-			}
+		if !k8sController.Fall.Equal(test.expectedFallthrough) {
+			t.Errorf("Test %d: Expected kubernetes controller to be initialized with fallthrough '%v'. Instead found fallthrough '%v' for input '%s'", i, test.expectedFallthrough, k8sController.Fall, test.input)
 		}
 		// upstream
 		foundUpstreams := k8sController.Proxy.Upstreams

plugin/pkg/fall/fall.go

@@ -7,71 +7,52 @@ import (
 
 // F can be nil to allow for no fallthrough, empty allow all zones to fallthrough or
 // contain a zone list that is checked.
-type F []string
-
-// New returns a new F.
-func New() *F { return new(F) }
+type F struct {
+	Zones []string
+}
 
 // Through will check if we should fallthrough for qname. Note that we've named the
 // variable in each plugin "Fall", so this then reads Fall.Through().
-func (f *F) Through(qname string) bool {
-	if f == nil {
-		return false
-	}
-	if len(*f) == 0 {
-		return true
-	}
-	zone := plugin.Zones(*f).Matches(qname)
-	return zone != ""
+func (f F) Through(qname string) bool {
+	return plugin.Zones(f.Zones).Matches(qname) != ""
 }
 
-// SetZones will set zones in f.
-func (f *F) SetZones(zones []string) {
+// setZones will set zones in f.
+func (f *F) setZones(zones []string) {
 	for i := range zones {
 		zones[i] = plugin.Host(zones[i]).Normalize()
 	}
-	*f = zones
+	f.Zones = zones
 }
 
-// Example returns an F with example.org. as the zone name.
-var Example = func() *F {
-	f := F([]string{"example.org."})
-	return &f
-}()
-
-// Zero returns a zero valued F.
-var Zero = func() *F {
-	f := F([]string{})
-	return &f
-}
-
-// IsNil returns true is f is nil.
-func (f *F) IsNil() bool { return f == nil }
-
-// IsZero returns true is f is zero (and not nil).
-func (f *F) IsZero() bool {
-	if f == nil {
-		return false
+// SetZonesFromArgs sets zones in f to the passed value or to "." if the slice is empty.
+func (f *F) SetZonesFromArgs(zones []string) {
+	if len(zones) == 0 {
+		f.setZones(Root().Zones)
+		return
 	}
-	return len(*f) == 0
+	f.setZones(zones)
 }
 
-// Equal returns true if f and g are equal. Only useful in tests, The (possible) zones
-// are *not* checked.
-func (f *F) Equal(g *F) bool {
-	if f.IsNil() {
-		if g.IsNil() {
-			return true
-		}
+// Equal returns true if f and g are equal.
+func (f F) Equal(g F) bool {
+	if len(f.Zones) != len(g.Zones) {
 		return false
 	}
-	if f.IsZero() {
-		if g.IsZero() {
-			return true
-		}
-	}
-	if len(*f) != len(*g) {
+	for i := range f.Zones {
+		if f.Zones[i] != g.Zones[i] {
 			return false
 		}
+	}
 	return true
 }
+
+// Zero returns a zero valued F.
+var Zero = func() F {
+	return F{[]string{}}
+}
+
+// Root returns F set to only ".".
+var Root = func() F {
+	return F{[]string{"."}}
+}

plugin/pkg/fall/fall_test.go

@@ -2,41 +2,58 @@ package fall
 
 import "testing"
 
-func TestIsNil(t *testing.T) {
-	var f *F
-	if !f.IsNil() {
-		t.Errorf("F should be nil")
+func TestEqual(t *testing.T) {
+	var z F
+	f := F{Zones: []string{"example.com."}}
+	g := F{Zones: []string{"example.net."}}
+	h := F{Zones: []string{"example.com."}}
+
+	if !f.Equal(h) {
+		t.Errorf("%v should equal %v", f, h)
+	}
+
+	if z.Equal(f) {
+		t.Errorf("%v should not be equal to %v", z, f)
+	}
+
+	if f.Equal(g) {
+		t.Errorf("%v should not be equal to %v", f, g)
 	}
 }
 
-func TestIsZero(t *testing.T) {
-	f := New()
-	if !f.IsZero() {
+func TestZero(t *testing.T) {
+	var f F
+	if !f.Equal(Zero()) {
 		t.Errorf("F should be zero")
 	}
 }
 
-func TestFallThroughExample(t *testing.T) {
-	if !Example.Through("example.org.") {
-		t.Errorf("example.org. should fall through")
+func TestSetZonesFromArgs(t *testing.T) {
+	var f F
+	f.SetZonesFromArgs([]string{})
+	if !f.Equal(Root()) {
+		t.Errorf("F should have the root zone")
 	}
-	if Example.Through("example.net.") {
-		t.Errorf("example.net. should not fall through")
+
+	f.SetZonesFromArgs([]string{"example.com", "example.net."})
+	expected := F{Zones: []string{"example.com.", "example.net."}}
+	if !f.Equal(expected) {
+		t.Errorf("F should be %v but is %v", expected, f)
 	}
 }
 
 func TestFallthrough(t *testing.T) {
-	var fall *F
+	var fall F
 	if fall.Through("foo.com.") {
-		t.Errorf("Expected false, got true for nil fallthrough")
+		t.Errorf("Expected false, got true for zero fallthrough")
 	}
 
-	fall = New()
+	fall.SetZonesFromArgs([]string{})
 	if !fall.Through("foo.net.") {
 		t.Errorf("Expected true, got false for all zone fallthrough")
 	}
 
-	fall.SetZones([]string{"foo.com", "bar.com"})
+	fall.SetZonesFromArgs([]string{"foo.com", "bar.com"})
 
 	if fall.Through("foo.net.") {
 		t.Errorf("Expected false, got true for non-matching fallthrough zone")

plugin/reverse/reverse.go

@@ -17,7 +17,7 @@ type Reverse struct {
 	Next     plugin.Handler
 	Networks networks
 
-	Fall *fall.F
+	Fall fall.F
 }
 
 // ServeDNS implements the plugin.Handler interface.

plugin/reverse/setup.go

@@ -34,7 +34,7 @@ func setupReverse(c *caddy.Controller) error {
 	return nil
 }
 
-func reverseParse(c *caddy.Controller) (nets networks, f *fall.F, err error) {
+func reverseParse(c *caddy.Controller) (nets networks, f fall.F, err error) {
 	zones := make([]string, len(c.ServerBlockKeys))
 	wildcard := false
 
@@ -87,8 +87,7 @@ func reverseParse(c *caddy.Controller) (nets networks, f *fall.F, err error) {
 				wildcard = true
 
 			case "fallthrough":
-				f = fall.New()
-				f.SetZones(c.RemainingArgs())
+				f.SetZonesFromArgs(c.RemainingArgs())
 
 			default:
 				return nil, f, c.ArgErr()