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

ServeFromSubPath: Redirect to URL with subpath when subpath missing #66724

Merged
merged 8 commits into from
Apr 24, 2023

Conversation

torkelo
Copy link
Member

@torkelo torkelo commented Apr 18, 2023

I almost always run Grafana locally with a subpath and serve_from_subpath set to true (to catch bugs related to subpath). However I have for years been frustrated by the fact that I am always logged out when I access Grafana without the subpath. This is because the session token is set with the subpath and we have no redirect in Grafana that corrects the URL so Grafana serves the index page even when the subpath is missing and the redirect to Grafana with subpath happens in the frontend.

The frontend thinks the user is not signed in as does the backend as it did not see any session token because the URL was outside the path for the session token.

This adds a redirect middleware that redirects all urls that are missing the subpath to a URL that contains it so

The tricky question is how much of a breaking change this is. As before the "serve_with_subpath" actually only strips the subpath from all requests urls so that they match url routes defined without them. So basically both work. So when you have "serve_with_sub_path" set to true and subpath is say "grafana" then all these requests work equally well

http://localhost:3000/metrics (prometheus metrics)
http://localhost:3000/grafana/metrics

etc, for all routes. They are handled identically. With this change, there will be a 301 redirect. So for instances that have serve_with_subpath and access without subpath with clients that don't follow redirect (not sure if there are such clients) then this could be a small breaking change. But would be nice to get help to evaluate that. If that is the case we would need to add a new option to revert to the old behavior of not redirecting.

@torkelo torkelo requested a review from a team as a code owner April 18, 2023 07:48
@torkelo torkelo requested review from papagian, suntala, yangkb09, sakjur and bergquist and removed request for a team April 18, 2023 07:48
@torkelo torkelo added this to the 10.0.0 milestone Apr 18, 2023
@torkelo torkelo added add to changelog no-backport Skip backport of PR labels Apr 18, 2023
@torkelo torkelo requested a review from marefr April 18, 2023 15:10
@bergquist
Copy link
Contributor

Prometheus follows 301 redirects when scraping metrics.

Kubernetes says Any code greater than or equal to 200 and less than 400 indicates success. Any other code indicates failure. but the golang http client follows redirects by default so it should work for k8s healthchecks as well :)

pkg/api/http_server.go Outdated Show resolved Hide resolved
pkg/middleware/subpath_redirect.go Outdated Show resolved Hide resolved
@torkelo
Copy link
Member Author

torkelo commented Apr 19, 2023

@bergquist pushed an update

  • rewrote it as a web.Middlware so I can use useMiddleware
  • And added some tests
  • Ignore /api/* requests

@torkelo torkelo requested a review from bergquist April 21, 2023 06:27
"github.com/stretchr/testify/require"
)

func TestSubPathRedirect(t *testing.T) {
Copy link
Contributor

@bergquist bergquist Apr 21, 2023

Choose a reason for hiding this comment

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

When testing multiple cases like this it's preffered to use table testing like this https://github.com/grafana/grafana/blob/serve-from-sub-path-redirect/pkg/api/plugin_proxy_test.go#L10

Not required thou!

@torkelo torkelo requested a review from bergquist April 22, 2023 05:28
Copy link
Contributor

@bergquist bergquist left a comment

Choose a reason for hiding this comment

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

LGTM!

@torkelo torkelo merged commit 57701fd into main Apr 24, 2023
@torkelo torkelo deleted the serve-from-sub-path-redirect branch April 24, 2023 07:55
@JohnnyQQQQ
Copy link
Member

While investigating why some of our test instances in the Alerting team are no longer accessible, I came across this particular PR.

Our setup involves two layers of reverse proxies, which seem to be incompatible with the new redirect introduced in this PR. Additionally, I conducted a test using the officially documented configuration and found that it appears to be broken.

Given that we have potentially numerous users and customers who are operating with the configuration we documented here, we might need to reconsider this PR. We could possibly revert it, or alternatively, we could place it behind a feature flag—perhaps something akin to a strict-mode.

To illustrate my point, I've created a gist with the documented configuration. Notably, switching the Grafana image to latest seems to resolve the issues.

@marefr
Copy link
Contributor

marefr commented May 16, 2023

@torkelo ☝️

@torkelo
Copy link
Member Author

torkelo commented May 17, 2023

@JohnnyQQQQ can you describe how?? Without this fix Grafana does not work properly with serve_from_sub_path set to true. If you have serve_from_sub_path true, then Grafana serves from that subpath, so a redirect has to happen for the session cookie to work as the session cookie will include the subpath.

If you do url rewrites in the nginx proxy then you should not set serve_from_sub_path to true.

While investigating why some of our test instances in the Alerting team are no longer accessible,

Does your nginx proxy has url rewrites? then you should not set serve_from_sub_path to true

The whole point of serve_from_sub_path was so that proxies did not have to do URL rewriting

@JohnnyQQQQ
Copy link
Member

@torkelo The main point I am bringing up is that our documentation(https://grafana.com/tutorials/run-grafana-behind-a-proxy/) uses a rewrite for serving with a subpath. We used it internally to set up a few test instances.

[server]
domain = example.com
root_url = %(protocol)s://%(domain)s:%(http_port)s/grafana/
serve_from_sub_path = true
# this is required to proxy Grafana Live WebSocket connections.
map $http_upgrade $connection_upgrade {
  default upgrade;
  '' close;
}

upstream grafana {
  server localhost:3000;
}

server {
  listen 80;
  root /usr/share/nginx/www;
  index index.html index.htm;

  location /grafana/ {
    rewrite  ^/grafana/(.*)  /$1 break;
    proxy_set_header Host $http_host;
    proxy_pass http://grafana;
  }

  # Proxy Grafana Live WebSocket connections.
  location /grafana/api/live/ {
    rewrite  ^/grafana/(.*)  /$1 break;
    proxy_http_version 1.1;
    proxy_set_header Upgrade $http_upgrade;
    proxy_set_header Connection $connection_upgrade;
    proxy_set_header Host $http_host;
    proxy_pass http://grafana;
  }
}

So everyone who used this documentation as a starting point will have a bad experience updating to Grafana 10. This configuration "worked" for our use case before merging this PR (see my gist linked in the comment above).

I also understand that the fix might be trivial in some cases (i.e., removing the rewrite). Still, many users might not operate Grafana on a day-to-day basis, so this change might be more intrusive than we think, and updating to Grafana 10 might result in a bad experience overall.

@torkelo
Copy link
Member Author

torkelo commented May 17, 2023

@JohnnyQQQQ yea, the tutorial was updated a year ago, with incorrect instructions grafana/tutorials@3b7e7b4

@torkelo
Copy link
Member Author

torkelo commented May 17, 2023

@JohnnyQQQQ yea, I am also worried this could affect others.

I don't have any other ideas for how to fix this bug which makes it so that Grafana shows you as signed out even though you are logged in, which is a very annoying bug.

@scottmuc
Copy link

scottmuc commented Jul 23, 2023

Thanks @JohnnyQQQQ for your writeup! I believe I've run into the same issue when upgrading to version 10. I've also used the documented configuration from the website. When I reverted back to version 9.x my reverse proxy configuration worked.

@torkelo, I'm free to try stuff out if you need a tester. All details are in the referenced GitHub issue above (Rebuild Raspberry PI).

edit I see that the tutorial documentation no longer contains the Rewrite rules. The page did have that when I implemented my installation of Grafana. Will try the upgrade again and see how it goes.

scottmuc added a commit to scottmuc/infrastructure that referenced this pull request Jul 23, 2023
The linked to reverse proxy documentation changed. The configuration I
orginally applied was incorrect.

See: grafana/grafana#66724
Issue: #61
@scottmuc
Copy link

Following up to my previous comment. I removed the Rewrite blocks and my upgrade to 10.0.2 is working fine. Not sure how to best communicate this. I followed the documentation and things worked. Then I upgrade, then have to notice the documentation changed. Thanks again for noticing this @JohnnyQQQQ and @torkelo for the updates.

bb-vishalsingh

This comment was marked as outdated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants