Add support for reading VictorOps API key from file (prometheus#3111)

* Read VictorOps API key from file * Update documentation * Improve tests Signed-off-by: Oktarian T-B <oktariantilneybassett@improbable.io> Signed-off-by: Yijie Qin <qinyijie@amazon.com>
qinxx108 · Dec 13, 2022 · adc1970 · adc1970
1 parent d2a1ca4
commit adc1970
Show file tree

Hide file tree

Showing 9 changed files with 174 additions and 17 deletions.
diff --git a/config/config.go b/config/config.go
@@ -335,6 +335,10 @@ func (c *Config) UnmarshalYAML(unmarshal func(interface{}) error) error {
 		return fmt.Errorf("at most one of opsgenie_api_key & opsgenie_api_key_file must be configured")
 	}
 
+	if c.Global.VictorOpsAPIKey != "" && len(c.Global.VictorOpsAPIKeyFile) > 0 {
+		return fmt.Errorf("at most one of victorops_api_key & victorops_api_key_file must be configured")
+	}
+
 	if len(c.Global.SMTPAuthPassword) > 0 && len(c.Global.SMTPAuthPasswordFile) > 0 {
 		return fmt.Errorf("at most one of smtp_auth_password & smtp_auth_password_file must be configured")
 	}
@@ -476,11 +480,12 @@ func (c *Config) UnmarshalYAML(unmarshal func(interface{}) error) error {
 			if !strings.HasSuffix(voc.APIURL.Path, "/") {
 				voc.APIURL.Path += "/"
 			}
-			if voc.APIKey == "" {
-				if c.Global.VictorOpsAPIKey == "" {
+			if voc.APIKey == "" && len(voc.APIKeyFile) == 0 {
+				if c.Global.VictorOpsAPIKey == "" && len(c.Global.VictorOpsAPIKeyFile) == 0 {
 					return fmt.Errorf("no global VictorOps API Key set")
 				}
 				voc.APIKey = c.Global.VictorOpsAPIKey
+				voc.APIKeyFile = c.Global.VictorOpsAPIKeyFile
 			}
 		}
 		for _, sns := range rcv.SNSConfigs {
@@ -718,6 +723,7 @@ type GlobalConfig struct {
 	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"`
+	VictorOpsAPIKeyFile  string     `yaml:"victorops_api_key_file,omitempty" json:"victorops_api_key_file,omitempty"`
 	TelegramAPIUrl       *URL       `yaml:"telegram_api_url,omitempty" json:"telegram_api_url,omitempty"`
 }
 

diff --git a/config/config_test.go b/config/config_test.go
@@ -1016,11 +1016,38 @@ func TestVictorOpsDefaultAPIKey(t *testing.T) {
 	}
 
 	defaultKey := conf.Global.VictorOpsAPIKey
+	overrideKey := Secret("qwe456")
 	if defaultKey != conf.Receivers[0].VictorOpsConfigs[0].APIKey {
 		t.Fatalf("Invalid victorops key: %s\nExpected: %s", conf.Receivers[0].VictorOpsConfigs[0].APIKey, defaultKey)
 	}
-	if defaultKey == conf.Receivers[1].VictorOpsConfigs[0].APIKey {
-		t.Errorf("Invalid victorops key: %s\nExpected: %s", conf.Receivers[0].VictorOpsConfigs[0].APIKey, "qwe456")
+	if overrideKey != conf.Receivers[1].VictorOpsConfigs[0].APIKey {
+		t.Errorf("Invalid victorops key: %s\nExpected: %s", conf.Receivers[0].VictorOpsConfigs[0].APIKey, string(overrideKey))
+	}
+}
+
+func TestVictorOpsDefaultAPIKeyFile(t *testing.T) {
+	conf, err := LoadFile("testdata/conf.victorops-default-apikey-file.yml")
+	if err != nil {
+		t.Fatalf("Error parsing %s: %s", "testdata/conf.victorops-default-apikey-file.yml", err)
+	}
+
+	defaultKey := conf.Global.VictorOpsAPIKeyFile
+	overrideKey := "/override_file"
+	if defaultKey != conf.Receivers[0].VictorOpsConfigs[0].APIKeyFile {
+		t.Fatalf("Invalid VictorOps key_file: %s\nExpected: %s", conf.Receivers[0].VictorOpsConfigs[0].APIKeyFile, defaultKey)
+	}
+	if overrideKey != conf.Receivers[1].VictorOpsConfigs[0].APIKeyFile {
+		t.Errorf("Invalid VictorOps key_file: %s\nExpected: %s", conf.Receivers[0].VictorOpsConfigs[0].APIKeyFile, overrideKey)
+	}
+}
+
+func TestVictorOpsBothAPIKeyAndFile(t *testing.T) {
+	_, err := LoadFile("testdata/conf.victorops-both-file-and-apikey.yml")
+	if err == nil {
+		t.Fatalf("Expected an error parsing %s: %s", "testdata/conf.victorops-both-file-and-apikey.yml", err)
+	}
+	if err.Error() != "at most one of victorops_api_key & victorops_api_key_file must be configured" {
+		t.Errorf("Expected: %s\nGot: %s", "at most one of victorops_api_key & victorops_api_key_file must be configured", err.Error())
 	}
 }
 

diff --git a/config/notifiers.go b/config/notifiers.go
@@ -540,7 +540,7 @@ type VictorOpsConfig struct {
 	HTTPConfig *commoncfg.HTTPClientConfig `yaml:"http_config,omitempty" json:"http_config,omitempty"`
 
 	APIKey            Secret            `yaml:"api_key,omitempty" json:"api_key,omitempty"`
-	APIKeyFile        Secret            `yaml:"api_key_file,omitempty" json:"api_key_file,omitempty"`
+	APIKeyFile        string            `yaml:"api_key_file,omitempty" json:"api_key_file,omitempty"`
 	APIURL            *URL              `yaml:"api_url" json:"api_url"`
 	RoutingKey        string            `yaml:"routing_key" json:"routing_key"`
 	MessageType       string            `yaml:"message_type" json:"message_type"`
@@ -560,6 +560,9 @@ func (c *VictorOpsConfig) UnmarshalYAML(unmarshal func(interface{}) error) error
 	if c.RoutingKey == "" {
 		return fmt.Errorf("missing Routing key in VictorOps config")
 	}
+	if c.APIKey != "" && len(c.APIKeyFile) > 0 {
+		return fmt.Errorf("at most one of api_key & api_key_file must be configured")
+	}
 
 	reservedFields := []string{"routing_key", "message_type", "state_message", "entity_display_name", "monitoring_tool", "entity_id", "entity_state"}
 

diff --git a/config/notifiers_test.go b/config/notifiers_test.go
@@ -291,21 +291,54 @@ http_config:
 	}
 }
 
-func TestVictorOpsRoutingKeyIsPresent(t *testing.T) {
-	in := `
+func TestVictorOpsConfiguration(t *testing.T) {
+	t.Run("valid configuration", func(t *testing.T) {
+		in := `
+routing_key: test
+api_key_file: /global_file
+`
+		var cfg VictorOpsConfig
+		err := yaml.UnmarshalStrict([]byte(in), &cfg)
+		if err != nil {
+			t.Fatalf("no error was expected:\n%v", err)
+		}
+	})
+
+	t.Run("routing key is missing", func(t *testing.T) {
+		in := `
 routing_key: ''
 `
-	var cfg VictorOpsConfig
-	err := yaml.UnmarshalStrict([]byte(in), &cfg)
+		var cfg VictorOpsConfig
+		err := yaml.UnmarshalStrict([]byte(in), &cfg)
 
-	expected := "missing Routing key in VictorOps config"
+		expected := "missing Routing key in VictorOps config"
 
-	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())
-	}
+		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())
+		}
+	})
+
+	t.Run("api_key and api_key_file both defined", func(t *testing.T) {
+		in := `
+routing_key: test
+api_key: xyz
+api_key_file: /global_file
+`
+		var cfg VictorOpsConfig
+		err := yaml.UnmarshalStrict([]byte(in), &cfg)
+
+		expected := "at most one of api_key & api_key_file must be configured"
+
+		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())
+		}
+	})
 }
 
 func TestVictorOpsCustomFieldsValidation(t *testing.T) {

diff --git a/config/testdata/conf.victorops-both-file-and-apikey.yml b/config/testdata/conf.victorops-both-file-and-apikey.yml
@@ -0,0 +1,21 @@
+global:
+  victorops_api_key: asd132
+  victorops_api_key_file: '/global_file'
+route:
+  group_by: ['alertname', 'cluster', 'service']
+  group_wait: 30s
+  group_interval: 5m
+  repeat_interval: 3h
+  receiver: team-Y-victorops
+  routes:
+    - match:
+        service: foo
+      receiver: team-X-victorops
+receivers:
+  - name: 'team-X-victorops'
+    victorops_configs:
+      - routing_key: 'team-X'
+  - name: 'team-Y-victorops'
+    victorops_configs:
+      - routing_key: 'team-Y'
+        api_key: qwe456
diff --git a/config/testdata/conf.victorops-default-apikey-file.yml b/config/testdata/conf.victorops-default-apikey-file.yml
@@ -0,0 +1,20 @@
+global:
+  victorops_api_key_file: '/global_file'
+route:
+  group_by: ['alertname', 'cluster', 'service']
+  group_wait: 30s
+  group_interval: 5m
+  repeat_interval: 3h
+  receiver: team-Y-victorops
+  routes:
+    - match:
+        service: foo
+      receiver: team-X-victorops
+receivers:
+  - name: 'team-X-victorops'
+    victorops_configs:
+      - routing_key: 'team-X'
+  - name: 'team-Y-victorops'
+    victorops_configs:
+      - routing_key: 'team-Y'
+        api_key_file: /override_file
diff --git a/docs/configuration.md b/docs/configuration.md
@@ -85,6 +85,7 @@ global:
   [ slack_api_url: <secret> ]
   [ slack_api_url_file: <filepath> ]
   [ victorops_api_key: <secret> ]
+  [ victorops_api_key_file: <filepath> ]
   [ victorops_api_url: <string> | default = "https://alert.victorops.com/integrations/generic/20131114/alert/" ]
   [ pagerduty_url: <string> | default = "https://events.pagerduty.com/v2/enqueue" ]
   [ opsgenie_api_key: <secret> ]
@@ -971,8 +972,13 @@ VictorOps notifications are sent out via the [VictorOps API](https://help.victor
 [ send_resolved: <boolean> | default = true ]
 
 # The API key to use when talking to the VictorOps API.
+# It is mutually exclusive with `api_key_file`.
 [ api_key: <secret> | default = global.victorops_api_key ]
 
+# Reads the API key to use when talking to the VictorOps API from a file.
+# It is mutually exclusive with `api_key`.
+[ api_key_file: <filepath> | default = global.victorops_api_key_file ]
+
 # The VictorOps API URL.
 [ api_url: <string> | default = global.victorops_api_url ]
 

diff --git a/notify/victorops/victorops.go b/notify/victorops/victorops.go
@@ -19,9 +19,12 @@ import (
 	"encoding/json"
 	"fmt"
 	"net/http"
+	"os"
+	"strings"
 
 	"github.com/go-kit/log"
 	"github.com/go-kit/log/level"
+	"github.com/pkg/errors"
 	commoncfg "github.com/prometheus/common/config"
 	"github.com/prometheus/common/model"
 
@@ -70,7 +73,19 @@ func (n *Notifier) Notify(ctx context.Context, as ...*types.Alert) (bool, error)
 		tmpl   = notify.TmplText(n.tmpl, data, &err)
 		apiURL = n.conf.APIURL.Copy()
 	)
-	apiURL.Path += fmt.Sprintf("%s/%s", n.conf.APIKey, tmpl(n.conf.RoutingKey))
+
+	var apiKey string
+	if n.conf.APIKey != "" {
+		apiKey = string(n.conf.APIKey)
+	} else {
+		content, fileErr := os.ReadFile(n.conf.APIKeyFile)
+		if fileErr != nil {
+			return false, errors.Wrap(fileErr, "failed to read API key from file")
+		}
+		apiKey = strings.TrimSpace(string(content))
+	}
+
+	apiURL.Path += fmt.Sprintf("%s/%s", apiKey, tmpl(n.conf.RoutingKey))
 	if err != nil {
 		return false, fmt.Errorf("templating error: %s", err)
 	}

diff --git a/notify/victorops/victorops_test.go b/notify/victorops/victorops_test.go
@@ -20,6 +20,7 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"net/url"
+	"os"
 	"testing"
 	"time"
 
@@ -119,6 +120,30 @@ func TestVictorOpsRedactedURL(t *testing.T) {
 	test.AssertNotifyLeaksNoSecret(ctx, t, notifier, secret)
 }
 
+func TestVictorOpsReadingApiKeyFromFile(t *testing.T) {
+	key := "key"
+	f, err := os.CreateTemp("", "victorops_test")
+	require.NoError(t, err, "creating temp file failed")
+	_, err = f.WriteString(key)
+	require.NoError(t, err, "writing to temp file failed")
+
+	ctx, u, fn := test.GetContextWithCancelingURL()
+	defer fn()
+
+	notifier, err := New(
+		&config.VictorOpsConfig{
+			APIURL:     &config.URL{URL: u},
+			APIKeyFile: f.Name(),
+			HTTPConfig: &commoncfg.HTTPClientConfig{},
+		},
+		test.CreateTmpl(t),
+		log.NewNopLogger(),
+	)
+	require.NoError(t, err)
+
+	test.AssertNotifyLeaksNoSecret(ctx, t, notifier, key)
+}
+
 func TestVictorOpsTemplating(t *testing.T) {
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		dec := json.NewDecoder(r.Body)
@@ -181,6 +206,7 @@ func TestVictorOpsTemplating(t *testing.T) {
 		t.Run(tc.name, func(t *testing.T) {
 			tc.cfg.HTTPConfig = &commoncfg.HTTPClientConfig{}
 			tc.cfg.APIURL = &config.URL{URL: u}
+			tc.cfg.APIKey = "test"
 			vo, err := New(tc.cfg, test.CreateTmpl(t), log.NewNopLogger())
 			require.NoError(t, err)
 			ctx := context.Background()