search

ppy / s3-nginx-proxy

A lightweight kubernetes deployable nginx-based caching reverse proxy
MIT License
27 stars 4 forks source link

Possible issue in config.js - parseSize function #3

Closed Lebvanih closed 1 year ago

Lebvanih commented 1 year ago

Hello,

I was testing the gateway today and noticed that the calculation of the memory was a bit weird for the cache_path. I initially had a sizeLimit of 500Mi but it got resolved as just "500". Checking the code here https://github.com/ppy/s3-nginx-proxy/blob/master/src/config.js#L30 I notice that "M" is twice in the switch statement, but Mi is not included, while G and Gi both exist. This looks like an oversight.

Also, I might be missing something, but here (and in the other 2 actually):

      case 'G':
        size *= 1000;
      case 'M':
        size *= 1000 * 1000;

Isn't there an issue with the "G" (and Gi) calculation? This imply in that function that G is smaller than M, while G is 1000 time bigger than M. At Line 41, the 4G is calculated as 4000 x 1000 x 1000. Which in that case seems correct compared to the snippet above.

Kr,

ThePooN commented 1 year ago

Hello, thanks for contributing!

You indeed caught an oversight with the parseSize function where Mi wasn't handled correctly due to a missing character in its case. It has been fixed in https://github.com/ppy/s3-nginx-proxy/commit/99252758ad4e06a4e205c42751eb44828aebe570, thanks for reporting!

As for the calculation: in the G and Gi cases, the break; statement is missing so it will also execute respectively the M or Mi case, respectively. You can try it out by pasting the function in the Node CLI or your browser's dev tools.

Let us know if you have any other issue or suggestion, I'll be happy to help!

ThePooN commented 1 year ago

I've also pushed a tagged release with the bugfix (as well as the Helm chart for Kubernetes use, which is undocumented at the moment but should be straight-forward for seasoned Helm users. happy to assist otherwise)