Skip to content

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

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

Conversation

pracucci
Copy link
Contributor

What this PR does:
Earlier this week I was testing the alertmanager in the local setup and I've learned the hard way that ruler->alertmanger notification doesn't work in the local dev setup and that if you configure -alertmanager.web.external-url with an ending / the routing doesn't work as expected.

This PR fixes the local dev env and adds a validation rule on -alertmanager.web.external-url.

Which issue(s) this PR fixes:
N/A

Checklist

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

…ith /

Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
@pstibrany
Copy link
Contributor

I've learned the hard way that ruler->alertmanger notification doesn't work in the local dev setup and that if you configure -alertmanager.web.external-url with an ending /

Instead of erroring out, can we fix the problem with /? Either by removing it from the path, or using path.Join when appending. Why does having / in external-url fail in the first place?

@pracucci
Copy link
Contributor Author

Why does having / in external-url fail in the first place?

I don't have time to investigate it (I consider it low priority). I see two options: accept the validation as a signal of a misconfiguration or I can roll it back and we just keep the fix for the dev env.

@pstibrany
Copy link
Contributor

I see two options: accept the validation as a signal of a misconfiguration or I can roll it back and we just keep the fix for the dev env.

Validation is better than rollback. Approved. We can improve it when there is more time.

@pracucci pracucci merged commit 8851245 into cortexproject:master Apr 16, 2021
@pracucci pracucci deleted the fix-alertmanager-local-dev-and-config-validation branch April 16, 2021 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants