bluerobotics / BlueOS

The open source platform for ROV, USV, robotic system operation, development, and expansion.
https://blueos.cloud/docs/
Other
168 stars 80 forks source link

Nginx: use prefixes instead of regexes #2913

Closed rotu closed 1 month ago

rotu commented 1 month ago

Nginx supports full regex-based rules, but we don't actually need them

  1. Switch to prefix-based locations, which are easier to understand and gets rid of unnecessary rewrite rules.
  2. This has the effect of also allowing the server to rewrite "Location" and "Refresh" headers, which would otherwise require manually using proxy_redirect.
  3. This avoids hardcoding the protocol and IP address (which make troubleshooting network issues very confusing!)
rotu commented 1 month ago

Note this also causes paths to the root to get normalized to / by a 301 redirect as described in the nginx docs, effectively reversing 3b6cd4c.

I think this is a good thing because it prevents accidentally changed behavior between using an API with or without the proxy.

In http://host:777, for instance, the relative URL bar refers to http://host:777/bar. If we remap that to a URL without a trailing slash http://host/p777, then the relative URL bar will refer to http://host/bar. Adding the trailing slash (http://host/p777/) makes the relative URL bar refer to http://host/p777/bar.

Root-relative URLs (e.g. /bar) are still a problem, however.

Williangalvani commented 1 month ago

The reasoning for this being done in #2815 is that nginx's redirects, when used behind another server (apache on a web server) was redirecting the browser to localhost. perhaps this can be fixed by setting the server with a variable passed from the proxying server...

rotu commented 1 month ago

The reasoning for this being done in #2815 is that nginx's redirects, when used behind another server (apache on a web server) was redirecting the browser to localhost. perhaps this can be fixed by setting the server with a variable passed from the proxying server...

I think the actual issue was not the presence of the redirect but the fact that the Location header is not being rewritten, e.g. with the proxy_redirect directive. I think that's done for you automagically when using proxy_pass with a base path.

Per the docs (Confusingly (to me, at least), URI here means the path name):

In some cases, the part of a request URI to be replaced cannot be determined: ...

  • When the URI is changed inside a proxied location using the rewrite directive, and this same configuration will be used to process a request (break):
    location /name/ {
    rewrite    /name/([^/]+) /users?name=$1 break;
    proxy_pass http://127.0.0.1;
    }

    In this case, the URI specified in the directive is ignored and the full changed request URI is passed to the server.

If you disagree with my assessment, what are the URLs and response headers you see in the problematic case (especially between Apache and nginx?)

Williangalvani commented 1 month ago

If you disagree with my assessment, what are the URLs and response headers you see in the problematic case (especially between Apache and nginx?)

I do not disagree, but I need input from @voorloopnul who's on vacation right now. Is the current situation a blocker for you?

If so, we could revert for a while (at least) until he's available again.

patrickelectric commented 1 month ago

@rotu it appears that the PR broke master, can you take a look ?

rotu commented 1 month ago

@patrickelectric Taking a look now. What are you looking at in particular?

patrickelectric commented 1 month ago

@patrickelectric Taking a look now. What are you looking at in particular?

Frontend appears to be broken, api is not accessible and bootstrap is putting in factory mode.

rotu commented 1 month ago

Turns out it did not like the leading colon in URLs.

2024/09/24 23:19:17 [emerg] 423#423: invalid URL prefix in /home/pi/tools/nginx/nginx.conf:58

I was confused because service nginx reload didn't cause the server to fall over in the same way that an invalid config would with service nginx restart.

patrickelectric commented 1 month ago

Thanks for fixing it!