search

hassio-addons / addon-bookstack

Bookstack - Home Assistant Community Add-ons
https://addons.community
MIT License
55 stars 13 forks source link

Fixed large upload sizes #280

Closed illuzn closed 5 months ago

illuzn commented 8 months ago

Proposed Changes

Using the environment variable, FILE_UPLOAD_SIZE_LIMIT, the file upload size may be set to an arbitrary value.

This is supported in /addon-bookstack/bookstack/rootfs/etc/nginx/nginx.conf by client_max_body_size 4G.

However, this does not correctly override the nginx server limits and php limits of 64M respectively.

This changes the nginx server_params.conf and the php81 www.conf to 4G which mirrors the above.

Consider whether it is appropriate to set limits of 0 (i.e. unlimited). I didn't do this since it may cause other issues.

These changes confirmed working on my production instance by manually changing those files and restarting the nginx and php81 services.

Related Issues

N/A

sinclairpaul commented 8 months ago

I think this is slightly excessive, and would need adjustments to PHP memory usage.

If it needs to be adjusted higher, why wouldn't we just set it to whatever the variable is set to?

illuzn commented 8 months ago

I think this is slightly excessive, and would need adjustments to PHP memory usage.

If it needs to be adjusted higher, why wouldn't we just set it to whatever the variable is set to?

Or to be honest we could make all of the limits something reasonable but not out of the realm of possibility say 300M. I ran into this problem when trying to upload an instruction manual to my home wiki which was 150mb and configuring the maximum file size via envvars didn't fix it.

Edit: I see what you mean now. #238 is tangentially related as is commit 926c223.

I note the default permitted upload size is 50mb. On my config, a 50mb jpeg on the lowest compression unpacks to around 200mb of memory (almost hitting the php memory limit) - obviously with higher compression this would exceed the memory limit. Perhaps this is best addressed by a note in the documentation about memory usage for large image files. Actually, I'm very surprised that #238 hit the memory limit (must be jpg with very high compression) because I cannot reproduce the issue locally (but can with a 50mb jpeg).

sinclairpaul commented 8 months ago

I think the default is 128, I didn't implement the change yet.

Ideally what I think I would like to do is enumerate the FILE_UPLOAD_SIZE_LIMIT variable and set the relevant settings based on that.

illuzn commented 8 months ago

I'd give it a crack but hassio addons are above my paygrade I'm afraid.

frenck commented 5 months ago

Because there hasn't been any activity on this PR for quite some time now, I've decided to close it for being stale.

Feel free to re-open this PR when you are ready to pick up work on it again 👍