Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for reading VictorOps API key from file #3111

Merged
merged 8 commits into from Oct 25, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 8 additions & 2 deletions config/config.go
Expand Up @@ -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")
}
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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"`
}

Expand Down
31 changes: 29 additions & 2 deletions config/config_test.go
Expand Up @@ -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())
}
}

Expand Down
5 changes: 4 additions & 1 deletion config/notifiers.go
Expand Up @@ -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"`
Expand All @@ -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"}

Expand Down
55 changes: 44 additions & 11 deletions config/notifiers_test.go
Expand Up @@ -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) {
Expand Down
21 changes: 21 additions & 0 deletions 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
20 changes: 20 additions & 0 deletions 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
6 changes: 6 additions & 0 deletions docs/configuration.md
Expand Up @@ -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> ]
Expand Down Expand Up @@ -973,8 +974,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.
OktarianTB marked this conversation as resolved.
Show resolved Hide resolved
# 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 ]

Expand Down
17 changes: 16 additions & 1 deletion notify/victorops/victorops.go
Expand Up @@ -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"

Expand Down Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might miss the reason why we need a new variable name but if there's none, I'd prefer to keep err :)

Suggested change
content, fileErr := os.ReadFile(n.conf.APIKeyFile)
content, err := os.ReadFile(n.conf.APIKeyFile)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Declaring it as err would be a shadow declaration of a previously defined err variable used for templating errors: 馃檪

var err error
var (
data = notify.GetTemplateData(ctx, n.tmpl, as, n.logger)
tmpl = notify.TmplText(n.tmpl, data, &err)
apiURL = n.conf.APIURL.Copy()
)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm it shouldn't cause a direct problem as long as tmpl isn't called in this branch but I don't mind keeping it explicit.
Other notifiers declare a tmplErrvariable to make it more obvious that the error variable shouldn't be reused for anything else but I feel that it's still a bit obfuscated (it's easy to call the templating function and forget checking the error). The general approach with a anonymous function might need to be revised but this isn't relevant to this change :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense! I agree that the templating error handling is not the best, just need to be careful I guess :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any other changes needed for this PR btw? 馃弫

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)
}
Expand Down
26 changes: 26 additions & 0 deletions notify/victorops/victorops_test.go
Expand Up @@ -20,6 +20,7 @@ import (
"net/http"
"net/http/httptest"
"net/url"
"os"
"testing"
"time"

Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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()
Expand Down