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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace notify.Factory with a build function that accepts typed configuration #63

Merged
merged 17 commits into from
Mar 28, 2023
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion logging/log.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
package logging

type LoggerFactory func(ctx ...interface{}) Logger
type LoggerFactory func(loggerName string, ctx ...interface{}) Logger
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this makes it clear that first element is always logger name whereas the remaining go to context (that's how it is implemented in Grafana)

Copy link
Contributor

Choose a reason for hiding this comment

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

I like this change, but wouldn't it mean that we'll be using a different interface than the one defined in Grafana's infra package? In order to use the logger, we would have to add this string as the first item in the ctx slice.
I don't think it's a big deal, just want to make sure we're aware of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regularly, in the logging context is a slice of tuples. go-kit adds {Missing} if slice size is odd. However, In Grafana logger assumes that the first element is not a tuple's label but a logger name and automatically adds the logger label. However, this package can (and will) use a different logger implementation but usages of this interface still assume that the first element of the context is a logger. This change makes this interface more explicit.

I like this change, but wouldn't it mean that we'll be using a different interface than the one defined in Grafana's infra package?

There is no interface for New method in infra. What I do here is basically define it.

https://github.com/grafana/grafana/blob/862a6a2fa625100b690c5a48a9b12e7a7dd42041/pkg/infra/log/log.go#L231-L238


type Logger interface {
// New returns a new contextual Logger that has this logger's context plus the given context.
Expand Down
191 changes: 142 additions & 49 deletions notify/factory.go
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
package notify

import (
"strings"
"fmt"

"github.com/prometheus/alertmanager/notify"
"github.com/prometheus/alertmanager/template"
"github.com/prometheus/alertmanager/types"

"github.com/grafana/alerting/images"
"github.com/grafana/alerting/logging"
"github.com/grafana/alerting/receivers"
"github.com/grafana/alerting/receivers/alertmanager"
"github.com/grafana/alerting/receivers/dinding"
Expand All @@ -27,57 +31,146 @@ import (
"github.com/grafana/alerting/receivers/wecom"
)

var receiverFactories = map[string]func(receivers.FactoryConfig) (NotificationChannel, error){
"prometheus-alertmanager": wrap(alertmanager.New),
"dingding": wrap(dinding.New),
"discord": wrap(discord.New),
"email": wrap(email.New),
"googlechat": wrap(googlechat.New),
"kafka": wrap(kafka.New),
"line": wrap(line.New),
"opsgenie": wrap(opsgenie.New),
"pagerduty": wrap(pagerduty.New),
"pushover": wrap(pushover.New),
"sensugo": wrap(sensugo.New),
"slack": wrap(slack.New),
"teams": wrap(teams.New),
"telegram": wrap(telegram.New),
"threema": wrap(threema.New),
"victorops": wrap(victorops.New),
"webhook": wrap(webhook.New),
"wecom": wrap(wecom.New),
"webex": wrap(webex.New),
}

type NotificationChannel interface {
notify.Notifier
notify.ResolvedSender
}
// BuildReceiverIntegrations builds notifiers of the receiver and wraps each of them in Integration.
yuri-tceretian marked this conversation as resolved.
Show resolved Hide resolved
func BuildReceiverIntegrations(
receiver GrafanaReceiverConfig,
tmpl *template.Template,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
tmpl *template.Template,
tmpl *alerting.Template,

you need the local types otherwise you'll import the alertmanage directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will address it in a follow-up PR as it touches many lines.

img images.ImageStore, // Used by some receivers to include as part of the source
yuri-tceretian marked this conversation as resolved.
Show resolved Hide resolved
newLogger logging.LoggerFactory,
yuri-tceretian marked this conversation as resolved.
Show resolved Hide resolved
newWebhookSender func(n receivers.Metadata) (receivers.WebhookSender, error),
newEmailSender func(n receivers.Metadata) (receivers.EmailSender, error),
Comment on lines +41 to +42
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think these functions should use primitives, such as:

func(ctx context.Context, client *http.Client, url string, body io.Reader) (*http.Response, error)

Instead of whatever interface was defined by Grafana with the WebhookSender and the EmailSender.

orgID int64,
version string,
) ([]*Integration, error) {

yuri-tceretian marked this conversation as resolved.
Show resolved Hide resolved
func Factory(receiverType string) (func(receivers.FactoryConfig) (NotificationChannel, error), bool) {
receiverType = strings.ToLower(receiverType)
factory, exists := receiverFactories[receiverType]
return factory, exists
}
type notificationChannel interface {
notify.Notifier
notify.ResolvedSender
}

// wrap wraps the notifier's factory errors with receivers.ReceiverValidationError
func wrap[T NotificationChannel](f func(fc receivers.FactoryConfig) (T, error)) func(receivers.FactoryConfig) (NotificationChannel, error) {
return func(fc receivers.FactoryConfig) (NotificationChannel, error) {
ch, err := f(fc)
if err != nil {
return nil, ReceiverValidationError{
Err: err,
// TODO it will be removed in the next PR
Cfg: &GrafanaReceiver{
UID: fc.Config.UID,
Name: fc.Config.Name,
Type: fc.Config.Type,
DisableResolveMessage: fc.Config.DisableResolveMessage,
Settings: fc.Config.Settings,
SecureSettings: nil,
},
var (
integrations []*Integration
errors types.MultiError
createIntegration = func(idx int, cfg receivers.Metadata, f func(logger logging.Logger) notificationChannel) {
yuri-tceretian marked this conversation as resolved.
Show resolved Hide resolved
yuri-tceretian marked this conversation as resolved.
Show resolved Hide resolved
logger := newLogger("ngalert.notifier."+cfg.Type, "notifierUID", cfg.UID)
n := f(logger)
i := NewIntegration(n, n, cfg.Type, idx)
integrations = append(integrations, i)
}
createIntegrationWithWebhook = func(idx int, cfg receivers.Metadata, f func(logger logging.Logger, w receivers.WebhookSender) notificationChannel) {
yuri-tceretian marked this conversation as resolved.
Show resolved Hide resolved
yuri-tceretian marked this conversation as resolved.
Show resolved Hide resolved
w, e := newWebhookSender(cfg)
if e != nil {
errors.Add(fmt.Errorf("unable to build webhook client for %s notifier %s (UID: %s): %w ", cfg.Type, cfg.Name, cfg.UID, e))
return
}
createIntegration(idx, cfg, func(logger logging.Logger) notificationChannel {
return f(logger, w)
})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

these to anonymous methods help reduce duplicated code below.

}
)

yuri-tceretian marked this conversation as resolved.
Show resolved Hide resolved
for i, cfg := range receiver.AlertmanagerConfigs {
createIntegration(i, cfg.Metadata, func(l logging.Logger) notificationChannel {
return alertmanager.New(cfg.Settings, cfg.Metadata, img, l)
})
yuri-tceretian marked this conversation as resolved.
Show resolved Hide resolved
}
for i, cfg := range receiver.DingdingConfigs {
createIntegrationWithWebhook(i, cfg.Metadata, func(l logging.Logger, w receivers.WebhookSender) notificationChannel {
return dinding.New(cfg.Settings, cfg.Metadata, tmpl, w, l)
})
}
for i, cfg := range receiver.DiscordConfigs {
createIntegrationWithWebhook(i, cfg.Metadata, func(l logging.Logger, w receivers.WebhookSender) notificationChannel {
return discord.New(cfg.Settings, cfg.Metadata, tmpl, w, img, l, version)
})
}
for i, cfg := range receiver.EmailConfigs {
mailCli, e := newEmailSender(cfg.Metadata)
if e != nil {
errors.Add(fmt.Errorf("unable to build email client for %s notifier %s (UID: %s): %w ", cfg.Type, cfg.Name, cfg.UID, e))
}
return ch, nil
createIntegration(i, cfg.Metadata, func(l logging.Logger) notificationChannel {
return email.New(cfg.Settings, cfg.Metadata, tmpl, mailCli, img, l)
})
}
for i, cfg := range receiver.GooglechatConfigs {
createIntegrationWithWebhook(i, cfg.Metadata, func(l logging.Logger, w receivers.WebhookSender) notificationChannel {
return googlechat.New(cfg.Settings, cfg.Metadata, tmpl, w, img, l, version)
})
}
for i, cfg := range receiver.KafkaConfigs {
createIntegrationWithWebhook(i, cfg.Metadata, func(l logging.Logger, w receivers.WebhookSender) notificationChannel {
return kafka.New(cfg.Settings, cfg.Metadata, tmpl, w, img, l)
})
}
for i, cfg := range receiver.LineConfigs {
createIntegrationWithWebhook(i, cfg.Metadata, func(l logging.Logger, w receivers.WebhookSender) notificationChannel {
return line.New(cfg.Settings, cfg.Metadata, tmpl, w, l)
})
}
for i, cfg := range receiver.OpsgenieConfigs {
createIntegrationWithWebhook(i, cfg.Metadata, func(l logging.Logger, w receivers.WebhookSender) notificationChannel {
return opsgenie.New(cfg.Settings, cfg.Metadata, tmpl, w, img, l)
})
}
for i, cfg := range receiver.PagerdutyConfigs {
createIntegrationWithWebhook(i, cfg.Metadata, func(l logging.Logger, w receivers.WebhookSender) notificationChannel {
return pagerduty.New(cfg.Settings, cfg.Metadata, tmpl, w, img, l)
})
}
for i, cfg := range receiver.PushoverConfigs {
createIntegrationWithWebhook(i, cfg.Metadata, func(l logging.Logger, w receivers.WebhookSender) notificationChannel {
return pushover.New(cfg.Settings, cfg.Metadata, tmpl, w, img, l)
})
}
for i, cfg := range receiver.SensugoConfigs {
createIntegrationWithWebhook(i, cfg.Metadata, func(l logging.Logger, w receivers.WebhookSender) notificationChannel {
return sensugo.New(cfg.Settings, cfg.Metadata, tmpl, w, img, l)
})
}
for i, cfg := range receiver.SlackConfigs {
createIntegrationWithWebhook(i, cfg.Metadata, func(l logging.Logger, w receivers.WebhookSender) notificationChannel {
return slack.New(cfg.Settings, cfg.Metadata, tmpl, w, img, l, version)
})
}
for i, cfg := range receiver.TeamsConfigs {
createIntegrationWithWebhook(i, cfg.Metadata, func(l logging.Logger, w receivers.WebhookSender) notificationChannel {
return teams.New(cfg.Settings, cfg.Metadata, tmpl, w, img, l)
})
}
for i, cfg := range receiver.TelegramConfigs {
createIntegrationWithWebhook(i, cfg.Metadata, func(l logging.Logger, w receivers.WebhookSender) notificationChannel {
return telegram.New(cfg.Settings, cfg.Metadata, tmpl, w, img, l)
})
}
for i, cfg := range receiver.ThreemaConfigs {
createIntegrationWithWebhook(i, cfg.Metadata, func(l logging.Logger, w receivers.WebhookSender) notificationChannel {
return threema.New(cfg.Settings, cfg.Metadata, tmpl, w, img, l)
})
}
for i, cfg := range receiver.VictoropsConfigs {
createIntegrationWithWebhook(i, cfg.Metadata, func(l logging.Logger, w receivers.WebhookSender) notificationChannel {
return victorops.New(cfg.Settings, cfg.Metadata, tmpl, w, img, l, version)
})
}
for i, cfg := range receiver.WebhookConfigs {
createIntegrationWithWebhook(i, cfg.Metadata, func(l logging.Logger, w receivers.WebhookSender) notificationChannel {
return webhook.New(cfg.Settings, cfg.Metadata, tmpl, w, img, l, orgID)
})
}
for i, cfg := range receiver.WecomConfigs {
createIntegrationWithWebhook(i, cfg.Metadata, func(l logging.Logger, w receivers.WebhookSender) notificationChannel {
return wecom.New(cfg.Settings, cfg.Metadata, tmpl, w, l)
})
}
for i, cfg := range receiver.WebexConfigs {
createIntegrationWithWebhook(i, cfg.Metadata, func(l logging.Logger, w receivers.WebhookSender) notificationChannel {
return webex.New(cfg.Settings, cfg.Metadata, tmpl, w, img, l, orgID)
})
}

if errors.Len() > 0 {
return nil, &errors
}
return integrations, nil
}
118 changes: 118 additions & 0 deletions notify/factory_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
package notify

import (
"context"
"errors"
"fmt"
"math/rand"
"testing"

"github.com/stretchr/testify/require"

"github.com/grafana/alerting/images"
"github.com/grafana/alerting/logging"
"github.com/grafana/alerting/receivers"
"github.com/grafana/alerting/templates"
)

func TestBuildReceiverIntegrations(t *testing.T) {
var orgID = rand.Int63()
var version = fmt.Sprintf("Grafana v%d", rand.Uint32())
imageStore := &images.FakeImageStore{}
tmpl := templates.ForTests(t)

webhookFactory := func(n receivers.Metadata) (receivers.WebhookSender, error) {
return receivers.MockNotificationService(), nil
}
emailFactory := func(n receivers.Metadata) (receivers.EmailSender, error) {
return receivers.MockNotificationService(), nil
}
loggerFactory := func(_ string, _ ...interface{}) logging.Logger {
return &logging.FakeLogger{}
}

getFullConfig := func(t *testing.T) (GrafanaReceiverConfig, int) {
recCfg := &APIReceiver{ConfigReceiver: ConfigReceiver{Name: "test-receiver"}}
for notifierType, cfg := range allKnownConfigs {
recCfg.Receivers = append(recCfg.Receivers, cfg.getRawNotifierConfig(notifierType))
}
parsed, err := BuildReceiverConfiguration(context.Background(), recCfg, GetDecryptedValueFnForTesting)
require.NoError(t, err)
return parsed, len(recCfg.Receivers)
}

t.Run("should build all supported notifiers", func(t *testing.T) {
fullCfg, qty := getFullConfig(t)

loggerNames := make(map[string]struct{}, qty)
logger := func(name string, _ ...interface{}) logging.Logger {
loggerNames[name] = struct{}{}
return &logging.FakeLogger{}
}

webhooks := make(map[receivers.Metadata]struct{}, qty)
wh := func(n receivers.Metadata) (receivers.WebhookSender, error) {
webhooks[n] = struct{}{}
return webhookFactory(n)
}

emails := make(map[receivers.Metadata]struct{}, qty)
em := func(n receivers.Metadata) (receivers.EmailSender, error) {
emails[n] = struct{}{}
return emailFactory(n)
}

integrations, err := BuildReceiverIntegrations(fullCfg, tmpl, imageStore, logger, wh, em, orgID, version)

require.NoError(t, err)
require.Len(t, integrations, qty)

t.Run("should call logger factory for each config", func(t *testing.T) {
require.Len(t, loggerNames, qty)
})
t.Run("should call webhook factory for each config that needs it", func(t *testing.T) {
require.Len(t, webhooks, 17) // we have 17 notifiers that support webhook
})
t.Run("should call email factory for each config that needs it", func(t *testing.T) {
require.Len(t, emails, 1) // we have only email notifier that needs sender
})
})
t.Run("should return errors if webhook factory fails", func(t *testing.T) {
fullCfg, _ := getFullConfig(t)
calls := 0
failingFactory := func(n receivers.Metadata) (receivers.WebhookSender, error) {
calls++
return nil, errors.New("bad-test")
}

integrations, err := BuildReceiverIntegrations(fullCfg, tmpl, imageStore, loggerFactory, failingFactory, emailFactory, orgID, version)

require.Empty(t, integrations)
require.NotNil(t, err)
require.ErrorContains(t, err, "bad-test")
require.Greater(t, calls, 0)
})
t.Run("should return errors if email factory fails", func(t *testing.T) {
fullCfg, _ := getFullConfig(t)
calls := 0
failingFactory := func(n receivers.Metadata) (receivers.EmailSender, error) {
calls++
return nil, errors.New("bad-test")
}

integrations, err := BuildReceiverIntegrations(fullCfg, tmpl, imageStore, loggerFactory, webhookFactory, failingFactory, orgID, version)

require.Empty(t, integrations)
require.NotNil(t, err)
require.ErrorContains(t, err, "bad-test")
require.Greater(t, calls, 0)
})
t.Run("should not produce any integration if config is empty", func(t *testing.T) {
cfg := GrafanaReceiverConfig{Name: "test"}

integrations, err := BuildReceiverIntegrations(cfg, tmpl, imageStore, loggerFactory, webhookFactory, emailFactory, orgID, version)

require.NoError(t, err)
require.Empty(t, integrations)
})
}
8 changes: 6 additions & 2 deletions notify/receivers.go
Original file line number Diff line number Diff line change
Expand Up @@ -347,8 +347,12 @@ type NotifierConfig[T interface{}] struct {
Settings T
}

// GetDecryptedValueFn is a function that returns the decrypted value of
// the given key. If the key is not present, then it returns the fallback value.
type GetDecryptedValueFn func(ctx context.Context, sjd map[string][]byte, key string, fallback string) string

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function is moved from the receivers package because it is not used there anymore.

// BuildReceiverConfiguration parses, decrypts and validates the APIReceiver.
func BuildReceiverConfiguration(ctx context.Context, api *APIReceiver, decrypt receivers.GetDecryptedValueFn) (GrafanaReceiverConfig, error) {
func BuildReceiverConfiguration(ctx context.Context, api *APIReceiver, decrypt GetDecryptedValueFn) (GrafanaReceiverConfig, error) {
result := GrafanaReceiverConfig{
Name: api.Name,
}
Expand All @@ -365,7 +369,7 @@ func BuildReceiverConfiguration(ctx context.Context, api *APIReceiver, decrypt r
}

// parseNotifier parses receivers and populates the corresponding field in GrafanaReceiverConfig. Returns an error if the configuration cannot be parsed.
func parseNotifier(ctx context.Context, result *GrafanaReceiverConfig, receiver *GrafanaReceiver, decrypt receivers.GetDecryptedValueFn) error {
func parseNotifier(ctx context.Context, result *GrafanaReceiverConfig, receiver *GrafanaReceiver, decrypt GetDecryptedValueFn) error {
secureSettings, err := decodeSecretsFromBase64(receiver.SecureSettings)
if err != nil {
return err
Expand Down
9 changes: 1 addition & 8 deletions notify/receivers_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -106,14 +106,7 @@ func TestProcessNotifierError(t *testing.T) {
}

func TestBuildReceiverConfiguration(t *testing.T) {
var decrypt receivers.GetDecryptedValueFn = func(ctx context.Context, sjd map[string][]byte, key string, fallback string) string {
v, ok := sjd[key]
if !ok {
return fallback
}
return string(v)
}

decrypt := GetDecryptedValueFnForTesting
t.Run("should decode secrets from base64", func(t *testing.T) {
recCfg := &APIReceiver{ConfigReceiver: ConfigReceiver{Name: "test-receiver"}}
for notifierType, cfg := range allKnownConfigs {
Expand Down
6 changes: 6 additions & 0 deletions notify/testing.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import (
"time"

"github.com/prometheus/alertmanager/types"

receiversTesting "github.com/grafana/alerting/receivers/testing"
)

func newFakeMaintanenceOptions(t *testing.T) *fakeMaintenanceOptions {
Expand Down Expand Up @@ -84,3 +86,7 @@ func (f *fakeNotifier) Notify(_ context.Context, _ ...*types.Alert) (bool, error
func (f *fakeNotifier) SendResolved() bool {
return true
}

func GetDecryptedValueFnForTesting(_ context.Context, sjd map[string][]byte, key string, fallback string) string {
return receiversTesting.DecryptForTesting(sjd)(key, fallback)
}
Loading