Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support to set the Slack URL in a file #2534

Merged
merged 2 commits into from
Apr 13, 2021
Merged

Add support to set the Slack URL in a file #2534

merged 2 commits into from
Apr 13, 2021

Conversation

julienduchesne
Copy link
Contributor

  • Added support for the file in both the global and the lower level
  • Tried to follow configuration patterns I saw in prometheus
  • The slack file is read on every request as mentioned in the prometheus issue to enable seamless switches

#2498

@julienduchesne
Add support to set the Slack URL in a file
59c7fd5 
- Added support for the file in both the global and the lower level
- Tried to follow configuration patterns I saw in prometheus
- The slack file is read on every request as mentioned in the prometheus issue to enable seamless switches

prometheus#2498
Signed-off-by: Julien Duchesne <julien.duchesne@grafana.com>
beorn7
beorn7 approved these changes Apr 12, 2021
View reviewed changes
Copy link
Member

@beorn7 beorn7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this works. @roidelapluie @simonpasquier do you want to have a look, too?

config/testdata/conf.slack-no-api-url.yml Outdated
- name: 'slack-notifications'
slack_configs:
- channel: '#alerts'
text: 'test'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No newline at end of file.

config/testdata/conf.slack-default-api-url-file.yml Outdated
- channel: '#alerts3'
text: 'test'
api_url: 'http://mysecret.example.com/'

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No newline at end of file. (But some whitespace on line 22, which just needs to be deleted?)

config/testdata/conf.slack-both-file-and-url.yml Outdated
slack_configs:
- channel: '#alerts1'
text: 'test'

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No newline at end of file. (But some whitespace on line 22, which just needs to be deleted?)

@beorn7
Copy link
Member

beorn7 commented Apr 12, 2021

Of course, eventually we have to do the same for the other fields listed in #2498 . Not sure if we can reuse some code or just have to do it all individually.

beorn7
beorn7 requested changes Apr 12, 2021
View reviewed changes
Copy link
Member

@beorn7 beorn7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just realized that docs/configuration.md needs to be updated, too. And perhaps an example in docs/notification_examples.md .

beorn7
beorn7 approved these changes Apr 13, 2021
View reviewed changes
Copy link
Member

@beorn7 beorn7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you.

@beorn7
Copy link
Member

beorn7 commented Apr 13, 2021

@julienduchesne unfortunately, now the DCO is missing on a commit. See https://github.com/prometheus/alertmanager/pull/2534/checks?check_run_id=2332859284

@julienduchesne
Updated docs. Fixed YAML newlines
f33427c 
Signed-off-by: Julien Duchesne <julien.duchesne@grafana.com>
@julienduchesne
Copy link
Contributor Author

@julienduchesne unfortunately, now the DCO is missing on a commit. See https://github.com/prometheus/alertmanager/pull/2534/checks?check_run_id=2332859284

Sorry about that. Fixed 👍

@beorn7
Copy link
Member

beorn7 commented Apr 13, 2021

Cool. Merging now, assuming that silence from @simonpasquier and @roidelapluie means consent. :->

@beorn7 beorn7 merged commit 22ac6df into prometheus:master Apr 13, 2021
@beorn7 beorn7 mentioned this pull request Apr 20, 2021
Open
@philipgough philipgough mentioned this pull request Aug 30, 2021
Merged
5 tasks
simonpasquier pushed a commit to prometheus-operator/prometheus-operator that referenced this pull request Sep 20, 2021
@philipgough
alertmanager: Support Slack webhook URL via file path in config (#4234)
32981c3 
* alertmanager: Add namespaced fields to logger

* alertmanager: Accept and parse all config during generation

During config generation, we want to accept the provided config
as is and generate accordingly. This will allow us to surface
any issues that occur with invalid or incorrect config
instead of skipping over potential invalid input.

* alertmanager: Support slack_api_url_file and api_url_file for Slack receiver

This change allows an operator to add two additional fields into the manually
managed secret.

See prometheus/alertmanager#2534

* alertmanager: Sanitize the config against a specific version

* alertmanager: Sanitize http config for all receivers
@jkroepke jkroepke mentioned this pull request Oct 1, 2021
Merged
ericrrath added a commit to ericrrath/alertmanager that referenced this pull request Aug 15, 2022
@ericrrath
SMTP config: add global and local password file fields
a93e4eb 
Add config fields (for both global email config and route-specific email
config) that specify path to file containing SMTP password.  We don't
want the password in the config file itself, and reading the password
from a k8s-secret-backed file keeps the password itself "encrypted at
rest" in etcd, and cleanly separated from the rest of the AM config.

I used the same approach as pull request prometheus#2534 "Add support to set the
Slack URL in the file"
<https://github.com/prometheus/alertmanager/pull/2534/files> in the
upstream repo.
@ericrrath ericrrath mentioned this pull request Aug 15, 2022
Merged
ericrrath added a commit to ericrrath/alertmanager that referenced this pull request Aug 15, 2022
@ericrrath
SMTP config: add global and local password file fields
aa036db 
Add config fields (for both global email config and route-specific email
config) that specify path to file containing SMTP password.  We don't
want the password in the config file itself, and reading the password
from a k8s-secret-backed file keeps the password itself "encrypted at
rest" in etcd, and cleanly separated from the rest of the AM config.

I used the same approach as pull request prometheus#2534 "Add support to set the
Slack URL in the file"
<https://github.com/prometheus/alertmanager/pull/2534/files> in the
upstream repo.

Signed-off-by: Eric R. Rath <eric.rath@oracle.com>
ericrrath added a commit to ericrrath/alertmanager that referenced this pull request Sep 12, 2022
@ericrrath
SMTP config: add global and local password file fields
e2971ee 
Add config fields (for both global email config and route-specific email
config) that specify path to file containing SMTP password.  We don't
want the password in the config file itself, and reading the password
from a k8s-secret-backed file keeps the password itself "encrypted at
rest" in etcd, and cleanly separated from the rest of the AM config.

I used the same approach as pull request prometheus#2534 "Add support to set the
Slack URL in the file"
<https://github.com/prometheus/alertmanager/pull/2534/files> in the
upstream repo.

Signed-off-by: Eric R. Rath <eric.rath@oracle.com>
simonpasquier added a commit that referenced this pull request Sep 16, 2022
@ericrrath @simonpasquier
SMTP config: add global and local password file fields (#3038)
efc956c 
* SMTP config: add global and local password file fields

Add config fields (for both global email config and route-specific email
config) that specify path to file containing SMTP password.  We don't
want the password in the config file itself, and reading the password
from a k8s-secret-backed file keeps the password itself "encrypted at
rest" in etcd, and cleanly separated from the rest of the AM config.

I used the same approach as pull request #2534 "Add support to set the
Slack URL in the file"
<https://github.com/prometheus/alertmanager/pull/2534/files> in the
upstream repo.

Signed-off-by: Eric R. Rath <eric.rath@oracle.com>

* changed *AuthPasswordFile field types to string per review feedback

Signed-off-by: Eric R. Rath <eric.rath@oracle.com>

* added error to getPassword() retval per review feedback

Signed-off-by: Eric R. Rath <eric.rath@oracle.com>

* simplified conf.smtp-* files

Signed-off-by: Eric R. Rath <eric.rath@oracle.com>

* update docs to reflect field type change

Signed-off-by: Eric R. Rath <eric.rath@oracle.com>

* don't treat username-without-password as invalid

Signed-off-by: Eric R. Rath <eric.rath@oracle.com>

* test cleanup

Signed-off-by: Eric R. Rath <eric.rath@oracle.com>

* Apply suggestions from code review

Co-authored-by: Simon Pasquier <spasquie@redhat.com>
Signed-off-by: Eric R. Rath <4080262+ericrrath@users.noreply.github.com>

* Updated per review feedback

Signed-off-by: Eric R. Rath <eric.rath@oracle.com>

* added sub-test per review feedback

Signed-off-by: Eric R. Rath <eric.rath@oracle.com>

* added test on Email.getPassword() per feedback

Signed-off-by: Eric R. Rath <eric.rath@oracle.com>

* only inherit global SMTP passwords if neither local password field is set

Signed-off-by: Eric R. Rath <eric.rath@oracle.com>

* removed blank line caught by gofumpt

Signed-off-by: Eric R. Rath <eric.rath@oracle.com>

Signed-off-by: Eric R. Rath <eric.rath@oracle.com>
Signed-off-by: Eric R. Rath <4080262+ericrrath@users.noreply.github.com>
Co-authored-by: Simon Pasquier <spasquie@redhat.com>
qinxx108 pushed a commit to qinxx108/alertmanager that referenced this pull request Dec 13, 2022
@ericrrath @simonpasquier @qinxx108
SMTP config: add global and local password file fields (prometheus#3038)
28ba3b5 
* SMTP config: add global and local password file fields

Add config fields (for both global email config and route-specific email
config) that specify path to file containing SMTP password.  We don't
want the password in the config file itself, and reading the password
from a k8s-secret-backed file keeps the password itself "encrypted at
rest" in etcd, and cleanly separated from the rest of the AM config.

I used the same approach as pull request prometheus#2534 "Add support to set the
Slack URL in the file"
<https://github.com/prometheus/alertmanager/pull/2534/files> in the
upstream repo.

Signed-off-by: Eric R. Rath <eric.rath@oracle.com>

* changed *AuthPasswordFile field types to string per review feedback

Signed-off-by: Eric R. Rath <eric.rath@oracle.com>

* added error to getPassword() retval per review feedback

Signed-off-by: Eric R. Rath <eric.rath@oracle.com>

* simplified conf.smtp-* files

Signed-off-by: Eric R. Rath <eric.rath@oracle.com>

* update docs to reflect field type change

Signed-off-by: Eric R. Rath <eric.rath@oracle.com>

* don't treat username-without-password as invalid

Signed-off-by: Eric R. Rath <eric.rath@oracle.com>

* test cleanup

Signed-off-by: Eric R. Rath <eric.rath@oracle.com>

* Apply suggestions from code review

Co-authored-by: Simon Pasquier <spasquie@redhat.com>
Signed-off-by: Eric R. Rath <4080262+ericrrath@users.noreply.github.com>

* Updated per review feedback

Signed-off-by: Eric R. Rath <eric.rath@oracle.com>

* added sub-test per review feedback

Signed-off-by: Eric R. Rath <eric.rath@oracle.com>

* added test on Email.getPassword() per feedback

Signed-off-by: Eric R. Rath <eric.rath@oracle.com>

* only inherit global SMTP passwords if neither local password field is set

Signed-off-by: Eric R. Rath <eric.rath@oracle.com>

* removed blank line caught by gofumpt

Signed-off-by: Eric R. Rath <eric.rath@oracle.com>

Signed-off-by: Eric R. Rath <eric.rath@oracle.com>
Signed-off-by: Eric R. Rath <4080262+ericrrath@users.noreply.github.com>
Co-authored-by: Simon Pasquier <spasquie@redhat.com>
Signed-off-by: Yijie Qin <qinyijie@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@beorn7 beorn7 beorn7 approved these changes

@simonpasquier simonpasquier Awaiting requested review from simonpasquier

@roidelapluie roidelapluie Awaiting requested review from roidelapluie

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@julienduchesne @beorn7