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

default route prefix to root #2813

Closed
brancz opened this Issue Jun 6, 2017 · 20 comments

Comments

Projects
None yet
6 participants
@brancz
Copy link
Member

brancz commented Jun 6, 2017

Currently the -web.route-prefix defaults to the path portion of -web.external-url. This makes it difficult for other components to communicate with the Prometheus API (for example Grafana), as it will need to know about the prefix, just for communication between the two applications. Load balancers and proxies have the ability and sometimes even default to rewriting request URLs to root anyways so it seems like a more sane choice to not prefix the API or in fact any HTTP endpoint.

My idea is that the -web.external-url is merely about user interaction as in, it is used for redirects, templating and for back-links in alerts.

While this is a small change, it is breaking so I am proposing to change this for the Prometheus 2.0 release.

@brancz brancz added the dev-2.0 label Jun 6, 2017

@brian-brazil

This comment has been minimized.

Copy link
Member

brian-brazil commented Jun 6, 2017

Load balancers and proxies have the ability and sometimes even default to rewriting request URLs to root anyways so it seems like a more sane choice to not prefix the API or in fact any HTTP endpoint.

Some load balancers have that ability, not all.

I believe the current behaviour is correct.

@fabxc

This comment has been minimized.

Copy link
Member

fabxc commented Jun 6, 2017

This is about the default for -web.route-prefix only. And the current default does not seem like sane behavior to me FWIW. What's sane about every application having to know behind which LB it sits and what path prefix that LB has configured for it – and also being able to prefix it's internal route multiplexer? On top preventing sitting behind multiple LBs with different configs.

@brian-brazil

This comment has been minimized.

Copy link
Member

brian-brazil commented Jun 6, 2017

What's sane about every application having to know behind which LB it sits and what path prefix that LB has configured for it

That's a standard thing when you're prefixing, it only works otherwise for simple cases.

On top preventing sitting behind multiple LBs with different configs.

I don't think this is a sane setup for us to try and support.

@brancz

This comment has been minimized.

Copy link
Member Author

brancz commented Jun 6, 2017

It also comes down to Prometheus to Alertmanager communication, does it really make sense for Prometheus to fire alerts against /alertmanager/proxy/some/path/api/v1/alerts? That would be the logical consequence, as I think whatever we do should be consistent in the Alertmanager.

@brian-brazil

This comment has been minimized.

Copy link
Member

brian-brazil commented Jun 6, 2017

does it really make sense for Prometheus to fire alerts against /alertmanager/proxy/some/path/api/v1/alerts?

If that's what the user has setup via their proxies, then yes.

@fabxc

This comment has been minimized.

Copy link
Member

fabxc commented Jun 6, 2017

I don't think this is a sane setup for us to try and support.

Example, I expose my Prometheus servers publicly with locked down write endpoints behind mydomain.com/<prometheus-name>. Additionally, internally, I expose my Prometheus servers with <prometheus-name>.internal.com but with write endpoints enabled. Seems perfectly reasonable.

If that's what the user has setup via their proxies, then yes.

Yes, of course. That's why this discussion is about changing the default we chose for this flag to what I believe is the sane choice.

@brian-brazil

This comment has been minimized.

Copy link
Member

brian-brazil commented Jun 6, 2017

Seems perfectly reasonable.

Not really. To do this in general the server would need to be aware of the different prefixes for the different domains so that it can use the correct prefix in responses.

@fabxc

This comment has been minimized.

Copy link
Member

fabxc commented Jun 6, 2017

That's a natural limitation, though I disagree that this means that setup won't happen frequently enough. But also it doesn't matter for the discussion.

Our thesis is: the sane behavior for an LB is to truncate the prefix when passing down a request. We would like to adjust the default to reflect that. This does not prevent anyone from doing it differently.

Taking the same problem to Alertmanager: Imagine Prometheus discovers AM pods in Kubernetes and wants to send them alerts via <pod-ip>:9093/api/v1/alerts. Why should that, by default, not work depending on whether or not those AMs are setup behind an ingress LB that provides access to them with a path prefix.
This internal alert traffic has nothing to do with how they are exposed. Consequentially, a sane setup does not leak the path prefix of the LB into the internal routing of the application.

That argument translates equally to Prometheus – also Prometheus and AM should obviously be consistent with their flag defaults.

@brian-brazil

This comment has been minimized.

Copy link
Member

brian-brazil commented Jun 6, 2017

Our thesis is: the sane behavior for an LB is to truncate the prefix when passing down a request.

I don't think that's a safe assumption. It's not the Apache default for example.

Why should that, by default, not work depending on whether or not those AMs are setup behind an ingress LB that provides access to them with a path prefix.

It may not work, because we have to choose some setup that won't work by default.

This internal alert traffic has nothing to do with how they are exposed.

There is no such thing as "internal" traffic, there is only traffic. If you make some endpoints disregard the prefix, then you break proxies that don't remove the prefix.

@fabxc

This comment has been minimized.

Copy link
Member

fabxc commented Jun 6, 2017

And the current default breaks everything because anyone communicating needs to know whatever LB might have been setup. There's absolutely such a thing as internal traffic. That's the very reason why in K8S for example, every pods gets its own IP, so we don't have to globally handle collisions on ports and paths and can decouple any ingress (i.e. external/user) traffic from how applications talk to each other.

@brian-brazil

This comment has been minimized.

Copy link
Member

brian-brazil commented Jun 6, 2017

From our perspective there's no such thing as internal traffic, there's http requests arriving at our endpoints and no way for us to tell what organisation specific semantics they have in terms of being internal/external (nor should there be).

Do you have examples of specific requests you believe it'd be sane to treat differently based on the http request?

@fabxc

This comment has been minimized.

Copy link
Member

fabxc commented Jun 6, 2017

This is not about treating anything differently. This is about changing a default flag value – nothing else. No mention of treating any traffic differently in any way at all from the Prometheus side.

@brian-brazil

This comment has been minimized.

Copy link
Member

brian-brazil commented Jun 6, 2017

I had missed that.

If we're going to be making a breaking change hear we should get an idea of how many users will break vs benefit.

@grobie

This comment has been minimized.

Copy link
Member

grobie commented Jun 6, 2017

IMHO most examples for apache and nginx don't rewrite paths, but both support it. Kubernetes proxy rewrites the path by default.

From a user standpoint, I think it's easier to understand the behavior if a flag's default value doesn't depend on the value of another flag.

I'm slightly in favor of this change, but I fear all the confused questions with this change. I guess we'll need to write a very good and detailed upgrade howto guide.

@StianOvrevage

This comment has been minimized.

Copy link

StianOvrevage commented Oct 12, 2017

I came to report this as a bug. But after reading the comments I found out it could be fixed with the route-prefix luckily :-)

Background: My setup is Kubernetes which does URL rewriting by default. Also, in my previous nginx-load-balancer deployments I use upstreams and proxy_pass which also by default will implicitly rewrite to /. Prometheus is the only application I have run that has this default behaviour. Grafana, Kibana, Elasticsearch etc all works by default from the root route, and only uses external-url for prefixing URLs in served code. In my current deployment only the first gateway (Kubernetes Ingress load balancer) has knowledge about the actual client URL being used. So the second application-nginx-server which also proxies to Prometheus has no way of knowing the actual URL served to the client at any given time, and hence which sub-route to call Prometheus on.

@grobie

This comment has been minimized.

Copy link
Member

grobie commented Nov 12, 2017

I guess we missed the chance to change the flag behavior. @brancz shall we close this one?

@brancz

This comment has been minimized.

Copy link
Member Author

brancz commented Nov 13, 2017

I've been wanting to write some user-guides on how to use the respective flags in different situations. Once we have that the situation should already be a lot better. I'm fine with closing this.

@brancz brancz closed this Nov 13, 2017

@carlpett

This comment has been minimized.

Copy link

carlpett commented Nov 14, 2017

@StianOvrevage @brancz Until those guides are in place, what is the recommended combination of flags for k8s and web.external-url/web.route-prefix?

My use case, and what I believe you were discussing as well, is having Prometheus primarily served (through an ingress) at https://some-domain/prometheus. I've set -web.external-url=https://my-domain/prometheus -web.route-prefix=/, in combination with ingress.kubernetes.io/rewrite-target: "/" on my ingress. This seems to work, and doesn't break the /metrics endpoint which it initially did...
However, if I access prometheus directly instead of through the ingress, I cannot get it to work. I'm not sure if this is a common use-case? Our developers run a lot of minikube, and typically don't use ingresses, but rather just use the NodePort. For this, neither /prometheus nor / work. Is it possible to support this in parallell with the above?

@brancz

This comment has been minimized.

Copy link
Member Author

brancz commented Nov 15, 2017

That is the currently expected behavior. What alertmanager actually does is ensure, that the loaded URL ends with a slash and redirect if it doesn’t. From there on onwards, it only performs requests relative to that path, which is why even after rewriting the URL everything works perfectly, however because it’s just a relative request, this continues to work with nodeport/port-forward. Not sure whether adopting this behavior in Prometheus would be considered breaking, but I think it would be an great UX improvement.

@lock

This comment has been minimized.

Copy link

lock bot commented Mar 23, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked and limited conversation to collaborators Mar 23, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.