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

Change default of max body size to 0 #98

Closed
aledbf opened this issue Dec 29, 2016 · 10 comments · Fixed by #148
Closed

Change default of max body size to 0 #98

aledbf opened this issue Dec 29, 2016 · 10 comments · Fixed by #148
Assignees
Labels

Comments

@aledbf
Copy link
Member

aledbf commented Dec 29, 2016

i.e. no upload limit

@euank
Copy link
Contributor

euank commented Dec 30, 2016

What motivates this configuration change?

The reason nginx defaults it so low is to remove possible DoS vectors iiuc, so I'd expect changing this default to require a bit more justification.

@aledbf
Copy link
Member Author

aledbf commented Dec 30, 2016

The reason nginx defaults it so low is to remove possible DoS vectors iiuc, so I'd expect changing this default to require a bit more justification.

Good point.

In the last week I helped at least 5 user with upload issues with the ingress controller (slack)

This is the list of issues I see with the current default:

  • too low (1m)
  • is hard to check the current value
    kubectl exec <ing pod> cat /etc/nginx/nginx.conf | grep client_max_body_size
  • maybe the docs are not clear on how is possible to increase the limit
  • is not possible to know if the error 413 comes from nginx or the upstream server
    (one of the users had another nginx behind the ingress controller)
  • increasing the value requires the use of a configmap

I am not sure if this list justifies the change (most are just UX "issues") but the first impression of the users is not good.

@euank
Copy link
Contributor

euank commented Dec 30, 2016

  • too low

The default's only too low if you want to accept uploads, which certainly isn't everyone.

  • is not possible to know if the error 413 comes from nginx or the upstream

The log lines should be quite different.
2016/12/30 1:11:11 [error] ... client intended to send too large body: ... vs ... upstream.name [...] 413

One of them, nginx reports an error itself, the other, nginx just reports a statuscode that it's proxying.


If you do want to accept uploads, the right solution isn't to set the size to '0', but to some reasonable value for the expected content (e.g. image upload sites might pick 20M, file hosting sites might pick 100M). I don't think 0 is ever the correct value.

I think we should improve the UX, and possibly increase the default by a small margin as a reasonable compromise perhaps.

@miracle2k
Copy link

I'd like to see the ability to change the value per host/ingress.

@gurvindersingh
Copy link

I think having this value configured using annotations on the ingress object might be a better solution. As you might need higher value for some services whereas for others you would prefer default 1m.

@gurvindersingh
Copy link

@aledbf the PR #100 make the default unlimited for all ingress, I still think we need a mechanism in general to override default settings for ingress object using annotation on object. Settings such as max body size, session stickiness etc. does require overriding in certain cases from default. Any plan on supporting that.

@aledbf
Copy link
Member Author

aledbf commented Jan 21, 2017

@gurvindersingh #148 allows custom body sizes using annotations

@aledbf
Copy link
Member Author

aledbf commented Jan 21, 2017

Leaving current default of "1m".

@sau-deep
Copy link

i have an admin feature where admin will be uploading files to s3 bucket. but upon heavy traffic ( backend deployed on elastic bean stalk ) new instances are being created and nginx client max body size is being omitted from nginx config. I tried available solution. .ebextensions , .platform config. but no luck. Can anybody help how to set upload size to 100mb permanaently. Is there any need to deploy my backend elsewhere, where nginx doesnt come into play. Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
6 participants