Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(conf) configurable request body handling #2602

Merged
merged 1 commit into from
Jun 13, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions kong.conf.default
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,36 @@
# `text/html`, `application/json`, and
# `application/xml`.

#client_max_body_size = 0 # Defines the maximum request body size allowed
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Nginx default is 1m. In the past, we've always respected Nginx's defaults when introducing new config variables that are just wrapping underplaying Nginx directives. Any reason for not doing so for this - sensitive - setting?

Additionally, when a config value is a 1-to-1 mapping of an Nginx directive, we also mention it in the description (controls the Nginx core module directive bearing the same name), and we include a note below with the link to the Nginx documentation (which is especially useful because it mentions size units like k and m that these directive use).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the Nginx default (1m) would be a breaking change, as we already define this as 0 in our template, hence the use of 0 here- my initial thought was to set it at 1m, but this breaks tests and a number of expected behaviors. Would be happy to set this to 1m in a separate change targetting 0.11 in a separate PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right, we have this hard-coded right now... We should probably introduce that change in 0.11 indeed. 0 is a dangerous default here, and contrary to our "least trust" policy introduced with 0.11 for saner/safer defaults.

# by requests proxied by Kong, specified in the
# Content-Length request header. If a request
# exceeds this limit, Kong will respond with a
# 413 (Request Entity Too Large). Setting this
# value to 0 disables checking the request body
# size.
# Note: See
# http://nginx.org/en/docs/http/ngx_http_core_module.html#client_max_body_size
# for further description of this parameter. Numeric values may be suffixed with
# 'k' or 'm' to denote limits in terms of kilobytes or megabytes.

#client_body_buffer_size = 8k # Defines the buffer size for reading the
# request body. If the client request body is
# larger than this value, the body will be
# buffered to disk. Note that when the body is
# buffered to disk Kong plugins that access or
# manipulate the request body may not work, so
# it is advisable to set this value as high as
# possible (e.g., set it as high as
# `client_max_body_size` to force request
# bodies to be kept in memory). Do note that
# high-concurrency environments will require
# significant memory allocations to process
# many concurrent large request bodies.
# Note: See
# http://nginx.org/en/docs/http/ngx_http_core_module.html#client_body_buffer_size
# for further description of this parameter. Numeric values may be suffixed with
# 'k' or 'm' to denote limits in terms of kilobytes or megabytes.

#------------------------------------------------------------------------------
# DATASTORE
#------------------------------------------------------------------------------
Expand Down
3 changes: 3 additions & 0 deletions kong/conf_loader.lua
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,9 @@ local CONF_INFERENCES = {
latency_tokens = {typ = "boolean"},
error_default_type = {enum = {"application/json", "application/xml",
"text/html", "text/plain"}},
client_max_body_size = {typ = "string"},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like we are missing client_body_buffer_size here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since client_body_buffer_size is a string, it isnt necessary to define it here. i can for explicitness if you like.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is indeed for explicitness. All string values are still listed here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

client_body_buffer_size = {typ = "string"},


database = {enum = {"postgres", "cassandra"}},
pg_port = {typ = "number"},
Expand Down
2 changes: 2 additions & 0 deletions kong/templates/kong_defaults.lua
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ upstream_keepalive = 60
server_tokens = on
latency_tokens = on
error_default_type = text/plain
client_max_body_size = 0
client_body_buffer_size = 8k

database = postgres
pg_host = 127.0.0.1
Expand Down
3 changes: 2 additions & 1 deletion kong/templates/nginx_kong.lua
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ error_log ${{PROXY_ERROR_LOG}} ${{LOG_LEVEL}};
>-- reset_timedout_connection on; # disabled until benchmarked
> end

client_max_body_size 0;
client_max_body_size ${{CLIENT_MAX_BODY_SIZE}};
proxy_ssl_server_name on;
underscores_in_headers on;

Expand Down Expand Up @@ -84,6 +84,7 @@ server {
access_log ${{PROXY_ACCESS_LOG}};
error_log ${{PROXY_ERROR_LOG}} ${{LOG_LEVEL}};

client_body_buffer_size ${{CLIENT_BODY_BUFFER_SIZE}};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should also add them to the fixture Nginx template in specs/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the fixtures/ max_body_size is still hard-coded to 0

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done-er :)


> if ssl then
listen ${{PROXY_LISTEN_SSL}} ssl;
Expand Down
24 changes: 24 additions & 0 deletions spec/01-unit/03-prefix_handler_spec.lua
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,30 @@ describe("NGINX conf compiler", function()
local nginx_conf = prefix_handler.compile_kong_conf(conf)
assert.matches("error_log syslog:server=.+:61828 error;", nginx_conf)
end)
it("defines the client_max_body_size by default", function()
local conf = assert(conf_loader(nil, {}))
local nginx_conf = prefix_handler.compile_kong_conf(conf)
assert.matches("client_max_body_size 0", nginx_conf, nil, true)
end)
it("writes the client_max_body_size as defined", function()
local conf = assert(conf_loader(nil, {
client_max_body_size = "1m",
}))
local nginx_conf = prefix_handler.compile_kong_conf(conf)
assert.matches("client_max_body_size 1m", nginx_conf, nil, true)
end)
it("defines the client_body_buffer_size directive by default", function()
local conf = assert(conf_loader(nil, {}))
local nginx_conf = prefix_handler.compile_kong_conf(conf)
assert.matches("client_body_buffer_size 8k", nginx_conf, nil, true)
end)
it("writes the client_body_buffer_size directive as defined", function()
local conf = assert(conf_loader(nil, {
client_body_buffer_size = "128k",
}))
local nginx_conf = prefix_handler.compile_kong_conf(conf)
assert.matches("client_body_buffer_size 128k", nginx_conf, nil, true)
end)
end)

describe("compile_nginx_conf()", function()
Expand Down
4 changes: 3 additions & 1 deletion spec/fixtures/custom_nginx.template
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ http {
>-- reset_timedout_connection on; # disabled until benchmarked
> end

client_max_body_size 0;
client_max_body_size ${{CLIENT_MAX_BODY_SIZE}};
proxy_ssl_server_name on;
underscores_in_headers on;

Expand Down Expand Up @@ -92,6 +92,8 @@ http {

access_log logs/access.log;

client_body_buffer_size ${{CLIENT_BODY_BUFFER_SIZE}};

> if ssl then
listen ${{PROXY_LISTEN_SSL}} ssl;
ssl_certificate ${{SSL_CERT}};
Expand Down