gzip: add stats to filter and fix accept-encoding identity bug #3647

lita · 2018-06-17T08:13:51Z

gzip filter: add stats to filter and fix accept-encoding identity bug

Description: This PR adds stats to the gzip filter. In the process of adding stats, I noticed a bug where the filter compresses when the Accept-Encoding is set to identity. I believe that goes against the RFC, as no transformation or encoding should happen to the data when "identity" is set. https://tools.ietf.org/html/rfc7231#section-5.3.4

Risk Level: Low

Testing: unit and integration

Docs Changes: Updated the gzip filter docs to clarify the behavior with identity and added more details with q=0.

Release Notes: Added a change to the release notes as this is user impacting.

Signed-off-by: Lita Cho <lcho@lyft.com>

mattklein123

Thank you! Generally looks great. Some comments to get started then will take another pass.

mattklein123 · 2018-06-18T00:15:39Z

docs/root/configuration/http_filters/gzip_filter.rst

+  that the "gzip" will have a higher weight then "\*". For example, if *accept-encoding*
+  is "gzip;q=0,\*;q=1", the filter will not compress. But if the header is set to
+  "\*;q=0,gzip;q=1", the filter will compress.
+- A request includes *accept-encoding* header includes "identity".


s/includes/whose value is ?

mattklein123 · 2018-06-18T00:15:59Z

docs/root/configuration/http_filters/gzip_filter.rst

@@ -39,9 +39,14 @@ response and request allow.
 By *default* compression will be *skipped* when:


Please add documentation for all the stats that you added. See other doc examples for how we do this.

mattklein123 · 2018-06-18T00:16:28Z

docs/root/intro/version_history.rst

@@ -36,6 +35,7 @@ Version history
 * debug: added symbolized stack traces (where supported)
 * grpc: support added for the full set of :ref:`Google gRPC call credentials
  <envoy_api_msg_core.GrpcService.GoogleGrpc.CallCredentials>`.
+* gzip filter: sending accept-encoding header as identity no longer compresses the payload.


nit: italicize both accept-encoding and identity. Also mention new stats.

mattklein123 · 2018-06-18T00:17:35Z

source/extensions/filters/http/gzip/gzip_filter.h

  bool disable_on_etag_header_;
  bool remove_accept_encoding_header_;
+  const std::string stats_prefix_;
+  Stats::Scope& scope_;


Instead of storing the scope here, you actually want to create the stats object in the config instead of the filter, and the return that on request. See other similar examples in other filters.

oh got it. didn't see this until now.

mattklein123 · 2018-06-18T00:17:59Z

source/extensions/filters/http/gzip/gzip_filter.cc

@@ -123,16 +128,20 @@ Http::FilterHeadersStatus GzipFilter::encodeHeaders(Http::HeaderMap& headers, bo
    headers.insertContentEncoding().value(Http::Headers::get().ContentEncodingValues.Gzip);
    compressor_.init(config_->compressionLevel(), config_->compressionStrategy(),
                     config_->windowBits(), config_->memoryLevel());
-  } else {
+


nit: del newline

mattklein123 · 2018-06-18T00:18:52Z

source/extensions/filters/http/gzip/gzip_filter.cc

    compressor_.compress(data, end_stream ? Compressor::State::Finish : Compressor::State::Flush);
+    stats_.total_compressed_bytes_.add(data.length());
+    stats_.compressed_.inc();


I think the compressed stat would be more useful if it were per-request, not per data frame similar to not_compressed?

Yea, Jose and I thought that the delta per data frame would be good enough, but happy to change it to per request. if you have a good example of how to do that, that would be awesome.

I would probably just invert the logic such that when you know that you are not "not compressing" you would increment the compressing stat. Not sure of the best way of doing that since I haven't looked at the filter code in detail, but I'm sure you can sort something out.

sounds good.

ah nvm, i see what you mean. Yup. will fix to move this into the encodeHeaders

mattklein123 · 2018-06-18T00:19:56Z

source/extensions/filters/http/gzip/gzip_filter.cc

@@ -191,15 +217,23 @@ bool GzipFilter::isContentTypeAllowed(Http::HeaderMap& headers) const {
 }

 bool GzipFilter::isEtagAllowed(Http::HeaderMap& headers) const {
-  return !(config_->disableOnEtagHeader() && headers.Etag());
+  bool isEtagAllowed = !(config_->disableOnEtagHeader() && headers.Etag());


nit: all variables snake_case. There are other examples in the PR, please audit. Also, this var can be const bool. Also other similar examples for this, please audit.

Ah yeah. Copy paste fail. Will fix.

Signed-off-by: Lita Cho <lcho@lyft.com>

mattklein123

Thanks, some more nits.

mattklein123 · 2018-06-19T22:57:14Z

source/extensions/filters/http/gzip/gzip_filter.cc

@@ -163,19 +170,36 @@ bool GzipFilter::isAcceptEncodingAllowed(Http::HeaderMap& headers) const {
      const auto q_value = StringUtil::trim(StringUtil::cropLeft(token, ";"));
      // If value is the gzip coding, check the qvalue and return.
      if (value == Http::Headers::get().AcceptEncodingValues.Gzip) {
-        return !StringUtil::caseCompare(q_value, ZeroQvalueString);
+        bool is_gzip = !StringUtil::caseCompare(q_value, ZeroQvalueString);


mattklein123 · 2018-06-19T22:57:25Z

source/extensions/filters/http/gzip/gzip_filter.cc

@@ -191,15 +215,23 @@ bool GzipFilter::isContentTypeAllowed(Http::HeaderMap& headers) const {
 }

 bool GzipFilter::isEtagAllowed(Http::HeaderMap& headers) const {
-  return !(config_->disableOnEtagHeader() && headers.Etag());
+  bool is_etag_allowed = !(config_->disableOnEtagHeader() && headers.Etag());


mattklein123 · 2018-06-19T22:57:35Z

source/extensions/filters/http/gzip/gzip_filter.cc

 }

 bool GzipFilter::isMinimumContentLength(Http::HeaderMap& headers) const {
  const Http::HeaderEntry* content_length = headers.ContentLength();
  if (content_length) {
    uint64_t length;
-    return StringUtil::atoul(content_length->value().c_str(), length) &&
-           length >= config_->minimumLength();
+    bool is_minimum_content_length = StringUtil::atoul(content_length->value().c_str(), length) &&


mattklein123 · 2018-06-19T22:58:10Z

source/extensions/filters/http/gzip/gzip_filter.h

@@ -18,14 +19,43 @@ namespace Extensions {
 namespace HttpFilters {
 namespace Gzip {

+/**
+ * All gzip filter stats. @see stats_macros.h
+ * Note that the total_bytes is only if the response


This comment is hard to understand. Rephrase?

mattklein123 · 2018-06-19T22:58:43Z

source/extensions/filters/http/gzip/gzip_filter.h

@@ -35,6 +65,8 @@ class GzipFilterConfig {
  }

  Runtime::Loader& runtime() { return runtime_; }
+  GzipStats& stats() { return stats_; }
+  const std::string stats_prefix() { return stats_prefix_; }


I don't think this is needed anymore

mattklein123 · 2018-06-19T22:59:35Z

docs/root/configuration/http_filters/gzip_filter.rst

+  compressed, Counter, Number of requests compressed.
+  not_compressed, Counter, Number of requests not compressed.
+  no_accept_header, Counter, Number of requests with no accept header sent.
+  header_identity, Counter, Number of requests sent with the "identity" set as the *accept-encoding*.


"with the" -> "with", same elsewhere where applicable

Signed-off-by: Lita Cho <lcho@lyft.com>

lita · 2018-06-20T21:11:38Z

@mattklein123 this is ready for another look.

mattklein123

nice, well done

Lita Cho added 3 commits June 17, 2018 01:13

Add Stats to Gzip Filter

f425e6f

Signed-off-by: Lita Cho <lcho@lyft.com>

Typo and wildcards

e749a00

Signed-off-by: Lita Cho <lcho@lyft.com>

fix stats

ce02912

Signed-off-by: Lita Cho <lcho@lyft.com>

mattklein123 reviewed Jun 18, 2018

View reviewed changes

Lita Cho added 3 commits June 18, 2018 13:40

CR reviews

e4f80a4

Signed-off-by: Lita Cho <lcho@lyft.com>

fix stats

e91eb0d

Signed-off-by: Lita Cho <lcho@lyft.com>

update

10e3b7f

Signed-off-by: Lita Cho <lcho@lyft.com>

mattklein123 self-assigned this Jun 19, 2018

Lita Cho added 3 commits June 18, 2018 22:05

space

85ba25c

Signed-off-by: Lita Cho <lcho@lyft.com>

update variable name

a24ea1e

Signed-off-by: Lita Cho <lcho@lyft.com>

Update comments

951d176

Signed-off-by: Lita Cho <lcho@lyft.com>

mattklein123 reviewed Jun 19, 2018

View reviewed changes

Lita Cho added 2 commits June 19, 2018 16:47

const and rewording comments

3da901a

Signed-off-by: Lita Cho <lcho@lyft.com>

update

498e367

Signed-off-by: Lita Cho <lcho@lyft.com>

mattklein123 approved these changes Jun 20, 2018

View reviewed changes

mattklein123 merged commit 08ba248 into envoyproxy:master Jun 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gzip: add stats to filter and fix accept-encoding identity bug #3647

gzip: add stats to filter and fix accept-encoding identity bug #3647

lita commented Jun 17, 2018

mattklein123 left a comment

mattklein123 Jun 18, 2018

mattklein123 Jun 18, 2018

lita Jun 19, 2018

mattklein123 Jun 18, 2018

mattklein123 Jun 18, 2018

lita Jun 18, 2018

mattklein123 Jun 18, 2018

mattklein123 Jun 18, 2018

lita Jun 18, 2018

mattklein123 Jun 18, 2018

lita Jun 18, 2018

lita Jun 18, 2018 •

edited

mattklein123 Jun 18, 2018

lita Jun 18, 2018

mattklein123 left a comment

mattklein123 Jun 19, 2018

mattklein123 Jun 19, 2018

mattklein123 Jun 19, 2018

mattklein123 Jun 19, 2018

mattklein123 Jun 19, 2018

mattklein123 Jun 19, 2018

lita commented Jun 20, 2018

mattklein123 left a comment

		@@ -39,9 +39,14 @@ response and request allow.
		By default compression will be skipped when:

gzip: add stats to filter and fix accept-encoding identity bug #3647

gzip: add stats to filter and fix accept-encoding identity bug #3647

Conversation

lita commented Jun 17, 2018

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

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

lita Jun 18, 2018 • edited

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lita commented Jun 20, 2018

mattklein123 left a comment

Choose a reason for hiding this comment

lita Jun 18, 2018 •

edited