fix(nginx): Handle large header responses for root#13803
fix(nginx): Handle large header responses for root#13803mschilli87 wants to merge 1 commit intonextcloud:masterfrom
Conversation
|
I will add the sign-off later today. If you auto-block PRs based on linting errors, it would be great to get notified by a comment. I only noticed the failing check because I still had the PR open in a browser tab from yesterday. Also, a PR template pointing with a short checklist of what you expect would be useful. |
See https://help.nextcloud.com/t/resolved-502-on-nextcloud-root-after-update/233954/11. Signed-off-by: Marcel Schilling <foss@mschilli.com>
e3d04cc to
568c29c
Compare
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
This comment was marked as resolved.
This comment was marked as resolved.
|
Hi, Thanks for your PR. You are absolutely right that it is not ideal that no one responded timely. That does not leave a good impression, and I understand your frustration. For now, I would prefer not to merge the change. If something in 32 caused these values to become necessary, we need to look into that. I do not think this was intentional, and increasing those limits is a big step, so it is not something we want to do without understanding the root cause. Regarding the invitation to the community survey, that is an automated process we have set up for most of our repositories. We do not expect you to participate. If you want to share your frustration about not receiving feedback within a reasonable time frame, the survey is actually a good place to do that because it goes directly to our community management. Thanks again for taking the time to contribute. |
|
@kesselb do you still have the same opinion on merging this or not? |
|
Hey @mschilli87, thanks for the contribution, it's genuinely apreciated 🙏 I get the frustration, and the survey bot timing was pretty unfortunate timing ngl. But 15 days is actually pretty normal for a busy OSS project like this one, lots of PRs in popular repos wait months before getting any response at all. It's not a reflection on your work, just the reality of a big backlog run mostly by volunteers 😅 And we are actively trying tobe better at this! We've managed to bring the open PR count here from 120+ down to around 40 in just a couple of weeks, so things are definetly moving in the right direction 💪 And to be fair, @kesselb did give you a pretty thougtful response in the end! It's not a rejection either, just a ask to understand why NC32 produces larger headers before bumping the recommended nginx config values. That's a reasonable technical concern. If you want to keep this moving, digging into the root cause on the NC32 side would be the way to go |
|
I get it and I don't mind waiting for months as long as I don't get a bot response suggesting there is no interest but apologies if I overreacted |
|
All good! Back to the topic, what do you think of Daniel's answer regarding the 32 change ? |
|
No I don't. I noticed the issue, tried to track it down, found this header response being too big and increased the limit on my setup. I am running it with the increased values ever since. I just figured it might be useful to bump the defaults as well. But if I am the only one with some weird workload or configuration that causes these big header responses, I don't mind if you close this without merging. It just took me a few hours to make it work on my system and I wanted to save others some time. That was all the motivation for this tiny PR. |
|
Follwing @kesselb's question in the forum, here the output of curl -i https://nextcloud.mydomain.tld: Note that this is with the changes proposed in this PR applied. |
How most of us ended up contributing at Nextcloud! That's a great spirit 😊 Are you sure the proxy part is needed ? If so, I would adjust the PR to add a dedicated comment for this |
Could also do 8k or 16k to have smaller default. |
☑️ Resolves
🖼️ Screenshots
I do not know how to locally build and render the documentation but I only added a few lines to the sample
nginxconfig files that fix an issue I faced after upgrading to NC32.