Parse notifier configs without creation of notifiers #62

yuri-tceretian · 2023-02-14T18:01:13Z

In Grafana Alertmanager configuration we have very complex API models that are usually a combination of upstream configurations and our custom fields. Those models are propagated everywhere in the code deep down to the notification-building logic where it is parsed and converted to Notifier. In order to validate the configuration in API handlers we have to access the notifiers factory and stub many fields because that factory requires many services.

For example,
https://github.com/grafana/grafana/blob/f066e8cdcd178b783d0c62959bea593d3c0c30b0/pkg/services/ngalert/api/tooling/definitions/provisioning_contactpoints.go#L105-L128

This PR creates an intermediate layer that parses, decrypts secrets and validates settings of all notifiers in a single receiver, and returns either a valid configuration or error. A new method ValidateAPIReceiver accepts only parameters that are required for performing this task.

A new struct GrafanaReceiverTyped resembles the receiver configuration from the upstream
https://github.com/prometheus/alertmanager/blob/fe1b045c00204dc47a0f63728b6162e9d5cf9b5e/config/config.go#L883-L899 However, due to additional information that Grafana notifier hold (uid, type, name) I had to add a generic wrapper NotifierConfig

This will help make validation code in API much clear and reduce the scope of raw settings.

Based on #61

The new config struct looks like the upstream Alertmanager's

// GrafanaReceiverTyped represents a parsed and validated APIReceiver
type GrafanaReceiverTyped struct {
	Name                string
	AlertmanagerConfigs []*NotifierConfig[alertmanager.Config]
	DingdingConfigs     []*NotifierConfig[dinding.Config]
	DiscordConfigs      []*NotifierConfig[discord.Config]
	EmailConfigs        []*NotifierConfig[email.Config]
	GooglechatConfigs   []*NotifierConfig[googlechat.Config]
	KafkaConfigs        []*NotifierConfig[kafka.Config]
	LineConfigs         []*NotifierConfig[line.Config]
	OpsgenieConfigs     []*NotifierConfig[opsgenie.Config]
	PagerdutyConfigs    []*NotifierConfig[pagerduty.Config]
	PushoverConfigs     []*NotifierConfig[pushover.Config]
	SensugoConfigs      []*NotifierConfig[sensugo.Config]
	SlackConfigs        []*NotifierConfig[slack.Config]
	TeamsConfigs        []*NotifierConfig[teams.Config]
	TelegramConfigs     []*NotifierConfig[telegram.Config]
	ThreemaConfigs      []*NotifierConfig[threema.Config]
	VictoropsConfigs    []*NotifierConfig[victorops.Config]
	WebhookConfigs      []*NotifierConfig[webhook.Config]
	WecomConfigs        []*NotifierConfig[wecom.Config]
	WebexConfigs        []*NotifierConfig[webex.Config]
}

yuri-tceretian · 2023-02-23T15:20:22Z

notify/factory.go

+					DisableResolveMessage: fc.Config.DisableResolveMessage,
+					Settings:              fc.Config.Settings,
+					SecureSettings:        nil,
+				},


temporary, this will go away in #63

yuri-tceretian · 2023-02-23T15:21:15Z

receivers/factory.go

+	Name                  string
+	Type                  string
+	DisableResolveMessage bool
+}


This struct will replace NotificationChannelConfig in #63

yuri-tceretian · 2023-02-23T15:22:36Z

notify/receivers.go

+			return nil
+		}
+	}
+}


this is a utility func to propagate errors in the case of validation failure.

yuri-tceretian · 2023-02-23T15:24:17Z

notify/receivers.go

@@ -292,3 +316,189 @@ func ProcessNotifierError(config *GrafanaReceiver, err error) error {

 	return err
 }
+
+// GrafanaReceiverTyped represents a parsed and validated APIReceiver
+type GrafanaReceiverTyped struct {


I think it should be called GrafanaReceiver.. but that name is already taken. Probably it makes sense to rename that to GrafanaNotifierSettings and this to GrafanaReceiverConfiguration

Remind me again how is this going to be used that will benefit from generics?

That is a way to have Metadata for all notifier configs

yuri-tceretian · 2023-02-23T15:25:21Z

notify/receivers.go

+	Name                  string            `json:"name"`
+	Type                  string            `json:"type"`
+	DisableResolveMessage bool              `json:"disableResolveMessage"`
+	Settings              json.RawMessage   `json:"settings"`


this will actually help move forward with refactoring in Grafana because we have to do conversions back and forth for that field.

gotjosh

Overall LGTM and I find this is a net positive, but please see my comments.

receivers/factory.go

gotjosh · 2023-02-23T18:16:36Z

notify/receivers.go

+	parseConfig := func(receiver *GrafanaReceiver) error {
+		// secure settings are already encrypted at this point
+		secureSettings := make(map[string][]byte, len(receiver.SecureSettings))
+
+		if receiver.SecureSettings != nil {
+			for k, v := range receiver.SecureSettings {
+				d, err := base64.StdEncoding.DecodeString(v)
+				if err != nil {
+					return InvalidReceiverError{
+						Receiver: receiver,
+						Err:      errors.New("failed to decode secure setting"),
+					}
+				}
+				secureSettings[k] = d
+			}
+		}
+
+		var decryptFn receivers.DecryptFunc = func(key string, fallback string) string {
+			return decrypt(ctx, secureSettings, key, fallback)
+		}


nit:

I'd like to move away from these nested functions, they make this part of the code really hard to read.

Why not just make this a separate function, even better why not embrace the pointer to structs here all of this a function that's part of APIReceiver. I don't really understand the relationship between these struct at all.

The code as written appears to take a functional approach - which even though is well intended I personally find it highly un-idiomatic.

For example, in this function the main actor is:

for _, receiver := range api.Receivers { err := parseConfig(receiver) if err != nil { return GrafanaReceiverTyped{}, &ReceiverValidationError{ Cfg: receiver, Err: err, } } }

Yet, I'm presented with parseConfig first which forces to me read this wall before I even begin to understand what I'm doing as part of this function.

A good example of the use of inline functions is when you care about concurrency and sending that execution to a goroutine - this is not any of that.

notify/receivers.go

gotjosh · 2023-02-28T10:53:51Z

notify/receivers.go

+	result := GrafanaReceiverTyped{
+		Name: api.Name,
+	}
+	for _, receiver := range api.Receivers {
+		secureSettings, err := decodeSecretsFromBase64(receiver.SecureSettings)
+		if err == nil {
+			err = parseReceiver(&result, receiver, func(key string, fallback string) string {
+				return decrypt(ctx, secureSettings, key, fallback)
+			})
+		}
+		if err != nil {
+			return GrafanaReceiverTyped{}, &ReceiverValidationError{
+				Cfg: receiver,
+				Err: fmt.Errorf("failed to parse notifier %s (UID: %s): %w", receiver.Name, receiver.UID, err),
+			}
+		}
+	}
+	return result, nil


nit: I'd re-structure this function.

Generally, functions tend to do four main things: Collecting input, handling failure, doing work and delivering results.

I'd stick to that order when possible.

In addition to that, the name of ValidateAPIReceiver is highly misleading - we're way past the name of validation. I suggest BuildReceiverIntegrations.

With the above, my suggestion is:

var ErrorAPIReceiverValidation = func(receiver *GrafanaReceiver, err error) error { return &ReceiverValidationError{ Cfg: receiver, Err: fmt.Errorf("failed to parse notifier %s (UID: %s): %w", receiver.Name, receiver.UID, err), } } // ValidateAPIReceiver parses, decrypts and validates the APIReceiver. GrafanaReceiverTyped that contains configurations of all notifiers configurations for this receiver func ValidateAPIReceiver(ctx context.Context, api *APIReceiver, decrypt receivers.GetDecryptedValueFn) (GrafanaReceiverTyped, error) { result := GrafanaReceiverTyped{ Name: api.Name, } for _, receiver := range api.Receivers { // First, decode the secure receiver integrations settings. secureSettings, err := decodeSecretsFromBase64(receiver.SecureSettings) if err != nil { return GrafanaReceiverTyped{}, ErrorAPIReceiverValidation(receiver, err) } // Then, create the decryption function that's common for all integrations of this receiver. decryptFn := func(key, fallback string) string { return decrypt(ctx, secureSettings, key, fallback) } // Finally, parse the receiver configuration. err = parseReceiver(&result, receiver, decryptFn) if err != nil { return GrafanaReceiverTyped{}, ErrorAPIReceiverValidation(receiver, err) } } return result, nil }

gotjosh · 2023-02-28T10:58:13Z

notify/receivers.go

+}
+
+func decodeSecretsFromBase64(secrets map[string]string) (map[string][]byte, error) {
+	// secure settings are already encrypted at this point


Suggested change

// secure settings are already encrypted at this point

// Secure settings are already encrypted at this point.

If secure settings are already encoded at this point - why do we need to have a decrypt function passed over? Should we call the function noopDecryptFn instead then?

this is a copy from another place as is. I did not add anything here

https://github.com/grafana/grafana/blob/f066e8cdcd178b783d0c62959bea593d3c0c30b0/pkg/services/ngalert/notifier/alertmanager.go#L345-L346

gotjosh · 2023-02-28T11:02:05Z

notify/receivers.go

+	if e.Cfg.Name != "" {
+		name = fmt.Sprintf("%q ", e.Cfg.Name)
+	}
+	s := fmt.Sprintf("failed to validate receiver %sof type %q: %s", name, e.Cfg.Type, e.Err.Error())


Suggested change

s := fmt.Sprintf("failed to validate receiver %sof type %q: %s", name, e.Cfg.Type, e.Err.Error())

s := fmt.Sprintf("failed to validate notifier %sof type %q: %s", name, e.Cfg.Type, e.Err.Error())

Other errors seem to use notifier instead?

I think that is expected because this error is returned by ValidateReceiver method... not a particular notifier.

receivers/factory.go

gotjosh · 2023-02-28T11:03:25Z

notify/receivers.go

@@ -292,3 +316,189 @@ func ProcessNotifierError(config *GrafanaReceiver, err error) error {

 	return err
 }
+
+// GrafanaReceiverTyped represents a parsed and validated APIReceiver
+type GrafanaReceiverTyped struct {


Remind me again how is this going to be used that will benefit from generics?

…e package

gotjosh

LGTM

gotjosh · 2023-03-02T16:20:27Z

notify/receivers.go

+	Settings T
+}
+
+// BuildReceiverConfiguration parses, decrypts and validates the APIReceiver. GrafanaReceiverConfig that contains configurations of all notifiers configurations for this receiver


Suggested change

// BuildReceiverConfiguration parses, decrypts and validates the APIReceiver. GrafanaReceiverConfig that contains configurations of all notifiers configurations for this receiver

// BuildReceiverConfiguration parses, decrypts and validates the APIReceiver.

gotjosh · 2023-03-02T16:20:49Z

notify/receivers.go

+	return result, nil
+}
+
+// parseNotifier parses receivers and populates corresponding field in GrafanaReceiverConfig. Returns error if configuration cannot be parsed


Suggested change

// parseNotifier parses receivers and populates corresponding field in GrafanaReceiverConfig. Returns error if configuration cannot be parsed

// parseNotifier parses receivers and populates the corresponding field in GrafanaReceiverConfig. Returns an error if the configuration cannot be parsed.

gotjosh · 2023-03-02T16:26:02Z

receivers/alertmanager/testing.go

@@ -0,0 +1,13 @@
+package alertmanager
+
+// FullValidConfigForTesting a string representation of JSON object that contains all fields supported by the notifier Config. Can be used without secrets


Suggested change

// FullValidConfigForTesting a string representation of JSON object that contains all fields supported by the notifier Config. Can be used without secrets

// FullValidConfigForTesting is a string representation of JSON object that contains all fields supported by the notifier Config. It can be used without secrets.

The same applies to every other instance of this comment.

gotjosh · 2023-03-02T16:26:33Z

receivers/alertmanager/testing.go

+	"basicAuthPassword": "admin"
+}`
+
+// FullValidSecretsForTesting is a string representation of JSON object that contains all fields that can be overridden from secrets


Suggested change

// FullValidSecretsForTesting is a string representation of JSON object that contains all fields that can be overridden from secrets

// FullValidSecretsForTesting is a string representation of a JSON object that contains all fields that can be overridden from secrets.

The same applies to every other instance of this comment.

yuri-tceretian added enhancement New feature or request kata:am_unify labels Feb 14, 2023

yuri-tceretian self-assigned this Feb 14, 2023

yuri-tceretian force-pushed the yuri-tceretian/typed-config branch 6 times, most recently from 825a2cd to 9f78cd5 Compare February 14, 2023 21:35

This was referenced Feb 14, 2023

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

Merged

Move integration building logic from Grafana #60

Closed

yuri-tceretian added 4 commits February 23, 2023 10:05

Update GrafanaReceiver.Setting to be raw JSON

614d424

introduce GrafanaReceiverTyped and NotifierConfig

08f941f

move init config

d5fde26

rename ReceiverInitError to ReceiverValidationError

cc80c05

yuri-tceretian force-pushed the yuri-tceretian/typed-config branch from 9f78cd5 to cc80c05 Compare February 23, 2023 15:08

yuri-tceretian marked this pull request as ready for review February 23, 2023 15:09

yuri-tceretian commented Feb 23, 2023

View reviewed changes

gotjosh suggested changes Feb 27, 2023

View reviewed changes

yuri-tceretian added 2 commits February 27, 2023 14:19

rename NotifierInfo to Metadata

ad0a807

refactor ValidateAPIReceiver to be more readable

1029b57

yuri-tceretian requested a review from gotjosh February 27, 2023 19:24

gotjosh reviewed Feb 28, 2023

View reviewed changes

yuri-tceretian added 3 commits February 28, 2023 10:33

rename func

f9874e2

dot

c8e292b

remove comment

9eb5100

yuri-tceretian added 5 commits February 28, 2023 10:41

move some code to parseReceiver

6668e76

fix terminology

4e1e974

more rename

cf6fc3b

add valid configs for all receivers so they can be used outside of th…

931fd6a

…e package

add tests for BuildReceiverConfiguration

85dbe42

yuri-tceretian force-pushed the yuri-tceretian/typed-config branch from 1f3e279 to 85dbe42 Compare March 1, 2023 15:23

yuri-tceretian requested a review from gotjosh March 2, 2023 15:37

gotjosh approved these changes Mar 2, 2023

View reviewed changes

apply Josh's suggestions for comments

542a0d1

yuri-tceretian merged commit b011b52 into main Mar 3, 2023

yuri-tceretian deleted the yuri-tceretian/typed-config branch March 3, 2023 17:53

grobinson-grafana mentioned this pull request Mar 16, 2023

Alerting: Update github.com/grafana/alerting grafana/grafana#64882

Merged

yuri-tceretian mentioned this pull request Mar 22, 2023

Alerting: use alerting GrafanaReceiver and BuildReceiverConfiguration in Grafana grafana/grafana#65224

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parse notifier configs without creation of notifiers #62

Parse notifier configs without creation of notifiers #62

yuri-tceretian commented Feb 14, 2023 •

edited

Loading

yuri-tceretian Feb 23, 2023

yuri-tceretian Feb 23, 2023

yuri-tceretian Feb 23, 2023

yuri-tceretian Feb 23, 2023

gotjosh Feb 28, 2023

yuri-tceretian Feb 28, 2023

yuri-tceretian Feb 23, 2023 •

edited

Loading

gotjosh left a comment

gotjosh Feb 23, 2023

gotjosh Feb 28, 2023

gotjosh Feb 28, 2023

gotjosh Feb 28, 2023

gotjosh Feb 28, 2023

yuri-tceretian Feb 28, 2023

gotjosh Feb 28, 2023

gotjosh Feb 28, 2023

yuri-tceretian Feb 28, 2023

gotjosh Feb 28, 2023

gotjosh left a comment

gotjosh Mar 2, 2023

gotjosh Mar 2, 2023

gotjosh Mar 2, 2023

gotjosh Mar 2, 2023

gotjosh Mar 2, 2023

gotjosh Mar 2, 2023

	// secure settings are already encrypted at this point
	// Secure settings are already encrypted at this point.

	s := fmt.Sprintf("failed to validate receiver %sof type %q: %s", name, e.Cfg.Type, e.Err.Error())
	s := fmt.Sprintf("failed to validate notifier %sof type %q: %s", name, e.Cfg.Type, e.Err.Error())

	// BuildReceiverConfiguration parses, decrypts and validates the APIReceiver. GrafanaReceiverConfig that contains configurations of all notifiers configurations for this receiver
	// BuildReceiverConfiguration parses, decrypts and validates the APIReceiver.

	// parseNotifier parses receivers and populates corresponding field in GrafanaReceiverConfig. Returns error if configuration cannot be parsed
	// parseNotifier parses receivers and populates the corresponding field in GrafanaReceiverConfig. Returns an error if the configuration cannot be parsed.

		@@ -0,0 +1,13 @@
		package alertmanager

		// FullValidConfigForTesting a string representation of JSON object that contains all fields supported by the notifier Config. Can be used without secrets

	// FullValidConfigForTesting a string representation of JSON object that contains all fields supported by the notifier Config. Can be used without secrets
	// FullValidConfigForTesting is a string representation of JSON object that contains all fields supported by the notifier Config. It can be used without secrets.

	// FullValidSecretsForTesting is a string representation of JSON object that contains all fields that can be overridden from secrets
	// FullValidSecretsForTesting is a string representation of a JSON object that contains all fields that can be overridden from secrets.

Parse notifier configs without creation of notifiers #62

Parse notifier configs without creation of notifiers #62

Conversation

yuri-tceretian commented Feb 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuri-tceretian Feb 23, 2023 • edited Loading

Choose a reason for hiding this comment

gotjosh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gotjosh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuri-tceretian commented Feb 14, 2023 •

edited

Loading

yuri-tceretian Feb 23, 2023 •

edited

Loading