Change config flags, add flags to disable Alertmanager notifications methods: email & webhoook #2187

Wing924 · 2020-02-26T11:03:20Z

What this PR does:

Added flags to disable Alertmanager notifications methods.

[CHANGE] Renamed Configs configuration options.
- configuration options
  - -database.* -> -configs.database.*
  - -database.migrations -> -configs.database.migrations-dir
- config file
  - configdb.uri: -> configs.database.uri:
  - configdb.migrationsdir: -> configs.database.migrations_dir:
  - configdb.passwordfile: -> configs.database.password_file:

Which issue(s) this PR fixes:

The Alertmanager notifications via email is disabled in cortex.
I tested on my cluster and found email works without any problems.

I asked on Slack and found the reason is email may attack SaaS.
I other words, we can enable email on on-premises.

Checklist

Tests updated
Documentation added
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

Signed-off-by: Wing924 <[email protected]>

bboreham · 2020-02-26T11:20:31Z

So many flags seems like overkill to me.
I could see an argument to disable (anything with an email address) and (anything with a URL); I don't really see the case to selectively disable Wechat over OpsGenie.

Wing924 · 2020-02-26T11:42:09Z

you’re right. we don’t need so many flags.
what flags should we keep?

pracucci · 2020-02-26T14:17:34Z

docs/configuration/config-file-reference.md

@@ -108,6 +108,10 @@ Where default_value is the value to use if the environment variable is undefined
 # The ruler_config configures the Cortex ruler.
 [ruler: <ruler_config>]

+# The configs_api_config configures the configs API used by the 'configs'
+# service to expose APIs to manage them.
+[configs_api: <configs_api_config>]


From the configuration perspective, I'm a bit concerned having another root block config. Ideally we should have 1 root config block for each service (even if in the past some weird things have been done), so think it would be better adding the NotificationsConfig as part of the ConfigDB config.

Signed-off-by: Wing924 <[email protected]>

Wing924 · 2020-02-27T07:06:07Z

@bboreham
I reduced the options to only email and webhook.

@pracucci
Thank you for your advice.
I changed the configurations structure.

gouthamve

LGTM with minor nit around the error message.

pkg/configs/api/api.go

pracucci

Very good job! I love your contributions: clean and concise ❤️

I left few minor comments, then will LGTM. Thanks!

docs/configuration/config-file-reference.md

tools/doc-generator/main.go

docs/configuration/config-file-reference.md

pkg/configs/api/api.go

Signed-off-by: Wing924 <[email protected]>

Wing924 · 2020-03-02T04:17:03Z

@pracucci
I fixed it as you described.
I renamed configs.Config to userconfig.Config due to circular dependency.
Please check it again.

pracucci

Thanks @Wing924 for addressing my feedback! I just left minor comments, but LGTM. I would like to get @jtlisi on this review, cause he's more familiar with this stuff.

CHANGELOG.md

pkg/configs/db/db.go

pkg/configs/api/api_test.go

pkg/configs/api/api.go

Signed-off-by: Wing924 <[email protected]>

pracucci

Thanks @Wing924 for addressing my comment! I left few comments and then it will be good to go to me! Thanks 🙏

CHANGELOG.md

pkg/configs/api/api.go

pkg/configs/api/api_test.go

Signed-off-by: Wing924 <[email protected]>

go.sum

pracucci

LGTM, with a nit!

Signed-off-by: Wing924 <[email protected]>

bboreham · 2020-03-03T15:16:21Z

Please edit the PR title and description to cover all that it now changes.

Signed-off-by: Wing924 <[email protected]>

Added flags to disable Alertmanager notifications methods.

cfb3ab5

Signed-off-by: Wing924 <[email protected]>

pull-request-size bot added the size/L label Feb 26, 2020

Wing924 added 2 commits February 26, 2020 20:03

fix

bba4f81

Signed-off-by: Wing924 <[email protected]>

add doc

61eb461

Signed-off-by: Wing924 <[email protected]>

pracucci reviewed Feb 26, 2020

View reviewed changes

Wing924 added 2 commits February 27, 2020 11:58

Merge branch 'master' into opt-in-alert-email

f1b27a9

fix

13caa80

Signed-off-by: Wing924 <[email protected]>

Wing924 requested a review from pracucci February 27, 2020 07:06

gouthamve reviewed Feb 28, 2020

View reviewed changes

pkg/configs/api/api.go Outdated Show resolved Hide resolved

pkg/configs/api/api.go Outdated Show resolved Hide resolved

pracucci reviewed Feb 28, 2020

View reviewed changes

Wing924 added 4 commits March 2, 2020 11:42

wip

488de6a

Signed-off-by: Wing924 <[email protected]>

Merge branch 'master' into opt-in-alert-email

2eecb5b

Signed-off-by: Wing924 <[email protected]>

fix

f0e2252

Signed-off-by: Wing924 <[email protected]>

fix

62e4fd8

Signed-off-by: Wing924 <[email protected]>

Wing924 requested review from pracucci and gouthamve March 2, 2020 04:17

pracucci reviewed Mar 2, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

pkg/configs/db/db.go Outdated Show resolved Hide resolved

pkg/configs/api/api_test.go Outdated Show resolved Hide resolved

pkg/configs/api/api.go Outdated Show resolved Hide resolved

Wing924 added 3 commits March 2, 2020 18:50

fix

1dd81f5

Signed-off-by: Wing924 <[email protected]>

fix

3672c50

Signed-off-by: Wing924 <[email protected]>

fix

13ce0c5

Signed-off-by: Wing924 <[email protected]>

pracucci reviewed Mar 3, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

pkg/configs/api/api.go Outdated Show resolved Hide resolved

pkg/configs/api/api.go Outdated Show resolved Hide resolved

pkg/configs/api/api_test.go Outdated Show resolved Hide resolved

Wing924 added 2 commits March 3, 2020 22:48

Merge branch 'master' into opt-in-alert-email

8c4dc50

fix

b252218

Signed-off-by: Wing924 <[email protected]>

pracucci reviewed Mar 3, 2020

View reviewed changes

go.sum Outdated Show resolved Hide resolved

pracucci approved these changes Mar 3, 2020

View reviewed changes

fix

91dcdc9

Signed-off-by: Wing924 <[email protected]>

Wing924 changed the title ~~Added flags to disable Alertmanager notifications methods.~~ Added flags to disable Alertmanager notifications methods: email & webhoook Mar 3, 2020

pull-request-size bot added size/XL and removed size/L labels Mar 3, 2020

Merge branch 'master' into opt-in-alert-email

30205f1

Signed-off-by: Wing924 <[email protected]>

bboreham changed the title ~~Added flags to disable Alertmanager notifications methods: email & webhoook~~ Change config flags, add flags to disable Alertmanager notifications methods: email & webhoook Mar 4, 2020

gouthamve approved these changes Mar 4, 2020

View reviewed changes

gouthamve merged commit f0d8d6a into cortexproject:master Mar 4, 2020

Wing924 deleted the opt-in-alert-email branch March 4, 2020 15:37

Change config flags, add flags to disable Alertmanager notifications methods: email & webhoook #2187

Change config flags, add flags to disable Alertmanager notifications methods: email & webhoook #2187

Uh oh!

Conversation

Wing924 commented Feb 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bboreham commented Feb 26, 2020

Uh oh!

Wing924 commented Feb 26, 2020

Uh oh!

pracucci Feb 26, 2020

Choose a reason for hiding this comment

Uh oh!

Wing924 commented Feb 27, 2020

Uh oh!

gouthamve left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Wing924 commented Mar 2, 2020

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

bboreham commented Mar 3, 2020

Uh oh!

Uh oh!

Wing924 commented Feb 26, 2020 •

edited

Loading