unit icon indicating copy to clipboard operation
unit copied to clipboard

Configuration: make large_header_buffer_size a valid setting.

Open ac000 opened this issue 2 years ago • 4 comments

This pull request has been updated and expanded on. It now consists of five commits

  • Docs: update changelog for some new settings.
  • Router: remove unused proxy_buffers structure member.
  • Configuration: add some missing proxy settings options.
  • Configuration: enable the setting of some more http options.
  • Configuration: make large_header_buffer_size a valid setting.

In reverse order

Configuration: make large_header_buffer_size a valid setting.

This commit allows to fix the issue reported here by allowing the settings.http.large_header_buffer_size option to be set in the config.

This is done as a separate commit as this is directly dealing with a user reported issue.

Configuration: enable the setting of some more http options.

This enables the setting of header_buffer_size & large_header_buffers in the settings.http options. These were simply missing from the list of valid config options.

Configuration: add some missing proxy settings options.

This adds a bunch of proxy options to be processed and as valid settings.http config options.

I've done this as a separate commit from the previous one as this was a slightly different situation in that these proxy options weren't listed in the nxt_router_http_conf array (nor as valid config options) and so weren't even looked at for updating.

It's also easier to squash this with the previous commit (if so desired) than split it up after the fact.

Router: remove unused proxy_buffers structure member.

This removes an unused structure member from nxt_socket_conf_t and while it did get set, it was never actually used anywhere. Removing it shrinks the nxt_socket_conf_t structure to fit in 3 cachelines (on x86-64 at least).

Docs: update changelog for some new settings.

This updates the changelog to mention all the new config options.

ac000 avatar May 04 '22 03:05 ac000

Nice find! Thanks for the PR @andrey-zelenkov. I was looking into the code and noticed that large_header_buffers is also a configuration option in nxt_router.c

./nxt_router.c:1880:            skcf->large_header_buffers = 4;

But not a valid configuration option in nxt_conf_validation.c. I will look closer to this gap in the code but wouldn't it be good to add the buffers to the PR as well?

tippexs avatar May 04 '22 05:05 tippexs

Sure, can do. There is also header_buffer_size that is not currently settable by the user, should it be?

ac000 avatar May 04 '22 14:05 ac000

Hmm, there is also

proxy_header_buffer_size
proxy_buffer_size
proxy_buffers
proxy_timeout
proxy_send_timeout
proxy_read_timeout

proxy_buffers seems to be unused.

These are all set in nxt_router.c::nxt_router_conf_create() along with the other http values. However they are not set in the nxt_router_http_conf array (or any other nxt_conf_map_t array).

Should they be added to the nxt_router_http_conf array and set as valid config options?

ac000 avatar May 04 '22 18:05 ac000

Dropped the 'Router: remove unused proxy_buffers structure member.' commit as that is perhaps better done as a separate pull-request.

ac000 avatar May 22 '22 20:05 ac000

Any movement on this PR? We are facing 431 due to large header size, which seems to be a similar issue as this. Any updates?

tagur87 avatar Oct 09 '22 14:10 tagur87

@tagur87 Can you test the below patch?

diff --git a/src/nxt_conf_validation.c b/src/nxt_conf_validation.c
index fe6c22e5..fde0644f 100644
--- a/src/nxt_conf_validation.c
+++ b/src/nxt_conf_validation.c
@@ -312,6 +312,9 @@ static nxt_conf_vldt_object_t  nxt_conf_vldt_http_members[] = {
     }, {
         .name       = nxt_string("max_body_size"),
         .type       = NXT_CONF_VLDT_INTEGER,
+    }, {
+        .name       = nxt_string("large_header_buffer_size"),
+        .type       = NXT_CONF_VLDT_INTEGER,
     }, {
         .name       = nxt_string("body_temp_path"),
         .type       = NXT_CONF_VLDT_STRING,

You can then set the config like

"settings": {
    "http": {
        "large_header_buffer_size": 16384
    }
},

ac000 avatar Oct 09 '22 16:10 ac000

@ac000 i can try, will take some time tho... where we are having issues is in an upstream container using unit. So will have to rebuild the container and integrate properly.

tagur87 avatar Oct 09 '22 17:10 tagur87

@ac000 - I was finally able to build unit with this patch and put it into our application, and it seems to have resolved the issues we are having with large header sizes.

What will be involved in getting this code merged and released for usage?

Thanks so much for the help!

tagur87 avatar Oct 12 '22 20:10 tagur87

@tagur87 Thanks for testing!

I will see about getting this patch through the review process and I will put this patch in a new pull request where it can be tracked separately.

ac000 avatar Oct 12 '22 21:10 ac000

Closing this as the source branch (again) only contains the single 'large_header_buffer_size' patch...

ac000 avatar Oct 12 '22 23:10 ac000

PR for the afore mentioed patch.

ac000 avatar Oct 12 '22 23:10 ac000