Bug 255850
| Summary: | [patch] www/nginx: restore dynamic tls patch | ||||||
|---|---|---|---|---|---|---|---|
| Product: | Ports & Packages | Reporter: | Ryan Steinmetz <zi> | ||||
| Component: | Individual Port(s) | Assignee: | Jochen Neumeister <joneum> | ||||
| Status: | Closed FIXED | ||||||
| Severity: | Affects Only Me | CC: | chris, joneum, osa | ||||
| Priority: | --- | Flags: | bugzilla:
maintainer-feedback?
(joneum) |
||||
| Version: | Latest | ||||||
| Hardware: | Any | ||||||
| OS: | Any | ||||||
| Attachments: |
|
||||||
Hi Ryan, this patch has been reviewed by NGINX Development team and rejected. I'd not recommend to add it back to the www/nginx port. -- Sergey Osokin It's off by default. You are not required to use it if you do not wish to. A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/ports/commit/?id=9ded70b005791a031dd561121de426a48d0345ac commit 9ded70b005791a031dd561121de426a48d0345ac Author: Ryan Steinmetz <zi@FreeBSD.org> AuthorDate: 2021-06-02 13:37:21 +0000 Commit: Ryan Steinmetz <zi@FreeBSD.org> CommitDate: 2021-06-02 13:37:21 +0000 www/nginx: Restore lost dynamic tls patch PR: 255850 Approved by: maintainer timeout (2+ weeks) www/nginx/Makefile.extmod | 2 + www/nginx/files/extra-patch-dynamic-tls (new) | 225 ++++++++++++++++++++++++++ 2 files changed, 227 insertions(+) (In reply to commit-hook from comment #3) Hi, pls revert. osa has clearly said here that the patch was rejected by the NGINX team. thank you. This commit does not have my approval. (In reply to Sergey A. Osokin from comment #1) Why Nginx doesn't approve this patch? This PR was to restore a patch that was lost during the upgrade to 1.20.0. No maintainer activity happened for almost 3 weeks, it was automatically approved via the maintainer timeout: https://www.freebsd.org/portmgr/policies_contributors/#pr_timeout There is no harm by having an additional patch present. (In reply to Christos Chatzaras from comment #5) Short answer is it's possible to reach the same goal with ssl_buffer_size 4k; directive. (In reply to Sergey A. Osokin from comment #7) If I remember correctly this patch improves TTFB. Don't know if it has any disadvantages. (In reply to Christos Chatzaras from comment #8) Thanks for the update, Christos. Could you provide test results that show/measure such improvements. (In reply to Ryan Steinmetz from comment #6) There was a reason why the patch was removed. But okay, I will remove it again in the next update Perhaps there would be value in you providing a reason to for it to no longer be an OPTION? So far, "nginx rejected it" and "there is a reason it was removed" is all that I've seen--no actual rationale for preventing users from having it as an option was provided. The only (transient) reason that I've been able to come up with is that the patch no longer cleanly applied to 1.20.0 because two structs were renamed in the newer release. However, that was a trivial fix and I opened a fresh PR with updated patch to address the issue. I've never seen such a passionate resistance to an optional nginx addon/patch, so this all strikes me as very weird. To me, it seems like this particular patch was singled out for some ulterior motive. Cloudflare's writeup provides more details: https://blog.cloudflare.com/optimizing-tls-over-tcp-to-reduce-latency/ As well as a rationale why statically configuring ssl_buffer_size to 4k can be improved upon: https://raw.githubusercontent.com/cloudflare/sslconfig/master/patches/nginx__1.11.5_dynamic_tls_records.patch And lastly, this was approved/added by the maintainer in 1.18.x: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=242668 |
Created attachment 224911 [details] dynamic tls patch - Re-add dynamic tls patch from cloudflare after 1.20.0 update - Bump PORTREVISION