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

nixos/nginx: add locations."name".uwsgiPass option and use it #346776

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

SuperSandro2000
Copy link
Member

@SuperSandro2000 SuperSandro2000 commented Oct 6, 2024

I am going to use that in another PR for searxng #346777 to reduce duplication for uwsgi.

This also allows to easily overwrite a uwsgi_pass target with an upstreams block.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Oct 6, 2024
@SuperSandro2000 SuperSandro2000 mentioned this pull request Oct 6, 2024
13 tasks
Copy link
Member

@Ma27 Ma27 left a comment

Choose a reason for hiding this comment

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

So I haven't checked, but I assume you can't have both proxyPass and uwsgiPass in the same location?

If so, we'll probably want an assertion checking for this?

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Oct 7, 2024
@SuperSandro2000
Copy link
Member Author

So I haven't checked, but I assume you can't have both proxyPass and uwsgiPass in the same location?

If so, we'll probably want an assertion checking for this?

Probably, I added one.

@SuperSandro2000 SuperSandro2000 requested a review from Ma27 October 7, 2024 20:14
@Ma27
Copy link
Member

Ma27 commented Oct 8, 2024

Probably, I added one.

I think you pushed that to the wrong PR.

@SuperSandro2000 SuperSandro2000 force-pushed the nginx-uwsgi-pass branch 2 times, most recently from 45dad5a to be26ca8 Compare October 8, 2024 11:37
@SuperSandro2000
Copy link
Member Author

Pushed the assertion to this PR and added the if blocks for proxyResolveWhileRunning.

@wegank wegank removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Oct 9, 2024
@Ma27
Copy link
Member

Ma27 commented Oct 12, 2024

@ofborg eval

@Ma27
Copy link
Member

Ma27 commented Oct 12, 2024

I'm a little surprised the check-meta check just shown an actual error. Also I don't see anything obvious in the change. cc @cole-h any idea what's up? Triggering eval didn't seem to help either here.

${command} X-Forwarded-For $proxy_add_x_forwarded_for;
${command} X-Forwarded-Proto $scheme;
${command} X-Forwarded-Host $host;
${command} X-Forwarded-Server $host;
Copy link
Member

Choose a reason for hiding this comment

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

Sorry for missing this before, but when taking a look at ${pkgs.nginx}/conf/uwsgi_params, I see:

uwsgi_param  REMOTE_ADDR        $remote_addr;
uwsgi_param  REMOTE_PORT        $remote_port;
uwsgi_param  SERVER_PORT        $server_port;
uwsgi_param  SERVER_NAME        $server_name;

Are you sure uwsgi_param is the correct thing here?
For instance, https://stackoverflow.com/questions/51207346/missing-nginx-headers-in-uwsgi-application-flask/51207405#51207405 says that you'd have to prefix the values here with HTTP_.

EDIT: according to pallets/werkzeug#1465 (comment) it also must be all uppercase.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. I must admit I didn't e2e test this with my other branch since creation. 😓

@SuperSandro2000 SuperSandro2000 added the backport release-24.11 Backport PR automatically label Nov 18, 2024
@SuperSandro2000
Copy link
Member Author

Just deployed this on top of #346777 and things are working as they should.

@Ma27
Copy link
Member

Ma27 commented Nov 22, 2024

Apologies, I have this tab open, will try to review soon.

Copy link
Member

@Ma27 Ma27 left a comment

Choose a reason for hiding this comment

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

Did you also test the combination with recommendedUwsgiSettings?
Because if I do so for mailman, I get

machine # [   53.922668] env[1344]: Traceback (most recent call last):
machine # [   53.923586] env[1344]:   File "/nix/store/p08j09lli1w0y8gk7j5mzvxd1m5v9nwj-python3-3.11.9-env/lib/python3.11/site-packages/django/core/handlers/wsgi.py", line 123, in __call__
machine # [   53.925868] env[1344]:     request = self.request_class(environ)
machine # [   53.926829] env[1344]:               ^^^^^^^^^^^^^^^^^^^^^^^^^^^
machine # [   53.927734] env[1344]:   File "/nix/store/p08j09lli1w0y8gk7j5mzvxd1m5v9nwj-python3-3.11.9-env/lib/python3.11/site-packages/django/core/handlers/wsgi.py", line 71, in __init__
machine # [   53.929852] env[1344]:     self.method = environ["REQUEST_METHOD"].upper()
machine # [   53.930968] env[1344]:                   ~~~~~~~^^^^^^^^^^^^^^^^^^
machine # curl: (22) T[   53.933110] env[1344]: KeyError: 'REQUEST_METHOD'

By adding all uwsgi params by hand with

uwsgi_param  QUERY_STRING       $query_string;
uwsgi_param  REQUEST_METHOD     $request_method;
uwsgi_param  CONTENT_TYPE       $content_type;
uwsgi_param  CONTENT_LENGTH     $content_length;

uwsgi_param  REQUEST_URI        $request_uri;
uwsgi_param  PATH_INFO          $document_uri;
uwsgi_param  DOCUMENT_ROOT      $document_root;
uwsgi_param  SERVER_PROTOCOL    $server_protocol;
uwsgi_param  REQUEST_SCHEME     $scheme;
uwsgi_param  HTTPS              $https if_not_empty;

uwsgi_param  REMOTE_ADDR        $remote_addr;
uwsgi_param  REMOTE_PORT        $remote_port;
uwsgi_param  SERVER_PORT        $server_port;
uwsgi_param  SERVER_NAME        $server_name;

I fixed the problem (REQUEST_METHOD wasn't alone, I triggered a redirect loop then on a curl against localhost).

So apparently nginx somewhere sets those defaults and our declaration discards these settings?

I think this also means that the global recommendedUwsgiSettings isn't really functional since it gets overwritten by whatever level sets the uwsgi params I shared above.

@SuperSandro2000
Copy link
Member Author

Did you also test the combination with recommendedUwsgiSettings?

I think I forgot that. 🙈 When enabling it for searxng, I get a very similar error.

So apparently nginx somewhere sets those defaults and our declaration discards these settings?

Yeah, we include include ${cfg.package}/conf/uwsgi_params; at a higher level and the uwsgi_param directives at the lower level completely overwrite that.

@SuperSandro2000
Copy link
Member Author

So, I fixed a typo in HTTP_X_REAL-IP, some missing ;, we close connections like we do for the normal proxy pass (inspired by the searx docs which pass through close for bot detection but I don't think we want to do that) and loaded conf/uwsgi_params everywhere we load the recommend values.

@gdamjan
Copy link
Contributor

gdamjan commented Dec 23, 2024

By adding all uwsgi params by hand with

yeah those are the nginx defaults:
https://github.com/nginx/nginx/blob/master/conf/uwsgi_params

at a higher level and the uwsgi_param directives at the lower level completely overwrite that.

super stupid behaviour by nginx, but, you can do the include whenever uwsgi_pass is used?

Comment on lines +106 to +111
uwsgi_param HTTP_HOST $host;
uwsgi_param HTTP_X_REAL_IP $remote_addr;
uwsgi_param HTTP_X_FORWARDED_FOR $proxy_add_x_forwarded_for;
uwsgi_param HTTP_X_FORWARDED_PROTO $scheme;
uwsgi_param HTTP_X_FORWARDED_HOST $host;
uwsgi_param HTTP_X_FORWARDED_SERVER $host;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit confused why these are needed and if anything even would use them

Copy link
Contributor

Choose a reason for hiding this comment

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

uwsgi_params are generally used by uwsgi and/or its plugins, not directly accessed by applications. Some of the very specific use-cases are here https://uwsgi-docs.readthedocs.io/en/latest/Nginx.html

I can't say the above params are recommendedUwsgiConfig

Copy link
Member Author

Choose a reason for hiding this comment

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

Those are basically a mapping of the proxy_pass variables.

@SuperSandro2000
Copy link
Member Author

you can do the include whenever uwsgi_pass is used?

Yep

@SuperSandro2000 SuperSandro2000 removed the backport release-24.11 Backport PR automatically label Jan 28, 2025
@SuperSandro2000
Copy link
Member Author

@Ma27 how do we want to proceed here?

@Ma27
Copy link
Member

Ma27 commented Jan 30, 2025

I'll try to get to another review soonish :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants