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.

Sign up for GitHub

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

Jump to bottom

feat(conf) configurable request body handling #2602

Merged
merged 1 commit into from
Jun 13, 2017
Merged

feat(conf) configurable request body handling #2602

merged 1 commit into from
Jun 13, 2017

Conversation

p0pr0ck5
Copy link
Contributor

@p0pr0ck5 p0pr0ck5 commented Jun 6, 2017 •
edited
Loading

Summary

Add directives for client_max_body_size and client_body_buffer_size, allowing users to work around request body read limitations without the need to use a custom Nginx config.

Note that we do not adjust or make configurable these values in the Admin API section, as there is currently no need.

Full changelog

  • Add client_max_body_size and client_body_buffer_size directives and config notes.

@p0pr0ck5 p0pr0ck5 added pr/status/please review pr/wip A work in progress PR opened to receive feedback and removed pr/status/please review labels Jun 6, 2017
@p0pr0ck5 p0pr0ck5 force-pushed the feat/config_client_body branch 2 times, most recently from ec7969d to 925eb86 Compare June 6, 2017 20:15
@p0pr0ck5 p0pr0ck5 added pr/status/please review and removed pr/wip A work in progress PR opened to receive feedback labels Jun 6, 2017
@p0pr0ck5
Copy link
Contributor Author

p0pr0ck5 commented Jun 7, 2017

These tests passed, but yesterday's shenanigans broke the webhook delivery.

https://travis-ci.org/Mashape/kong/builds/240117836
https://travis-ci.org/Mashape/kong/builds/240117829

thibaultcha
thibaultcha requested changes Jun 12, 2017
View reviewed changes
Copy link
Member

@thibaultcha thibaultcha left a comment

Choose a reason for hiding this comment

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

One more thing: can we also open a PR against the 0.10 branch on getkong.org? The documentation section of the website is mostly copy-pasting from this default file, with some formatting tweaking.

kong/conf_loader.lua
@@ -70,6 +70,7 @@ 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

kong.conf.default
@@ -185,6 +185,27 @@
# `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.

kong/templates/nginx_kong.lua
@@ -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 :)

@thibaultcha thibaultcha added pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. and removed pr/status/please review labels Jun 12, 2017
@p0pr0ck5
Copy link
Contributor Author

Docs PR in Kong/docs.konghq.com#437

@p0pr0ck5 p0pr0ck5 force-pushed the feat/config_client_body branch 2 times, most recently from 2431145 to 08fba6d Compare June 12, 2017 21:43
thibaultcha
thibaultcha previously approved these changes Jun 12, 2017
View reviewed changes
@thibaultcha thibaultcha added pr/ready (but hold merge) No more concerns, but do not merge yet (probably a conflict of interest with another PR or release) and removed pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. labels Jun 12, 2017
@thibaultcha
Copy link
Member

thibaultcha commented Jun 12, 2017 •
edited
Loading

Last minute second thought: I feel like we should just add a couple unit tests for those 2 new properties, to make sure the conf loader and the Nginx template both parse/set the values for those.

This is mainly to ensure forward compatibility and set these properties in stone.

@thibaultcha thibaultcha dismissed their stale review June 12, 2017 22:20

Let's add a couple of unit tests

@p0pr0ck5 p0pr0ck5 force-pushed the feat/config_client_body branch 2 times, most recently from 808220d to bc27dea Compare June 12, 2017 22:39
thibaultcha
thibaultcha approved these changes Jun 12, 2017
View reviewed changes
Copy link
Member

@thibaultcha thibaultcha left a comment

Choose a reason for hiding this comment

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

Cool!

@p0pr0ck5 p0pr0ck5 force-pushed the feat/config_client_body branch from bc27dea to 960b7a9 Compare June 13, 2017 15:33
@p0pr0ck5
feat(conf) configurable request body handling
a84d701 
Add directives for client_max_body_size and client_body_buffer_size,
allowing users to work around request body read limitations without
the need to use a custom Nginx config.
@p0pr0ck5 p0pr0ck5 force-pushed the feat/config_client_body branch from 960b7a9 to a84d701 Compare June 13, 2017 15:35
@p0pr0ck5 p0pr0ck5 merged commit 4c907bc into master Jun 13, 2017
@p0pr0ck5 p0pr0ck5 deleted the feat/config_client_body branch June 13, 2017 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thibaultcha thibaultcha thibaultcha approved these changes

Assignees
No one assigned
Labels
pr/ready (but hold merge) No more concerns, but do not merge yet (probably a conflict of interest with another PR or release)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@p0pr0ck5 @thibaultcha