From 3ab5bbca9733cc7b537aaf2d3b4552f141000ad4 Mon Sep 17 00:00:00 2001 From: Jason Roberts Date: Sun, 17 Jun 2018 09:00:00 -0500 Subject: [PATCH 1/6] Support adding custom fields to VictorOps notifications Signed-off-by: Jason Roberts --- config/notifiers.go | 15 +++++---- notify/impl.go | 82 ++++++++++++++++++++++++++++----------------- notify/impl_test.go | 49 +++++++++++++++++++++++++++ 3 files changed, 109 insertions(+), 37 deletions(-) diff --git a/config/notifiers.go b/config/notifiers.go index 16fd634774..ab8f2eca36 100644 --- a/config/notifiers.go +++ b/config/notifiers.go @@ -486,13 +486,14 @@ type VictorOpsConfig struct { HTTPConfig *commoncfg.HTTPClientConfig `yaml:"http_config,omitempty" json:"http_config,omitempty"` - APIKey Secret `yaml:"api_key" json:"api_key"` - 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"` - EntityDisplayName string `yaml:"entity_display_name" json:"entity_display_name"` - MonitoringTool string `yaml:"monitoring_tool" json:"monitoring_tool"` + APIKey Secret `yaml:"api_key" json:"api_key"` + 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"` + EntityDisplayName string `yaml:"entity_display_name" json:"entity_display_name"` + MonitoringTool string `yaml:"monitoring_tool" json:"monitoring_tool"` + CustomFields map[string]string `yaml:"custom_fields,omitempty" json:"custom_fields,omitempty"` } // UnmarshalYAML implements the yaml.Unmarshaler interface. diff --git a/notify/impl.go b/notify/impl.go index c42d64b03a..f0007f3d2c 100644 --- a/notify/impl.go +++ b/notify/impl.go @@ -1285,16 +1285,38 @@ const ( victorOpsEventResolve = "RECOVERY" ) -type victorOpsMessage struct { - MessageType string `json:"message_type"` - EntityID string `json:"entity_id"` - EntityDisplayName string `json:"entity_display_name"` - StateMessage string `json:"state_message"` - MonitoringTool string `json:"monitoring_tool"` -} - // Notify implements the Notifier interface. func (n *VictorOps) Notify(ctx context.Context, as ...*types.Alert) (bool, error) { + + var err error + var ( + 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)) + ) + + c, err := commoncfg.NewClientFromConfig(*n.conf.HTTPConfig, "victorops") + if err != nil { + return false, err + } + + buf, err := n.createVictorOpsPayload(ctx, as...) + if err != nil { + return true, err + } + + resp, err := ctxhttp.Post(ctx, c, apiURL, contentTypeJSON, buf) + if err != nil { + return true, err + } + + defer resp.Body.Close() + + return n.retry(resp.StatusCode) +} + +//Create the json payload to be sent to victorops api +func (n *VictorOps) createVictorOpsPayload(ctx context.Context, as ...*types.Alert) (*bytes.Buffer, error) { victorOpsAllowedEvents := map[string]bool{ "INFO": true, "WARNING": true, @@ -1303,7 +1325,7 @@ func (n *VictorOps) Notify(ctx context.Context, as ...*types.Alert) (bool, error key, ok := GroupKey(ctx) if !ok { - return false, fmt.Errorf("group key missing") + return nil, fmt.Errorf("group key missing") } var err error @@ -1330,36 +1352,36 @@ func (n *VictorOps) Notify(ctx context.Context, as ...*types.Alert) (bool, error level.Debug(n.logger).Log("msg", "Truncated stateMessage due to VictorOps stateMessage limit", "truncated_state_message", stateMessage, "incident", key) } - msg := &victorOpsMessage{ - MessageType: messageType, - EntityID: hashKey(key), - EntityDisplayName: tmpl(n.conf.EntityDisplayName), - StateMessage: stateMessage, - MonitoringTool: tmpl(n.conf.MonitoringTool), + msg := map[string]string{ + "message_type": messageType, + "entity_id": hashKey(key), + "entity_display_name": tmpl(n.conf.EntityDisplayName), + "state_message": stateMessage, + "monitoring_tool": tmpl(n.conf.MonitoringTool), } if err != nil { - return false, fmt.Errorf("templating error: %s", err) + return nil, fmt.Errorf("templating error: %s", err) } - var buf bytes.Buffer - if err := json.NewEncoder(&buf).Encode(msg); err != nil { - return false, err - } + for k, v := range n.conf.CustomFields { - c, err := commoncfg.NewClientFromConfig(*n.conf.HTTPConfig, "victorops") - if err != nil { - return false, err + //Validate if the custom field is not one of the fixed fields above. + if _, ok := msg[k]; !ok { + msg[k] = tmpl(v) + if err != nil { + return nil, fmt.Errorf("templating error: %s", err) + } + } else { + level.Debug(n.logger).Log("msg", "Ignoring custom field %q as it is already defined as a fixed field", "omitted_field", k, "incident", key) + } } - resp, err := post(ctx, c, apiURL.String(), contentTypeJSON, &buf) - if err != nil { - return true, err + var buf bytes.Buffer + if err := json.NewEncoder(&buf).Encode(msg); err != nil { + return nil, err } - - defer resp.Body.Close() - - return n.retry(resp.StatusCode) + return &buf, nil } func (n *VictorOps) retry(statusCode int) (bool, error) { diff --git a/notify/impl_test.go b/notify/impl_test.go index 3ecd6a665d..405819b64c 100644 --- a/notify/impl_test.go +++ b/notify/impl_test.go @@ -15,6 +15,7 @@ package notify import ( "context" + "encoding/json" "fmt" "io/ioutil" "net/http" @@ -322,3 +323,51 @@ func TestEmailConfigMissingAuthParam(t *testing.T) { require.Error(t, err) require.Equal(t, err.Error(), "missing password for PLAIN auth mechanism; missing password for LOGIN auth mechanism") } + +func TestVictorOpsCustomFields(t *testing.T) { + logger := log.NewNopLogger() + tmpl := createTmpl(t) + conf := &config.VictorOpsConfig{ + APIKey: `12345`, + APIURL: `http://nowhere.com`, + EntityDisplayName: `{{ .CommonLabels.Message }}`, + StateMessage: `{{ .CommonLabels.Message }}`, + RoutingKey: `test`, + MessageType: ``, + MonitoringTool: `AM`, + CustomFields: map[string]string{ + "Field_A": "{{ .CommonLabels.Message }}", + //A field that should be ignored + "state_message": "discarded", + }, + } + + notifier := NewVictorOps(conf, tmpl, logger) + + ctx := context.Background() + ctx = WithGroupKey(ctx, "1") + + alert := &types.Alert{ + Alert: model.Alert{ + Labels: model.LabelSet{ + "Message": "message", + }, + StartsAt: time.Now(), + EndsAt: time.Now().Add(time.Hour), + }, + } + + msg, err := notifier.createVictorOpsPayload(ctx, alert) + require.NoError(t, err) + + var m map[string]string + err = json.Unmarshal(msg.Bytes(), &m) + + require.NoError(t, err) + + //Verify the invalid custom field didn't override the static field + require.Equal(t, "message", m["state_message"]) + + //Verify that a custom field was added to the payload and templatized + require.Equal(t, "message", m["Field_A"]) +} From 863d52c69349764cb021d7f393a2dd708e338db5 Mon Sep 17 00:00:00 2001 From: Jason Roberts Date: Mon, 18 Jun 2018 09:09:46 -0500 Subject: [PATCH 2/6] Response to feedback Signed-off-by: Jason Roberts --- notify/impl.go | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/notify/impl.go b/notify/impl.go index f0007f3d2c..33096fa679 100644 --- a/notify/impl.go +++ b/notify/impl.go @@ -1315,7 +1315,7 @@ func (n *VictorOps) Notify(ctx context.Context, as ...*types.Alert) (bool, error return n.retry(resp.StatusCode) } -//Create the json payload to be sent to victorops api +// Create the json payload to be sent to victorops api func (n *VictorOps) createVictorOpsPayload(ctx context.Context, as ...*types.Alert) (*bytes.Buffer, error) { victorOpsAllowedEvents := map[string]bool{ "INFO": true, @@ -1366,14 +1366,14 @@ func (n *VictorOps) createVictorOpsPayload(ctx context.Context, as ...*types.Ale for k, v := range n.conf.CustomFields { - //Validate if the custom field is not one of the fixed fields above. - if _, ok := msg[k]; !ok { - msg[k] = tmpl(v) - if err != nil { - return nil, fmt.Errorf("templating error: %s", err) - } - } else { - level.Debug(n.logger).Log("msg", "Ignoring custom field %q as it is already defined as a fixed field", "omitted_field", k, "incident", key) + // Validate if the custom field is not one of the fixed fields above. + if _, ok := msg[k]; ok { + level.Debug(n.logger).Log("msg", "Ignoring custom field as it is already defined as a fixed field", "omitted_field", k, "incident", key) + continue + } + msg[k] = tmpl(v) + if err != nil { + return nil, fmt.Errorf("templating error: %s", err) } } From e108c15b4e5c78791ff699cba8676f01da267103 Mon Sep 17 00:00:00 2001 From: Jason Roberts Date: Sat, 23 Jun 2018 17:58:21 -0500 Subject: [PATCH 3/6] Added logic to validate victorops custom fields to config load time Signed-off-by: Jason Roberts --- config/notifiers.go | 9 +++++++++ config/notifiers_test.go | 43 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+) diff --git a/config/notifiers.go b/config/notifiers.go index ab8f2eca36..a95fa52fe7 100644 --- a/config/notifiers.go +++ b/config/notifiers.go @@ -506,6 +506,15 @@ func (c *VictorOpsConfig) UnmarshalYAML(unmarshal func(interface{}) error) error if c.RoutingKey == "" { return fmt.Errorf("missing Routing key in VictorOps config") } + + reservedFields := []string{"routing_key", "message_type", "state_message", "entity_display_name", "monitoring_tool", "entity_id", "entity_state"} + + for _, v := range reservedFields { + if _, ok := c.CustomFields[v]; ok { + return fmt.Errorf("VictorOps config contains custom field %s which cannot be used as it conflicts with the fixed/static fields", v) + } + } + return nil } diff --git a/config/notifiers_test.go b/config/notifiers_test.go index a8e37bab76..a91c874a40 100644 --- a/config/notifiers_test.go +++ b/config/notifiers_test.go @@ -287,6 +287,49 @@ routing_key: '' } } +func TestVictorOpsCustomFieldsValidation(t *testing.T) { + in := ` +routing_key: 'test' +custom_fields: + entity_state: 'state_message' +` + var cfg VictorOpsConfig + err := yaml.UnmarshalStrict([]byte(in), &cfg) + + expected := "VictorOps config contains custom field entity_state which cannot be used as it conflicts with the fixed/static fields" + + if err == nil { + t.Fatalf("no error returned, expected:\n%v", expected) + } + if err.Error() != expected { + t.Errorf("\nexpected:\n%v\ngot:\n%v", expected, err.Error()) + } + + in = ` +routing_key: 'test' +custom_fields: + my_special_field: 'special_label' +` + + err = yaml.UnmarshalStrict([]byte(in), &cfg) + + expected = "special_label" + + if err != nil { + t.Fatalf("Unexpected error returned, got:\n%v", err.Error()) + } + + val, ok := cfg.CustomFields["my_special_field"] + + if !ok { + t.Fatalf("Expected Custom Field to have value %v set, field is empty", expected) + } + if val != expected { + t.Errorf("\nexpected custom field my_special_field value:\n%v\ngot:\n%v", expected, val) + } + +} + func TestPushoverUserKeyIsPresent(t *testing.T) { in := ` user_key: '' From 2ce00483ae2cf2ff3e96b60537d0db5a464eba03 Mon Sep 17 00:00:00 2001 From: Jason Roberts Date: Mon, 25 Jun 2018 15:06:43 -0500 Subject: [PATCH 4/6] Cleanup victorops notifier of logic duplicated in config check Signed-off-by: Jason Roberts --- notify/impl.go | 7 +------ notify/impl_test.go | 5 ----- 2 files changed, 1 insertion(+), 11 deletions(-) diff --git a/notify/impl.go b/notify/impl.go index 33096fa679..173e8076e7 100644 --- a/notify/impl.go +++ b/notify/impl.go @@ -1364,13 +1364,8 @@ func (n *VictorOps) createVictorOpsPayload(ctx context.Context, as ...*types.Ale return nil, fmt.Errorf("templating error: %s", err) } + // Add custom fields to the payload. for k, v := range n.conf.CustomFields { - - // Validate if the custom field is not one of the fixed fields above. - if _, ok := msg[k]; ok { - level.Debug(n.logger).Log("msg", "Ignoring custom field as it is already defined as a fixed field", "omitted_field", k, "incident", key) - continue - } msg[k] = tmpl(v) if err != nil { return nil, fmt.Errorf("templating error: %s", err) diff --git a/notify/impl_test.go b/notify/impl_test.go index 405819b64c..ba7f2f1cef 100644 --- a/notify/impl_test.go +++ b/notify/impl_test.go @@ -337,8 +337,6 @@ func TestVictorOpsCustomFields(t *testing.T) { MonitoringTool: `AM`, CustomFields: map[string]string{ "Field_A": "{{ .CommonLabels.Message }}", - //A field that should be ignored - "state_message": "discarded", }, } @@ -365,9 +363,6 @@ func TestVictorOpsCustomFields(t *testing.T) { require.NoError(t, err) - //Verify the invalid custom field didn't override the static field - require.Equal(t, "message", m["state_message"]) - //Verify that a custom field was added to the payload and templatized require.Equal(t, "message", m["Field_A"]) } From 4952cc0a4baf539c7dff2cdb4e5ce4e26a124aa1 Mon Sep 17 00:00:00 2001 From: Jason Roberts Date: Sun, 9 Dec 2018 22:19:26 -0600 Subject: [PATCH 5/6] rebase and further cleanup from feedback Signed-off-by: Jason Roberts --- notify/impl.go | 16 ++++++++-------- notify/impl_test.go | 9 +++++++-- 2 files changed, 15 insertions(+), 10 deletions(-) diff --git a/notify/impl.go b/notify/impl.go index 173e8076e7..eb4459305a 100644 --- a/notify/impl.go +++ b/notify/impl.go @@ -1292,8 +1292,9 @@ func (n *VictorOps) Notify(ctx context.Context, as ...*types.Alert) (bool, error var ( 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)) + apiURL = n.conf.APIURL.Copy() ) + apiURL.Path += fmt.Sprintf("%s/%s", n.conf.APIKey, tmpl(n.conf.RoutingKey)) c, err := commoncfg.NewClientFromConfig(*n.conf.HTTPConfig, "victorops") if err != nil { @@ -1305,7 +1306,7 @@ func (n *VictorOps) Notify(ctx context.Context, as ...*types.Alert) (bool, error return true, err } - resp, err := ctxhttp.Post(ctx, c, apiURL, contentTypeJSON, buf) + resp, err := post(ctx, c, apiURL.String(), contentTypeJSON, buf) if err != nil { return true, err } @@ -1315,7 +1316,7 @@ func (n *VictorOps) Notify(ctx context.Context, as ...*types.Alert) (bool, error return n.retry(resp.StatusCode) } -// Create the json payload to be sent to victorops api +// Create the json payload to be sent to victorops api. func (n *VictorOps) createVictorOpsPayload(ctx context.Context, as ...*types.Alert) (*bytes.Buffer, error) { victorOpsAllowedEvents := map[string]bool{ "INFO": true, @@ -1330,14 +1331,13 @@ func (n *VictorOps) createVictorOpsPayload(ctx context.Context, as ...*types.Ale 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 = n.conf.APIURL.Copy() + alerts = types.Alerts(as...) + data = n.tmpl.Data(receiverName(ctx, n.logger), groupLabels(ctx, n.logger), as...) + tmpl = tmplText(n.tmpl, data, &err) + 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 diff --git a/notify/impl_test.go b/notify/impl_test.go index ba7f2f1cef..a123fa3558 100644 --- a/notify/impl_test.go +++ b/notify/impl_test.go @@ -327,9 +327,14 @@ func TestEmailConfigMissingAuthParam(t *testing.T) { func TestVictorOpsCustomFields(t *testing.T) { logger := log.NewNopLogger() tmpl := createTmpl(t) + + url, err := url.Parse("http://nowhere.com") + + require.NoError(t, err, "unexpected error parsing mock url") + conf := &config.VictorOpsConfig{ APIKey: `12345`, - APIURL: `http://nowhere.com`, + APIURL: &config.URL{url}, EntityDisplayName: `{{ .CommonLabels.Message }}`, StateMessage: `{{ .CommonLabels.Message }}`, RoutingKey: `test`, @@ -363,6 +368,6 @@ func TestVictorOpsCustomFields(t *testing.T) { require.NoError(t, err) - //Verify that a custom field was added to the payload and templatized + // Verify that a custom field was added to the payload and templatized. require.Equal(t, "message", m["Field_A"]) } From b23f2c6c65fbd1c0e0904e605fce1bfcac01394a Mon Sep 17 00:00:00 2001 From: Jason Roberts Date: Mon, 10 Dec 2018 09:05:28 -0600 Subject: [PATCH 6/6] another grammer fix Signed-off-by: Jason Roberts --- notify/impl.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/notify/impl.go b/notify/impl.go index eb4459305a..bf6142cced 100644 --- a/notify/impl.go +++ b/notify/impl.go @@ -1316,7 +1316,7 @@ func (n *VictorOps) Notify(ctx context.Context, as ...*types.Alert) (bool, error return n.retry(resp.StatusCode) } -// Create the json payload to be sent to victorops api. +// Create the JSON payload to be sent to the VictorOps API. func (n *VictorOps) createVictorOpsPayload(ctx context.Context, as ...*types.Alert) (*bytes.Buffer, error) { victorOpsAllowedEvents := map[string]bool{ "INFO": true,