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

Add option to pass through compression #27

Merged
merged 3 commits into from
Feb 8, 2022

Conversation

kylewlacy
Copy link
Contributor

We are using rails-reverse-proxy in front of a small server that serves static compressed assets. Eventually we noticed that assets were always being served uncompressed, and after some digging, I came across the spot where the Accept-Encoding header is stripped from proxied requests.

This PR adds a new :compression option to control how rails-reverse-proxy handles the Accept-Encoding header. :disabled (default) disables all compression, just like rails-reverse-proxy does today. :passthrough forwards the Accept-Encoding header, which will then allow the proxied server to return compressed responses (but it does not compress uncompressed proxy responses). A future PR could add another option to allow uncompressed responses to be compressed by rails-reverse-proxy, but I decided not to add it to this PR since we didn't need it for our server.


Also, I don't have any experience working with Varnish, so I can't say for certain if compression: :passthrough would still cause Varnish to error out (like the previous comment in the code suggests), so hopefully someone else can chime in on that.

I will say I found some really weird behavior with Rack and the Accept-Encoding header. Previously, proxied request headers were set with Net::HTTPRequest#initialize_http_header, but for some reason, it looked like Rack would ignore the Accept-Encoding header? I'm not sure if that was because the version of Rack we were using, our version of Rails, or some other dependency... but I found that removing #initialize_http_header and using HTTPRequest::new to set the header instead seemed to fix it. I wonder if the issue with Varnish was caused by this same issue as well...

@axsuul
Copy link
Owner

axsuul commented Feb 1, 2018

@kylewlacy Thanks for your submission! I'll try to take a look at this before the week ends!

@axsuul
Copy link
Owner

axsuul commented Mar 11, 2018

@kylewlacy Apologies for getting to this so late!

I'd like to propose changing the option to reset_accept_encoding instead of compression, only because labeling it as compression might seem misleading.

I was looking more into the Accept-Encoding header and it looks to be used often for encoding content with a compression algorithm, but not always.

reset_accept_encoding would also simplify possible values to just true and false since I don't foresee any other possible settings for it. This also falls more in line with the naming conventions of other options (i.e. verify_ssl).

reset_accept_encoding of false (DEFAULT) would not intercept Accept-Encoding and let it passthrough
reset_accept_encoding of true would set Accept-Encoding to nil

With this we can set Accept-Encoding to a specific value and still be aware what's going on

reverse_proxy "http://localhost:8080", 
  reset_accept_encoding: false, 
  headers: { 'Accept-Encoding' => 'gzip' }

whereas in this instance, we are aware that Accept-Encoding would be reset even though it's explicitly set

reverse_proxy "http://localhost:8080", 
  reset_accept_encoding: true, 
  headers: { 'Accept-Encoding' => 'gzip' }

About the Varnish stuff, I would need test it with this new version. I don't mind cutting a major version just in case there's breaking changes.

Super interested to hear your thoughts on this and thanks again for your time! 👍

@kylewlacy
Copy link
Contributor Author

@axsuul That sounds reasonable to me! I agree that reset_accept_encoding makes it clearer as well as using a boolean (originally I envisioned adding other possible values like :gzip to always gzip the output, but after thinking it over this seems too far out of scope IMO). I'll amend this PR with your proposed changes when I get some free time

The default is to now to not clear the `Accept-Encoding` header (this can be changed back to the older behavior by passing `reset_accept_encoding: true`)
@kylewlacy
Copy link
Contributor Author

@axsuul Okay, sorry for the delay! The last two weeks have been kind of hectic for me. I finally had some free time so I updated this PR with the changes you suggested. Feel free to look it over again and I can make any other changes if needed

Joe-noh added a commit to Joe-noh/rails-reverse-proxy that referenced this pull request Mar 12, 2019
Joe-noh added a commit to Joe-noh/rails-reverse-proxy that referenced this pull request Apr 23, 2021
@saiqulhaq
Copy link

any updates on this PR @axsuul?
@kylewlacy already updated the option name

@axsuul axsuul merged commit df3184e into axsuul:master Feb 8, 2022
@axsuul
Copy link
Owner

axsuul commented Feb 8, 2022

Sorry for the delay, released as 0.11.0

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

Successfully merging this pull request may close these issues.

3 participants