filter: implemented gzip http filter #2087

gsagula · 2017-11-21T07:51:13Z

Description:
This PR implements http filter that compresses data dispatched from an upstream service upon client request. To enable this filter, create an object literal under filters with name field “gzip” and an empty config object for minimum configuration. Envoy will deliver compressed content to any http request that contains gzip in accept-encoding header.

#269

Risk Level: Medium

Testing: unit testing, integration testing and manual testing thru envoy front-proxy.

Note: envoy::api::v2::filter will be included in this PR.

mattklein123 · 2017-11-21T16:40:09Z

Awesome! @dnoe can you own the review on this one?

dnoe

Looks good! Just a few comments to pick through.

dnoe · 2017-11-30T14:50:08Z

source/common/http/filter/gzip_filter.h

+private:
+  bool isContentTypeAllowed(const HeaderMap& headers);
+
+  bool skipCompression_;


Use skip_compression_ to match the other member name style.

dnoe · 2017-11-30T14:51:36Z

source/common/http/filter/gzip_filter.h

+  }
+
+private:
+  bool isContentTypeAllowed(const HeaderMap& headers);


I think it can be a const method.

dnoe · 2017-11-30T14:57:34Z

source/common/http/filter/gzip_filter.h

+        compression_level_(json_config.getString("compression_level", "default")),
+        compression_strategy_(json_config.getString("compression_strategy", "default")),
+        content_types_(json_config.getStringArray("content_types", true)),
+        memory_level_(json_config.getInteger("memory_level", 8)), window_bits_{31} {}


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.

dnoe · 2017-11-30T15:01:06Z

source/common/http/filter/gzip_filter.h

+
+private:
+  const std::string compression_level_{};
+  const std::string compression_strategy_{};


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.

dnoe · 2017-11-30T15:04:30Z

source/common/http/filter/gzip_filter.cc

+FilterHeadersStatus GzipFilter::encodeHeaders(HeaderMap& headers, bool) {
+  // TODO(gsagula): In order to fully implement RFC2616-14.3, more work is required here. The
+  // current implementation only checks if `gzip` is found in `accept-encoding` header, but
+  // it disregards the presence of qvalue or the order/priority of other encoding types.


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 :)

dnoe · 2017-11-30T15:12:24Z

source/common/http/filter/gzip_filter.cc

+    return true;
+  }
+  for (const auto& content_type : config_->getContentTypes()) {
+    if (headers.ContentType()->value().find(content_type.c_str())) {


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.

gsagula · 2017-11-30T22:14:45Z

Thanks @dnoe for reviewing it. I will take look at that over the weekend.

gsagula · 2017-12-05T08:33:53Z

@dnoe I did the initial changes that you suggested. I also included the part of the RFC2616 that was missing plus some more robust configuration options. Please take another look when you have a chance. Thanks!

dnoe

Mostly looks good, just a few nits and comments. Still needs documentation for the filter config but it looks like that's meant to come in a follow up PR.

dnoe · 2017-12-05T16:13:39Z

source/common/http/filter/gzip_filter.cc

+  if (headers.AcceptEncoding() != nullptr) {
+    return std::regex_search(
+        headers.AcceptEncoding()->value().c_str(),
+        std::regex{"(?!.*gzip;\\s*q=0(,|$))(?=(.*gzip)|(^\\*$))", std::regex::optimize});


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")

dnoe · 2017-12-05T16:16:23Z

source/common/http/filter/gzip_filter.cc

+
+bool GzipFilter::isContentTypeAllowed(const HeaderMap& headers) const {
+  if (config_->getContentTypeValues().size() > 0 && headers.ContentType() != nullptr) {
+    for (auto const& value : config_->getContentTypeValues()) {


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.

dnoe · 2017-12-05T16:16:52Z

source/common/http/filter/gzip_filter.cc

+
+bool GzipFilter::isCacheControlAllowed(const HeaderMap& headers) const {
+  if (config_->getCacheControlValues().size() > 0 && headers.CacheControl() != nullptr) {
+    for (auto const& value : config_->getCacheControlValues()) {


nit: const auto

mattklein123 · 2017-12-05T17:21:56Z

@gsagula @dnoe quick comment: can we please make this v2 ready in this PR? That will mean doing the data-plane-api filter config PR in parallel and updating this one. Thank you.

This is looks sweet BTW!

dnoe

Awesome, CONSTRUCT_ON_FIRST_USE is perfect. Just a few things.

dnoe · 2017-12-06T14:43:05Z

source/common/http/filter/gzip_filter.cc

@@ -36,7 +44,7 @@ FilterHeadersStatus GzipFilter::encodeHeaders(HeaderMap& headers, bool end_strea
  }

  return Http::FilterHeadersStatus::Continue;
-} // namespace Http
+}


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.

dnoe · 2017-12-06T14:48:56Z

source/common/http/filter/gzip_filter.cc

 namespace Envoy {
 namespace Http {

+namespace {


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.

dnoe · 2017-12-07T14:23:41Z

Looks good to me. Did you see @mattklein123 comment about making it V2 ready? Let me know here or on Slack if you have questions about that and what needs to be done in the data-plane-api repository.

gsagula · 2017-12-07T17:55:37Z

@dnoe I started working on proto yesterday. I reach you on Slack if I have any question. Thank you again for reviewing it!

gsagula · 2017-12-15T16:50:56Z

@dnoe Please take another look whenever you can. Thanks!

dnoe · 2017-12-18T14:14:48Z

Will take a look at this today (sorry I didn't managed to get to it before the weekend!)

gsagula · 2017-12-18T16:36:42Z

No worries! I also had a busy week and couldn't send this before Thursday night. Take your time!

dnoe

Looks good! A few remaining things to look at.

dnoe · 2017-12-18T20:33:25Z

source/common/http/filter/gzip_filter.cc

+
+  for (const auto& value : gzip.cache_control()) {
+    cache_control_values_.insert(value);
+  }


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! 👍

dnoe · 2017-12-18T20:36:21Z

source/common/http/filter/gzip_filter.cc

+
+ZlibCompressionLevelEnum
+GzipFilterConfig::compressionLevelEnum(const GzipV2CompressionLevelEnum& compression_level) {
+  if (compression_level == GzipV2CompressionLevelEnum::Gzip_CompressionLevel_Enum_BEST) {


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.

dnoe · 2017-12-18T20:37:52Z

source/common/http/filter/gzip_filter.cc

+
+ZlibCompressionStrategyEnum GzipFilterConfig::compressionStrategyEnum(
+    const GzipV2CompressionStrategyEnum& compression_strategy) {
+  if (compression_strategy == GzipV2CompressionStrategyEnum::Gzip_CompressionStrategy_RLE) {


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

dnoe · 2017-12-18T20:41:59Z

source/common/http/filter/gzip_filter.cc

+  return ZlibCompressionStrategyEnum::Standard;
+}
+
+const uint64_t GzipFilter::WINDOW_BITS{15 | 16};


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.

mattklein123 · 2017-12-21T06:00:15Z

@gsagula do you mind merging master to get the IPv6 tests to run? Is this ready for an additional pass by @htuch or myself? Very excited to see this land, I think we will end up using this at Lyft next year.

gsagula · 2017-12-21T16:56:20Z

@mattklein123 It's ready. Would be great to have yours or @htuch feedback on this.

jmarantz · 2017-12-27T19:56:24Z

source/common/http/filter/gzip_filter.cc

+
+bool GzipFilter::isContentTypeAllowed() const {
+  if (config_->contentTypeValues().size() && response_headers_->ContentType()) {
+    return std::any_of(config_->contentTypeValues().begin(), config_->contentTypeValues().end(),


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.

jmarantz · 2017-12-27T20:00:10Z

source/common/http/filter/gzip_filter.cc

+}
+
+bool GzipFilter::isCacheControlAllowed() const {
+  if (config_->cacheControlValues().size() && response_headers_->CacheControl()) {


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:

If nginx's gzip module does it, then Envoy's should too.

Provide mechanism not policy about how the proxy should behave, and let the config figure out what to do (ie "least opinionated as possible)

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.

jmarantz · 2017-12-27T20:15:58Z

test/common/http/filter/gzip_filter_test.cc

+
+// Acceptance Testing with default configuration.
+TEST_F(GzipFilterTest, AcceptanceGzipEncoding) {
+  setUpTest(R"EOF({})EOF");


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

jmarantz · 2017-12-29T03:21:29Z

source/common/http/filter/gzip_filter.cc

+bool GzipFilter::isContentTypeAllowed(HeaderMap& headers) const {
+  if (config_->contentTypeValues().size() && headers.ContentType()) {
+    std::cmatch match;
+    std::regex_search(headers.ContentType()->value().c_str(), match, contentTypeRegex());


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.

jmarantz · 2017-12-29T03:24:01Z

source/common/http/filter/gzip_filter.cc

+}
+
+bool GzipFilter::isContentTypeAllowed(HeaderMap& headers) const {
+  if (config_->contentTypeValues().size() && headers.ContentType()) {


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.

jmarantz · 2017-12-29T03:24:49Z

source/common/http/filter/gzip_filter.cc

+  }
+
+  return headers.TransferEncoding() &&
+         headers.TransferEncoding()->value().find(


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

jmarantz · 2017-12-29T03:25:37Z

source/common/http/filter/gzip_filter.cc

+}
+
+bool GzipFilter::isCacheControlAllowed(HeaderMap& headers) const {
+  if (config_->cacheControlValues().size() && headers.CacheControl()) {


use !empty() rather than size()

jmarantz · 2017-12-29T03:26:24Z

source/common/http/filter/gzip_filter.cc

+}
+
+bool GzipFilter::isTransferEncodingAllowed(HeaderMap& headers) const {
+  if (headers.TransferEncoding()) {


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.

jmarantz · 2017-12-29T03:27:40Z

source/common/http/filter/gzip_filter.cc

+
+FilterHeadersStatus GzipFilter::decodeHeaders(HeaderMap& headers, bool) {
+  // TODO(gsagula): The current implementation checks for the presence of 'gzip' and if the same
+  // is followed by Qvalue. Since gzip is the only available encoding right now, order/priority of


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.

jmarantz · 2017-12-29T03:28:41Z

source/common/http/filter/gzip_filter.cc

+namespace Envoy {
+namespace Http {
+
+static const std::regex& acceptEncodingRegex() {


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.

mattklein123

This is super sweet. Thanks for the high quality PR. A couple of random comments. I'm going to defer to @jmarantz for logic review as I have very little familiarity with high level web stuff so can't help that much with the various headers and when to use them.

@gsagula do you mind finalizing the docs PR in data-plane-api? (unhide filter docs, clean them up, etc.) Then we can do a final pass on everything together.

Thank you!

mattklein123 · 2017-12-29T18:08:44Z

source/common/config/filter_json.cc

+    *cache_control = json_cache_control;
+  }
+
+  const auto& json_compression_level = json_config.getString("compression_level", "default");


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

mattklein123 · 2017-12-29T18:10:30Z

source/common/http/filter/gzip_filter.cc

+  return (window_bits_ > 0 ? window_bits_ : DEFAULT_WINDOW_BITS) | GZIP_HEADER_VALUE;
+}
+
+GzipFilter::GzipFilter(GzipFilterConfigSharedPtr config)


nit: const GzipFilterConfigSharedPtr&

mattklein123 · 2017-12-29T18:10:55Z

source/common/http/filter/gzip_filter.cc

+GzipFilter::GzipFilter(GzipFilterConfigSharedPtr config)
+    : skip_compression_{true}, compressed_data_(), compressor_(), config_(config) {}
+
+void GzipFilter::onDestroy() {}


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

mattklein123 · 2017-12-29T18:13:57Z

source/common/http/filter/gzip_filter.h

+
+  Compressor::ZlibCompressorImpl compressor_;
+
+  GzipFilterConfigSharedPtr config_{nullptr};


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

gsagula · 2017-12-29T19:00:51Z

@mattklein123 Working on all the above.

@jmarantz I'm happy to go with the design changes that you suggested earlier. I will open another PR in data-plane to unlock docs, so we can work on the config changes from there.

Thanks for reviewing it.

jmarantz

This is looking better, but I'm still looking for the Vary:Accept-Encoding addition.

jmarantz · 2018-01-01T14:16:29Z

source/common/common/utility.cc

@@ -98,6 +98,27 @@ void StringUtil::rtrim(std::string& source) {
  }
 }

+void StringUtil::ltrim(std::string& source) {
+  std::size_t pos = source.find_first_not_of(" \t\f\v\n\r");


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.

jmarantz · 2018-01-01T14:24:01Z

source/common/http/filter/gzip_filter.cc

+
+bool GzipFilter::isCacheControlAllowed(HeaderMap& headers) const {
+  if (headers.CacheControl() && !config_->cacheControlValues().empty()) {
+    auto header_values = StringUtil::split(headers.CacheControl()->value().c_str(), ',');


don't forget to trim the tokens. If you just split on "," you'll wind up with tokens like " max-age=0" and " public". For some reason, it's common to have "public, max-age=300" in headers.

I'm not really sure if splitting off the max-age value is really a concern. E.g. would someone want to say "I'm willing to gzip, but only if it's cached less than 5 minutes"?

But you need to strip the whitespace so you can find "no-transform".

jmarantz · 2018-01-01T14:49:47Z

source/common/http/filter/gzip_filter.cc

+}
+
+bool GzipFilter::isTransferEncodingAllowed(HeaderMap& headers) const {
+  if (headers.TransferEncoding()) {


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.

jmarantz · 2018-01-01T15:00:04Z

source/common/http/filter/gzip_filter.cc

+namespace Envoy {
+namespace Http {
+
+static const std::regex& acceptEncodingRegex() {


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.

gsagula · 2018-01-02T09:20:50Z

@jmarantz Thanks again for the detailed review. I have my eyes on the comments above. I'm back to work tomorrow, so I won't be able to look at them till evening.

I did some modifications in gzip.proto (not in use yet) based on our previous discussions:

included option to disable Vary:Accept-Encoding (enabled by default)
removed cache-control. *I'd rather keep it simple for now and if users really need it, I can add it upon feature request. Also it looks like that accepting any proxied request and including Vary:Accept-Encoding is a common configuration.
(Add Via header for proxied requests #1030)

Note that I will also include verification for "Cache-Control: no-transform".

Please take a look at whenever you have time:
gsagula/data-plane-api@84623b6

Signed-off-by: Gabriel <[email protected]>

…ted api SHA Signed-off-by: Gabriel <[email protected]>

gsagula · 2018-02-06T23:16:32Z

Manual system-testing with real browser & envoy as a forward-proxy
Simplification to cropLeft
Changed static Vari header values to camel-case
Updated Envoy API SHA

@jmarantz I think we are done here, but feel free to give it another pass.

mattklein123

Epic PR! LGTM from a quick skim. @jmarantz do you mind doing the final pass on this one?

mattklein123 · 2018-02-07T23:45:06Z

source/common/config/filter_json.cc

@@ -430,5 +430,54 @@ void FilterJson::translateClientSslAuthFilter(
                                      *proto_config.mutable_ip_white_list());
 }

+void FilterJson::translateGzipFilter(const Json::Object& json_config,


Did we decide to keep v1 support or are we dropping it? If we are keeping it we should add docs here: https://www.envoyproxy.io/docs/envoy/latest/api-v1/http_filters/http_filters. My preference is to drop it though and encourage people to upgrade to v2. Either way is fine.

We decided to drop V1 as per Harvey's recommendation, but I forgot to remove it from Envoy code. I'll do that tomorrow 👀

mattklein123 · 2018-02-07T23:49:37Z

Oh also needs a master merge @gsagula

Signed-off-by: Gabriel <[email protected]>

gsagula · 2018-02-08T09:14:27Z

Thanks @mattklein123

gsagula · 2018-02-08T09:14:32Z

@jmarantz Lastest commit addresses all the places where code needs to handle case-insensitive header values. I tried to extend xxHash instead of defining a new one, but I ran out of time. I'll get back to it at some point.

jmarantz · 2018-02-08T14:36:48Z

xxHash vs a new one can be a TODO.

RE rebasing: this is a bit of the blind leading the blind here, but I don't think you need to rebase. You need to merge.

git checkout master
git fetch upstream
git pull
git checkout filter_gzip
git merge master

But I might have that wrong. I attempted to do that flow for one of my own branches and I think it it did the right thing.

jmarantz · 2018-02-08T14:37:35Z

In any case, please go ahead & merge so I can see the final state before my walkthrough. I'd recommend saving a simple tree of all your actual code somewhere outside of git's control :)

Signed-off-by: Gabriel <[email protected]>

gsagula · 2018-02-08T16:54:00Z

I think it still needs to merge upstream -> git merge upstream/master.

git checkout master
git fetch upstream
git merge upstream/master ..

Sounds good. Resolving last conflicts.

Signed-off-by: Gabriel <[email protected]>

gsagula · 2018-02-08T17:18:51Z

@jmarantz It's ready for another pass. Whenever you get a chance. Thanks!

Signed-off-by: Gabriel <[email protected]>

mattklein123

Few more v1 reverts needed I think?

mattklein123 · 2018-02-08T21:09:50Z

source/common/config/well_known_names.h

-      : v1_converter_({BUFFER, CORS, DYNAMO, FAULT, GRPC_HTTP1_BRIDGE, GRPC_JSON_TRANSCODER,
-                       GRPC_WEB, HEALTH_CHECK, IP_TAGGING, RATE_LIMIT, ROUTER, LUA,
-                       EXT_AUTHORIZATION}) {}
+      : v1_converter_({BUFFER, CORS, DYNAMO, ENVOY_GZIP, FAULT, GRPC_HTTP1_BRIDGE,


We should revert this change.

mattklein123 · 2018-02-08T21:10:11Z

source/common/json/config_schemas.cc

@@ -1075,6 +1075,48 @@ const std::string Json::Schema::GRPC_JSON_TRANSCODER_FILTER_SCHEMA(R"EOF(
  }
  )EOF");

+const char Json::Schema::GZIP_HTTP_FILTER_SCHEMA[] = R"EOF(


Should revert this change.

mattklein123 · 2018-02-08T21:10:24Z

source/common/json/config_schemas.h

@@ -39,6 +39,7 @@ class Schema {
  static const std::string BUFFER_HTTP_FILTER_SCHEMA;
  static const std::string FAULT_HTTP_FILTER_SCHEMA;
  static const std::string GRPC_JSON_TRANSCODER_FILTER_SCHEMA;
+  static const char GZIP_HTTP_FILTER_SCHEMA[];


mattklein123 · 2018-02-08T21:11:11Z

source/server/config/http/gzip.cc

+
+HttpFilterFactoryCb GzipFilterConfig::createFilterFactory(const Json::Object& json_config,
+                                                          const std::string&, FactoryContext&) {
+  envoy::config::filter::http::gzip::v2::Gzip proto_config;


I think this version can never happen if we drop v1? NOT_IMPLEMENTED?

Signed-off-by: Gabriel <[email protected]>

gsagula · 2018-02-09T01:47:19Z

@mattklein123 Done with V1 cleanup. Sorry, I tried to do that very quickly this morning so that you or Joshua could take another look, but I missed some spots.

jmarantz

this looks great, thanks for pushing this all the way through!

mattklein123

Awesome work! Thanks for the great contribution.

* use route directive regardless of rpc status Signed-off-by: Kuat Yessenov <[email protected]> * log response code Signed-off-by: Kuat Yessenov <[email protected]>

andre-rosa · 2022-06-20T14:20:04Z

source/common/common/hash.h

+  static uint64_t djb2CaseInsensitiveHash(absl::string_view input) {
+    uint64_t hash = 5381;
+    for (unsigned char c : input) {
+      hash += ((hash << 5) + hash) + absl::ascii_tolower(c);


Seems like an unintended deviation from the original DJB2 hash having:

hash += ((hash << 5) + hash) + absl::ascii_tolower(c);

instead of:

hash = ((hash << 5) + hash) + absl::ascii_tolower(c);

While at it, a typo in ingnoring.

@andre-rosa can i suggest opening a PR with your suggested change - seems like the typo is fixed already so i would suggest looking at current main

Thanks for your answer. Please check #21787.

mattklein123 requested a review from dnoe November 21, 2017 16:39

dnoe reviewed Nov 30, 2017

View reviewed changes

gsagula force-pushed the filter_gzip branch from e6ad864 to 300f26b Compare December 5, 2017 07:47

dnoe reviewed Dec 5, 2017

View reviewed changes

dnoe reviewed Dec 6, 2017

View reviewed changes

gsagula force-pushed the filter_gzip branch 2 times, most recently from 5229c58 to cd5e964 Compare December 15, 2017 07:28

dnoe reviewed Dec 18, 2017

View reviewed changes

gsagula force-pushed the filter_gzip branch from 5428fa8 to 74a1fe2 Compare December 19, 2017 09:25

jmarantz reviewed Dec 27, 2017

View reviewed changes

gsagula force-pushed the filter_gzip branch from eef60e8 to 15993b0 Compare December 28, 2017 09:28

jmarantz reviewed Dec 29, 2017

View reviewed changes

mattklein123 reviewed Dec 29, 2017

View reviewed changes

jmarantz reviewed Jan 1, 2018

View reviewed changes

gsagula force-pushed the filter_gzip branch from 0895e8d to 6856ef7 Compare February 5, 2018 08:03

Gabriel added 4 commits February 5, 2018 00:08

filter_gzip: code review changes

62808a7

Signed-off-by: Gabriel <[email protected]>

filter_gzip: updated repository locations and rebased

dad5fad

Signed-off-by: Gabriel <[email protected]>

filter_gzip: fixed conflicts

1f0d0cf

Signed-off-by: Gabriel <[email protected]>

filter_gzip: rebased and added option to remove accept-encoding header

16a3a96

Signed-off-by: Gabriel <[email protected]>

gsagula force-pushed the filter_gzip branch from 6856ef7 to 16a3a96 Compare February 5, 2018 08:10

filter_gzip: changed vary header values to be case sensitive and upda…

b99155d

…ted api SHA Signed-off-by: Gabriel <[email protected]>

mattklein123 reviewed Feb 7, 2018

View reviewed changes

filter_gzip: added support to handle case-insensitive header values

cd363a7

Signed-off-by: Gabriel <[email protected]>

filter_gzip: merged master and removed V1 support

35d4bd3

Signed-off-by: Gabriel <[email protected]>

filter_gzip: merged master and fixed format

f291227

Signed-off-by: Gabriel <[email protected]>

filter_gzip: removed unused code from filter_json

ce30782

Signed-off-by: Gabriel <[email protected]>

mattklein123 reviewed Feb 8, 2018

View reviewed changes

Gabriel added 2 commits February 8, 2018 15:34

filter_gzip: removed the remaining V1 code

04d5ce8

Signed-off-by: Gabriel <[email protected]>

filter_gzip: resolved conflict

a31fe2e

Signed-off-by: Gabriel <[email protected]>

jmarantz approved these changes Feb 9, 2018

View reviewed changes

mattklein123 approved these changes Feb 9, 2018

View reviewed changes

mattklein123 merged commit a555089 into envoyproxy:master Feb 9, 2018

gsagula deleted the filter_gzip branch April 14, 2018 20:52

andre-rosa reviewed Jun 20, 2022

View reviewed changes


		Compressor::ZlibCompressorImpl compressor_;

		GzipFilterConfigSharedPtr config_{nullptr};

filter: implemented gzip http filter #2087

filter: implemented gzip http filter #2087

Conversation

gsagula commented Nov 21, 2017 • edited Loading

mattklein123 commented Nov 21, 2017

dnoe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsagula commented Nov 30, 2017

gsagula commented Dec 5, 2017

dnoe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 commented Dec 5, 2017

dnoe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnoe commented Dec 7, 2017

gsagula commented Dec 7, 2017

gsagula commented Dec 15, 2017

dnoe commented Dec 18, 2017

gsagula commented Dec 18, 2017

dnoe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsagula Dec 19, 2017 • edited Loading

Choose a reason for hiding this comment

mattklein123 commented Dec 21, 2017

gsagula commented Dec 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmarantz Dec 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsagula Dec 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsagula commented Nov 21, 2017 •

edited

Loading

gsagula Dec 19, 2017 •

edited

Loading

gsagula commented Dec 21, 2017 •

edited

Loading

jmarantz Dec 29, 2017 •

edited

Loading

gsagula Dec 30, 2017 •

edited

Loading

gsagula commented Jan 2, 2018 •

edited

Loading

gsagula commented Feb 6, 2018 •

edited

Loading