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.

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

support loading webhook URL from a file #3223

Merged
merged 5 commits into from Mar 3, 2023

Conversation

sr
Copy link
Contributor

@sr sr commented Jan 19, 2023

Both DeadMansSnitch and Healthchecks.io treat the URL as a secret, so I am guessing this is a common enough use case.

/cc #2498

Copy link
Member

@simonpasquier simonpasquier left a comment

Choose a reason for hiding this comment

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

lgtm. @gotjosh are you fine with this change? It's a bit puzzling that the url field isn't a secret type but changing it would be inconvenient for many users and the url_file field fills a gap for some webhooks.

@gotjosh
Copy link
Member

gotjosh commented Jan 19, 2023

How bad would it be if we changed URL to Secret? I definitely think this should be a secret.

@sr
Copy link
Contributor Author

sr commented Jan 19, 2023

AFAICT the only place where it's marshalled back is in the status page. So changing WebhookConfig.URL to be a SecretURL would cause it be shown as the string <secret>, vs then the actual URL now.

Side note, if it were to be changed to a secret, notify.RedactURL(err) would also be needed here:

return true, err

/cc prometheus#2498

Signed-off-by: Simon Rozet <me@simonrozet.com>
@sr
Copy link
Contributor Author

sr commented Jan 23, 2023

@gotjosh @simonpasquier Should I make the SecretURL change here or in a separate PR?

@gotjosh
Copy link
Member

gotjosh commented Jan 25, 2023

Can you please put together a separate PR? Thanks @sr.

@sr sr mentioned this pull request Jan 25, 2023
Signed-off-by: Simon Rozet <me@simonrozet.com>
Signed-off-by: Simon Rozet <me@simonrozet.com>
Signed-off-by: Simon Rozet <me@simonrozet.com>
@sr
Copy link
Contributor Author

sr commented Feb 3, 2023

Sweet, thanks for merging #3228.

I've merged in main and added a few more tests, including a couple test cases I should have added in #3200.

make test is passing locally. The failure on CI looks like a flake:
https://app.circleci.com/pipelines/github/prometheus/alertmanager/3322/workflows/6647f932-9964-42fc-9291-f0e1f560254c/jobs/14718

@sr
Copy link
Contributor Author

sr commented Feb 24, 2023

@simonpasquier Build is now green. Good to merge?

@simonpasquier simonpasquier merged commit 41eb121 into prometheus:main Mar 3, 2023
@simonpasquier
Copy link
Member

thanks!

@sr sr deleted the webhook-file branch March 7, 2023 11:57
hoperays pushed a commit to hoperays/alertmanager that referenced this pull request Apr 23, 2023
* support loading webhook URL from a file

/cc prometheus#2498

Signed-off-by: Simon Rozet <me@simonrozet.com>

* notify/webhook: add test for reading url from file

Signed-off-by: Simon Rozet <me@simonrozet.com>

* notify/pushover: add tests for reading secrets from files

Signed-off-by: Simon Rozet <me@simonrozet.com>

---------

Signed-off-by: Simon Rozet <me@simonrozet.com>
@aned
Copy link

aned commented Oct 12, 2023

Is it possible to have one secrets file with multiple receiver_name: api_url, so the alertmanager can parse it based on the receiver_name instead of having many files?

radek-ryckowski pushed a commit to goldmansachs/alertmanager that referenced this pull request Nov 6, 2023
* support loading webhook URL from a file

/cc prometheus#2498

Signed-off-by: Simon Rozet <me@simonrozet.com>

* notify/webhook: add test for reading url from file

Signed-off-by: Simon Rozet <me@simonrozet.com>

* notify/pushover: add tests for reading secrets from files

Signed-off-by: Simon Rozet <me@simonrozet.com>

---------

Signed-off-by: Simon Rozet <me@simonrozet.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants