Use skip_compression_ to match the other member name style.

I think it can be a const method.

Any particular reason for making memory_level configurable but not window_bits?

window_bits changes the encoding type. I'm not sure when user will want to change it in the case of GZIP.

It might make more sense to do the string->enum resolution at construction time and then store the enum here instead. You'd still retain the above functions but make them static functions that take the string and return enum, and call them in the ctor initializer list.

I think qvalue is a typo

'qvalue' is described in the RFC, e.g Accept-Encoding: gzip;q=1.0, identity; q=0.5, *;q=0

Nevermind :)

This looks like it'll return true if there isn't an exact match but a subset of the content type is contained in the white list. Is that definitely what you want?

Not really, I fixed it by providing a prefixed list of content-types that the user can select it from in the config.

My understanding is that std::regex::optimize flag makes matching faster at the expense of slower construction. Since this uses a temporary it'll still be doing construction for each call so this trade off doesn't seem to make sense. Thoughts?

(I think ideally you could have this std::regex live the same lifespan as Envoy itself, but watch out for the "static initialization order fiasco")

Today I learned that const auto and auto const are equivalent! I think everywhere else in Envoy you'll see it written as const auto, so please consider changing it as a nit.

nit: const auto

Please keep this namespace comment in.

It looks like that namespace Http was in a wrong place. It should be in the line 128 only.

What's the purpose of this namespace?

I just realized that this doesn't apply to c++11 or up anymore. I will remove it.

I think there's a simpler way to initialize these which will eliminate the constructor body and allow you to make all the members of the GzipFilterConfig const. See the part about RepeatedPtrField in https://developers.google.com/protocol-buffers/docs/reference/cpp-generated

This should work: cache_control_values_(gzip.cache_control().cbegin(), gzip.cache_control.cend())

I knew that repeated provides STL-like iterators, but I didn't realized that it could be initialized like that. Cool, thanks! 👍

I think this can be done with a switch statement since what comes from the proto is a true enum. It'll probably be a bit more readable code than the if statements.

I've tried switch here, but for some reason didn't work. Let me try it again.

Same for this, if a switch statement works prefer it to repeated if statements.

Can you add a comment about where these values come from?

I added this to get rid of the magic number, but good call about the comment.

It looks like you are not using a fast lookup here. Why not pull the content type up and use config_->contentTypeValues().find()?

Maybe it's because content-type could be something like "text/html; charset=utf8", but can't you parse out the substring of the content-type before the ";" and then do the unordered_set find?

Moreover, if you had a compressible type "foobar" but you had content-type:foo that would match this impl, if I'm reading it correctly. In practice that might not be an issue.

👀

why would cache-control influence the compressibility of something? Except maybe cache-control:no-transform.

It does not affect compression, but the necessity for such. Since the response is going to be cached, there are situations where skip compression might be desirable. Less common I believe, envoy could be behind another proxy that might cache certain content and then serve it compressed.

RE "response is going to be cached", I think that's what Vary:Accept-Encoding is for. HTTP caches should be able to hold a gzipped response and serve it to a non-gzip-accepting client by decompressing it on the fly, if that header is in the cached response. https://blog.stackpath.com/accept-encoding-vary-important is worth a read.

RE "behind another proxy" -- I think in that case the site admin would just disable gzipping in Envoy then, right? Why would it need to be based on cache-control headers?

I'm not objecting to this setting, I just want to better understand what the usage model is and its relationship to caches.

👀

Thanks for pointing this out Joshua.
I think that Vary should be also available in the configuration. In regards to cache-control, I was thinking more in in terms of proxied requests, like Nginx does on the presence of Via request header. See gzip_proxied at http://nginx.org/en/docs/http/ngx_http_gzip_module.html
Thoughts?

OK, sounds like you are going for a drop-in replacement for nginx gzip support? Fair enough. I don't know why someone would configure cache-control's effect on gzipping in an nginx proxy either, but it must have been put in for a reason. In that case, let's just do it efficiently. In the case of cache-control, I think it means splitting any number of attributes on ",", and doing a hash lookup in your set on each of them. E.g. cache-control: no-cache, no-store, must-revalidate, max-age=0 should (IMO) result in 4 exact hash lookups.

The most relevant setting (AFAICT), cache-control:no-transform, is not mentioned in the nginx doc you referenced, and I don't see it in your PR either. By default, IMO, you should honor cache-control:no-transform and avoid gzipping any response with that header.

I think it's very important for proxies to get the defaults right, especially Vary:accept-encoding, as messing these up can result in hard-to-diagnose downstream failures.

Good point on having cache-control:no-transform by default. I tried to make the feature as least opinionated as possible, so the users could opt for skipping compression on whatever cache-control value/s they need to, including no-transform.
I will work on these changes, thanks!

I'd defer to @htuch and @mattklein123 and @alyssawilk on the design philosophy here, as I am still new to Envoy development. A few options are:

1. If nginx's gzip module does it, then Envoy's should too.
2. Provide mechanism not policy about how the proxy should behave, and let the config figure out what to do (ie "least opinionated as possible)
3. Provide a paved path toward a few different sensible scenarios

There is some subtlety here that I think is worth getting right in the C++ code, so I kind of favor option 3, and these scenarios come to mind:

a. Enable gzip with some level of compression aggressiveness, respecting cache-control:no-transform from origin, and adding vary:accept-encoding to the response to the client.
b. Disable gzip completely, e.g. because the Envoy being configured is sitting in a high-bandwidth rack and some other proxy will gzip en route to client.

But I totally understand if the market for Envoy wants it to be a clean drop-in replacement for nginx and whatever features that has in its gzip setup.

The challenge with #2 above (least opinionated) is that you then need the person configuring Envoy to know the HTTP semantics and best practices, and enforce them via config.

why this complicated constant rather than just "{}"?

Thanks, this looks a lot better.

I'm not super-intuitive about regexes. Will this strip any extra whitespace too?
Note could also write this with str::find and some sort of string-view-trimming helper function (if such exists in absl::). That might be faster.

In any case this function deserves some direct unit-testing wihout waking up the rest of the gzip filter.

use .empty() to check whether it's empty, rather than taking the size(), which might in theory be a non-trivial calculation, depending on the STL impl.

probably worth putting TransferEncoding() in a temp as I think it's actually a lookup.

use !empty() rather than size()

probably worth doing a pass over all these methods and storing header lookups in temps rather than doing them twice.

There is a great chance that the second call to TransferEncoding() might never happen, reason why this is also the second last checking in the if statement. Do you think that it's worth doing all these assignments to temps?

IMO yes...HeaderMap's impl currently instantiates a discrete member variable per common header so the lookup is fast, but I'm wondering if that might be something that gets re-tuned later as that policy might be a drag on header construction speed as the number of potentially interesting headers grows. And in any case the temp is free, so I don't see a downside to a temp in readability or performance.

note there is also "deflate"(which isn't used much anymore and "br" (brotli) which is "up and coming", though I'm not sure how fast :)

Sorry, comment should be more specific -> "Since gzip is the only available content-encoding in Envoy at the moment".

I haven't look at Brotli specification yet, but I saw that it's supposed to be superior to gzip which is quite impressive.

use anon namespace rather than 'static' (from google style guide).

That's what I initially did, but it got caught in previous review. So, I thought it wasn't relevant anymore and ended up moving back to static.

re-reading https://google.github.io/styleguide/cppguide.html#Unnamed_Namespaces_and_Static_Variables it does not forbid 'static', but within the context of this style guide, everyone I know that writes C++ based on this guide interprets anonymous namespace to be the preferred mechanism here, even though they have the same effect. I think this interpretation might have been due to an earlier version of the styleguide or because the only example given is with an anonymous namespace.

technically this is taking a reference to a temporary. I would lose the & on the getString calls. Same below.

nit: const GzipFilterConfigSharedPtr&

nit: I would just define this in the header file.

nit: The initialization here is redundant and can be removed.

If you use absl::string_view you will not have to copy/allocate any string data for this operation.

https://github.com/abseil/abseil-cpp/blob/master/absl/strings/string_view.h

absl is already incorporated into the Envoy build.

I admit that you may need to construct a temp std::string later to do the hash lookup though, but in one case it would at least save you one intermediate copy. You could also avoid that intermediate copy by using std::unordered_setabsl::string_view but you'll need to keep the backing store for the strings separately then. Not sure if that's worth it, but it would save allocations/string-copying per request in the server.