Re-add full -web.external-url functionality #289
Conversation
| RegisterWeb(router) | ||
| api.Register(router.WithPrefix("/api")) | ||
| RegisterWeb(router.WithPrefix(amURL.Path)) | ||
| api.Register(router.WithPrefix(amURL.Path + "/api")) |
There was a problem hiding this comment.
Hmm, we're usually just +-ing together pre-sanitized paths in Prometheus, including in your own https://github.com/prometheus/common/blob/master/route/route.go. Not that it matters much, but maybe we should keep it consistent?
There was a problem hiding this comment.
path.Join does not work well for URLs in general, I usually decide against using it.
There was a problem hiding this comment.
path.Join is fine in general, but can only be applied to paths, not full URLs. Otherwise it'll get rid of duplicated slashes and do all kinds of other things you might not want.
There was a problem hiding this comment.
Note that this is not an issue here though, since we're dealing with paths only. Both path.Join or + would work fine, just that I think + is what we usually use in Prometheus in this situation.
There was a problem hiding this comment.
A full URL is not a path. Of course that doesn't work and it shouldn't. But this is a path (url.URL.Path) :)
Do as you wish. But it would be correct to apply here and keep things working, when sanitization somewhere up the chain changes.
There was a problem hiding this comment.
Na gut, path.Join for @fabxc :)
| controller: 'SilencesCtrl', | ||
| reloadOnSearch: false | ||
| }). | ||
| when('/status', { | ||
| templateUrl: '/app/partials/status.html', | ||
| templateUrl: 'app/partials/status.html', | ||
| controller: 'StatusCtrl' | ||
| }). | ||
| otherwise({ | ||
| redirectTo: '/alerts' |
There was a problem hiding this comment.
This redirect to /alerts won't work correctly when proxying to a different base URL.
(In our case /alertmanager...)
There was a problem hiding this comment.
As far as I can tell, this all works fine for me. Have you tested it? E.g. I run:
./alertmanager -web.external-url=http://localhost:9093/foo/bar
Going to http://localhost:9093/foo/bar correctly redirects me to http://localhost:9093/foo/bar/#/alerts.
Note that this is an angular route name that appears in the hash part of the URL, and is thus not part of the normal path.
Or am I misunderstanding something?
|
👍 |
|
Doesn't this merit a 0.1.2 release? Our configuration won't work if we can't proxy. |
|
hey... when do you will release this fix? |
|
@fabxc would probably know when a next release is planned for the Alertmanager. |
|
I'd like some ideas or opinions on that. |
|
Thanks for your fast reply... You are right.. it's a little bit hacky... Why you are using angular for this frontend? Mhm. But I need this fix to get alertmanger working behind a reverser proxy. I should build my own release for now... |
|
It's hacky independent of the frontend. Look at the Prometheus code for comparison... Still would prefer a counter-proposal. |
|
FYI, @flecno @mrwacky42 there was a |
Fixes #212