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

feat: enable proxy_buffering #2358

Merged
merged 1 commit into from Dec 25, 2023
Merged

feat: enable proxy_buffering #2358

merged 1 commit into from Dec 25, 2023

Conversation

buchdag
Copy link
Member

@buchdag buchdag commented Dec 25, 2023

nginx strongly discourage using the proxy_buffering off directive., treating it as a straight on mistake.

We’re surprised by how often we see proxy_buffering off in configurations. Perhaps it is intended to reduce the latency experienced by clients, but the effect is negligible while the side effects are numerous: with proxy buffering disabled, rate limiting and caching don’t work even if configured, performance suffers, and so on.

There are only a small number of use cases where disabling proxy buffering might make sense (such as long polling), so we strongly discourage changing the default.

It was added over 9 years ago in response to #1 (11faa5f), seemingly fixing this issue but without any real explanation as to what the root cause was and why this was the accepted fix. I highly doubt that this issue persist today and that proxy_buffering alone truncate responses over 32k.

@buchdag buchdag added the type/feat PR for a new feature label Dec 25, 2023
@buchdag buchdag merged commit 795cc13 into main Dec 25, 2023
2 checks passed
@buchdag buchdag deleted the proxy_buffering branch December 25, 2023 19:45
@StefanDanielSchwarz
Copy link

StefanDanielSchwarz commented Mar 12, 2024

@buchdag Just wanted to let you know that this was a breaking change for Nextcloud Docker when dealing with files larger than 1 GiB. Had to work around that by explicitly setting it in its proxy configs:

Fix: Disable proxy_buffering by StefanDanielSchwarz · Pull Request #2178 · nextcloud/docker

@buchdag
Copy link
Member Author

buchdag commented Mar 12, 2024

@StefanDanielSchwarz I don't really think that calling out nginx-proxy for "introducing a breaking change" is fair here, given that nextcloud/docker use the latest versions of the nginx proxy images instead of using tagged releases (the alpine tag is basically alpine's based latest)

You can't really both use the latest version of any given Docker image and expect it to never introduce a change that will disrupt your specific setup. The DB containers in nextcloud/docker examples are tagged versions, it shouldn't be different for the nginx proxy containers. What would happen if we released a new major V2 version that completely broke backward compatibility with V1.x ?

@StefanDanielSchwarz
Copy link

StefanDanielSchwarz commented Mar 12, 2024

@buchdag I hope my previous comment didn't come across as accusatory. I just wanted to bring to your attention that this change, after so many years, had an unexpected side effect in this particular case. I still fully support your decision, which is why I didn't open an issue or PR here and instead fixed it in the Nextcloud Docker repo.

I agree that best practice is to avoid using the latest tag. However, I personally prefer having an up-to-date proxy (especially if it's accessible from the Internet) over using an older version with potential security vulnerabilities. In my opinion, in cases like this, it's better to fail fast and fix issues like this one, rather than risking a working but outdated and potentially insecure system.

@buchdag
Copy link
Member Author

buchdag commented Mar 12, 2024

I hope my previous comment didn't come across as accusatory

To be honest I thought it was at first, but now understand it wasn't. Apologies for the kinda defensive reaction.

Regarding having an up-to-date proxy, did you know that you can run nginx-proxy as two separate containers (three if you use acme-companion too) ? It's not ideal yet because you still have to download the template manually and mount it into the docker-gen image so updating the template is a bit awkward, but that make it possible to run an up to date nginx version without necessarily updating the docker-gen container and/or the proxy template if you prefer not to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/feat PR for a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants