Parse values given to ?passing in the API

This PR fixes GH-2212 in the most backwards-compatible way I can think
of. If the user does not pass a value for `?passing`, it's assumed to be
true, which mirrors the current behavior. However, if the user passes
any value for passing, that value is parsed as a bool using strconv.

It's important to note that this is technically a breaking change.
Previously using `?passing=false` would return only passing nodes. While
this behavior is obviously incorrect, it was the previous behavior. We
should call this out very clearly in the CHANGELOG.

command/agent/health_endpoint.go

@@ -3,6 +3,7 @@ package agent
 import (
 	"fmt"
 	"net/http"
+	"strconv"
 	"strings"
 
 	"github.com/hashicorp/consul/api"
@@ -148,7 +149,22 @@ func (s *HTTPServer) HealthServiceNodes(resp http.ResponseWriter, req *http.Requ
 
 	// Filter to only passing if specified
 	if _, ok := params[api.HealthPassing]; ok {
+		val := params.Get(api.HealthPassing)
+		// Backwards-compat to allow users to specify ?passing without a value. This
+		// should be removed in Consul 0.10.
+		if val == "" {
 			out.Nodes = filterNonPassing(out.Nodes)
+		} else {
+			filter, err := strconv.ParseBool(val)
+			if err != nil {
+				resp.WriteHeader(400)
+				fmt.Fprint(resp, "Invalid value for ?passing")
+				return nil, nil
+			}
+			if filter {
+				out.Nodes = filterNonPassing(out.Nodes)
+			}
+		}
 	}
 
 	// Translate addresses after filtering so we don't waste effort.

command/agent/health_endpoint_test.go

@@ -1,7 +1,9 @@
 package agent
 
 import (
+	"bytes"
 	"fmt"
+	"io/ioutil"
 	"net/http"
 	"net/http/httptest"
 	"reflect"
@@ -15,7 +17,7 @@ import (
 
 func TestHealthChecksInState(t *testing.T) {
 	t.Parallel()
-	t.Run("", func(t *testing.T) {
+	t.Run("warning", func(t *testing.T) {
 		a := NewTestAgent(t.Name(), nil)
 		defer a.Shutdown()
 
@@ -38,7 +40,7 @@ func TestHealthChecksInState(t *testing.T) {
 		})
 	})
 
-	t.Run("", func(t *testing.T) {
+	t.Run("passing", func(t *testing.T) {
 		a := NewTestAgent(t.Name(), nil)
 		defer a.Shutdown()
 
@@ -612,6 +614,7 @@ func TestHealthServiceNodes_PassingFilter(t *testing.T) {
 		t.Fatalf("err: %v", err)
 	}
 
+	t.Run("bc_no_query_value", func(t *testing.T) {
 		req, _ := http.NewRequest("GET", "/v1/health/service/consul?passing", nil)
 		resp := httptest.NewRecorder()
 		obj, err := a.srv.HealthServiceNodes(resp, req)
@@ -626,6 +629,59 @@ func TestHealthServiceNodes_PassingFilter(t *testing.T) {
 		if len(nodes) != 0 {
 			t.Fatalf("bad: %v", obj)
 		}
+	})
+
+	t.Run("passing_true", func(t *testing.T) {
+		req, _ := http.NewRequest("GET", "/v1/health/service/consul?passing=true", nil)
+		resp := httptest.NewRecorder()
+		obj, err := a.srv.HealthServiceNodes(resp, req)
+		if err != nil {
+			t.Fatalf("err: %v", err)
+		}
+
+		assertIndex(t, resp)
+
+		// Should be 0 health check for consul
+		nodes := obj.(structs.CheckServiceNodes)
+		if len(nodes) != 0 {
+			t.Fatalf("bad: %v", obj)
+		}
+	})
+
+	t.Run("passing_false", func(t *testing.T) {
+		req, _ := http.NewRequest("GET", "/v1/health/service/consul?passing=false", nil)
+		resp := httptest.NewRecorder()
+		obj, err := a.srv.HealthServiceNodes(resp, req)
+		if err != nil {
+			t.Fatalf("err: %v", err)
+		}
+
+		assertIndex(t, resp)
+
+		// Should be 0 health check for consul
+		nodes := obj.(structs.CheckServiceNodes)
+		if len(nodes) != 1 {
+			t.Fatalf("bad: %v", obj)
+		}
+	})
+
+	t.Run("passing_bad", func(t *testing.T) {
+		req, _ := http.NewRequest("GET", "/v1/health/service/consul?passing=nope-nope-nope", nil)
+		resp := httptest.NewRecorder()
+		a.srv.HealthServiceNodes(resp, req)
+
+		if code := resp.Code; code != 400 {
+			t.Errorf("bad response code %d, expected %d", code, 400)
+		}
+
+		body, err := ioutil.ReadAll(resp.Body)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if !bytes.Contains(body, []byte("Invalid value for ?passing")) {
+			t.Errorf("bad %s", body)
+		}
+	})
 }
 
 func TestHealthServiceNodes_WanTranslation(t *testing.T) {