Configuration: make large_header_buffer_size a valid setting. #689
Conversation
|
Nice find! Thanks for the PR @andrey-zelenkov. I was looking into the code and noticed that ./nxt_router.c:1880: skcf->large_header_buffers = 4;But not a valid configuration option in |
tippexs
added
z-enhancement ⬆️
|
Sure, can do. There is also header_buffer_size that is not currently settable by the user, should it be? |
|
Hmm, there is also 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
force-pushed
the
config_lhbs
branch
2 times, most recently
from
92ce244 to
21326e6
Compare
|
Dropped the 'Router: remove unused proxy_buffers structure member.' commit as that is perhaps better done as a separate pull-request. |
A user reported on GitHub that when making a HTTP request with a long
URL (over 10,000 characters) they got a HTTP 431 error (Request Header
Fields Too Large).
This can be resolved in the code by updating the following line in
src/nxt_router.c::nxt_router_conf_create()
skcf->large_header_buffer_size = 8192;
However, requiring users to modify unit and install custom versions is
less than ideal. We could increase the value, but to what?
This commit takes the option of allowing the user to set this option in
their config.
large_header_buffer_size is already set by the configuration system in
nxt_router.c it just isn't set as a valid config option in
nxt_conf_validation.c
With this change users can set this option in their config if required
by the following
"settings": {
"http": {
"large_header_buffer_size": 16384
}
},
It retains its default value of 8192 if this is not set.
With this commit, without the above setting or too low a value, with a
long URL you get a 431 error. With the above setting set to a large
enough value, the request is successful.
Closes: nginx#521
settings.http.header_buffer_size and settings.http.large_header_buffers
are set internally and are in the nxt_router_http_conf array but not set
as valid config options.
Add entries for these to the nxt_conf_vldt_http_members array, then
users can set these values like
"settings": {
"http": {
"header_buffer_size": 512,
"large_header_buffers": 8
}
},
If these values aren't set they retain their default values.
There are some http related settings that are set internally but not
exposed to the config system.
Add
- proxy_buffer_size
- proxy_header_buffer_size
- proxy_timeout
- proxy_send_timeout
- proxy_read_timeout
to the http configuration in nxt_router.c and the http configuration
validation in nxt_conf_validation.c
This doesn't add 'proxy_buffers' as that is unused and will be removed
in a follow up commit.
This then allows users to set these such as
"settings": {
"http": {
"proxy_buffer_size": 65536,
"proxy_header_buffer_size": 8192,
"proxy_timeout": 30,
"proxy_send_timeout": 60,
"proxy_read_timeout": 60
}
}
If these values aren't set they retain their default values.
There are eight new options made available under settings.http - header_buffer_size - large_header_buffer_size - large_header_buffers - proxy_buffer_size - proxy_header_buffer_size - proxy_timeout - proxy_send_timeout - proxy_read_timeout
|
Any movement on this PR? We are facing 431 due to large header size, which seems to be a similar issue as this. |
|
@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 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. |
|
@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 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. |
|
Closing this as the source branch (again) only contains the single 'large_header_buffer_size' patch... |
|
PR for the afore mentioed patch. |
3 participants
This pull request has been updated and expanded on. It now consists of five commits
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.