Allow to set the value for client_max_body_size

[zedtux]

I'm trying to make ownCloud working and everything work great until I try to upload a file of more than 3 Mb.

It seems that the instruction client_max_body_size is missing.

It would be nice to add another variable to manipulate the client_max_body_size value.

[zedtux]

This issue is no more urgent as I did an implementation in my fork : https://github.com/zedtux/nginx-proxy.
It's not really clean ... but it's working :)

It's fine for me until you implement a cleaner solution.

[dingdreher]

can we merge this?

[zedtux]

Well as I did it working as I can, I'm not sure it will be accepted. A cleaner way would be surely required.
In the other hand, until this is done in the clean way, you can use my fork (You will find some more features).

[jwilder]

I don't think there is an open PR to actually merge anything ATM but the reason it's not in the current template is that it can open up a DoS vulnerability if the limit is set too high and applies to too many endpoints. There are some other settings things that should be adjusted if this is increased as well. I'm looking at ways to support though.

[md5]

I've successfully increased this setting by dropping my own conf file into /etc/nginx/conf.d. This does mean that it's scoped to the whole nginx server config, but that was fine in our case.

[zedtux]

@jwilder what you're explaining is true with or without Docker right ? So if someone set something wrong, it is definitively wrong and you can't avoid it.

@md5 the point is about to be able to define it per container, meaning by passing an argument. You can have a look to my fork if you want to see an example.

[jwilder]

@zedtux Yes, it's more about nginx config. I'd like to get something in place that would make it hard to do the wrong thing for most users but not prevent anyone from really customizing things if they need to. Usually you only need a large body size for a small number of endpoints on a host and not all of them.

[thaJeztah]

Weird thought; perhaps the same approach as the TLS; have a directory that contains templates for a specific virtual host. If a template is found for a matching virtual host, include it and update it with the right ip-addresses?

[jwilder]

@thaJeztah That's an interesting idea.

[md5]

My first thought was to use template.Parse or template.ParseFile somehow to defined a new named template, but I realized this would somehow have to be called from within the template.

There's also template.ParseGlob to look at. However, I'm not sure if there is a way for a template function to get a reference to the current template in order to call ParseGlob on it.

[Nitesedge]

can we merge this? I tested out jperville's template change and it appears to work for what I need with owncloud.

[StormBP]

merge this. please

[md5]

@StormBP I can't speak for @jwilder, but I don't think that https://github.com/zedtux/nginx-proxy is in a state to be merged. It has a bunch of duplicative but incompatible changes along with changes that could be done in other ways.

In terms of setting client_max_body_size, that can be done currently on a proxy-wide basis by creating a file under /etc/nginx/conf.d that includes client_max_body_size 100m; or whatever you want. The same thing can be done for the server_tokens off; in zedtux/nginx-proxy. That file can be either created in a Dockerfile that derives FROM jwilder/nginx-proxy or it can be mounted in with -v.

The other case that people seem to want is the ability to set client_max_body_size on a per-VIRTUAL_HOST basis. For this, as for other things, I don't think a proliferation of environment variables is the right approach. I prefer the idea proposed by @thaJeztah in #29 (comment), namely the ability to add a per-VIRTUAL_HOST conf file that will be loaded based on a naming convention. This is more in line with the way other things are done in this image (cf. SSL, basic auth).

The only other change I noticed in the @zedtux fork that should be added is the addition of application/javascript to gzip_types. That could be added in a simple PR for just that change.

Update: The application/javascript change is actually the other way, so nothing to see there.

[jwilder]

Yeah, @md5 summed up my thoughts as well. I don't think adding another env var for this feature is the right way to go. I think supporting custom templates per vhost would be much more extensible.

[zedtux]

I agree with @md5. What I'm doing in my fork is just dirty but working stuff for my personal use.

Implementing those idea in the perfect way is the responsibility of this upstream repository in my opinion, meaning when done my fork is no more needed.