From 532f8d1435ed47c462b9ff00c60b3cb5aa41d0ec Mon Sep 17 00:00:00 2001 From: Seth Vargo Date: Fri, 9 Jun 2017 14:36:00 -0400 Subject: [PATCH 1/3] 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) { From ee1b5d50247262232a51ef8e0b9f4c0109037658 Mon Sep 17 00:00:00 2001 From: Seth Vargo Date: Fri, 9 Jun 2017 14:51:34 -0400 Subject: [PATCH 2/3] Update comment --- command/agent/health_endpoint_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/command/agent/health_endpoint_test.go b/command/agent/health_endpoint_test.go index 46ab1d1730..3dde89ad6c 100644 --- a/command/agent/health_endpoint_test.go +++ b/command/agent/health_endpoint_test.go @@ -658,7 +658,8 @@ func TestHealthServiceNodes_PassingFilter(t *testing.T) { assertIndex(t, resp) - // Should be 0 health check for consul + // Should be 1 consul, it's unhealthy, but we specifically asked for + // everything. nodes := obj.(structs.CheckServiceNodes) if len(nodes) != 1 { t.Fatalf("bad: %v", obj) From 89f16984f95d25e6ff9393c736309477db482b21 Mon Sep 17 00:00:00 2001 From: Seth Vargo Date: Fri, 9 Jun 2017 14:55:04 -0400 Subject: [PATCH 3/3] Simplify --- command/agent/health_endpoint.go | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/command/agent/health_endpoint.go b/command/agent/health_endpoint.go index 29e6bff113..2e9765a9f5 100644 --- a/command/agent/health_endpoint.go +++ b/command/agent/health_endpoint.go @@ -152,18 +152,21 @@ func (s *HTTPServer) HealthServiceNodes(resp http.ResponseWriter, req *http.Requ val := params.Get(api.HealthPassing) // Backwards-compat to allow users to specify ?passing without a value. This // should be removed in Consul 0.10. + var filter bool if val == "" { - out.Nodes = filterNonPassing(out.Nodes) + filter = true } else { - filter, err := strconv.ParseBool(val) + var err error + 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) - } + } + + if filter { + out.Nodes = filterNonPassing(out.Nodes) } }