From 389d3c3b9ac6865dcd3cb4cb0ab4b134c31c049f Mon Sep 17 00:00:00 2001 From: beorn7 Date: Wed, 13 May 2020 23:35:31 +0200 Subject: [PATCH] Properly handle empty job and label values An empty job name was always an error, but it was previously only detected when pushing to the PGW and receiving an error. With this commit, the error is detected before pushing. An empty label value should have been OK but was encoded in a way that couldn't be pushed to the PGW. Cf. https://github.com/prometheus/pushgateway/issues/344 . This commit changes the creation of the path in the URL so that it works with empty label values. Signed-off-by: beorn7 --- prometheus/push/push.go | 29 ++++++++++++++++++++--------- prometheus/push/push_test.go | 31 ++++++++++++++++++++++++++++++- 2 files changed, 50 insertions(+), 10 deletions(-) diff --git a/prometheus/push/push.go b/prometheus/push/push.go index 77ce9c837..c1a6cb99f 100644 --- a/prometheus/push/push.go +++ b/prometheus/push/push.go @@ -37,6 +37,7 @@ package push import ( "bytes" "encoding/base64" + "errors" "fmt" "io/ioutil" "net/http" @@ -56,6 +57,8 @@ const ( base64Suffix = "@base64" ) +var errJobEmpty = errors.New("job name is empty") + // HTTPDoer is an interface for the one method of http.Client that is used by Pusher type HTTPDoer interface { Do(*http.Request) (*http.Response, error) @@ -80,14 +83,17 @@ type Pusher struct { } // New creates a new Pusher to push to the provided URL with the provided job -// name. You can use just host:port or ip:port as url, in which case “http://” -// is added automatically. Alternatively, include the schema in the -// URL. However, do not include the “/metrics/jobs/…” part. +// name (which must not be empty). You can use just host:port or ip:port as url, +// in which case “http://” is added automatically. Alternatively, include the +// schema in the URL. However, do not include the “/metrics/jobs/…” part. func New(url, job string) *Pusher { var ( reg = prometheus.NewRegistry() err error ) + if job == "" { + err = errJobEmpty + } if !strings.Contains(url, "://") { url = "http://" + url } @@ -267,7 +273,7 @@ func (p *Pusher) push(method string) error { return err } defer resp.Body.Close() - // Pushgateway 0.10+ responds with StatusOK, earlier versions with StatusAccepted. + // Depending on version and configuration of the PGW, StatusOK or StatusAccepted may be returned. if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusAccepted { body, _ := ioutil.ReadAll(resp.Body) // Ignore any further error as this is for an error message only. return fmt.Errorf("unexpected status code %d while pushing to %s: %s", resp.StatusCode, p.fullURL(), body) @@ -278,9 +284,11 @@ func (p *Pusher) push(method string) error { // fullURL assembles the URL used to push/delete metrics and returns it as a // string. The job name and any grouping label values containing a '/' will // trigger a base64 encoding of the affected component and proper suffixing of -// the preceding component. If the component does not contain a '/' but other -// special character, the usual url.QueryEscape is used for compatibility with -// older versions of the Pushgateway and for better readability. +// the preceding component. Similarly, an empty grouping label value will be +// encoded as base64 just with a single `=` padding character (to avoid an empty +// path component). If the component does not contain a '/' but other special +// characters, the usual url.QueryEscape is used for compatibility with older +// versions of the Pushgateway and for better readability. func (p *Pusher) fullURL() string { urlComponents := []string{} if encodedJob, base64 := encodeComponent(p.job); base64 { @@ -299,9 +307,12 @@ func (p *Pusher) fullURL() string { } // encodeComponent encodes the provided string with base64.RawURLEncoding in -// case it contains '/'. If not, it uses url.QueryEscape instead. It returns -// true in the former case. +// case it contains '/' and as "=" in case it is empty. If neither is the case, +// it uses url.QueryEscape instead. It returns true in the former two cases. func encodeComponent(s string) (string, bool) { + if s == "" { + return "=", true + } if strings.Contains(s, "/") { return base64.RawURLEncoding.EncodeToString([]byte(s)), true } diff --git a/prometheus/push/push_test.go b/prometheus/push/push_test.go index 1fabe39c6..99155b0ed 100644 --- a/prometheus/push/push_test.go +++ b/prometheus/push/push_test.go @@ -176,6 +176,36 @@ func TestPush(t *testing.T) { t.Error("unexpected path:", lastPath) } + // Empty label value triggers special base64 encoding. + if err := New(pgwOK.URL, "testjob"). + Grouping("empty", ""). + Collector(metric1). + Collector(metric2). + Push(); err != nil { + t.Fatal(err) + } + if lastMethod != http.MethodPut { + t.Errorf("got method %q for Push, want %q", lastMethod, http.MethodPut) + } + if !bytes.Equal(lastBody, wantBody) { + t.Errorf("got body %v, want %v", lastBody, wantBody) + } + if lastPath != "/metrics/job/testjob/empty@base64/=" { + t.Error("unexpected path:", lastPath) + } + + // Empty job name results in error. + if err := New(pgwErr.URL, ""). + Collector(metric1). + Collector(metric2). + Push(); err == nil { + t.Error("push with empty job succeded") + } else { + if got, want := err, errJobEmpty; got != want { + t.Errorf("got error %q, want %q", got, want) + } + } + // Push some Collectors with a broken PGW. if err := New(pgwErr.URL, "testjob"). Collector(metric1). @@ -251,5 +281,4 @@ func TestPush(t *testing.T) { if lastPath != "/metrics/job/testjob/a/x/b/y" && lastPath != "/metrics/job/testjob/b/y/a/x" { t.Error("unexpected path:", lastPath) } - }