Alertmanager configuration validate-only mode #3437 #3440

dannystaple · 2022-11-11T16:57:21Z

What this PR does

This introduces an additional flag for mimirtool alertmanager load to validate configuration only, without proceeding to upload it to server. This can be used to validate configuration, for example in PR CI pipelines before attempting to merge and deploy.

Which issue(s) this PR fixes or relates to

Fixes #3437

Checklist

Tests updated - no tests exist for mimirtool alertmanager load
Documentation added
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

CLAassistant · 2022-11-11T16:57:26Z

All committers have signed the CLA.

docs/sources/operators-guide/architecture/components/alertmanager.md

osg-grafana

A nit

…ger.md Co-authored-by: Ursula Kallio <ursula.kallio@grafana.com>

Logiraptor · 2022-11-11T17:22:55Z

I liked the suggestion you had in the issue about mimirtool alertmanager verify (or maybe mimirtool alertmanager validate. Is there a reason that isn't an option? I think it would be preferable to have an explicit command to avoid the need to specify address and tenant when it's not used. What do you think?

dannystaple · 2022-11-11T17:24:07Z

I liked the suggestion you had in the issue about mimirtool alertmanager verify (or maybe mimirtool alertmanager validate. Is there a reason that isn't an option? I think it would be preferable to have an explicit command to avoid the need to specify address and tenant when it's not used. What do you think?

Let me take a look at that. Certainly sounds reasonable as the id/address is a little ugly.

…ystaple/mimir into 3437-alertmanager-config-validation

dannystaple · 2022-11-11T18:07:25Z

Ok - committed mimirtool alertmanager verify.

pracucci

Good job! I left few minor comments before merging. Thanks!

pracucci · 2022-11-15T12:11:10Z

CHANGELOG.md

@@ -97,6 +97,7 @@

 ### Mimirtool

+* [ENHANCEMENT] Added `mimirtool alertmanager verify` command to validate configuration without uploading. #3247


[nit] I think you can proudly call it a new feature ;) We're also used to reference the PR number, so #3440 in this case.

Suggested change

* [ENHANCEMENT] Added `mimirtool alertmanager verify` command to validate configuration without uploading. #3247

* [FEATURE] Added `mimirtool alertmanager verify` command to validate configuration without uploading. #3440

pracucci · 2022-11-15T12:13:05Z

docs/sources/operators-guide/tools/mimirtool.md

@@ -92,6 +92,14 @@ mimirtool alertmanager load <config_file>
 mimirtool alertmanager load <config_file> <template_files>...
 ```

+### Validate Alertmanager configuration


The ##### Example below is related to "Load Alertmanager configuration". I think you should:

Move ### Validate Alertmanager configuration after the example below (right before #### Delete Alertmanager configuration)

Change ### Validate Alertmanager configuration to #### Validate Alertmanager configuration (4 hashes) to keep formatting consistent

That makes sense. Done.

pracucci · 2022-11-15T12:17:57Z

pkg/mimirtool/commands/alerts.go

+	getAlertsCmd.Flag("address", "Address of the Grafana Mimir cluster; alternatively, set "+envVars.Address+".").Envar(envVars.Address).Required().StringVar(&a.ClientConfig.Address)
+	getAlertsCmd.Flag("id", "Grafana Mimir tenant ID; alternatively, set "+envVars.TenantID+".").Envar(envVars.TenantID).Required().StringVar(&a.ClientConfig.ID)


[nit] Since there's a bit of repetition here between getAlertsCmd, deleteCmd and loadalertCmd, what if we register these flags in a for loop, something like (pseudocode):

for _, cmd := range []*kingpin.CmdClause{getAlertsCmd, deleteCmd, loadalertCmd } { cmd.Flag("address", ...) cmd.Flag("id", ...) }

I like that - more DRY. Done.

pkg/mimirtool/commands/alerts.go

pracucci · 2022-11-15T12:19:58Z

pkg/mimirtool/commands/alerts.go

@@ -104,27 +113,39 @@ func (a *AlertmanagerCommand) getConfig(k *kingpin.ParseContext) error {
 	return p.PrintAlertmanagerConfig(cfg, templates)
 }

-func (a *AlertmanagerCommand) loadConfig(k *kingpin.ParseContext) error {
+func (a *AlertmanagerCommand) prepareAlertManagerConfig(k *kingpin.ParseContext) (error, string, map[string]string) {


[nit] WDYT about calling it "read"? We're basically reading it from file.

Suggested change

func (a *AlertmanagerCommand) prepareAlertManagerConfig(k *kingpin.ParseContext) (error, string, map[string]string) {

func (a *AlertmanagerCommand) readAlertmanagerConfigFromFile(k *kingpin.ParseContext) (error, string, map[string]string) {

Agreed. And done.

Co-authored-by: Marco Pracucci <marco@pracucci.com>

Bump from enhancement to feature.

jhesketh

lgtm

pracucci

LGTM (modulo a nit). Thanks!

pracucci · 2022-11-17T13:16:35Z

pkg/mimirtool/commands/alerts.go

+		cmd.Flag("id", "Grafana Mimir tenant ID; alternatively, set "+envVars.TenantID+".").Envar(envVars.TenantID).Required().StringVar(&a.ClientConfig.ID)
+	}
+
+	vertifyalertCmd := alertCmd.Command("verify", "Verify Alertmanager tenant configuration and template files.").Action(a.verifyAlertmanagerConfig)


[nit] Doh, there's a small typo here:

Suggested change

vertifyalertCmd := alertCmd.Command("verify", "Verify Alertmanager tenant configuration and template files.").Action(a.verifyAlertmanagerConfig)

verifyalertCmd := alertCmd.Command("verify", "Verify Alertmanager tenant configuration and template files.").Action(a.verifyAlertmanagerConfig)

Ok - fixed.

Co-authored-by: Marco Pracucci <marco@pracucci.com> Add scheme to DNS service discovery docs (grafana#3450) * Add schema to DNS service discovery docs The missing schema leads to errors like `improperly formatted alertmanager URL "alertmanager.mimir.svc.cluster.local:8080/alertmanager" (maybe the scheme is missing?)`. * Update docs/sources/operators-guide/configure/about-dns-service-discovery.md Co-authored-by: Ursula Kallio <ursula.kallio@grafana.com> Co-authored-by: Marco Pracucci <marco@pracucci.com> Co-authored-by: Ursula Kallio <ursula.kallio@grafana.com> Alertmanager configuration validate-only mode grafana#3437 (grafana#3440) * Alertmanager configuration validate-only mode grafana#3437 * Documentation update for alertmanager config validation * Update changelog * Update docs/sources/operators-guide/architecture/components/alertmanager.md Co-authored-by: Ursula Kallio <ursula.kallio@grafana.com> * Swap a command line flag for a verify subcommand. * Update pkg/mimirtool/commands/alerts.go Co-authored-by: Marco Pracucci <marco@pracucci.com> * For loop to reduce repetition * Use review suggestion. * Make alertmanager verification docs more consistent. Bump from enhancement to feature. * fmt * Fix the golint issue. * Fix typo * Make doc result (as specified by lint process) Co-authored-by: Ursula Kallio <ursula.kallio@grafana.com> Co-authored-by: Marco Pracucci <marco@pracucci.com> lint fix

* add runbook urls for alerts Co-authored-by: Joshua Hesketh <josh@hesketh.net.au> Update CHANGELOG.md Co-authored-by: Joshua Hesketh <josh@hesketh.net.au> * Update CHANGELOG.md Co-authored-by: Marco Pracucci <marco@pracucci.com> Add scheme to DNS service discovery docs (#3450) * Add schema to DNS service discovery docs The missing schema leads to errors like `improperly formatted alertmanager URL "alertmanager.mimir.svc.cluster.local:8080/alertmanager" (maybe the scheme is missing?)`. * Update docs/sources/operators-guide/configure/about-dns-service-discovery.md Co-authored-by: Ursula Kallio <ursula.kallio@grafana.com> Co-authored-by: Marco Pracucci <marco@pracucci.com> Co-authored-by: Ursula Kallio <ursula.kallio@grafana.com> Alertmanager configuration validate-only mode #3437 (#3440) * Alertmanager configuration validate-only mode #3437 * Documentation update for alertmanager config validation * Update changelog * Update docs/sources/operators-guide/architecture/components/alertmanager.md Co-authored-by: Ursula Kallio <ursula.kallio@grafana.com> * Swap a command line flag for a verify subcommand. * Update pkg/mimirtool/commands/alerts.go Co-authored-by: Marco Pracucci <marco@pracucci.com> * For loop to reduce repetition * Use review suggestion. * Make alertmanager verification docs more consistent. Bump from enhancement to feature. * fmt * Fix the golint issue. * Fix typo * Make doc result (as specified by lint process) Co-authored-by: Ursula Kallio <ursula.kallio@grafana.com> Co-authored-by: Marco Pracucci <marco@pracucci.com> lint fix Co-authored-by: Joshua Hesketh <josh@hesketh.net.au>

) * Alertmanager configuration validate-only mode grafana#3437 * Documentation update for alertmanager config validation * Update changelog * Update docs/sources/operators-guide/architecture/components/alertmanager.md Co-authored-by: Ursula Kallio <ursula.kallio@grafana.com> * Swap a command line flag for a verify subcommand. * Update pkg/mimirtool/commands/alerts.go Co-authored-by: Marco Pracucci <marco@pracucci.com> * For loop to reduce repetition * Use review suggestion. * Make alertmanager verification docs more consistent. Bump from enhancement to feature. * fmt * Fix the golint issue. * Fix typo * Make doc result (as specified by lint process) Co-authored-by: Ursula Kallio <ursula.kallio@grafana.com> Co-authored-by: Marco Pracucci <marco@pracucci.com>

* add runbook urls for alerts Co-authored-by: Joshua Hesketh <josh@hesketh.net.au> Update CHANGELOG.md Co-authored-by: Joshua Hesketh <josh@hesketh.net.au> * Update CHANGELOG.md Co-authored-by: Marco Pracucci <marco@pracucci.com> Add scheme to DNS service discovery docs (grafana#3450) * Add schema to DNS service discovery docs The missing schema leads to errors like `improperly formatted alertmanager URL "alertmanager.mimir.svc.cluster.local:8080/alertmanager" (maybe the scheme is missing?)`. * Update docs/sources/operators-guide/configure/about-dns-service-discovery.md Co-authored-by: Ursula Kallio <ursula.kallio@grafana.com> Co-authored-by: Marco Pracucci <marco@pracucci.com> Co-authored-by: Ursula Kallio <ursula.kallio@grafana.com> Alertmanager configuration validate-only mode grafana#3437 (grafana#3440) * Alertmanager configuration validate-only mode grafana#3437 * Documentation update for alertmanager config validation * Update changelog * Update docs/sources/operators-guide/architecture/components/alertmanager.md Co-authored-by: Ursula Kallio <ursula.kallio@grafana.com> * Swap a command line flag for a verify subcommand. * Update pkg/mimirtool/commands/alerts.go Co-authored-by: Marco Pracucci <marco@pracucci.com> * For loop to reduce repetition * Use review suggestion. * Make alertmanager verification docs more consistent. Bump from enhancement to feature. * fmt * Fix the golint issue. * Fix typo * Make doc result (as specified by lint process) Co-authored-by: Ursula Kallio <ursula.kallio@grafana.com> Co-authored-by: Marco Pracucci <marco@pracucci.com> lint fix Co-authored-by: Joshua Hesketh <josh@hesketh.net.au>

dannystaple added 3 commits November 11, 2022 16:43

Alertmanager configuration validate-only mode grafana#3437

76b8339

Documentation update for alertmanager config validation

1aa3f2b

Update changelog

c64cdc7

dannystaple requested review from a team as code owners November 11, 2022 16:57

osg-grafana reviewed Nov 11, 2022

View reviewed changes

docs/sources/operators-guide/architecture/components/alertmanager.md Outdated Show resolved Hide resolved

osg-grafana approved these changes Nov 11, 2022

View reviewed changes

Update docs/sources/operators-guide/architecture/components/alertmana…

f89e273

…ger.md Co-authored-by: Ursula Kallio <ursula.kallio@grafana.com>

dannystaple added 2 commits November 11, 2022 18:05

Swap a command line flag for a verify subcommand.

1b4def7

Merge branch '3437-alertmanager-config-validation' of github.com:dann…

334d765

…ystaple/mimir into 3437-alertmanager-config-validation

pracucci reviewed Nov 15, 2022

View reviewed changes

dannystaple and others added 6 commits November 15, 2022 12:26

Update pkg/mimirtool/commands/alerts.go

221d276

Co-authored-by: Marco Pracucci <marco@pracucci.com>

For loop to reduce repetition

577df3c

Use review suggestion.

2dbc503

Make alertmanager verification docs more consistent.

4b1929f

Bump from enhancement to feature.

fmt

2a9d9be

Fix the golint issue.

d610195

jhesketh approved these changes Nov 17, 2022

View reviewed changes

pracucci approved these changes Nov 17, 2022

View reviewed changes

Fix typo

720389c

pracucci enabled auto-merge (squash) November 17, 2022 13:28

Make doc result (as specified by lint process)

85de674

auto-merge was automatically disabled November 17, 2022 14:02
Head branch was pushed to by a user without write access

pracucci enabled auto-merge (squash) November 17, 2022 14:10

pracucci merged commit cba8c18 into grafana:main Nov 17, 2022

dannystaple deleted the 3437-alertmanager-config-validation branch November 17, 2022 14:32

		@@ -97,6 +97,7 @@

		### Mimirtool

		* [ENHANCEMENT] Added `mimirtool alertmanager verify` command to validate configuration without uploading. #3247

	* [ENHANCEMENT] Added `mimirtool alertmanager verify` command to validate configuration without uploading. #3247
	* [FEATURE] Added `mimirtool alertmanager verify` command to validate configuration without uploading. #3440

		getAlertsCmd.Flag("address", "Address of the Grafana Mimir cluster; alternatively, set "+envVars.Address+".").Envar(envVars.Address).Required().StringVar(&a.ClientConfig.Address)
		getAlertsCmd.Flag("id", "Grafana Mimir tenant ID; alternatively, set "+envVars.TenantID+".").Envar(envVars.TenantID).Required().StringVar(&a.ClientConfig.ID)

	func (a AlertmanagerCommand) prepareAlertManagerConfig(k kingpin.ParseContext) (error, string, map[string]string) {
	func (a AlertmanagerCommand) readAlertmanagerConfigFromFile(k kingpin.ParseContext) (error, string, map[string]string) {

	vertifyalertCmd := alertCmd.Command("verify", "Verify Alertmanager tenant configuration and template files.").Action(a.verifyAlertmanagerConfig)
	verifyalertCmd := alertCmd.Command("verify", "Verify Alertmanager tenant configuration and template files.").Action(a.verifyAlertmanagerConfig)

Alertmanager configuration validate-only mode #3437 #3440

Alertmanager configuration validate-only mode #3437 #3440

Uh oh!

Conversation

dannystaple commented Nov 11, 2022

What this PR does

Which issue(s) this PR fixes or relates to

Checklist

Uh oh!

CLAassistant commented Nov 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

osg-grafana left a comment

Choose a reason for hiding this comment

Uh oh!

Logiraptor commented Nov 11, 2022

Uh oh!

dannystaple commented Nov 11, 2022

Uh oh!

dannystaple commented Nov 11, 2022

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhesketh left a comment

Choose a reason for hiding this comment

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

CLAassistant commented Nov 11, 2022 •

edited

Loading