From 53cd1b487157526c55f3825616a4aca45559b484 Mon Sep 17 00:00:00 2001 From: Dao Thanh Tung Date: Mon, 2 Jan 2023 03:04:14 +0800 Subject: [PATCH] fix: `stale` querystring parameter value as boolean (#15605) * Add changes to make stale querystring param boolean Signed-off-by: dttung2905 * Make error message more consistent Signed-off-by: dttung2905 * Changes from code review + Adding CHANGELOG file Signed-off-by: dttung2905 * Changes from code review to use github.com/shoenig/test package Signed-off-by: dttung2905 * Change must.Nil() to must.NoError() Signed-off-by: dttung2905 * Minor fix on the import order Signed-off-by: dttung2905 * Fix existing code format too Signed-off-by: dttung2905 * Minor changes addressing code review feedbacks Signed-off-by: dttung2905 * swap must.EqOp() order of param provided Signed-off-by: dttung2905 Signed-off-by: dttung2905 --- .changelog/15605.txt | 3 +++ command/agent/http.go | 14 +++++++++---- command/agent/http_test.go | 43 +++++++++++++++++++++++--------------- 3 files changed, 39 insertions(+), 21 deletions(-) create mode 100644 .changelog/15605.txt diff --git a/.changelog/15605.txt b/.changelog/15605.txt new file mode 100644 index 000000000..b89069399 --- /dev/null +++ b/.changelog/15605.txt @@ -0,0 +1,3 @@ +```release-note:bug +api: Fix stale querystring parameter value as boolean +``` diff --git a/command/agent/http.go b/command/agent/http.go index 76a754e49..4ef6e5e13 100644 --- a/command/agent/http.go +++ b/command/agent/http.go @@ -783,10 +783,16 @@ func parseWait(resp http.ResponseWriter, req *http.Request, b *structs.QueryOpti } // parseConsistency is used to parse the ?stale query params. -func parseConsistency(req *http.Request, b *structs.QueryOptions) { +func parseConsistency(resp http.ResponseWriter, req *http.Request, b *structs.QueryOptions) { query := req.URL.Query() - if _, ok := query["stale"]; ok { - b.AllowStale = true + if staleVal, ok := query["stale"]; ok { + staleQuery, err := strconv.ParseBool(staleVal[0]) + if err != nil { + resp.WriteHeader(400) + resp.Write([]byte(fmt.Sprintf("Expect `true` or `false` for `stale` query string parameter, got %s", staleVal[0]))) + } + + b.AllowStale = staleQuery || staleVal[0] == "" } } @@ -884,7 +890,7 @@ func (s *HTTPServer) parseToken(req *http.Request, token *string) { func (s *HTTPServer) parse(resp http.ResponseWriter, req *http.Request, r *string, b *structs.QueryOptions) bool { s.parseRegion(req, r) s.parseToken(req, &b.AuthToken) - parseConsistency(req, b) + parseConsistency(resp, req, b) parsePrefix(req, b) parseNamespace(req, &b.Namespace) parsePagination(req, b) diff --git a/command/agent/http_test.go b/command/agent/http_test.go index 496232a90..05fe2929b 100644 --- a/command/agent/http_test.go +++ b/command/agent/http_test.go @@ -29,6 +29,7 @@ import ( "github.com/hashicorp/nomad/nomad/structs" "github.com/hashicorp/nomad/nomad/structs/config" "github.com/hashicorp/nomad/testutil" + "github.com/shoenig/test/must" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -476,29 +477,37 @@ func TestParseWait_InvalidIndex(t *testing.T) { func TestParseConsistency(t *testing.T) { ci.Parallel(t) var b structs.QueryOptions + var resp *httptest.ResponseRecorder - req, err := http.NewRequest("GET", - "/v1/catalog/nodes?stale", nil) - if err != nil { - t.Fatalf("err: %v", err) + testCases := [2]string{"/v1/catalog/nodes?stale", "/v1/catalog/nodes?stale=true"} + for _, urlPath := range testCases { + req, err := http.NewRequest("GET", urlPath, nil) + must.NoError(t, err) + resp = httptest.NewRecorder() + parseConsistency(resp, req, &b) + must.True(t, b.AllowStale) } - parseConsistency(req, &b) - if !b.AllowStale { - t.Fatalf("Bad: %v", b) - } + req, err := http.NewRequest("GET", "/v1/catalog/nodes?stale=false", nil) + must.NoError(t, err) + resp = httptest.NewRecorder() + parseConsistency(resp, req, &b) + must.False(t, b.AllowStale) + + req, err = http.NewRequest("GET", "/v1/catalog/nodes?stale=random", nil) + must.NoError(t, err) + resp = httptest.NewRecorder() + parseConsistency(resp, req, &b) + must.False(t, b.AllowStale) + must.EqOp(t, 400, resp.Code) b = structs.QueryOptions{} - req, err = http.NewRequest("GET", - "/v1/catalog/nodes?consistent", nil) - if err != nil { - t.Fatalf("err: %v", err) - } + req, err = http.NewRequest("GET", "/v1/catalog/nodes?consistent", nil) + must.NoError(t, err) - parseConsistency(req, &b) - if b.AllowStale { - t.Fatalf("Bad: %v", b) - } + resp = httptest.NewRecorder() + parseConsistency(resp, req, &b) + must.False(t, b.AllowStale) } func TestParseRegion(t *testing.T) {