Validate configured -alertmanager.web.external-url and fail if ends with / (#4081)

* Validate configured -alertmanager.web.external-url and fail if ends with /

Signed-off-by: Marco Pracucci <[email protected]>

* Fixed linter error

Signed-off-by: Marco Pracucci <[email protected]>

Author: pracucci

CHANGELOG.md

@@ -41,6 +41,7 @@
 * [ENHANCEMENT] Distributor: added per-distributor limits: max number of inflight requests (`-distributor.instance-limits.max-inflight-push-requests`) and max ingestion rate in samples/sec (`-distributor.instance-limits.max-ingestion-rate`). If not set, these two are unlimited. Also added metrics to expose current values (`cortex_distributor_inflight_push_requests`, `cortex_distributor_ingestion_rate_samples_per_second`) as well as limits (`cortex_distributor_instance_limits` with various `limit` label values). #4071
 * [ENHANCEMENT] Ruler: Added `-ruler.enabled-tenants` and `-ruler.disabled-tenants` to explicitly enable or disable rules processing for specific tenants. #4074
 * [ENHANCEMENT] Block Storage Ingester: `/flush` now accepts two new parameters: `tenant` to specify tenant to flush and `wait=true` to make call synchronous. Multiple tenants can be specified by repeating `tenant` parameter. If no `tenant` is specified, all tenants are flushed, as before. #4073
+* [ENHANCEMENT] Alertmanager: validate configured `-alertmanager.web.external-url` and fail if ends with `/`. #4081
 * [ENHANCEMENT] Allow configuration of Cassandra's host selection policy. #4069
 * [BUGFIX] Ruler-API: fix bug where `/api/v1/rules/<namespace>/<group_name>` endpoint return `400` instead of `404`. #4013
 * [BUGFIX] Distributor: reverted changes done to rate limiting in #3825. #3948

development/tsdb-blocks-storage-s3-gossip/config/cortex.yaml

@@ -89,6 +89,9 @@ ruler:
     kvstore:
       store: memberlist
 
+  alertmanager_url: http://alertmanager:8010/alertmanager
+  enable_alertmanager_v2: false
+
 ruler_storage:
   backend: s3
   s3:

development/tsdb-blocks-storage-s3-gossip/docker-compose.yml

@@ -220,7 +220,7 @@ services:
       context:    .
       dockerfile: dev.dockerfile
     image: cortex
-    command: ["sh", "-c", "sleep 3 && exec ./dlv exec ./cortex --listen=:18010 --headless=true --api-version=2 --accept-multiclient --continue -- -config.file=./config/cortex.yaml -target=alertmanager -server.http-listen-port=8010 -server.grpc-listen-port=9010  -alertmanager.web.external-url=localhost:8010"]
+    command: ["sh", "-c", "sleep 3 && exec ./dlv exec ./cortex --listen=:18010 --headless=true --api-version=2 --accept-multiclient --continue -- -config.file=./config/cortex.yaml -target=alertmanager -server.http-listen-port=8010 -server.grpc-listen-port=9010 -alertmanager.web.external-url=http://localhost:8010/alertmanager"]
     depends_on:
       - consul
       - minio

development/tsdb-blocks-storage-s3/config/cortex.yaml

@@ -78,6 +78,9 @@ ruler:
       consul:
         host: consul:8500
 
+  alertmanager_url: http://alertmanager-1:8031/alertmanager,http://alertmanager-2:8032/alertmanager,http://alertmanager-3:8033/alertmanager
+  enable_alertmanager_v2: false
+
 ruler_storage:
   backend: s3
   s3:

development/tsdb-blocks-storage-s3/config/rules.yaml

@@ -9,7 +9,6 @@ groups:
     rules:
       - alert: TooManyServices
         expr: count(up) > 1
-        for: 1m
         labels:
           severity: page
         annotations:

development/tsdb-blocks-storage-s3/docker-compose.yml

@@ -220,7 +220,7 @@ services:
       context:    .
       dockerfile: dev.dockerfile
     image: cortex
-    command: ["sh", "-c", "sleep 3 && exec ./dlv exec ./cortex --listen=:18031 --headless=true --api-version=2 --accept-multiclient --continue -- -config.file=./config/cortex.yaml -target=alertmanager -server.http-listen-port=8031 -server.grpc-listen-port=9031 -alertmanager.web.external-url=localhost:8031"]
+    command: ["sh", "-c", "sleep 3 && exec ./dlv exec ./cortex --listen=:18031 --headless=true --api-version=2 --accept-multiclient --continue -- -config.file=./config/cortex.yaml -target=alertmanager -server.http-listen-port=8031 -server.grpc-listen-port=9031 -alertmanager.web.external-url=http://localhost:8031/alertmanager"]
     depends_on:
       - consul
       - minio
@@ -235,7 +235,7 @@ services:
       context:    .
       dockerfile: dev.dockerfile
     image: cortex
-    command: ["sh", "-c", "sleep 3 && exec ./dlv exec ./cortex --listen=:18032 --headless=true --api-version=2 --accept-multiclient --continue -- -config.file=./config/cortex.yaml -target=alertmanager -server.http-listen-port=8032 -server.grpc-listen-port=9032 -alertmanager.web.external-url=localhost:8032"]
+    command: ["sh", "-c", "sleep 3 && exec ./dlv exec ./cortex --listen=:18032 --headless=true --api-version=2 --accept-multiclient --continue -- -config.file=./config/cortex.yaml -target=alertmanager -server.http-listen-port=8032 -server.grpc-listen-port=9032 -alertmanager.web.external-url=http://localhost:8032/alertmanager"]
     depends_on:
       - consul
       - minio
@@ -250,7 +250,7 @@ services:
       context:    .
       dockerfile: dev.dockerfile
     image: cortex
-    command: ["sh", "-c", "sleep 3 && exec ./dlv exec ./cortex --listen=:18033 --headless=true --api-version=2 --accept-multiclient --continue -- -config.file=./config/cortex.yaml -target=alertmanager -server.http-listen-port=8033 -server.grpc-listen-port=9033 -alertmanager.web.external-url=localhost:8033"]
+    command: ["sh", "-c", "sleep 3 && exec ./dlv exec ./cortex --listen=:18033 --headless=true --api-version=2 --accept-multiclient --continue -- -config.file=./config/cortex.yaml -target=alertmanager -server.http-listen-port=8033 -server.grpc-listen-port=9033 -alertmanager.web.external-url=http://localhost:8033/alertmanager"]
     depends_on:
       - consul
       - minio

pkg/alertmanager/multitenant.go

@@ -85,6 +85,8 @@ const (
 
 var (
 	statusTemplate *template.Template
+
+	errInvalidExternalURL = errors.New("the configured external URL is invalid: should not end with /")
 )
 
 func init() {
@@ -176,6 +178,10 @@ func (cfg *ClusterConfig) RegisterFlags(f *flag.FlagSet) {
 
 // Validate config and returns error on failure
 func (cfg *MultitenantAlertmanagerConfig) Validate() error {
+	if cfg.ExternalURL.URL != nil && strings.HasSuffix(cfg.ExternalURL.Path, "/") {
+		return errInvalidExternalURL
+	}
+
 	if err := cfg.Store.Validate(); err != nil {
 		return errors.Wrap(err, "invalid storage config")
 	}

pkg/alertmanager/multitenant_test.go

@@ -86,25 +86,47 @@ func mockAlertmanagerConfig(t *testing.T) *MultitenantAlertmanagerConfig {
 }
 
 func TestMultitenantAlertmanagerConfig_Validate(t *testing.T) {
-	// Default values only.
-	{
-		cfg := &MultitenantAlertmanagerConfig{}
-		flagext.DefaultValues(cfg)
-		assert.NoError(t, cfg.Validate())
-	}
-	// Invalid persist interval (zero).
-	{
-		cfg := &MultitenantAlertmanagerConfig{}
-		flagext.DefaultValues(cfg)
-		cfg.Persister.Interval = 0
-		assert.Equal(t, errInvalidPersistInterval, cfg.Validate())
+	tests := map[string]struct {
+		setup    func(t *testing.T, cfg *MultitenantAlertmanagerConfig)
+		expected error
+	}{
+		"should pass with default config": {
+			setup:    func(t *testing.T, cfg *MultitenantAlertmanagerConfig) {},
+			expected: nil,
+		},
+		"should fail if persistent interval is 0": {
+			setup: func(t *testing.T, cfg *MultitenantAlertmanagerConfig) {
+				cfg.Persister.Interval = 0
+			},
+			expected: errInvalidPersistInterval,
+		},
+		"should fail if persistent interval is negative": {
+			setup: func(t *testing.T, cfg *MultitenantAlertmanagerConfig) {
+				cfg.Persister.Interval = -1
+			},
+			expected: errInvalidPersistInterval,
+		},
+		"should fail if external URL ends with /": {
+			setup: func(t *testing.T, cfg *MultitenantAlertmanagerConfig) {
+				require.NoError(t, cfg.ExternalURL.Set("http://localhost/prefix/"))
+			},
+			expected: errInvalidExternalURL,
+		},
+		"should succeed if external URL does not end with /": {
+			setup: func(t *testing.T, cfg *MultitenantAlertmanagerConfig) {
+				require.NoError(t, cfg.ExternalURL.Set("http://localhost/prefix"))
+			},
+			expected: nil,
+		},
 	}
-	// Invalid persist interval (negative).
-	{
-		cfg := &MultitenantAlertmanagerConfig{}
-		flagext.DefaultValues(cfg)
-		cfg.Persister.Interval = -1
-		assert.Equal(t, errInvalidPersistInterval, cfg.Validate())
+
+	for testName, testData := range tests {
+		t.Run(testName, func(t *testing.T) {
+			cfg := &MultitenantAlertmanagerConfig{}
+			flagext.DefaultValues(cfg)
+			testData.setup(t, cfg)
+			assert.Equal(t, testData.expected, cfg.Validate())
+		})
 	}
 }