From 8c646c68a8eb8c2fb4d57b005b1d0ae0f3ca38a5 Mon Sep 17 00:00:00 2001 From: Benjamin Raskin Date: Wed, 8 May 2019 18:17:30 -0400 Subject: [PATCH 1/8] Fix step parsing --- .../api/v1/handler/prometheus/native/common.go | 9 +++++++-- .../v1/handler/prometheus/native/common_test.go | 16 ++++++++++++++++ 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/src/query/api/v1/handler/prometheus/native/common.go b/src/query/api/v1/handler/prometheus/native/common.go index efae61a513..161d337c8e 100644 --- a/src/query/api/v1/handler/prometheus/native/common.go +++ b/src/query/api/v1/handler/prometheus/native/common.go @@ -76,8 +76,13 @@ func parseDuration(r *http.Request, key string) (time.Duration, error) { } // Try parsing as an integer value specifying seconds, the Prometheus default - if seconds, intErr := strconv.ParseInt(str, 10, 64); intErr == nil { - return time.Duration(seconds) * time.Second, nil + if seconds, intErr := strconv.ParseFloat(str, 64); intErr == nil { + ts := seconds * float64(time.Second) + if ts > float64(math.MaxInt64) || ts < float64(math.MinInt64) { + return 0, fmt.Errorf("cannot parse %q to a valid duration. It overflows int64", str) + } + + return time.Duration(ts), nil } return 0, err diff --git a/src/query/api/v1/handler/prometheus/native/common_test.go b/src/query/api/v1/handler/prometheus/native/common_test.go index 8c34a2ea93..33bdebd4c9 100644 --- a/src/query/api/v1/handler/prometheus/native/common_test.go +++ b/src/query/api/v1/handler/prometheus/native/common_test.go @@ -112,6 +112,14 @@ func TestParseDuration(t *testing.T) { assert.Equal(t, 10*time.Second, v) } +func TestParseFloatDuration(t *testing.T) { + r, err := http.NewRequest(http.MethodGet, "/foo?step=10.00m", nil) + require.NoError(t, err) + v, err := parseDuration(r, stepParam) + require.NoError(t, err) + assert.Equal(t, 10*time.Minute, v) +} + func TestParseDurationParsesIntAsSeconds(t *testing.T) { r, err := http.NewRequest(http.MethodGet, "/foo?step=30", nil) require.NoError(t, err) @@ -120,6 +128,14 @@ func TestParseDurationParsesIntAsSeconds(t *testing.T) { assert.Equal(t, 30*time.Second, v) } +func TestParseDurationParsesFloatAsSeconds(t *testing.T) { + r, err := http.NewRequest(http.MethodGet, "/foo?step=30.00", nil) + require.NoError(t, err) + v, err := parseDuration(r, stepParam) + require.NoError(t, err) + assert.Equal(t, 30*time.Second, v) +} + func TestParseDurationError(t *testing.T) { r, err := http.NewRequest(http.MethodGet, "/foo?step=bar10", nil) require.NoError(t, err) From 748f1043dd8e5c786dc62399ce7c04390ee9e5f5 Mon Sep 17 00:00:00 2001 From: Benjamin Raskin Date: Wed, 8 May 2019 18:24:20 -0400 Subject: [PATCH 2/8] Change comment --- src/query/api/v1/handler/prometheus/native/common.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/query/api/v1/handler/prometheus/native/common.go b/src/query/api/v1/handler/prometheus/native/common.go index 161d337c8e..cd505e7e78 100644 --- a/src/query/api/v1/handler/prometheus/native/common.go +++ b/src/query/api/v1/handler/prometheus/native/common.go @@ -75,8 +75,8 @@ func parseDuration(r *http.Request, key string) (time.Duration, error) { return value, nil } - // Try parsing as an integer value specifying seconds, the Prometheus default - if seconds, intErr := strconv.ParseFloat(str, 64); intErr == nil { + // Try parsing as a float value specifying seconds, the Prometheus default + if seconds, floatErr := strconv.ParseFloat(str, 64); floatErr == nil { ts := seconds * float64(time.Second) if ts > float64(math.MaxInt64) || ts < float64(math.MinInt64) { return 0, fmt.Errorf("cannot parse %q to a valid duration. It overflows int64", str) From 1f76b766cc3e7108bd7e98ab0b9f294f29d8b181 Mon Sep 17 00:00:00 2001 From: Benjamin Raskin Date: Wed, 8 May 2019 18:28:39 -0400 Subject: [PATCH 3/8] Add overflow error test --- src/query/api/v1/handler/prometheus/native/common_test.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/query/api/v1/handler/prometheus/native/common_test.go b/src/query/api/v1/handler/prometheus/native/common_test.go index 33bdebd4c9..a614d9e4bf 100644 --- a/src/query/api/v1/handler/prometheus/native/common_test.go +++ b/src/query/api/v1/handler/prometheus/native/common_test.go @@ -23,6 +23,7 @@ package native import ( "bytes" "encoding/json" + "fmt" "math" "net/http" "net/url" @@ -143,6 +144,13 @@ func TestParseDurationError(t *testing.T) { assert.Error(t, err) } +func TestParseDurationOverflowError(t *testing.T) { + r, err := http.NewRequest(http.MethodGet, fmt.Sprintf("/foo?step=%v", float64(math.MaxInt64)), nil) + require.NoError(t, err) + _, err = parseDuration(r, stepParam) + assert.Error(t, err) +} + func TestParseBlockType(t *testing.T) { logging.InitWithCores(nil) From 15316b317c194e8b5cee06d395187ca83082a1d1 Mon Sep 17 00:00:00 2001 From: Benjamin Raskin Date: Wed, 8 May 2019 18:30:09 -0400 Subject: [PATCH 4/8] String interpolation --- src/query/api/v1/handler/prometheus/native/common_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/query/api/v1/handler/prometheus/native/common_test.go b/src/query/api/v1/handler/prometheus/native/common_test.go index a614d9e4bf..e48446b62f 100644 --- a/src/query/api/v1/handler/prometheus/native/common_test.go +++ b/src/query/api/v1/handler/prometheus/native/common_test.go @@ -145,7 +145,7 @@ func TestParseDurationError(t *testing.T) { } func TestParseDurationOverflowError(t *testing.T) { - r, err := http.NewRequest(http.MethodGet, fmt.Sprintf("/foo?step=%v", float64(math.MaxInt64)), nil) + r, err := http.NewRequest(http.MethodGet, fmt.Sprintf("/foo?step=%f", float64(math.MaxInt64)), nil) require.NoError(t, err) _, err = parseDuration(r, stepParam) assert.Error(t, err) From ef0e3fb9ad4073f423a3e6c88df3b1eb9bad86ed Mon Sep 17 00:00:00 2001 From: Benjamin Raskin Date: Wed, 8 May 2019 18:58:21 -0400 Subject: [PATCH 5/8] Address comments --- src/query/api/v1/handler/prometheus/native/common.go | 2 +- src/query/api/v1/handler/prometheus/native/common_test.go | 8 +++++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/query/api/v1/handler/prometheus/native/common.go b/src/query/api/v1/handler/prometheus/native/common.go index cd505e7e78..0831fea5c0 100644 --- a/src/query/api/v1/handler/prometheus/native/common.go +++ b/src/query/api/v1/handler/prometheus/native/common.go @@ -79,7 +79,7 @@ func parseDuration(r *http.Request, key string) (time.Duration, error) { if seconds, floatErr := strconv.ParseFloat(str, 64); floatErr == nil { ts := seconds * float64(time.Second) if ts > float64(math.MaxInt64) || ts < float64(math.MinInt64) { - return 0, fmt.Errorf("cannot parse %q to a valid duration. It overflows int64", str) + return 0, fmt.Errorf("cannot parse %s to a valid duration: int64 overflow", str) } return time.Duration(ts), nil diff --git a/src/query/api/v1/handler/prometheus/native/common_test.go b/src/query/api/v1/handler/prometheus/native/common_test.go index e48446b62f..74a842cf1a 100644 --- a/src/query/api/v1/handler/prometheus/native/common_test.go +++ b/src/query/api/v1/handler/prometheus/native/common_test.go @@ -114,10 +114,16 @@ func TestParseDuration(t *testing.T) { } func TestParseFloatDuration(t *testing.T) { - r, err := http.NewRequest(http.MethodGet, "/foo?step=10.00m", nil) + r, err := http.NewRequest(http.MethodGet, "/foo?step=10.85m", nil) require.NoError(t, err) v, err := parseDuration(r, stepParam) require.NoError(t, err) + assert.Equal(t, 10*time.Minute+51*time.Second, v) + + r, err = http.NewRequest(http.MethodGet, "/foo?step=10.00m", nil) + require.NoError(t, err) + v, err = parseDuration(r, stepParam) + require.NoError(t, err) assert.Equal(t, 10*time.Minute, v) } From 2bb2954eec001a6cbd0c41bfc64b8ac821f054c9 Mon Sep 17 00:00:00 2001 From: Benjamin Raskin Date: Wed, 8 May 2019 19:03:12 -0400 Subject: [PATCH 6/8] Store as const --- src/query/api/v1/handler/prometheus/native/common.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/query/api/v1/handler/prometheus/native/common.go b/src/query/api/v1/handler/prometheus/native/common.go index 0831fea5c0..b66c02226f 100644 --- a/src/query/api/v1/handler/prometheus/native/common.go +++ b/src/query/api/v1/handler/prometheus/native/common.go @@ -53,6 +53,9 @@ const ( blockTypeParam = "block-type" formatErrStr = "error parsing param: %s, error: %v" + + maxInt64 = float64(math.MaxInt64) + minInt64 = float64(math.MinInt64) ) func parseTime(r *http.Request, key string) (time.Time, error) { @@ -78,7 +81,7 @@ func parseDuration(r *http.Request, key string) (time.Duration, error) { // Try parsing as a float value specifying seconds, the Prometheus default if seconds, floatErr := strconv.ParseFloat(str, 64); floatErr == nil { ts := seconds * float64(time.Second) - if ts > float64(math.MaxInt64) || ts < float64(math.MinInt64) { + if ts > maxInt64 || ts < minInt64 { return 0, fmt.Errorf("cannot parse %s to a valid duration: int64 overflow", str) } From 87b4d3dc323a166f8b3f3d235a3fbd4eb6203318 Mon Sep 17 00:00:00 2001 From: Benjamin Raskin Date: Thu, 9 May 2019 09:40:20 -0400 Subject: [PATCH 7/8] Check negative step --- .../api/v1/handler/prometheus/native/common.go | 4 ++++ .../v1/handler/prometheus/native/common_test.go | 15 +++++++++++++-- 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/src/query/api/v1/handler/prometheus/native/common.go b/src/query/api/v1/handler/prometheus/native/common.go index b66c02226f..debc595694 100644 --- a/src/query/api/v1/handler/prometheus/native/common.go +++ b/src/query/api/v1/handler/prometheus/native/common.go @@ -119,6 +119,10 @@ func parseParams(r *http.Request, timeoutOpts *prometheus.TimeoutOpts) (models.R if err != nil { return params, xhttp.NewParseError(fmt.Errorf(formatErrStr, stepParam, err), http.StatusBadRequest) } + if step <= 0 { + err := fmt.Errorf("expected postive step, instead got: %d", step) + return params, xhttp.NewParseError(fmt.Errorf(formatErrStr, stepParam, err), http.StatusBadRequest) + } params.Step = step query, err := parseQuery(r) diff --git a/src/query/api/v1/handler/prometheus/native/common_test.go b/src/query/api/v1/handler/prometheus/native/common_test.go index 74a842cf1a..0781318b01 100644 --- a/src/query/api/v1/handler/prometheus/native/common_test.go +++ b/src/query/api/v1/handler/prometheus/native/common_test.go @@ -105,6 +105,17 @@ func TestInvalidTarget(t *testing.T) { require.Equal(t, err.Code(), http.StatusBadRequest) } +func TestInvalidStep(t *testing.T) { + req, _ := http.NewRequest("GET", PromReadURL, nil) + vals := defaultParams() + vals.Del(stepParam) + vals.Add(stepParam, "-10.50s") + req.URL.RawQuery = vals.Encode() + _, err := parseParams(req, timeoutOpts) + require.NotNil(t, err, "unable to parse request") + require.Equal(t, err.Code(), http.StatusBadRequest) +} + func TestParseDuration(t *testing.T) { r, err := http.NewRequest(http.MethodGet, "/foo?step=10s", nil) require.NoError(t, err) @@ -114,11 +125,11 @@ func TestParseDuration(t *testing.T) { } func TestParseFloatDuration(t *testing.T) { - r, err := http.NewRequest(http.MethodGet, "/foo?step=10.85m", nil) + r, err := http.NewRequest(http.MethodGet, "/foo?step=10.50m", nil) require.NoError(t, err) v, err := parseDuration(r, stepParam) require.NoError(t, err) - assert.Equal(t, 10*time.Minute+51*time.Second, v) + assert.Equal(t, 10*time.Minute+30*time.Second, v) r, err = http.NewRequest(http.MethodGet, "/foo?step=10.00m", nil) require.NoError(t, err) From e86a4b765d48397f920efd30146af3c8f673fc51 Mon Sep 17 00:00:00 2001 From: Benjamin Raskin Date: Thu, 9 May 2019 09:44:44 -0400 Subject: [PATCH 8/8] Update error --- src/query/api/v1/handler/prometheus/native/common.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/query/api/v1/handler/prometheus/native/common.go b/src/query/api/v1/handler/prometheus/native/common.go index debc595694..d1827e0933 100644 --- a/src/query/api/v1/handler/prometheus/native/common.go +++ b/src/query/api/v1/handler/prometheus/native/common.go @@ -120,7 +120,7 @@ func parseParams(r *http.Request, timeoutOpts *prometheus.TimeoutOpts) (models.R return params, xhttp.NewParseError(fmt.Errorf(formatErrStr, stepParam, err), http.StatusBadRequest) } if step <= 0 { - err := fmt.Errorf("expected postive step, instead got: %d", step) + err := fmt.Errorf("expected postive step size, instead got: %d", step) return params, xhttp.NewParseError(fmt.Errorf(formatErrStr, stepParam, err), http.StatusBadRequest) } params.Step = step