From 824b93496e55ee15c3a883fb0d7807e54bfe3691 Mon Sep 17 00:00:00 2001 From: Marco Pracucci Date: Thu, 15 Apr 2021 09:54:12 +0200 Subject: [PATCH 1/2] Validate configured -alertmanager.web.external-url and fail if ends with / Signed-off-by: Marco Pracucci --- CHANGELOG.md | 1 + .../config/cortex.yaml | 3 + .../docker-compose.yml | 2 +- .../tsdb-blocks-storage-s3/config/cortex.yaml | 3 + .../tsdb-blocks-storage-s3/config/rules.yaml | 1 - .../tsdb-blocks-storage-s3/docker-compose.yml | 6 +- pkg/alertmanager/multitenant.go | 6 ++ pkg/alertmanager/multitenant_test.go | 58 +++++++++++++------ 8 files changed, 57 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f1b62bff23f..9e74ed0630b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,6 +40,7 @@ * [ENHANCEMENT] Cortex is now built with Go 1.16. #4062 * [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 * [BUGFIX] Ruler-API: fix bug where `/api/v1/rules//` endpoint return `400` instead of `404`. #4013 * [BUGFIX] Distributor: reverted changes done to rate limiting in #3825. #3948 * [BUGFIX] Ingester: Fix race condition when opening and closing tsdb concurrently. #3959 diff --git a/development/tsdb-blocks-storage-s3-gossip/config/cortex.yaml b/development/tsdb-blocks-storage-s3-gossip/config/cortex.yaml index 1a0f78956fc..bc79dabb8c1 100644 --- a/development/tsdb-blocks-storage-s3-gossip/config/cortex.yaml +++ b/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: diff --git a/development/tsdb-blocks-storage-s3-gossip/docker-compose.yml b/development/tsdb-blocks-storage-s3-gossip/docker-compose.yml index 73b5dfddaff..bfc5fbf7fb7 100644 --- a/development/tsdb-blocks-storage-s3-gossip/docker-compose.yml +++ b/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 diff --git a/development/tsdb-blocks-storage-s3/config/cortex.yaml b/development/tsdb-blocks-storage-s3/config/cortex.yaml index 10b425eae83..a94ca8520b6 100644 --- a/development/tsdb-blocks-storage-s3/config/cortex.yaml +++ b/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: diff --git a/development/tsdb-blocks-storage-s3/config/rules.yaml b/development/tsdb-blocks-storage-s3/config/rules.yaml index b65a4590da9..a9b90df759c 100644 --- a/development/tsdb-blocks-storage-s3/config/rules.yaml +++ b/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: diff --git a/development/tsdb-blocks-storage-s3/docker-compose.yml b/development/tsdb-blocks-storage-s3/docker-compose.yml index ef62c702668..c943c30b8a2 100644 --- a/development/tsdb-blocks-storage-s3/docker-compose.yml +++ b/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 diff --git a/pkg/alertmanager/multitenant.go b/pkg/alertmanager/multitenant.go index 3fce502096b..661cbe23cc1 100644 --- a/pkg/alertmanager/multitenant.go +++ b/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") } diff --git a/pkg/alertmanager/multitenant_test.go b/pkg/alertmanager/multitenant_test.go index 3357c4badd8..0e3acb2c898 100644 --- a/pkg/alertmanager/multitenant_test.go +++ b/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(cfg *MultitenantAlertmanagerConfig) + expected error + }{ + "should pass with default config": { + setup: func(cfg *MultitenantAlertmanagerConfig) {}, + expected: nil, + }, + "should fail if persistent interval is 0": { + setup: func(cfg *MultitenantAlertmanagerConfig) { + cfg.Persister.Interval = 0 + }, + expected: errInvalidPersistInterval, + }, + "should fail if persistent interval is negative": { + setup: func(cfg *MultitenantAlertmanagerConfig) { + cfg.Persister.Interval = -1 + }, + expected: errInvalidPersistInterval, + }, + "should fail if external URL ends with /": { + setup: func(cfg *MultitenantAlertmanagerConfig) { + cfg.ExternalURL.Set("http://localhost/prefix/") + }, + expected: errInvalidExternalURL, + }, + "should succeed if external URL does not end with /": { + setup: func(cfg *MultitenantAlertmanagerConfig) { + 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(cfg) + assert.Equal(t, testData.expected, cfg.Validate()) + }) } } From db81b131eebc66538c57a3c5988bce391fbdd593 Mon Sep 17 00:00:00 2001 From: Marco Pracucci Date: Thu, 15 Apr 2021 10:37:08 +0200 Subject: [PATCH 2/2] Fixed linter error Signed-off-by: Marco Pracucci --- pkg/alertmanager/multitenant_test.go | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/pkg/alertmanager/multitenant_test.go b/pkg/alertmanager/multitenant_test.go index 0e3acb2c898..02cc78e16a9 100644 --- a/pkg/alertmanager/multitenant_test.go +++ b/pkg/alertmanager/multitenant_test.go @@ -87,34 +87,34 @@ func mockAlertmanagerConfig(t *testing.T) *MultitenantAlertmanagerConfig { func TestMultitenantAlertmanagerConfig_Validate(t *testing.T) { tests := map[string]struct { - setup func(cfg *MultitenantAlertmanagerConfig) + setup func(t *testing.T, cfg *MultitenantAlertmanagerConfig) expected error }{ "should pass with default config": { - setup: func(cfg *MultitenantAlertmanagerConfig) {}, + setup: func(t *testing.T, cfg *MultitenantAlertmanagerConfig) {}, expected: nil, }, "should fail if persistent interval is 0": { - setup: func(cfg *MultitenantAlertmanagerConfig) { + setup: func(t *testing.T, cfg *MultitenantAlertmanagerConfig) { cfg.Persister.Interval = 0 }, expected: errInvalidPersistInterval, }, "should fail if persistent interval is negative": { - setup: func(cfg *MultitenantAlertmanagerConfig) { + setup: func(t *testing.T, cfg *MultitenantAlertmanagerConfig) { cfg.Persister.Interval = -1 }, expected: errInvalidPersistInterval, }, "should fail if external URL ends with /": { - setup: func(cfg *MultitenantAlertmanagerConfig) { - cfg.ExternalURL.Set("http://localhost/prefix/") + 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(cfg *MultitenantAlertmanagerConfig) { - cfg.ExternalURL.Set("http://localhost/prefix") + setup: func(t *testing.T, cfg *MultitenantAlertmanagerConfig) { + require.NoError(t, cfg.ExternalURL.Set("http://localhost/prefix")) }, expected: nil, }, @@ -124,7 +124,7 @@ func TestMultitenantAlertmanagerConfig_Validate(t *testing.T) { t.Run(testName, func(t *testing.T) { cfg := &MultitenantAlertmanagerConfig{} flagext.DefaultValues(cfg) - testData.setup(cfg) + testData.setup(t, cfg) assert.Equal(t, testData.expected, cfg.Validate()) }) }