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

improve some general documentation #613

Merged
merged 1 commit into from
Nov 20, 2024

Conversation

nisbet-hubbard
Copy link
Contributor

I assume the suggestion of proxy_buffering off; was prompted by a warning such as an upstream response is buffered to a temporary file because the image file exceeds the (8 4k|8k) in-memory buffer size. In which case the user should probably increase the default buffer size in the first instance (if there’s enough RAM) rather than turning off buffering to suppress the warning. I’m linking to Vershinin‘s article because it seems to be the most thorough one on this subject.

adduser is a symlink to useradd on recent Fedora/RHEL and doesn’t accept the same flags as Debian so I propose removing them from that paragraph.

@tamara-schmitz
Copy link
Contributor

How did you determine your proxy_buffering_size settings if you changed it at all?

@tamara-schmitz
Copy link
Contributor

I read the article that you added and it encourages adjusting the buffer size to the likely maximum HTTP header size a request can reach to prevent Nginx from temporarily writing a request to disk. It also recommends disabling proxy_buffering by default if Nginx is not used for caching and just as a TLS termination point.

It would nice to determine a typical HTTP header size and make recommendations for good defaults based on those.

@nisbet-hubbard
Copy link
Contributor Author

nisbet-hubbard commented Nov 16, 2024

//> How did you determine your proxy_buffering_size settings if you changed it at all?

Good question. Since we’re talking about uploading large files, however, the relevant directive is proxy_buffers. After deciding the required buffer size, it’s a question of ‘several big vs. many small buffers’. See advice from nginx’s Maxim Dounin: https://forum.nginx.org/read.php?2,2746,2842

But he also said

As long as files are big – it probably doesn't make sense to even
try to eliminate warnings by increasing buffers. Instead, you
have to derive buffer sizes from amount of memory you want to use
for buffering

One can also turn buffering off if one doesn’t need to deal with slow clients.

@nisbet-hubbard
Copy link
Contributor Author

nisbet-hubbard commented Nov 16, 2024

//I’ve removed reference to Vershinin’s article because it’s focussed on proxy_buffer_size, which isn’t relevant to uploading large media files to conduwuit and can only confuse users.

I’ve found time to do extensive testing for uploading to conduwuit under different scenarios. My conclusions are as follows:

The only directive that’s absolutely required is client_max_body_size. Synapse recommends 50M, but the appropriate number of course depends on what’s configured for max_request_size in conduwuit.toml.

proxy_request_buffering off; and proxy_buffering off; are optimisations that may not suit every use case (useful for limited resources + small number of clients). Note that proxy_request_buffering affects users uploading files to conduwuit, whereas proxy_buffering controls users or bridges downloading files from conduwuit.

I’ve updated my patch accordingly.

@nisbet-hubbard
Copy link
Contributor Author

Just saw the new version

This is due to Nginx storing the entire POST content in-memory (/tmp) and running out of memory if on low memory hardware.

What you’re seeing here is disk buffering. It’s caused by the typical upload exceeding the default 8x4k in-memory proxy_buffers.

nginx throws two kinds of warnings when this happens

a client request body is buffered to a temporary file

and when downloaded from conduwuit

an upstream response is buffered to a temporary file

@girlbossceo
Copy link
Owner

Corrections/changes/clarifications are welcome

@nisbet-hubbard
Copy link
Contributor Author

nisbet-hubbard commented Nov 16, 2024

Basically I guess the one thing we really need to put in there is client_max_body_size.

After that, the user has three choices:

  1. Ignore the warnings and just let the upload be buffered to a temporary file.
  2. Increase proxy_buffers and client_body_buffer_size so uploads get buffered in memory. (This effectively means having to match both to client_max_body_size.)
  3. Set proxy_request_buffering off; and proxy_buffering off; and let nginx send files synchronously. (This is discouraged by the nginx team because it’s blocking.)

3 may suit conduwuit instances with a couple of users, whilst high concurrency servers should probably go for a compromise between 1 and 2. I gather we can just spell out the trade-offs and let admins decide for themselves.

Could you provide some feedback as to your preference so I know what to do with this PR?

@tamara-schmitz
Copy link
Contributor

tamara-schmitz commented Nov 16, 2024

One can also turn buffering off if one doesn’t need to deal with slow clients.

I missed that part. Since the Nginx is public facing to the Internet, it is possible to encounter slow clients. Hmm yeah keeping buffering on and increasing buffer size seems like a better idea.

@tamara-schmitz
Copy link
Contributor

tamara-schmitz commented Nov 16, 2024

I think Nginx could use its own section just like Caddy.

I also think it would be cool if we could recommend defaults.
Mostly let's set a generous value for the amount of proxy_buffers. Each buffer is allocated on demand.

I can post the settings that I use but they are educated but also arbitrary:

        proxy_buffers 128 64k; # 8M
        proxy_read_timeout 10m; # allow for long idle connections and websockets
        proxy_write_timeout 10m;
        keepalive_timeout 10m;

I keep proxy_buffer_size at its default of the arch page_size. Same with proxy_busy_buffer size which on x86_64 would imply 4k and 68k for each.

My values assume that one may upload files to the Conduwuit server, hence the bigger buffer size.

Also very much a nitpick (and i am not a maintainer here): it would be nicer to split up the useradd correction into a second commit.

@girlbossceo
Copy link
Owner

I think Nginx could use its own section just like Caddy.

I would prefer not. Having a bunch of sections on reverse proxies was intentionally removed and leaving only a Caddy section was added to steer users towards using a simple, fast, no-headache, and effective reverse proxy unlike Nginx/Apache. The most I am willing to do, and have been doing, is having "reverse proxy caveats" like:

Lighttpd is not supported as it seems to mess with the X-Matrix Authorization header, making federation non-functional. If a workaround is found, feel free to share to get it added to the documentation here.

If using Apache, you need to use nocanon in your ProxyPass directive to prevent this (note that Apache isn't very good as a general reverse proxy and we discourage the usage of it if you can).

If using Nginx, you need to give conduwuit the request URI using $request_uri, or like so:

If you feel like "reverse proxy caveats" should be in its own section, I'm up for doing that. But I am not going to steer users towards Nginx and they should be reviewing the 100000+ "nginx reverse proxy" examples out there if they want to use Nginx instead of Caddy.

@tamara-schmitz
Copy link
Contributor

If you feel like "reverse proxy caveats" should be in its own section, I'm up for doing that. But I am not going to steer users towards Nginx and they should be reviewing the 100000+ "nginx reverse proxy" examples out there if they want to use Nginx instead of Caddy.

That feels appropriate. Or something like

"Caddy"
"Other reverse proxies"

@tamara-schmitz
Copy link
Contributor

A common pattern you want to see in docs is this. It repeats over each section or chapter.

  • Brief introduction as to what we are doing
  • Give instructions on how to get basic thing done
  • Give further instructions for customizations, fancy things, btws etc.

For the reverse proxy section that would mean:

  • Let's set up a reverse proxy
  • Here is how to setup caddy using the example config
  • If you wanna use something else, here are the requirements, details, caveats

@girlbossceo
Copy link
Owner

"Caddy"
"Other reverse proxy caveats"

This is what I'm fine with doing.

@nisbet-hubbard
Copy link
Contributor Author

nisbet-hubbard commented Nov 17, 2024

Since what June wants is absolutely minimal caveat about non-Caddy proxies, I’ve left only the part about matching client_max_body_size to max_request_size as defined in conduwuit.toml. Because that’s the only thing where nginx defaults really prevent a user from uploading >1M to conduwuit.

The whole buffering thing really depends on the user’s environment. The default 8 4k buffers are good for most requests/responses for conduwuit. Buffering anything bigger than 32k (ie big uploads) to disk is a sensible way to make the transfer non-blocking. The user will see warnings about this, but they should usually just ignore them. proxy_buffering off is only appropriate in specific situations like long-running connections where you send small chunks of data periodically and need them to be immediately delivered to client(s).

@girlbossceo, does this look good to you?

@gstrauss
Copy link

Lighttpd is not supported as it seems to mess with the X-Matrix Authorization header, making federation non-functional. If a workaround is found, feel free to share to get it added to the documentation here.

Where might I find more details about that? lighttpd does not modify the Authorization header. lighttpd mod_auth supports HTTP Basic and HTTP Digest authentication. While X-Matrix is not recognized by lighttpd mod_auth, the Authorization header will be passed to backends, e.g. FastCGI or CGI, as HTTP_AUTHORIZATION.

@girlbossceo
Copy link
Owner

@gstrauss #388

@nisbet-hubbard I'll review it later, but please rebase this PR onto main when you can.

@nisbet-hubbard
Copy link
Contributor Author

Done.

@girlbossceo
Copy link
Owner

I'm kinda confused as to why the suggestion of disabling proxy_buffering was removed now. I thought this was what the PR was trying to achieve; by providing an alternative to just disabling it all together with a bit of guidance?

The advice was added because multiple Nginx users have reported sporadic media failures, both downloading remote media and uploading media, which were all solved by disabling proxy_buffering. None of them were aware of any tuning to solve the issue, but it was all consistent that disabling that solved the issue and there was nothing for conduwuit to do.

I'm not sure if my comment about going against creating a dedicated Nginx section was misinterpreted, but it wasn't saying "I don't want this tiny piece of advice to tune buffers". It was to avoid creating extensive amounts of documentation on one single reverse proxy software that's needlessly complex for the majority of users, while also duplicating the 1000+ "nginx reverse proxy example" guides out there, so much that it needs its own page/section.

So, either this PR needs to put back the advice of disabling proxy_buffering if media download and upload failures occur when using Nginx like it was befoer, or identify what parameters could/can be changed/tuned by the user to remediate such issues.

@nisbet-hubbard
Copy link
Contributor Author

Thanks for filling us in on the background! I agree with your conclusion about the two paths forward.

From what you’re saying, proxy_buffering off really looks like a band-aid solution in a situation where the users reporting the bug didn’t know how to go about debugging it. #505 (comment), for instance, is a permissions issue but the user ended up going proxy_buffering off without knowing how that fixed it. The downside is of course that this makes all connections blocking: https://forum.nginx.org/read.php?2,223795,223831#msg-223831.

I suppose what we want to avoid is adding these band-aid solutions to the docs whenever users discover a workaround to their local nginx configuration problems, usually without even identifying the real problem. The advice about $request_uri seems to be one such fix, which I’ve confirmed is not necessary in a well-configured nginx instance. Instead, we should probably ask readers to post their debug logs and have more experienced nginx users look at them or perhaps even redirect their questions to the nginx forum in some cases.

What’s your thoughts on this?

@girlbossceo
Copy link
Owner

girlbossceo commented Nov 17, 2024

The way I decide for me to add those tiny bits of ad-hoc reverse proxy advice like the Apache, Lighttpd, and Nginx ones are if they're surprisingly a common issue.

Like the proxy_buffering one, there were a good portion of unique users joining the Matrix room and complaining about media download issues that conduwuit logs couldn't do anything but return a 5xx error code (from the reverse proxy itself). It was only until after these users dug into their Nginx logs and found that disabling proxy_buffering solved the issue. Since it seemed to be a pretty frequent issue, that was why I added it. I insisted these users just use Caddy, but they were set on using Nginx.

Same with the $request_uri one. Multiple users have reported X-Matrix signature verification issues over a period of time, and they found this issue to be the reason.

Or if they're critical configuration changes that need to be made to function properly, like Apache having to use nocanon otherwise you won't get any workable federation period.

So, I do agree that these users should just be using Caddy, or spend some time trying to figure out the issue themselves through Nginx logs, and asking for Nginx support elsewhere, but if it's a really common issue then that would be where I'd like that advice to be under the "reverse proxy caveats" section if there was one that would be made.

I guess I'm fine with removing the disable proxy_buffering advice and just merging this PR as is, but if it comes up again either in my Matrix rooms or in here, then I'm going to add it back because it's clearly a caveat that needs to be mentioned.

@nisbet-hubbard
Copy link
Contributor Author

Yeah, makes sense to me.

Can definitely sympathise with your having to accommodate users who neither wish to spend time debugging their nginx nor switch to Caddy. Also read about bikeshedding re broken X-Matrix signatures over at Conduit.

Multiple users have reported X-Matrix signature verification issues over a period of time

Maybe you can mention this in the docs so first-time users know they don’t need the $request_uri unless there’s this specific issue?

but if it comes up again either in my Matrix rooms or in here, then I'm going to add it back

It may help if the next time this comes up you ask the reporter to do error_log /var/log/nginx/error.log debug; and paste the debug-level log in the issue and ping me or @tamara-schmitz so at least we have the record for what’s going on even if it can’t get fixed immediately.

@girlbossceo girlbossceo merged commit 876c6e9 into girlbossceo:main Nov 20, 2024
8 checks passed
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.

4 participants