Skip to content

Nginx: use prefixes instead of regexes#2913

Merged
patrickelectric merged 1 commit intobluerobotics:masterfrom
rotu:internal-meerkat
Sep 24, 2024
Merged

Nginx: use prefixes instead of regexes#2913
patrickelectric merged 1 commit intobluerobotics:masterfrom
rotu:internal-meerkat

Conversation

@rotu
Copy link
Contributor

@rotu rotu commented Sep 22, 2024

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
Copy link
Contributor Author

rotu commented Sep 22, 2024

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
Copy link
Member

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
Copy link
Contributor Author

rotu commented Sep 23, 2024

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
Copy link
Member

Williangalvani commented Sep 24, 2024

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 patrickelectric self-assigned this Sep 24, 2024
Copy link
Member

@patrickelectric patrickelectric left a comment

Choose a reason for hiding this comment

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

I think that is safe for us to merge and see how it goes.
We can revert before stable 1.4 if necessary.

@patrickelectric patrickelectric merged commit de7be54 into bluerobotics:master Sep 24, 2024
@patrickelectric
Copy link
Member

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

@rotu
Copy link
Contributor Author

rotu commented Sep 24, 2024

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

@patrickelectric
Copy link
Member

@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
Copy link
Contributor Author

rotu commented Sep 24, 2024

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.

rotu added a commit to rotu/BlueOS that referenced this pull request Sep 24, 2024
Undo my introduction of invalid syntax in bluerobotics#2913, which breaks blueos.

It turns out that you can't start a URL with a leading colon. The only reason I thought it gave the same results is because `service nginx reload` bailed when loading the new config, but left the server running with the same state.
patrickelectric pushed a commit that referenced this pull request Sep 24, 2024
Undo my introduction of invalid syntax in #2913, which breaks blueos.

It turns out that you can't start a URL with a leading colon. The only reason I thought it gave the same results is because `service nginx reload` bailed when loading the new config, but left the server running with the same state.
@patrickelectric
Copy link
Member

Thanks for fixing it!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Comments