From 532f8d1435ed47c462b9ff00c60b3cb5aa41d0ec Mon Sep 17 00:00:00 2001
From: Seth Vargo <seth@sethvargo.com>
Date: Fri, 9 Jun 2017 14:36:00 -0400
Subject: [PATCH] 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      | 18 +++++-
 command/agent/health_endpoint_test.go | 84 ++++++++++++++++++++++-----
 2 files changed, 87 insertions(+), 15 deletions(-)

diff --git a/command/agent/health_endpoint.go b/command/agent/health_endpoint.go
index 5b0bac4c49..29e6bff113 100644
--- a/command/agent/health_endpoint.go
+++ b/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 {
-		out.Nodes = filterNonPassing(out.Nodes)
+		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.
diff --git a/command/agent/health_endpoint_test.go b/command/agent/health_endpoint_test.go
index d017f1f772..46ab1d1730 100644
--- a/command/agent/health_endpoint_test.go
+++ b/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,20 +614,74 @@ func TestHealthServiceNodes_PassingFilter(t *testing.T) {
 		t.Fatalf("err: %v", err)
 	}
 
-	req, _ := http.NewRequest("GET", "/v1/health/service/consul?passing", nil)
-	resp := httptest.NewRecorder()
-	obj, err := a.srv.HealthServiceNodes(resp, req)
-	if err != nil {
-		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)
+		if err != nil {
+			t.Fatalf("err: %v", err)
+		}
 
-	assertIndex(t, resp)
+		assertIndex(t, resp)
 
-	// Should be 0 health check for consul
-	nodes := obj.(structs.CheckServiceNodes)
-	if len(nodes) != 0 {
-		t.Fatalf("bad: %v", obj)
-	}
+		// Should be 0 health check for consul
+		nodes := obj.(structs.CheckServiceNodes)
+		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) {