healhcheck: various cleanups (#1106)

* healhcheck: various cleanups

Network wasn't used. IgnorePaths wasn't used. Move checkdown function to
common function shared between proxy protocols. And some naming fixed.

Also reset the Fails on a succesful healthcheck back to 0.

remove newlines from log

* compile

* fix test

plugin/kubernetes/apiproxy.go

@@ -23,10 +23,8 @@ type apiProxy struct {
 func (p *proxyHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	upstream := p.Select()
 	network := "tcp"
-	if upstream.Network != "" {
-		network = upstream.Network
-	}
 	address := upstream.Name
+
 	d, err := net.Dial(network, address)
 	if err != nil {
 		log.Printf("[ERROR] Unable to establish connection to upstream %s://%s: %s", network, address, err)

plugin/kubernetes/kubernetes.go

@@ -190,9 +190,9 @@ func (k *Kubernetes) getClientConfig() (*rest.Config, error) {
 
 						down := false
 
-						uh.CheckMu.Lock()
+						uh.Lock()
 						until := uh.OkUntil
-						uh.CheckMu.Unlock()
+						uh.Unlock()
 
 						if !until.IsZero() && time.Now().After(until) {
 							down = true

plugin/pkg/healthcheck/healthcheck.go

@@ -19,15 +19,13 @@ type UpstreamHostDownFunc func(*UpstreamHost) bool
 type UpstreamHost struct {
 	Conns       int64  // must be first field to be 64-bit aligned on 32-bit systems
 	Name        string // IP address (and port) of this upstream host
-	Network           string // Network (tcp, unix, etc) of the host, default "" is "tcp"
 	Fails       int32
 	FailTimeout time.Duration
 	OkUntil     time.Time
 	CheckDown   UpstreamHostDownFunc
 	CheckURL    string
-	WithoutPathPrefix string
 	Checking    bool
-	CheckMu           sync.Mutex
+	sync.Mutex
 }
 
 // Down checks whether the upstream host is down or not.
@@ -39,9 +37,9 @@ func (uh *UpstreamHost) Down() bool {
 		fails := atomic.LoadInt32(&uh.Fails)
 		after := false
 
-		uh.CheckMu.Lock()
+		uh.Lock()
 		until := uh.OkUntil
-		uh.CheckMu.Unlock()
+		uh.Unlock()
 
 		if !until.IsZero() && time.Now().After(until) {
 			after = true
@@ -106,45 +104,45 @@ func (u *HealthCheck) Stop() error {
 // otherwise checks will back up, potentially a lot of them if a host is
 // absent for a long time.  This arrangement makes checks quickly see if
 // they are the only one running and abort otherwise.
-func healthCheckURL(nextTs time.Time, host *UpstreamHost) {
+func (uh *UpstreamHost) healthCheckURL(nextTs time.Time) {
 
 	// lock for our bool check.  We don't just defer the unlock because
 	// we don't want the lock held while http.Get runs
-	host.CheckMu.Lock()
+	uh.Lock()
 
 	// are we mid check?  Don't run another one
-	if host.Checking {
-		host.CheckMu.Unlock()
+	if uh.Checking {
+		uh.Unlock()
 		return
 	}
 
-	host.Checking = true
-	host.CheckMu.Unlock()
-
-	//log.Printf("[DEBUG] Healthchecking %s, nextTs is %s\n", url, nextTs.Local())
+	uh.Checking = true
+	uh.Unlock()
 
 	// fetch that url.  This has been moved into a go func because
 	// when the remote host is not merely not serving, but actually
 	// absent, then tcp syn timeouts can be very long, and so one
 	// fetch could last several check intervals
-	if r, err := http.Get(host.CheckURL); err == nil {
+	if r, err := http.Get(uh.CheckURL); err == nil {
 		io.Copy(ioutil.Discard, r.Body)
 		r.Body.Close()
 
 		if r.StatusCode < 200 || r.StatusCode >= 400 {
-			log.Printf("[WARNING] Host %s health check returned HTTP code %d\n",
-				host.Name, r.StatusCode)
+			log.Printf("[WARNING] Host %s health check returned HTTP code %d", uh.Name, r.StatusCode)
 			nextTs = time.Unix(0, 0)
+		} else {
+			// We are healthy again, reset fails
+			atomic.StoreInt32(&uh.Fails, 0)
 		}
 	} else {
-		log.Printf("[WARNING] Host %s health check probe failed: %v\n", host.Name, err)
+		log.Printf("[WARNING] Host %s health check probe failed: %v", uh.Name, err)
 		nextTs = time.Unix(0, 0)
 	}
 
-	host.CheckMu.Lock()
-	host.Checking = false
-	host.OkUntil = nextTs
-	host.CheckMu.Unlock()
+	uh.Lock()
+	uh.Checking = false
+	uh.OkUntil = nextTs
+	uh.Unlock()
 }
 
 func (u *HealthCheck) healthCheck() {
@@ -174,11 +172,11 @@ func (u *HealthCheck) healthCheck() {
 			host.CheckURL = "http://" + net.JoinHostPort(checkHostName, checkPort) + u.Path
 		}
 
-		// calculate this before the get
+		// calculate next timestamp before the get
 		nextTs := time.Now().Add(u.Future)
 
 		// locks/bools should prevent requests backing up
-		go healthCheckURL(nextTs, host)
+		go host.healthCheckURL(nextTs)
 	}
 }
 

plugin/proxy/down.go

@@ -0,0 +1,30 @@
+package proxy
+
+import (
+	"sync/atomic"
+	"time"
+
+	"github.com/coredns/coredns/plugin/pkg/healthcheck"
+)
+
+// Default CheckDown functions for use in the proxy plugin.
+var checkDownFunc = func(upstream *staticUpstream) healthcheck.UpstreamHostDownFunc {
+	return func(uh *healthcheck.UpstreamHost) bool {
+
+		down := false
+
+		uh.Lock()
+		until := uh.OkUntil
+		uh.Unlock()
+
+		if !until.IsZero() && time.Now().After(until) {
+			down = true
+		}
+
+		fails := atomic.LoadInt32(&uh.Fails)
+		if fails >= upstream.MaxFails && upstream.MaxFails != 0 {
+			down = true
+		}
+		return down
+	}
+}

plugin/proxy/google.go

@@ -10,7 +10,6 @@ import (
 	"net"
 	"net/http"
 	"net/url"
-	"sync/atomic"
 	"time"
 
 	"github.com/coredns/coredns/plugin/pkg/healthcheck"
@@ -198,7 +197,6 @@ func newUpstream(hosts []string, old *staticUpstream) Upstream {
 			Future:      60 * time.Second,
 		},
 		ex:                old.ex,
-		WithoutPathPrefix: old.WithoutPathPrefix,
 		IgnoredSubDomains: old.IgnoredSubDomains,
 	}
 
@@ -209,28 +207,7 @@ func newUpstream(hosts []string, old *staticUpstream) Upstream {
 			Conns:       0,
 			Fails:       0,
 			FailTimeout: upstream.FailTimeout,
-
-			CheckDown: func(upstream *staticUpstream) healthcheck.UpstreamHostDownFunc {
-				return func(uh *healthcheck.UpstreamHost) bool {
-
-					down := false
-
-					uh.CheckMu.Lock()
-					until := uh.OkUntil
-					uh.CheckMu.Unlock()
-
-					if !until.IsZero() && time.Now().After(until) {
-						down = true
-					}
-
-					fails := atomic.LoadInt32(&uh.Fails)
-					if fails >= upstream.MaxFails && upstream.MaxFails != 0 {
-						down = true
-					}
-					return down
-				}
-			}(upstream),
-			WithoutPathPrefix: upstream.WithoutPathPrefix,
+			CheckDown:   checkDownFunc(upstream),
 		}
 
 		upstream.Hosts[i] = uh

plugin/proxy/lookup.go

@@ -40,28 +40,7 @@ func NewLookupWithOption(hosts []string, opts Options) Proxy {
 			Conns:       0,
 			Fails:       0,
 			FailTimeout: upstream.FailTimeout,
-
-			CheckDown: func(upstream *staticUpstream) healthcheck.UpstreamHostDownFunc {
-				return func(uh *healthcheck.UpstreamHost) bool {
-
-					down := false
-
-					uh.CheckMu.Lock()
-					until := uh.OkUntil
-					uh.CheckMu.Unlock()
-
-					if !until.IsZero() && time.Now().After(until) {
-						down = true
-					}
-
-					fails := atomic.LoadInt32(&uh.Fails)
-					if fails >= upstream.MaxFails && upstream.MaxFails != 0 {
-						down = true
-					}
-					return down
-				}
-			}(upstream),
-			WithoutPathPrefix: upstream.WithoutPathPrefix,
+			CheckDown:   checkDownFunc(upstream),
 		}
 
 		upstream.Hosts[i] = uh

plugin/proxy/upstream.go

@@ -4,7 +4,6 @@ import (
 	"fmt"
 	"net"
 	"strconv"
-	"sync/atomic"
 	"time"
 
 	"github.com/coredns/coredns/plugin"
@@ -20,7 +19,6 @@ type staticUpstream struct {
 
 	healthcheck.HealthCheck
 
-	WithoutPathPrefix string
 	IgnoredSubDomains []string
 	ex                Exchanger
 }
@@ -69,28 +67,7 @@ func NewStaticUpstreams(c *caddyfile.Dispenser) ([]Upstream, error) {
 				Conns:       0,
 				Fails:       0,
 				FailTimeout: upstream.FailTimeout,
-
-				CheckDown: func(upstream *staticUpstream) healthcheck.UpstreamHostDownFunc {
-					return func(uh *healthcheck.UpstreamHost) bool {
-
-						down := false
-
-						uh.CheckMu.Lock()
-						until := uh.OkUntil
-						uh.CheckMu.Unlock()
-
-						if !until.IsZero() && time.Now().After(until) {
-							down = true
-						}
-
-						fails := atomic.LoadInt32(&uh.Fails)
-						if fails >= upstream.MaxFails && upstream.MaxFails != 0 {
-							down = true
-						}
-						return down
-					}
-				}(upstream),
-				WithoutPathPrefix: upstream.WithoutPathPrefix,
+				CheckDown:   checkDownFunc(upstream),
 			}
 
 			upstream.Hosts[i] = uh
@@ -158,11 +135,6 @@ func parseBlock(c *caddyfile.Dispenser, u *staticUpstream) error {
 				u.Future = 3 * time.Second
 			}
 		}
-	case "without":
-		if !c.NextArg() {
-			return c.ArgErr()
-		}
-		u.WithoutPathPrefix = c.Val()
 	case "except":
 		ignoredDomains := c.RemainingArgs()
 		if len(ignoredDomains) == 0 {

plugin/proxy/upstream_test.go

@@ -84,13 +84,6 @@ proxy . 8.8.8.8:53 {
 		},
 		{
 			`
-proxy . 8.8.8.8:53 {
-    without without
-}`,
-			false,
-		},
-		{
-			`
 proxy . 8.8.8.8:53 {
     except miek.nl example.org 10.0.0.0/24
 }`,