Bug 255850 - [patch] www/nginx: restore dynamic tls patch
Summary: [patch] www/nginx: restore dynamic tls patch
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: Jochen Neumeister
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-05-13 19:26 UTC by Ryan Steinmetz
Modified: 2021-06-02 18:01 UTC (History)
3 users (show)

See Also:
bugzilla: maintainer-feedback? (joneum)


Attachments
dynamic tls patch (8.88 KB, text/plain)
2021-05-13 19:26 UTC, Ryan Steinmetz
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Ryan Steinmetz freebsd_committer freebsd_triage 2021-05-13 19:26:21 UTC
Created attachment 224911 [details]
dynamic tls patch

- Re-add dynamic tls patch from cloudflare after 1.20.0 update
- Bump PORTREVISION
Comment 1 Sergey A. Osokin freebsd_committer 2021-05-20 21:41:18 UTC
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
Comment 2 Ryan Steinmetz freebsd_committer freebsd_triage 2021-05-20 21:44:45 UTC
It's off by default.  You are not required to use it if you do not wish to.
Comment 3 commit-hook freebsd_committer 2021-06-02 13:38:50 UTC
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(+)
Comment 4 Jochen Neumeister freebsd_committer 2021-06-02 14:21:40 UTC
(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.
Comment 5 Christos Chatzaras 2021-06-02 14:25:28 UTC
(In reply to Sergey A. Osokin from comment #1)

Why Nginx doesn't approve this patch?
Comment 6 Ryan Steinmetz freebsd_committer freebsd_triage 2021-06-02 15:00:17 UTC
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.
Comment 7 Sergey A. Osokin freebsd_committer 2021-06-02 15:07:08 UTC
(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.
Comment 8 Christos Chatzaras 2021-06-02 15:26:38 UTC
(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.
Comment 9 Sergey A. Osokin freebsd_committer 2021-06-02 16:51:52 UTC
(In reply to Christos Chatzaras from comment #8)

Thanks for the update, Christos.

Could you provide test results that show/measure such improvements.
Comment 10 Jochen Neumeister freebsd_committer 2021-06-02 17:19:35 UTC
(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
Comment 11 Ryan Steinmetz freebsd_committer freebsd_triage 2021-06-02 18:01:51 UTC
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