From 810dd74fb99608b568c402d42786803aa1937afc Mon Sep 17 00:00:00 2001 From: Aliaksandr Valialkin Date: Sat, 7 May 2022 00:02:54 +0300 Subject: [PATCH] lib/promscrape: properly implement ScrapeConfig.clone() Previously ScrapeConfig.clone() was improperly copying promauth.Secret fields - their contents was replaced with `` value. This led to inability to use passwords and secrets in `-promscrape.config` file. The bug has been introduced in v1.77.0 in the commit 67b10896d2579716bbf80d171890f1d9d482c9c5 Updates https://github.com/VictoriaMetrics/VictoriaMetrics/issues/2551 --- docs/CHANGELOG.md | 5 +- lib/promauth/config.go | 8 +-- lib/promrelabel/config.go | 12 ++-- lib/promrelabel/config_test.go | 6 +- lib/promrelabel/if_expression.go | 15 ++++ lib/promscrape/client.go | 4 +- lib/promscrape/config.go | 17 ++--- lib/promscrape/config_test.go | 69 +++++++++++++++++++ lib/promscrape/discovery/gce/api.go | 2 +- lib/promscrape/discovery/gce/gce.go | 6 +- lib/promscrape/discovery/gce/gce_test.go | 2 +- .../discovery/kubernetes/api_watcher.go | 2 +- lib/promscrape/discoveryutils/client.go | 2 +- lib/promutils/duration.go | 10 +-- lib/proxy/proxy.go | 32 ++++----- 15 files changed, 139 insertions(+), 53 deletions(-) diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 15de03e7a..faef8f7bb 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -19,11 +19,12 @@ The following tip changes can be tested by building VictoriaMetrics components f * FEATURE: [vmagent](https://docs.victoriametrics.com/vmagent.html): expose `vmagent_remotewrite_global_rows_pushed_before_relabel_total` and `vmagent_remotewrite_rows_pushed_after_relabel_total` metrics at `http://vmagent:8429/metrics`, which can be used for monitoring the rate of rows (aka samples) pushed to remote storage before and after the relabeling via `-remoteWrite.relabelConfig` and `-remoteWrite.urlRelabelConfig`. See [relabeling docs](https://docs.victoriametrics.com/vmagent.html#relabeling) for details. * FEATURE: [vmctl](https://docs.victoriametrics.com/vmctl.html): add ability to skip `db` label during InfluxDB data import when `influx-skip-database-label` option is used. See [this pull request](https://github.com/VictoriaMetrics/VictoriaMetrics/pull/2544). Thanks to @mback2k . +* BUGFIX: [vmagent](https://docs.victoriametrics.com/vmagent.html): properly process passwords and secrets specified in the file pointed by `-promscrape.config` command-line flag. All the passwords and secrets were mistakenly replaced with `` string in `v1.77.0`. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/2551). +* BUGFIX: [vmagent](https://docs.victoriametrics.com/vmagent.html): rename `vmagent_remote_write_rate_limit_reached_total` metric to `vmagent_remotewrite_rate_limit_reached_total`, so its name is consistent with the rest of `vmagent_remotewrite_` metrics. +* BUGFIX: [vmagent](https://docs.victoriametrics.com/vmagent.html): rename `promscrape_stale_samples_created_total` metric to `vm_promscrape_stale_samples_created_total`, so its name is consistent with the rest of `vm_promscrape_` metrics. * BUGFIX: [vmctl](https://docs.victoriametrics.com/vmctl.html): properly import InfluxDB measurements if they contain `db` tag. Previously this could result in incomplete import of measurmenet tags. See [this pull request](https://github.com/VictoriaMetrics/VictoriaMetrics/pull/2536). Thanks to @mback2k for the bugfix. * BUGFIX: [vmui](https://docs.victoriametrics.com/#vmui): do not reset the selected relative time range when entering new query. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/2402#issuecomment-1115817302). * BUGFIX: [vmui](https://docs.victoriametrics.com/#vmui): properly show the graph when clicking on `Prometheus` link in Grafana graph editor. Previously the graph wasn't shown until clicking on the `Graph` tab. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/2402#issuecomment-1115830648). -* BUGFIX: [vmagent](https://docs.victoriametrics.com/vmagent.html): rename `vmagent_remote_write_rate_limit_reached_total` metric to `vmagent_remotewrite_rate_limit_reached_total`, so its name is consistent with the rest of `vmagent_remotewrite_` metrics. -* BUGFIX: [vmagent](https://docs.victoriametrics.com/vmagent.html): rename `promscrape_stale_samples_created_total` metric to `vm_promscrape_stale_samples_created_total`, so its name is consistent with the rest of `vm_promscrape_` metrics. * BUGFIX: [vmbackup](https://docs.victoriametrics.com/vmbackup.html): disallow writing backups to `-storageDataPath` directory, since this directory is managed solely by VictoriaMetrics or `vmstorage`. Other apps shouldn't write into this directory. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/2503). diff --git a/lib/promauth/config.go b/lib/promauth/config.go index 16528b1df..bcacf9d11 100644 --- a/lib/promauth/config.go +++ b/lib/promauth/config.go @@ -26,7 +26,7 @@ import ( // This is needed for hiding secret strings in /config page output. // See https://github.com/VictoriaMetrics/VictoriaMetrics/issues/1764 type Secret struct { - s string + S string } // NewSecret returns new secret for s. @@ -35,7 +35,7 @@ func NewSecret(s string) *Secret { return nil } return &Secret{ - s: s, + S: s, } } @@ -52,7 +52,7 @@ func (s *Secret) UnmarshalYAML(f func(interface{}) error) error { if err := f(&secret); err != nil { return fmt.Errorf("cannot parse secret: %w", err) } - s.s = secret + s.S = secret return nil } @@ -61,7 +61,7 @@ func (s *Secret) String() string { if s == nil { return "" } - return s.s + return s.S } // TLSConfig represents TLS config. diff --git a/lib/promrelabel/config.go b/lib/promrelabel/config.go index ce088c1f2..094904aa8 100644 --- a/lib/promrelabel/config.go +++ b/lib/promrelabel/config.go @@ -38,7 +38,7 @@ type RelabelConfig struct { // // regex: "foo|bar" type MultiLineRegex struct { - s string + S string } // UnmarshalYAML unmarshals mlr from YAML passed to f. @@ -51,7 +51,7 @@ func (mlr *MultiLineRegex) UnmarshalYAML(f func(interface{}) error) error { if err != nil { return err } - mlr.s = s + mlr.S = s return nil } @@ -88,7 +88,7 @@ func stringValue(v interface{}) (string, error) { // MarshalYAML marshals mlr to YAML. func (mlr *MultiLineRegex) MarshalYAML() (interface{}, error) { - a := strings.Split(mlr.s, "|") + a := strings.Split(mlr.S, "|") if len(a) == 1 { return a[0], nil } @@ -179,7 +179,7 @@ func parseRelabelConfig(rc *RelabelConfig) (*parsedRelabelConfig, error) { regexCompiled := defaultRegexForRelabelConfig regexOriginalCompiled := defaultOriginalRegexForRelabelConfig if rc.Regex != nil { - regex := rc.Regex.s + regex := rc.Regex.S regexOrig := regex if rc.Action != "replace_all" && rc.Action != "labelmap_all" { regex = "^(?:" + regex + ")$" @@ -243,7 +243,7 @@ func parseRelabelConfig(rc *RelabelConfig) (*parsedRelabelConfig, error) { return nil, fmt.Errorf("unexpected `modulus` for `action=hashmod`: %d; must be greater than 0", modulus) } case "keep_metrics": - if (rc.Regex == nil || rc.Regex.s == "") && rc.If == nil { + if (rc.Regex == nil || rc.Regex.S == "") && rc.If == nil { return nil, fmt.Errorf("`regex` must be non-empty for `action=keep_metrics`") } if len(sourceLabels) > 0 { @@ -252,7 +252,7 @@ func parseRelabelConfig(rc *RelabelConfig) (*parsedRelabelConfig, error) { sourceLabels = []string{"__name__"} action = "keep" case "drop_metrics": - if (rc.Regex == nil || rc.Regex.s == "") && rc.If == nil { + if (rc.Regex == nil || rc.Regex.S == "") && rc.If == nil { return nil, fmt.Errorf("`regex` must be non-empty for `action=drop_metrics`") } if len(sourceLabels) > 0 { diff --git a/lib/promrelabel/config_test.go b/lib/promrelabel/config_test.go index 71a16e822..f460a2183 100644 --- a/lib/promrelabel/config_test.go +++ b/lib/promrelabel/config_test.go @@ -128,7 +128,7 @@ func TestParseRelabelConfigsFailure(t *testing.T) { SourceLabels: []string{"aaa"}, TargetLabel: "xxx", Regex: &MultiLineRegex{ - s: "foo[bar", + S: "foo[bar", }, }, }) @@ -248,7 +248,7 @@ func TestParseRelabelConfigsFailure(t *testing.T) { Action: "drop_metrics", SourceLabels: []string{"foo"}, Regex: &MultiLineRegex{ - s: "bar", + S: "bar", }, }, }) @@ -266,7 +266,7 @@ func TestParseRelabelConfigsFailure(t *testing.T) { Action: "keep_metrics", SourceLabels: []string{"foo"}, Regex: &MultiLineRegex{ - s: "bar", + S: "bar", }, }, }) diff --git a/lib/promrelabel/if_expression.go b/lib/promrelabel/if_expression.go index dd610f153..334713f8d 100644 --- a/lib/promrelabel/if_expression.go +++ b/lib/promrelabel/if_expression.go @@ -1,6 +1,7 @@ package promrelabel import ( + "encoding/json" "fmt" "regexp" @@ -36,6 +37,20 @@ func (ie *IfExpression) Parse(s string) error { return nil } +// UnmarshalJSON unmarshals ie from JSON data. +func (ie *IfExpression) UnmarshalJSON(data []byte) error { + var s string + if err := json.Unmarshal(data, &s); err != nil { + return err + } + return ie.Parse(s) +} + +// MarshalJSON marshals ie to JSON. +func (ie *IfExpression) MarshalJSON() ([]byte, error) { + return json.Marshal(ie.s) +} + // UnmarshalYAML unmarshals ie from YAML passed to f. func (ie *IfExpression) UnmarshalYAML(f func(interface{}) error) error { var s string diff --git a/lib/promscrape/client.go b/lib/promscrape/client.go index de1fe09c3..1ad31af1d 100644 --- a/lib/promscrape/client.go +++ b/lib/promscrape/client.go @@ -71,7 +71,7 @@ func newClient(sw *ScrapeWork) *client { // Send full sw.ScrapeURL in requests to a proxy host for non-TLS scrape targets // like net/http package from Go does. // See https://en.wikipedia.org/wiki/Proxy_server#Web_proxy_servers - pu := proxyURL.URL() + pu := proxyURL.GetURL() host = pu.Host requestURI = sw.ScrapeURL isTLS = pu.Scheme == "https" @@ -110,7 +110,7 @@ func newClient(sw *ScrapeWork) *client { } var sc *http.Client var proxyURLFunc func(*http.Request) (*url.URL, error) - if pu := sw.ProxyURL.URL(); pu != nil { + if pu := sw.ProxyURL.GetURL(); pu != nil { proxyURLFunc = http.ProxyURL(pu) } sc = &http.Client{ diff --git a/lib/promscrape/config.go b/lib/promscrape/config.go index 4b7b8cf53..55385a64e 100644 --- a/lib/promscrape/config.go +++ b/lib/promscrape/config.go @@ -1,6 +1,7 @@ package promscrape import ( + "encoding/json" "flag" "fmt" "net/url" @@ -161,17 +162,17 @@ func (cfg *Config) mustRestart(prevCfg *Config) { } func areEqualScrapeConfigs(a, b *ScrapeConfig) bool { - sa := a.marshal() - sb := b.marshal() + sa := a.marshalJSON() + sb := b.marshalJSON() return string(sa) == string(sb) } -func (sc *ScrapeConfig) unmarshal(data []byte) error { - return yaml.UnmarshalStrict(data, sc) +func (sc *ScrapeConfig) unmarshalJSON(data []byte) error { + return json.Unmarshal(data, sc) } -func (sc *ScrapeConfig) marshal() []byte { - data, err := yaml.Marshal(sc) +func (sc *ScrapeConfig) marshalJSON() []byte { + data, err := json.Marshal(sc) if err != nil { logger.Panicf("BUG: cannot marshal ScrapeConfig: %s", err) } @@ -430,9 +431,9 @@ func (cfg *Config) parseData(data []byte, path string) ([]byte, error) { } func (sc *ScrapeConfig) clone() *ScrapeConfig { - data := sc.marshal() + data := sc.marshalJSON() var scCopy ScrapeConfig - if err := scCopy.unmarshal(data); err != nil { + if err := scCopy.unmarshalJSON(data); err != nil { logger.Panicf("BUG: cannot unmarshal scrape config: %s", err) } return &scCopy diff --git a/lib/promscrape/config_test.go b/lib/promscrape/config_test.go index b52d02d23..6659e5376 100644 --- a/lib/promscrape/config_test.go +++ b/lib/promscrape/config_test.go @@ -11,6 +11,9 @@ import ( "github.com/VictoriaMetrics/VictoriaMetrics/lib/promauth" "github.com/VictoriaMetrics/VictoriaMetrics/lib/prompbmarshal" + "github.com/VictoriaMetrics/VictoriaMetrics/lib/promrelabel" + "github.com/VictoriaMetrics/VictoriaMetrics/lib/promscrape/discovery/gce" + "github.com/VictoriaMetrics/VictoriaMetrics/lib/promutils" "github.com/VictoriaMetrics/VictoriaMetrics/lib/proxy" ) @@ -1853,3 +1856,69 @@ func equalStaticConfigForScrapeWorks(a, b []*ScrapeWork) bool { } return true } + +func TestScrapeConfigClone(t *testing.T) { + f := func(sc *ScrapeConfig) { + t.Helper() + scCopy := sc.clone() + if !reflect.DeepEqual(sc, scCopy) { + t.Fatalf("unexpected result after unmarshalJSON() for JSON:\n%s", sc.marshalJSON()) + } + } + + f(&ScrapeConfig{}) + + bFalse := false + var ie promrelabel.IfExpression + if err := ie.Parse(`{foo=~"bar",baz!="z"}`); err != nil { + t.Fatalf("unexpected error: %s", err) + } + f(&ScrapeConfig{ + JobName: "foo", + ScrapeInterval: promutils.NewDuration(time.Second * 47), + HonorLabels: true, + HonorTimestamps: &bFalse, + Params: map[string][]string{ + "foo": {"bar", "baz"}, + }, + HTTPClientConfig: promauth.HTTPClientConfig{ + Authorization: &promauth.Authorization{ + Credentials: promauth.NewSecret("foo"), + }, + BasicAuth: &promauth.BasicAuthConfig{ + Username: "user_x", + Password: promauth.NewSecret("pass_x"), + }, + BearerToken: promauth.NewSecret("zx"), + OAuth2: &promauth.OAuth2Config{ + ClientSecret: promauth.NewSecret("aa"), + Scopes: []string{"foo", "bar"}, + TLSConfig: &promauth.TLSConfig{ + CertFile: "foo", + }, + }, + TLSConfig: &promauth.TLSConfig{ + KeyFile: "aaa", + }, + }, + ProxyURL: proxy.MustNewURL("https://foo.bar:3434/assdf/dsfd?sdf=dsf"), + RelabelConfigs: []promrelabel.RelabelConfig{{ + SourceLabels: []string{"foo", "aaa"}, + Regex: &promrelabel.MultiLineRegex{ + S: "foo\nbar", + }, + If: &ie, + }}, + SampleLimit: 10, + GCESDConfigs: []gce.SDConfig{{ + Project: "foo", + Zone: gce.ZoneYAML{ + Zones: []string{"a", "b"}, + }, + }}, + StreamParse: true, + ProxyClientConfig: promauth.ProxyClientConfig{ + BearerTokenFile: "foo", + }, + }) +} diff --git a/lib/promscrape/discovery/gce/api.go b/lib/promscrape/discovery/gce/api.go index 182f46371..695fc0842 100644 --- a/lib/promscrape/discovery/gce/api.go +++ b/lib/promscrape/discovery/gce/api.go @@ -47,7 +47,7 @@ func newAPIConfig(sdc *SDConfig) (*apiConfig, error) { project = proj logger.Infof("autodetected the current GCE project: %q", project) } - zones := sdc.Zone.zones + zones := sdc.Zone.Zones if len(zones) == 0 { // Autodetect the current zone. zone, err := getCurrentZone() diff --git a/lib/promscrape/discovery/gce/gce.go b/lib/promscrape/discovery/gce/gce.go index e84904929..dcfb012e5 100644 --- a/lib/promscrape/discovery/gce/gce.go +++ b/lib/promscrape/discovery/gce/gce.go @@ -26,7 +26,7 @@ type SDConfig struct { // ZoneYAML holds info about zones. type ZoneYAML struct { - zones []string + Zones []string } // UnmarshalYAML implements yaml.Unmarshaler @@ -50,13 +50,13 @@ func (z *ZoneYAML) UnmarshalYAML(unmarshal func(interface{}) error) error { default: return fmt.Errorf("unexpected type unmarshaled for ZoneYAML: %T; contents: %#v", v, v) } - z.zones = zones + z.Zones = zones return nil } // MarshalYAML implements yaml.Marshaler func (z ZoneYAML) MarshalYAML() (interface{}, error) { - return z.zones, nil + return z.Zones, nil } // GetLabels returns gce labels according to sdc. diff --git a/lib/promscrape/discovery/gce/gce_test.go b/lib/promscrape/discovery/gce/gce_test.go index 254a5bddc..b9bd2700e 100644 --- a/lib/promscrape/discovery/gce/gce_test.go +++ b/lib/promscrape/discovery/gce/gce_test.go @@ -10,7 +10,7 @@ func TestMarshallingSDConfigWithZoneYAML(t *testing.T) { sdConfig := SDConfig{ Project: "test-project", Zone: ZoneYAML{ - zones: []string{"zone-a", "zone-b"}, + Zones: []string{"zone-a", "zone-b"}, }, } diff --git a/lib/promscrape/discovery/kubernetes/api_watcher.go b/lib/promscrape/discovery/kubernetes/api_watcher.go index 2fff3f249..f1dbc5002 100644 --- a/lib/promscrape/discovery/kubernetes/api_watcher.go +++ b/lib/promscrape/discovery/kubernetes/api_watcher.go @@ -75,7 +75,7 @@ func newAPIWatcher(apiServer string, ac *promauth.Config, sdc *SDConfig, swcFunc } selectors := sdc.Selectors attachNodeMetadata := sdc.AttachMetadata.Node - proxyURL := sdc.ProxyURL.URL() + proxyURL := sdc.ProxyURL.GetURL() gw := getGroupWatcher(apiServer, ac, namespaces, selectors, attachNodeMetadata, proxyURL) role := sdc.role() return &apiWatcher{ diff --git a/lib/promscrape/discoveryutils/client.go b/lib/promscrape/discoveryutils/client.go index cbd84aaf3..6050b60bb 100644 --- a/lib/promscrape/discoveryutils/client.go +++ b/lib/promscrape/discoveryutils/client.go @@ -75,7 +75,7 @@ func NewClient(apiServer string, ac *promauth.Config, proxyURL *proxy.URL, proxy // Send full urls in requests to a proxy host for non-TLS apiServer // like net/http package from Go does. // See https://en.wikipedia.org/wiki/Proxy_server#Web_proxy_servers - pu := proxyURL.URL() + pu := proxyURL.GetURL() hostPort = pu.Host isTLS = pu.Scheme == "https" if isTLS { diff --git a/lib/promutils/duration.go b/lib/promutils/duration.go index edfe0dd1d..2fc57c52e 100644 --- a/lib/promutils/duration.go +++ b/lib/promutils/duration.go @@ -8,19 +8,19 @@ import ( // Duration is duration, which must be used in Prometheus-compatible yaml configs. type Duration struct { - d time.Duration + D time.Duration } // NewDuration returns Duration for given d. func NewDuration(d time.Duration) *Duration { return &Duration{ - d: d, + D: d, } } // MarshalYAML implements yaml.Marshaler interface. func (pd Duration) MarshalYAML() (interface{}, error) { - return pd.d.String(), nil + return pd.D.String(), nil } // UnmarshalYAML implements yaml.Unmarshaler interface. @@ -33,7 +33,7 @@ func (pd *Duration) UnmarshalYAML(unmarshal func(interface{}) error) error { if err != nil { return err } - pd.d = time.Duration(ms) * time.Millisecond + pd.D = time.Duration(ms) * time.Millisecond return nil } @@ -42,7 +42,7 @@ func (pd *Duration) Duration() time.Duration { if pd == nil { return 0 } - return pd.d + return pd.D } // ParseDuration parses duration string in Prometheus format diff --git a/lib/proxy/proxy.go b/lib/proxy/proxy.go index 449a5aad1..46c29bfbd 100644 --- a/lib/proxy/proxy.go +++ b/lib/proxy/proxy.go @@ -19,7 +19,7 @@ import ( // URL implements YAML.Marshaler and yaml.Unmarshaler interfaces for url.URL. type URL struct { - url *url.URL + URL *url.URL } // MustNewURL returns new URL for the given u. @@ -29,31 +29,31 @@ func MustNewURL(u string) *URL { logger.Panicf("BUG: cannot parse u=%q: %s", u, err) } return &URL{ - url: pu, + URL: pu, } } -// URL return the underlying url. -func (u *URL) URL() *url.URL { - if u == nil || u.url == nil { +// GetURL return the underlying url. +func (u *URL) GetURL() *url.URL { + if u == nil || u.URL == nil { return nil } - return u.url + return u.URL } // IsHTTPOrHTTPS returns true if u is http or https func (u *URL) IsHTTPOrHTTPS() bool { - pu := u.URL() + pu := u.GetURL() if pu == nil { return false } - scheme := u.url.Scheme + scheme := u.URL.Scheme return scheme == "http" || scheme == "https" } // String returns string representation of u. func (u *URL) String() string { - pu := u.URL() + pu := u.GetURL() if pu == nil { return "" } @@ -66,10 +66,10 @@ func (u *URL) GetAuthHeader(ac *promauth.Config) string { if ac != nil { authHeader = ac.GetAuthHeader() } - if u == nil || u.url == nil { + if u == nil || u.URL == nil { return authHeader } - pu := u.url + pu := u.URL if pu.User != nil && len(pu.User.Username()) > 0 { userPasswordEncoded := base64.StdEncoding.EncodeToString([]byte(pu.User.String())) authHeader = "Basic " + userPasswordEncoded @@ -79,10 +79,10 @@ func (u *URL) GetAuthHeader(ac *promauth.Config) string { // MarshalYAML implements yaml.Marshaler interface. func (u *URL) MarshalYAML() (interface{}, error) { - if u.url == nil { + if u.URL == nil { return nil, nil } - return u.url.String(), nil + return u.URL.String(), nil } // UnmarshalYAML implements yaml.Unmarshaler interface. @@ -95,16 +95,16 @@ func (u *URL) UnmarshalYAML(unmarshal func(interface{}) error) error { if err != nil { return fmt.Errorf("cannot parse proxy_url=%q as *url.URL: %w", s, err) } - u.url = parsedURL + u.URL = parsedURL return nil } // NewDialFunc returns dial func for the given u and ac. func (u *URL) NewDialFunc(ac *promauth.Config) (fasthttp.DialFunc, error) { - if u == nil || u.url == nil { + if u == nil || u.URL == nil { return defaultDialFunc, nil } - pu := u.url + pu := u.URL switch pu.Scheme { case "http", "https", "socks5", "tls+socks5": default: