From 132fd5f6ef8bfcc8ca2d8dbc30b8b9cffde93b83 Mon Sep 17 00:00:00 2001 From: Simon Pasquier Date: Fri, 13 Jul 2018 15:01:51 +0200 Subject: [PATCH 1/5] config: validate URLs at config load time Signed-off-by: Simon Pasquier --- config/config.go | 174 +++++++++++++++++++++++++--------- config/config_test.go | 107 +++++++++++++++++++-- config/notifiers.go | 24 ++--- config/notifiers_test.go | 4 +- config/testdata/conf.good.yml | 2 +- notify/impl.go | 62 ++++++------ notify/impl_test.go | 12 ++- 7 files changed, 284 insertions(+), 101 deletions(-) diff --git a/config/config.go b/config/config.go index 77afda477c..54413f8d11 100644 --- a/config/config.go +++ b/config/config.go @@ -18,6 +18,7 @@ import ( "errors" "fmt" "io/ioutil" + "net/url" "path/filepath" "regexp" "strings" @@ -39,7 +40,7 @@ func (s Secret) MarshalYAML() (interface{}, error) { return nil, nil } -//UnmarshalYAML implements the yaml.Unmarshaler interface for Secrets. +// UnmarshalYAML implements the yaml.Unmarshaler interface for Secret. func (s *Secret) UnmarshalYAML(unmarshal func(interface{}) error) error { type plain Secret return unmarshal((*plain)(s)) @@ -50,6 +51,81 @@ func (s Secret) MarshalJSON() ([]byte, error) { return json.Marshal("") } +// URL is a custom type that allows validation at configuration load time. +type URL struct { + *url.URL +} + +// MarshalYAML implements the yaml.Marshaler interface for URL. +func (u URL) MarshalYAML() (interface{}, error) { + if u.URL != nil { + return u.URL.String(), nil + } + return nil, nil +} + +// UnmarshalYAML implements the yaml.Unmarshaler interface for URL. +func (u *URL) UnmarshalYAML(unmarshal func(interface{}) error) error { + var s string + if err := unmarshal(&s); err != nil { + return err + } + urlp, err := url.Parse(s) + if err != nil { + return err + } + u.URL = urlp + return nil +} + +// MarshalJSON implements the json.Marshaler interface for URL. +func (u URL) MarshalJSON() ([]byte, error) { + if u.URL != nil { + return json.Marshal(u.URL.String()) + } + return nil, nil +} + +// UnmarshalJSON implements the json.Marshaler interface for URL. +func (u *URL) UnmarshalJSON(data []byte) error { + var s string + if err := json.Unmarshal(data, &s); err != nil { + return err + } + urlp, err := url.Parse(s) + if err != nil { + return err + } + u.URL = urlp + return nil +} + +// SecretURL is a URL that must not be revealed on marshaling. +type SecretURL URL + +// MarshalYAML implements the yaml.Marshaler interface for SecretURL. +func (s SecretURL) MarshalYAML() (interface{}, error) { + if s.URL != nil { + return "", nil + } + return nil, nil +} + +// UnmarshalYAML implements the yaml.Unmarshaler interface for SecretURL. +func (s *SecretURL) UnmarshalYAML(unmarshal func(interface{}) error) error { + return unmarshal((*URL)(s)) +} + +// MarshalJSON implements the json.Marshaler interface for SecretURL. +func (s SecretURL) MarshalJSON() ([]byte, error) { + return json.Marshal("") +} + +// UnmarshalJSON implements the json.Marshaler interface for SecretURL. +func (s *SecretURL) UnmarshalJSON(data []byte) error { + return json.Unmarshal(data, (*URL)(s)) +} + // Load parses the YAML input s into a Config. func Load(s string) (*Config, error) { cfg := &Config{} @@ -188,8 +264,8 @@ func (c *Config) UnmarshalYAML(unmarshal func(interface{}) error) error { if sc.HTTPConfig == nil { sc.HTTPConfig = c.Global.HTTPConfig } - if sc.APIURL == "" { - if c.Global.SlackAPIURL == "" { + if sc.APIURL == nil { + if c.Global.SlackAPIURL == nil { return fmt.Errorf("no global Slack API URL set") } sc.APIURL = c.Global.SlackAPIURL @@ -199,14 +275,14 @@ func (c *Config) UnmarshalYAML(unmarshal func(interface{}) error) error { if hc.HTTPConfig == nil { hc.HTTPConfig = c.Global.HTTPConfig } - if hc.APIURL == "" { - if c.Global.HipchatAPIURL == "" { + if hc.APIURL == nil { + if c.Global.HipchatAPIURL == nil { return fmt.Errorf("no global Hipchat API URL set") } hc.APIURL = c.Global.HipchatAPIURL } - if !strings.HasSuffix(hc.APIURL, "/") { - hc.APIURL += "/" + if !strings.HasSuffix(hc.APIURL.Path, "/") { + hc.APIURL.Path += "/" } if hc.AuthToken == "" { if c.Global.HipchatAuthToken == "" { @@ -224,8 +300,8 @@ func (c *Config) UnmarshalYAML(unmarshal func(interface{}) error) error { if pdc.HTTPConfig == nil { pdc.HTTPConfig = c.Global.HTTPConfig } - if pdc.URL == "" { - if c.Global.PagerdutyURL == "" { + if pdc.URL == nil { + if c.Global.PagerdutyURL == nil { return fmt.Errorf("no global PagerDuty URL set") } pdc.URL = c.Global.PagerdutyURL @@ -235,14 +311,14 @@ func (c *Config) UnmarshalYAML(unmarshal func(interface{}) error) error { if ogc.HTTPConfig == nil { ogc.HTTPConfig = c.Global.HTTPConfig } - if ogc.APIURL == "" { - if c.Global.OpsGenieAPIURL == "" { + if ogc.APIURL == nil { + if c.Global.OpsGenieAPIURL == nil { return fmt.Errorf("no global OpsGenie URL set") } ogc.APIURL = c.Global.OpsGenieAPIURL } - if !strings.HasSuffix(ogc.APIURL, "/") { - ogc.APIURL += "/" + if !strings.HasSuffix(ogc.APIURL.Path, "/") { + ogc.APIURL.Path += "/" } if ogc.APIKey == "" { if c.Global.OpsGenieAPIKey == "" { @@ -256,8 +332,8 @@ func (c *Config) UnmarshalYAML(unmarshal func(interface{}) error) error { wcc.HTTPConfig = c.Global.HTTPConfig } - if wcc.APIURL == "" { - if c.Global.WeChatAPIURL == "" { + if wcc.APIURL == nil { + if c.Global.WeChatAPIURL == nil { return fmt.Errorf("no global Wechat URL set") } wcc.APIURL = c.Global.WeChatAPIURL @@ -277,22 +353,22 @@ func (c *Config) UnmarshalYAML(unmarshal func(interface{}) error) error { wcc.CorpID = c.Global.WeChatAPICorpID } - if !strings.HasSuffix(wcc.APIURL, "/") { - wcc.APIURL += "/" + if !strings.HasSuffix(wcc.APIURL.Path, "/") { + wcc.APIURL.Path += "/" } } for _, voc := range rcv.VictorOpsConfigs { if voc.HTTPConfig == nil { voc.HTTPConfig = c.Global.HTTPConfig } - if voc.APIURL == "" { - if c.Global.VictorOpsAPIURL == "" { + if voc.APIURL == nil { + if c.Global.VictorOpsAPIURL == nil { return fmt.Errorf("no global VictorOps URL set") } voc.APIURL = c.Global.VictorOpsAPIURL } - if !strings.HasSuffix(voc.APIURL, "/") { - voc.APIURL += "/" + if !strings.HasSuffix(voc.APIURL.Path, "/") { + voc.APIURL.Path += "/" } if voc.APIKey == "" { if c.Global.VictorOpsAPIKey == "" { @@ -344,11 +420,19 @@ var DefaultGlobalConfig = GlobalConfig{ SMTPHello: "localhost", SMTPRequireTLS: true, - PagerdutyURL: "https://events.pagerduty.com/v2/enqueue", - HipchatAPIURL: "https://api.hipchat.com/", - OpsGenieAPIURL: "https://api.opsgenie.com/", - WeChatAPIURL: "https://qyapi.weixin.qq.com/cgi-bin/", - VictorOpsAPIURL: "https://alert.victorops.com/integrations/generic/20131114/alert/", + PagerdutyURL: mustParseURL("https://events.pagerduty.com/v2/enqueue"), + HipchatAPIURL: mustParseURL("https://api.hipchat.com/"), + OpsGenieAPIURL: mustParseURL("https://api.opsgenie.com/"), + WeChatAPIURL: mustParseURL("https://qyapi.weixin.qq.com/cgi-bin/"), + VictorOpsAPIURL: mustParseURL("https://alert.victorops.com/integrations/generic/20131114/alert/"), +} + +func mustParseURL(s string) *URL { + u, err := url.Parse(s) + if err != nil { + panic(err) + } + return &URL{u} } // GlobalConfig defines configuration parameters that are valid globally @@ -360,25 +444,25 @@ type GlobalConfig struct { HTTPConfig *commoncfg.HTTPClientConfig `yaml:"http_config,omitempty" json:"http_config,omitempty"` - SMTPFrom string `yaml:"smtp_from,omitempty" json:"smtp_from,omitempty"` - SMTPHello string `yaml:"smtp_hello,omitempty" json:"smtp_hello,omitempty"` - SMTPSmarthost string `yaml:"smtp_smarthost,omitempty" json:"smtp_smarthost,omitempty"` - SMTPAuthUsername string `yaml:"smtp_auth_username,omitempty" json:"smtp_auth_username,omitempty"` - SMTPAuthPassword Secret `yaml:"smtp_auth_password,omitempty" json:"smtp_auth_password,omitempty"` - SMTPAuthSecret Secret `yaml:"smtp_auth_secret,omitempty" json:"smtp_auth_secret,omitempty"` - SMTPAuthIdentity string `yaml:"smtp_auth_identity,omitempty" json:"smtp_auth_identity,omitempty"` - SMTPRequireTLS bool `yaml:"smtp_require_tls,omitempty" json:"smtp_require_tls,omitempty"` - SlackAPIURL Secret `yaml:"slack_api_url,omitempty" json:"slack_api_url,omitempty"` - PagerdutyURL string `yaml:"pagerduty_url,omitempty" json:"pagerduty_url,omitempty"` - HipchatAPIURL string `yaml:"hipchat_api_url,omitempty" json:"hipchat_api_url,omitempty"` - HipchatAuthToken Secret `yaml:"hipchat_auth_token,omitempty" json:"hipchat_auth_token,omitempty"` - OpsGenieAPIURL string `yaml:"opsgenie_api_url,omitempty" json:"opsgenie_api_url,omitempty"` - OpsGenieAPIKey Secret `yaml:"opsgenie_api_key,omitempty" json:"opsgenie_api_key,omitempty"` - WeChatAPIURL string `yaml:"wechat_api_url,omitempty" json:"wechat_api_url,omitempty"` - WeChatAPISecret Secret `yaml:"wechat_api_secret,omitempty" json:"wechat_api_secret,omitempty"` - WeChatAPICorpID string `yaml:"wechat_api_corp_id,omitempty" json:"wechat_api_corp_id,omitempty"` - VictorOpsAPIURL string `yaml:"victorops_api_url,omitempty" json:"victorops_api_url,omitempty"` - VictorOpsAPIKey Secret `yaml:"victorops_api_key,omitempty" json:"victorops_api_key,omitempty"` + SMTPFrom string `yaml:"smtp_from,omitempty" json:"smtp_from,omitempty"` + SMTPHello string `yaml:"smtp_hello,omitempty" json:"smtp_hello,omitempty"` + SMTPSmarthost string `yaml:"smtp_smarthost,omitempty" json:"smtp_smarthost,omitempty"` + SMTPAuthUsername string `yaml:"smtp_auth_username,omitempty" json:"smtp_auth_username,omitempty"` + SMTPAuthPassword Secret `yaml:"smtp_auth_password,omitempty" json:"smtp_auth_password,omitempty"` + SMTPAuthSecret Secret `yaml:"smtp_auth_secret,omitempty" json:"smtp_auth_secret,omitempty"` + SMTPAuthIdentity string `yaml:"smtp_auth_identity,omitempty" json:"smtp_auth_identity,omitempty"` + SMTPRequireTLS bool `yaml:"smtp_require_tls,omitempty" json:"smtp_require_tls,omitempty"` + SlackAPIURL *SecretURL `yaml:"slack_api_url,omitempty" json:"slack_api_url,omitempty"` + PagerdutyURL *URL `yaml:"pagerduty_url,omitempty" json:"pagerduty_url,omitempty"` + HipchatAPIURL *URL `yaml:"hipchat_api_url,omitempty" json:"hipchat_api_url,omitempty"` + HipchatAuthToken Secret `yaml:"hipchat_auth_token,omitempty" json:"hipchat_auth_token,omitempty"` + OpsGenieAPIURL *URL `yaml:"opsgenie_api_url,omitempty" json:"opsgenie_api_url,omitempty"` + OpsGenieAPIKey Secret `yaml:"opsgenie_api_key,omitempty" json:"opsgenie_api_key,omitempty"` + WeChatAPIURL *URL `yaml:"wechat_api_url,omitempty" json:"wechat_api_url,omitempty"` + WeChatAPISecret Secret `yaml:"wechat_api_secret,omitempty" json:"wechat_api_secret,omitempty"` + WeChatAPICorpID string `yaml:"wechat_api_corp_id,omitempty" json:"wechat_api_corp_id,omitempty"` + VictorOpsAPIURL *URL `yaml:"victorops_api_url,omitempty" json:"victorops_api_url,omitempty"` + VictorOpsAPIKey Secret `yaml:"victorops_api_key,omitempty" json:"victorops_api_key,omitempty"` } // UnmarshalYAML implements the yaml.Unmarshaler interface. diff --git a/config/config_test.go b/config/config_test.go index c812035a74..069e39c215 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -15,6 +15,7 @@ package config import ( "encoding/json" + "net/url" "reflect" "regexp" "strings" @@ -251,7 +252,7 @@ receivers: func TestHideConfigSecrets(t *testing.T) { c, _, err := LoadFile("testdata/conf.good.yml") if err != nil { - t.Errorf("Error parsing %s: %s", "testdata/conf.good.yml", err) + t.Fatalf("Error parsing %s: %s", "testdata/conf.good.yml", err) } // String method must not reveal authentication credentials. @@ -292,6 +293,98 @@ func TestJSONMarshalSecret(t *testing.T) { require.Equal(t, "{\"S\":\"\\u003csecret\\u003e\"}", string(c), "Secret not properly elided.") } +func TestMarshalSecretURL(t *testing.T) { + urlp, err := url.Parse("http://example.com/") + if err != nil { + t.Fatal(err) + } + u := &SecretURL{urlp} + + c, err := json.Marshal(u) + if err != nil { + t.Fatal(err) + } + // u003c -> "<" + // u003e -> ">" + require.Equal(t, "\"\\u003csecret\\u003e\"", string(c), "SecretURL not properly elided in JSON.") + + c, err = yaml.Marshal(u) + if err != nil { + t.Fatal(err) + } + require.Equal(t, "\n", string(c), "SecretURL not properly elidedin YAML.") +} + +func TestUnmarshalSecretURL(t *testing.T) { + b := []byte(`"http://example.com/se cret"`) + var u SecretURL + + err := json.Unmarshal(b, &u) + if err != nil { + t.Fatal(err) + } + require.Equal(t, "http://example.com/se%20cret", u.String(), "SecretURL not properly unmarshalled in JSON.") + + err = yaml.Unmarshal(b, &u) + if err != nil { + t.Fatal(err) + } + + require.Equal(t, "http://example.com/se%20cret", u.String(), "SecretURL not properly unmarshalled in YAML.") +} + +func TestMarshalURL(t *testing.T) { + urlp, err := url.Parse("http://example.com/") + if err != nil { + t.Fatal(err) + } + u := &URL{urlp} + + c, err := json.Marshal(u) + if err != nil { + t.Fatal(err) + } + require.Equal(t, "\"http://example.com/\"", string(c), "URL not properly marshalled in JSON.") + + c, err = yaml.Marshal(u) + if err != nil { + t.Fatal(err) + } + require.Equal(t, "http://example.com/\n", string(c), "URL not properly marshalled in YAML.") +} + +func TestUnmarshalURL(t *testing.T) { + b := []byte(`"http://example.com/a b"`) + var u URL + + err := json.Unmarshal(b, &u) + if err != nil { + t.Fatal(err) + } + require.Equal(t, "http://example.com/a%20b", u.String(), "URL not properly unmarshalled in JSON.") + + err = json.Unmarshal(b, &u) + if err != nil { + t.Fatal(err) + } + require.Equal(t, "http://example.com/a%20b", u.String(), "URL not properly unmarshalled in YAML.") +} + +func TestUnmarshalInvalidURL(t *testing.T) { + b := []byte(`"://example.com"`) + var u URL + + err := json.Unmarshal(b, &u) + if err == nil { + t.Errorf("Expected an error parsing URL") + } + + err = yaml.Unmarshal(b, &u) + if err == nil { + t.Errorf("Expected an error parsing URL") + } +} + func TestJSONUnmarshalMarshaled(t *testing.T) { c, _, err := LoadFile("testdata/conf.good.yml") if err != nil { @@ -323,13 +416,13 @@ func TestEmptyFieldsAndRegex(t *testing.T) { SMTPSmarthost: "localhost:25", SMTPFrom: "alertmanager@example.org", HipchatAuthToken: "mysecret", - HipchatAPIURL: "https://hipchat.foobar.org/", - SlackAPIURL: "mysecret", + HipchatAPIURL: mustParseURL("https://hipchat.foobar.org/"), + SlackAPIURL: (*SecretURL)(mustParseURL("http://slack.example.com/")), SMTPRequireTLS: true, - PagerdutyURL: "https://events.pagerduty.com/v2/enqueue", - OpsGenieAPIURL: "https://api.opsgenie.com/", - WeChatAPIURL: "https://qyapi.weixin.qq.com/cgi-bin/", - VictorOpsAPIURL: "https://alert.victorops.com/integrations/generic/20131114/alert/", + PagerdutyURL: mustParseURL("https://events.pagerduty.com/v2/enqueue"), + OpsGenieAPIURL: mustParseURL("https://api.opsgenie.com/"), + WeChatAPIURL: mustParseURL("https://qyapi.weixin.qq.com/cgi-bin/"), + VictorOpsAPIURL: mustParseURL("https://alert.victorops.com/integrations/generic/20131114/alert/"), }, Templates: []string{ diff --git a/config/notifiers.go b/config/notifiers.go index ba4517e885..583827e69d 100644 --- a/config/notifiers.go +++ b/config/notifiers.go @@ -15,7 +15,6 @@ package config import ( "fmt" - "net/url" "strings" "time" @@ -198,7 +197,7 @@ type PagerdutyConfig struct { ServiceKey Secret `yaml:"service_key,omitempty" json"service_key,omitempty"` RoutingKey Secret `yaml:"routing_key,omitempty" json:"routing_key,omitempty"` - URL string `yaml:"url,omitempty" json:"url,omitempty"` + URL *URL `yaml:"url,omitempty" json:"url,omitempty"` Client string `yaml:"client,omitempty" json:"client,omitempty"` ClientURL string `yaml:"client_url,omitempty" json:"client_url,omitempty"` Description string `yaml:"description,omitempty" json:"description,omitempty"` @@ -289,7 +288,7 @@ type SlackConfig struct { HTTPConfig *commoncfg.HTTPClientConfig `yaml:"http_config,omitempty" json:"http_config,omitempty"` - APIURL Secret `yaml:"api_url,omitempty" json:"api_url,omitempty"` + APIURL *SecretURL `yaml:"api_url,omitempty" json:"api_url,omitempty"` // Slack channel override, (like #other-channel or @username). Channel string `yaml:"channel,omitempty" json:"channel,omitempty"` @@ -323,7 +322,7 @@ type HipchatConfig struct { HTTPConfig *commoncfg.HTTPClientConfig `yaml:"http_config,omitempty" json:"http_config,omitempty"` - APIURL string `yaml:"api_url,omitempty" json:"api_url,omitempty"` + APIURL *URL `yaml:"api_url,omitempty" json:"api_url,omitempty"` AuthToken Secret `yaml:"auth_token,omitempty" json:"auth_token,omitempty"` RoomID string `yaml:"room_id,omitempty" json:"room_id,omitempty"` From string `yaml:"from,omitempty" json:"from,omitempty"` @@ -353,7 +352,7 @@ type WebhookConfig struct { HTTPConfig *commoncfg.HTTPClientConfig `yaml:"http_config,omitempty" json:"http_config,omitempty"` // URL to send POST request to. - URL string `yaml:"url" json:"url"` + URL *URL `yaml:"url" json:"url"` } // UnmarshalYAML implements the yaml.Unmarshaler interface. @@ -363,17 +362,12 @@ func (c *WebhookConfig) UnmarshalYAML(unmarshal func(interface{}) error) error { if err := unmarshal((*plain)(c)); err != nil { return err } - if c.URL == "" { + if c.URL == nil { return fmt.Errorf("missing URL in webhook config") } - url, err := url.Parse(c.URL) - if err != nil { - return err - } - if url.Scheme != "https" && url.Scheme != "http" { + if c.URL.Scheme != "https" && c.URL.Scheme != "http" { return fmt.Errorf("scheme required for webhook url") } - c.URL = url.String() return nil } @@ -386,7 +380,7 @@ type WechatConfig struct { APISecret Secret `yaml:"api_secret,omitempty" json:"api_secret,omitempty"` CorpID string `yaml:"corp_id,omitempty" json:"corp_id,omitempty"` Message string `yaml:"message,omitempty" json:"message,omitempty"` - APIURL string `yaml:"api_url,omitempty" json:"api_url,omitempty"` + APIURL *URL `yaml:"api_url,omitempty" json:"api_url,omitempty"` ToUser string `yaml:"to_user,omitempty" json:"to_user,omitempty"` ToParty string `yaml:"to_party,omitempty" json:"to_party,omitempty"` ToTag string `yaml:"to_tag,omitempty" json:"to_tag,omitempty"` @@ -416,7 +410,7 @@ type OpsGenieConfig struct { HTTPConfig *commoncfg.HTTPClientConfig `yaml:"http_config,omitempty" json:"http_config,omitempty"` APIKey Secret `yaml:"api_key,omitempty" json:"api_key,omitempty"` - APIURL string `yaml:"api_url,omitempty" json:"api_url,omitempty"` + APIURL *URL `yaml:"api_url,omitempty" json:"api_url,omitempty"` Message string `yaml:"message,omitempty" json:"message,omitempty"` Description string `yaml:"description,omitempty" json:"description,omitempty"` Source string `yaml:"source,omitempty" json:"source,omitempty"` @@ -441,7 +435,7 @@ type VictorOpsConfig struct { HTTPConfig *commoncfg.HTTPClientConfig `yaml:"http_config,omitempty" json:"http_config,omitempty"` APIKey Secret `yaml:"api_key" json:"api_key"` - APIURL string `yaml:"api_url" json:"api_url"` + APIURL *URL `yaml:"api_url" json:"api_url"` RoutingKey string `yaml:"routing_key" json:"routing_key"` MessageType string `yaml:"message_type" json:"message_type"` StateMessage string `yaml:"state_message" json:"state_message"` diff --git a/config/notifiers_test.go b/config/notifiers_test.go index c3f897f852..18dd0b406c 100644 --- a/config/notifiers_test.go +++ b/config/notifiers_test.go @@ -166,9 +166,7 @@ room_id: '' } func TestWebhookURLIsPresent(t *testing.T) { - in := ` -url: '' -` + in := `{}` var cfg WebhookConfig err := yaml.UnmarshalStrict([]byte(in), &cfg) diff --git a/config/testdata/conf.good.yml b/config/testdata/conf.good.yml index 861984aaad..1fb59bb4c9 100644 --- a/config/testdata/conf.good.yml +++ b/config/testdata/conf.good.yml @@ -9,7 +9,7 @@ global: hipchat_auth_token: "mysecret" # Alternative host for Hipchat. hipchat_api_url: 'https://hipchat.foobar.org/' - slack_api_url: "mysecret" + slack_api_url: "http://mysecret.example.com/" diff --git a/notify/impl.go b/notify/impl.go index bc88b635e6..18af211b2a 100644 --- a/notify/impl.go +++ b/notify/impl.go @@ -165,7 +165,7 @@ func (w *Webhook) Notify(ctx context.Context, alerts ...*types.Alert) (bool, err return false, err } - req, err := http.NewRequest("POST", w.conf.URL, &buf) + req, err := http.NewRequest("POST", w.conf.URL.String(), &buf) if err != nil { return true, err } @@ -477,7 +477,11 @@ func (n *PagerDuty) notifyV1( Details: details, } - n.conf.URL = "https://events.pagerduty.com/generic/2010-04-15/create_event.json" + url, err := url.Parse("https://events.pagerduty.com/generic/2010-04-15/create_event.json") + if err != nil { + return false, err + } + n.conf.URL = &config.URL{url} if eventType == pagerDutyEventTrigger { msg.Client = tmpl(n.conf.Client) @@ -493,7 +497,7 @@ func (n *PagerDuty) notifyV1( return false, err } - resp, err := ctxhttp.Post(ctx, c, n.conf.URL, contentTypeJSON, &buf) + resp, err := ctxhttp.Post(ctx, c, n.conf.URL.String(), contentTypeJSON, &buf) if err != nil { return true, err } @@ -551,7 +555,7 @@ func (n *PagerDuty) notifyV2( return false, err } - resp, err := ctxhttp.Post(ctx, c, n.conf.URL, contentTypeJSON, &buf) + resp, err := ctxhttp.Post(ctx, c, n.conf.URL.String(), contentTypeJSON, &buf) if err != nil { return true, err } @@ -745,7 +749,7 @@ func (n *Slack) Notify(ctx context.Context, as ...*types.Alert) (bool, error) { return false, err } - resp, err := ctxhttp.Post(ctx, c, string(n.conf.APIURL), contentTypeJSON, &buf) + resp, err := ctxhttp.Post(ctx, c, n.conf.APIURL.String(), contentTypeJSON, &buf) if err != nil { return true, err } @@ -798,8 +802,10 @@ func (n *Hipchat) Notify(ctx context.Context, as ...*types.Alert) (bool, error) tmplText = tmplText(n.tmpl, data, &err) tmplHTML = tmplHTML(n.tmpl, data, &err) roomid = tmplText(n.conf.RoomID) - url = fmt.Sprintf("%sv2/room/%s/notification?auth_token=%s", n.conf.APIURL, roomid, n.conf.AuthToken) + // n.conf.APIURL is already checked at configuration load time. + url, _ = url.Parse(n.conf.APIURL.String()) ) + url.Path += fmt.Sprintf("v2/room/%s/notification?auth_token=%s", roomid, n.conf.AuthToken) if n.conf.MessageFormat == "html" { msg = tmplHTML(n.conf.Message) @@ -828,7 +834,7 @@ func (n *Hipchat) Notify(ctx context.Context, as ...*types.Alert) (bool, error) return false, err } - resp, err := ctxhttp.Post(ctx, c, url, contentTypeJSON, &buf) + resp, err := ctxhttp.Post(ctx, c, url.String(), contentTypeJSON, &buf) if err != nil { return true, err } @@ -918,13 +924,9 @@ func (n *Wechat) Notify(ctx context.Context, as ...*types.Alert) (bool, error) { return false, fmt.Errorf("templating error: %s", err) } - apiURL := n.conf.APIURL + "gettoken" - - u, err := url.Parse(apiURL) - if err != nil { - return false, err - } - + // n.conf.APIURL is already checked at configuration load time. + u, _ := url.Parse(n.conf.APIURL.String()) + u.Path += "gettoken" u.RawQuery = parameters.Encode() req, err := http.NewRequest(http.MethodGet, u.String(), nil) @@ -974,9 +976,11 @@ func (n *Wechat) Notify(ctx context.Context, as ...*types.Alert) (bool, error) { return false, err } - postMessageURL := n.conf.APIURL + "message/send?access_token=" + n.accessToken + // n.conf.APIURL is already checked at configuration load time. + postMessageURL, _ := url.Parse(n.conf.APIURL.String()) + postMessageURL.Path += "message/send?access_token=" + n.accessToken - req, err := http.NewRequest(http.MethodPost, postMessageURL, &buf) + req, err := http.NewRequest(http.MethodPost, postMessageURL.String(), &buf) if err != nil { return true, err } @@ -1094,14 +1098,14 @@ func (n *OpsGenie) createRequest(ctx context.Context, as ...*types.Alert) (*http } var ( - msg interface{} - apiURL string - alias = hashKey(key) - alerts = types.Alerts(as...) + msg interface{} + apiURL, _ = url.Parse(n.conf.APIURL.String()) + alias = hashKey(key) + alerts = types.Alerts(as...) ) switch alerts.Status() { case model.AlertResolved: - apiURL = fmt.Sprintf("%sv2/alerts/%s/close?identifierType=alias", n.conf.APIURL, alias) + apiURL.Path += fmt.Sprintf("v2/alerts/%s/close?identifierType=alias", alias) msg = &opsGenieCloseMessage{Source: tmpl(n.conf.Source)} default: message := tmpl(n.conf.Message) @@ -1110,7 +1114,7 @@ func (n *OpsGenie) createRequest(ctx context.Context, as ...*types.Alert) (*http level.Debug(n.logger).Log("msg", "Truncated message to %q due to OpsGenie message limit", "truncated_message", message, "incident", key) } - apiURL = n.conf.APIURL + "v2/alerts" + apiURL.Path += "v2/alerts" var teams []map[string]string for _, t := range safeSplit(string(tmpl(n.conf.Teams)), ",") { teams = append(teams, map[string]string{"name": t}) @@ -1138,7 +1142,7 @@ func (n *OpsGenie) createRequest(ctx context.Context, as ...*types.Alert) (*http return nil, false, err } - req, err := http.NewRequest("POST", apiURL, &buf) + req, err := http.NewRequest("POST", apiURL.String(), &buf) if err != nil { return nil, true, err } @@ -1203,13 +1207,15 @@ func (n *VictorOps) Notify(ctx context.Context, as ...*types.Alert) (bool, error var err error var ( - alerts = types.Alerts(as...) - data = n.tmpl.Data(receiverName(ctx, n.logger), groupLabels(ctx, n.logger), as...) - tmpl = tmplText(n.tmpl, data, &err) - apiURL = fmt.Sprintf("%s%s/%s", n.conf.APIURL, n.conf.APIKey, tmpl(n.conf.RoutingKey)) + alerts = types.Alerts(as...) + data = n.tmpl.Data(receiverName(ctx, n.logger), groupLabels(ctx, n.logger), as...) + tmpl = tmplText(n.tmpl, data, &err) + // n.conf.APIURL is already checked at configuration load time. + apiURL, _ = url.Parse(n.conf.APIURL.String()) messageType = tmpl(n.conf.MessageType) stateMessage = tmpl(n.conf.StateMessage) ) + apiURL.Path += fmt.Sprintf("%s/%s", n.conf.APIKey, tmpl(n.conf.RoutingKey)) if alerts.Status() == model.AlertFiring && !victorOpsAllowedEvents[messageType] { messageType = victorOpsEventTrigger @@ -1246,7 +1252,7 @@ func (n *VictorOps) Notify(ctx context.Context, as ...*types.Alert) (bool, error return false, err } - resp, err := ctxhttp.Post(ctx, c, apiURL, contentTypeJSON, &buf) + resp, err := ctxhttp.Post(ctx, c, apiURL.String(), contentTypeJSON, &buf) if err != nil { return true, err } diff --git a/notify/impl_test.go b/notify/impl_test.go index db08f0decb..8b5510cf3c 100644 --- a/notify/impl_test.go +++ b/notify/impl_test.go @@ -32,7 +32,11 @@ import ( ) func TestWebhookRetry(t *testing.T) { - notifier := &Webhook{conf: &config.WebhookConfig{URL: "http://example.com/"}} + u, err := url.Parse("http://example.com") + if err != nil { + t.Fatalf("failed to parse URL: %v", err) + } + notifier := &Webhook{conf: &config.WebhookConfig{URL: &config.URL{u}}} for statusCode, expected := range retryTests(defaultRetryCodes()) { actual, _ := notifier.retry(statusCode) require.Equal(t, expected, actual, fmt.Sprintf("error on status %d", statusCode)) @@ -211,6 +215,10 @@ func readBody(t *testing.T, r *http.Request) string { } func TestOpsGenie(t *testing.T) { + u, err := url.Parse("https://opsgenie/api") + if err != nil { + t.Fatalf("failed to parse URL: %v", err) + } logger := log.NewNopLogger() tmpl := createTmpl(t) conf := &config.OpsGenieConfig{ @@ -225,7 +233,7 @@ func TestOpsGenie(t *testing.T) { Note: `{{ .CommonLabels.Note }}`, Priority: `{{ .CommonLabels.Priority }}`, APIKey: `s3cr3t`, - APIURL: `https://opsgenie/api`, + APIURL: &config.URL{u}, } notifier := NewOpsGenie(conf, tmpl, logger) From 348dcbacfd381f3b8e1f4f14abefed5b6ff6e792 Mon Sep 17 00:00:00 2001 From: Simon Pasquier Date: Fri, 13 Jul 2018 15:51:27 +0200 Subject: [PATCH 2/5] Address Brian and Lucas comments Signed-off-by: Simon Pasquier --- config/config_test.go | 2 +- notify/impl.go | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/config/config_test.go b/config/config_test.go index 069e39c215..a845540d57 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -312,7 +312,7 @@ func TestMarshalSecretURL(t *testing.T) { if err != nil { t.Fatal(err) } - require.Equal(t, "\n", string(c), "SecretURL not properly elidedin YAML.") + require.Equal(t, "\n", string(c), "SecretURL not properly elided in YAML.") } func TestUnmarshalSecretURL(t *testing.T) { diff --git a/notify/impl.go b/notify/impl.go index 18af211b2a..2762faa76d 100644 --- a/notify/impl.go +++ b/notify/impl.go @@ -1098,7 +1098,8 @@ func (n *OpsGenie) createRequest(ctx context.Context, as ...*types.Alert) (*http } var ( - msg interface{} + msg interface{} + // n.conf.APIURL is already checked at configuration load time. apiURL, _ = url.Parse(n.conf.APIURL.String()) alias = hashKey(key) alerts = types.Alerts(as...) From a078112c7b7c391d4993cd3b234a0132ba262c3c Mon Sep 17 00:00:00 2001 From: Simon Pasquier Date: Tue, 17 Jul 2018 10:21:43 +0200 Subject: [PATCH 3/5] Shallow copy of URL instead of reparsing it Signed-off-by: Simon Pasquier --- notify/impl.go | 27 +++++++++++---------------- 1 file changed, 11 insertions(+), 16 deletions(-) diff --git a/notify/impl.go b/notify/impl.go index 2762faa76d..713566737b 100644 --- a/notify/impl.go +++ b/notify/impl.go @@ -802,8 +802,7 @@ func (n *Hipchat) Notify(ctx context.Context, as ...*types.Alert) (bool, error) tmplText = tmplText(n.tmpl, data, &err) tmplHTML = tmplHTML(n.tmpl, data, &err) roomid = tmplText(n.conf.RoomID) - // n.conf.APIURL is already checked at configuration load time. - url, _ = url.Parse(n.conf.APIURL.String()) + url = *n.conf.APIURL ) url.Path += fmt.Sprintf("v2/room/%s/notification?auth_token=%s", roomid, n.conf.AuthToken) @@ -924,8 +923,7 @@ func (n *Wechat) Notify(ctx context.Context, as ...*types.Alert) (bool, error) { return false, fmt.Errorf("templating error: %s", err) } - // n.conf.APIURL is already checked at configuration load time. - u, _ := url.Parse(n.conf.APIURL.String()) + u := *n.conf.APIURL u.Path += "gettoken" u.RawQuery = parameters.Encode() @@ -976,8 +974,7 @@ func (n *Wechat) Notify(ctx context.Context, as ...*types.Alert) (bool, error) { return false, err } - // n.conf.APIURL is already checked at configuration load time. - postMessageURL, _ := url.Parse(n.conf.APIURL.String()) + postMessageURL := *n.conf.APIURL postMessageURL.Path += "message/send?access_token=" + n.accessToken req, err := http.NewRequest(http.MethodPost, postMessageURL.String(), &buf) @@ -1098,11 +1095,10 @@ func (n *OpsGenie) createRequest(ctx context.Context, as ...*types.Alert) (*http } var ( - msg interface{} - // n.conf.APIURL is already checked at configuration load time. - apiURL, _ = url.Parse(n.conf.APIURL.String()) - alias = hashKey(key) - alerts = types.Alerts(as...) + msg interface{} + apiURL = *n.conf.APIURL + alias = hashKey(key) + alerts = types.Alerts(as...) ) switch alerts.Status() { case model.AlertResolved: @@ -1208,11 +1204,10 @@ func (n *VictorOps) Notify(ctx context.Context, as ...*types.Alert) (bool, error var err error var ( - alerts = types.Alerts(as...) - data = n.tmpl.Data(receiverName(ctx, n.logger), groupLabels(ctx, n.logger), as...) - tmpl = tmplText(n.tmpl, data, &err) - // n.conf.APIURL is already checked at configuration load time. - apiURL, _ = url.Parse(n.conf.APIURL.String()) + alerts = types.Alerts(as...) + data = n.tmpl.Data(receiverName(ctx, n.logger), groupLabels(ctx, n.logger), as...) + tmpl = tmplText(n.tmpl, data, &err) + apiURL = *n.conf.APIURL messageType = tmpl(n.conf.MessageType) stateMessage = tmpl(n.conf.StateMessage) ) From 353e2b909013269c5cad00a7d48d5416bae7263a Mon Sep 17 00:00:00 2001 From: Simon Pasquier Date: Tue, 17 Jul 2018 10:24:05 +0200 Subject: [PATCH 4/5] Unshadow net/url package Signed-off-by: Simon Pasquier --- notify/impl.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/notify/impl.go b/notify/impl.go index 713566737b..a13e7d7017 100644 --- a/notify/impl.go +++ b/notify/impl.go @@ -477,11 +477,11 @@ func (n *PagerDuty) notifyV1( Details: details, } - url, err := url.Parse("https://events.pagerduty.com/generic/2010-04-15/create_event.json") + apiURL, err := url.Parse("https://events.pagerduty.com/generic/2010-04-15/create_event.json") if err != nil { return false, err } - n.conf.URL = &config.URL{url} + n.conf.URL = &config.URL{apiURL} if eventType == pagerDutyEventTrigger { msg.Client = tmpl(n.conf.Client) @@ -802,9 +802,9 @@ func (n *Hipchat) Notify(ctx context.Context, as ...*types.Alert) (bool, error) tmplText = tmplText(n.tmpl, data, &err) tmplHTML = tmplHTML(n.tmpl, data, &err) roomid = tmplText(n.conf.RoomID) - url = *n.conf.APIURL + apiURL = *n.conf.APIURL ) - url.Path += fmt.Sprintf("v2/room/%s/notification?auth_token=%s", roomid, n.conf.AuthToken) + apiURL.Path += fmt.Sprintf("v2/room/%s/notification?auth_token=%s", roomid, n.conf.AuthToken) if n.conf.MessageFormat == "html" { msg = tmplHTML(n.conf.Message) @@ -833,7 +833,7 @@ func (n *Hipchat) Notify(ctx context.Context, as ...*types.Alert) (bool, error) return false, err } - resp, err := ctxhttp.Post(ctx, c, url.String(), contentTypeJSON, &buf) + resp, err := ctxhttp.Post(ctx, c, apiURL.String(), contentTypeJSON, &buf) if err != nil { return true, err } From d440d3ac302c1c8248250a8d123069636d6d3f8f Mon Sep 17 00:00:00 2001 From: Simon Pasquier Date: Tue, 24 Jul 2018 17:57:28 +0200 Subject: [PATCH 5/5] Make a deep-copy of URL struct Signed-off-by: Simon Pasquier --- config/config.go | 6 ++++++ notify/impl.go | 10 +++++----- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/config/config.go b/config/config.go index 54413f8d11..b6b23edf7e 100644 --- a/config/config.go +++ b/config/config.go @@ -56,6 +56,12 @@ type URL struct { *url.URL } +// Copy makes a deep-copy of the struct. +func (u *URL) Copy() *URL { + v := *u.URL + return &URL{&v} +} + // MarshalYAML implements the yaml.Marshaler interface for URL. func (u URL) MarshalYAML() (interface{}, error) { if u.URL != nil { diff --git a/notify/impl.go b/notify/impl.go index a13e7d7017..5e20576b1f 100644 --- a/notify/impl.go +++ b/notify/impl.go @@ -802,7 +802,7 @@ func (n *Hipchat) Notify(ctx context.Context, as ...*types.Alert) (bool, error) tmplText = tmplText(n.tmpl, data, &err) tmplHTML = tmplHTML(n.tmpl, data, &err) roomid = tmplText(n.conf.RoomID) - apiURL = *n.conf.APIURL + apiURL = n.conf.APIURL.Copy() ) apiURL.Path += fmt.Sprintf("v2/room/%s/notification?auth_token=%s", roomid, n.conf.AuthToken) @@ -923,7 +923,7 @@ func (n *Wechat) Notify(ctx context.Context, as ...*types.Alert) (bool, error) { return false, fmt.Errorf("templating error: %s", err) } - u := *n.conf.APIURL + u := n.conf.APIURL.Copy() u.Path += "gettoken" u.RawQuery = parameters.Encode() @@ -974,7 +974,7 @@ func (n *Wechat) Notify(ctx context.Context, as ...*types.Alert) (bool, error) { return false, err } - postMessageURL := *n.conf.APIURL + postMessageURL := n.conf.APIURL.Copy() postMessageURL.Path += "message/send?access_token=" + n.accessToken req, err := http.NewRequest(http.MethodPost, postMessageURL.String(), &buf) @@ -1096,7 +1096,7 @@ func (n *OpsGenie) createRequest(ctx context.Context, as ...*types.Alert) (*http var ( msg interface{} - apiURL = *n.conf.APIURL + apiURL = n.conf.APIURL.Copy() alias = hashKey(key) alerts = types.Alerts(as...) ) @@ -1207,7 +1207,7 @@ func (n *VictorOps) Notify(ctx context.Context, as ...*types.Alert) (bool, error alerts = types.Alerts(as...) data = n.tmpl.Data(receiverName(ctx, n.logger), groupLabels(ctx, n.logger), as...) tmpl = tmplText(n.tmpl, data, &err) - apiURL = *n.conf.APIURL + apiURL = n.conf.APIURL.Copy() messageType = tmpl(n.conf.MessageType) stateMessage = tmpl(n.conf.StateMessage) )