filters: add generic compressor filter #7057

rojkov · 2019-05-23T08:28:03Z

Description:

Add generic compressor_lib which can be reused by all HTTP compression filters to

parse Accepted-Encoding;
update HTTP headers;
decide which encoding to use for compression in case there are more than one compression filters in the same chain.

Risk Level: High
Testing: unit tests, manual tests
Docs Changes: N/A
Release Notes: N/A

Contributes to #4429

Design doc: Envoy HTTP Filters: Generic Compressor and Decompressor

Generalize `GzipFilter` into `CompressorFilter` accepting a compressor, e.g. gzip, performing actual data transformation. Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@linux.intel.com>

rojkov · 2019-05-23T08:43:29Z

/retest

repokitteh-read-only · 2019-05-23T08:43:40Z

🔨 rebuilding ci/circleci: coverage (failed build)
🔨 rebuilding ci/circleci: tsan (failed build)
🔨 rebuilding ci/circleci: clang_tidy (failed build)
🔨 rebuilding ci/circleci: asan (failed build)
🔨 rebuilding ci/circleci: compile_time_options (failed build)
🔨 rebuilding ci/circleci: release (failed build)
🔨 rebuilding ci/circleci: api (failed build)
🔨 rebuilding ci/circleci: format (failed build)
🔨 rebuilding ci/circleci: docs (failed build)
🔨 rebuilding ci/circleci: ipv6_tests (failed build)

🐱

Caused by: a #7057 (comment) was created by @rojkov.

see: more, trace.

Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@linux.intel.com>

dio · 2019-05-24T11:07:04Z

/azp run

azure-pipelines · 2019-05-24T11:07:22Z

Azure Pipelines successfully started running 2 pipeline(s).

dio · 2019-05-24T11:32:01Z

@gsagula FYI.

gsagula · 2019-05-24T17:55:56Z

@dio I can do the first pass if someone else is already doing it.

dio · 2019-05-27T04:01:52Z

@gsagula yes, please. Thanks!

gsagula

I'm still looking at AcceptEncodingAllowed member function. I will add more comments later. Thanks!

source/extensions/filters/http/common/compressor/compressor.cc

Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@linux.intel.com>

dio · 2019-05-29T13:10:06Z

@rojkov please merge master to pick up #7090. Thanks!

… allowed Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@linux.intel.com>

rojkov · 2019-05-29T13:45:32Z

@dio Thanks! I've merged the master and added one more test case.

dio · 2019-05-29T13:53:22Z

We have a formatting error in master due to s/absl::make_unique/std::make_unique check. The fix is here: #7081 (think it will be merged soon). Sorry.

rojkov · 2019-05-29T13:59:36Z

Ok, I'll pull it in when it gets merged.

stale · 2019-06-06T15:16:19Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

rojkov · 2019-06-10T08:57:04Z

@gsagula are you still looking at the PR?

gsagula · 2019-06-10T16:43:33Z

@rojkov Yes, sorry. I had a pretty busy week. I will look at the rest today. Thanks for pinging me.

stale · 2019-06-19T15:31:28Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

…ers-v4

mattklein123 · 2020-02-27T16:39:31Z

@rojkov just to clarify you want to review this PR, right? Can you update the title/description and then ping me when it's ready to review? Thank you.

rojkov · 2020-02-27T17:34:55Z

@mattklein123 I've updated the description. Please take a look.

rojkov · 2020-02-28T08:21:53Z

Also added a link to the design doc to put the PR in perspective.

mattklein123

Thanks a ton for working on this. At a high level this LGTM but I would like to take another pass once these comments are addressed. Thank you!

/wait

source/extensions/filters/http/common/compressor/compressor.h

mattklein123 · 2020-02-28T19:22:23Z

source/extensions/filters/http/common/compressor/compressor.h

+/**
+ * A filter that compresses data dispatched from the upstream upon client request.
+ */
+class CompressorFilter : public Http::StreamFilter {


Please derive from PassThroughFilter to avoid a bunch of boilerplate below.

mattklein123 · 2020-02-28T19:24:36Z

source/extensions/filters/http/common/compressor/compressor.h

+  StringUtil::CaseUnorderedSet content_type_values_;
+  bool disable_on_etag_header_;
+  bool remove_accept_encoding_header_;
+  CompressorStats stats_;
+  Runtime::Loader& runtime_;
+  std::string content_encoding_;


nit: please constify all possible member variables here and below, using the initializer list where possible if not already doing so.

mattklein123 · 2020-02-28T19:26:15Z

source/extensions/filters/http/common/compressor/compressor.cc

+    accept_encoding_ = std::make_unique<std::string>(accept_encoding->value().getStringView());
+  }
+
+  if (config_->runtime().snapshot().featureEnabled(config_->featureName(), 100)) {


Where does feature name come from here exactly? Is this just for purposes of having a kill switch? In new code we prefer an explicit kill switch on the config if needed. See RuntimeFeatureFlag

This is a slightly modified copy-paste from the gzip filter. I presume this is used for canary tests.

Probably RuntimeFractionalPercent would be a better candidate.

featureName is supposed to be a constant set in a class derived from CompressorFilterConfig, e.g. GzipFilterConfig.

Yeah I think this is all before we had the explicit RuntimeFeatureFlag. Can you add this to the new config and use that instead if we are doing a new thing? That will be better for the future and we can remove this explicit featureName() variable as a hard coded thing. Thank you!

source/extensions/filters/http/common/compressor/compressor.cc

mattklein123 · 2020-02-28T19:31:11Z

source/extensions/filters/http/common/compressor/compressor.cc

+}
+
+std::unique_ptr<CompressorFilter::EncodingDecision>
+CompressorFilter::chooseEncoding(const Http::ResponseHeaderMap& headers) const {


This function will need an explicit fuzzer. Also, should any of this be split out into a generic accept-encoding parser in HTTP utilities? Please add a TODO for writing the fuzzer. cc @asraa

Also, should any of this be split out into a generic accept-encoding parser in HTTP utilities?

I'm not sure. Where such utility could be re-used?

Anything that parses accept-encoding/q values? It's fine to keep here for now but I could see this easily being reimplemented later. cc @jmarantz

mattklein123 · 2020-02-28T19:34:21Z

source/extensions/filters/http/common/compressor/compressor.cc

+      config_->stats().header_not_valid_.inc();
+      break;
+    default:
+      config_->stats().header_compressor_overshadowed_.inc();


What does overshadowed mean? Note that this will need documentation once the filter itself is actually created.

Yes, the refactored gzip filter includes this stat documented in its .rst file. For now I added the description to compressor.h.

mattklein123 · 2020-02-28T19:35:35Z

source/extensions/filters/http/common/compressor/compressor.cc

+  // There could be many compressors registered for the same content encoding, e.g. consider a case
+  // when there are two gzip filters using different compression levels for different content sizes.
+  // In such case we ignore duplicates (or different filters for the same encoding) registered last.


In particular this method needs massively more comments, both at the high level and throughout. Please describe in detail how filter state is used, how things interact, etc.

and reduce its complexity a bit by moving stat updates to a new private metod shouldCompress(). Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@linux.intel.com>

rojkov · 2020-03-03T14:28:35Z

@mattklein123 Please take a look. The PR is ready for another pass.

mattklein123

Thanks this looks great to ship and iterate. Can you check/fix format and also update the killswitch mechanism and then I think we can ship? Thank you!

/wait

mattklein123 · 2020-03-03T20:22:30Z

source/extensions/filters/http/common/compressor/compressor.cc

+    accept_encoding_ = std::make_unique<std::string>(accept_encoding->value().getStringView());
+  }
+
+  if (config_->runtime().snapshot().featureEnabled(config_->featureName(), 100)) {


Yeah I think this is all before we had the explicit RuntimeFeatureFlag. Can you add this to the new config and use that instead if we are doing a new thing? That will be better for the future and we can remove this explicit featureName() variable as a hard coded thing. Thank you!

Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@linux.intel.com>

…ers-v4

Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@linux.intel.com>

rojkov · 2020-03-04T13:42:14Z

@mattklein123 Thank you! I've added the kill switch.

Though the coverage check started to fail. But the failures don't seem to be related to this PR.

mattklein123

Thanks looks great, just a small spelling nit. Thank you!

/wait

mattklein123 · 2020-03-04T17:14:28Z

tools/spelling/spelling_dictionary.txt

@@ -401,6 +401,7 @@ boolean
 booleans
 bools
 borks
+br


Sorry but to avoid adding this, can you potentially try adding backticks around the thing that triggered this? I think this is what @zuercher did in the associated PR. @zuercher perhaps we should have some error text that guides people on different ways of fixing the issue so we avoid more things going in the dictionary that shouldn't?

Sure, I removed br completely.

Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@linux.intel.com>

mattklein123

Thanks!

htuch · 2020-03-09T23:27:22Z

I think a dedicate fuzzer for this filter makes sense, since there is a fair bit of parsing and dependent logic CC @asraa.

gzip: generalize gzip into a general compression filter

321d72e

Generalize `GzipFilter` into `CompressorFilter` accepting a compressor, e.g. gzip, performing actual data transformation. Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@linux.intel.com>

Dmitry Rozhkov added 2 commits May 23, 2019 15:02

fix clang-tidy warnings

86d8f82

Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@linux.intel.com>

fix spellcheck errors

fd920c4

Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@linux.intel.com>

rojkov force-pushed the brotli-multi-filters branch from 07cc308 to fd920c4 Compare May 23, 2019 12:02

mattklein123 assigned dio May 23, 2019

dio requested a review from gsagula May 27, 2019 04:01

gsagula reviewed May 29, 2019

View reviewed changes

source/extensions/filters/http/common/compressor/compressor.cc Show resolved Hide resolved

optimize iterating over allowed compressors

6a1f90f

Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@linux.intel.com>

dio assigned gsagula May 29, 2019

Dmitry Rozhkov added 2 commits May 29, 2019 16:17

tests: verify removeAcceptEncoding is not used when compressor is not…

207d983

… allowed Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@linux.intel.com>

Merge remote-tracking branch 'upstream/master' into brotli-multi-filters

eeb4c4f

Merge remote-tracking branch 'upstream/master' into brotli-multi-filters

054be50

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Jun 6, 2019

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Jun 10, 2019

rojkov mentioned this pull request Jun 13, 2019

Symmetric Compression/Decompression between Envoys #4445

Closed

Merge remote-tracking branch 'upstream/master' into brotli-multi-filt…

6b3a23e

…ers-v4

repokitteh-read-only bot removed the waiting label Feb 27, 2020

rojkov changed the title ~~gzip: generalize gzip into a general compression filter~~ filters: add generic compressor filter Feb 27, 2020

mattklein123 requested changes Feb 28, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Feb 28, 2020

describe chooseEncoding() in details

1f89041

and reduce its complexity a bit by moving stat updates to a new private metod shouldCompress(). Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@linux.intel.com>

repokitteh-read-only bot removed the waiting label Mar 3, 2020

mattklein123 requested changes Mar 3, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Mar 3, 2020

use excplicit kill switch defined in the config

4a70888

Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@linux.intel.com>

repokitteh-read-only bot removed the waiting label Mar 4, 2020

Dmitry Rozhkov added 3 commits March 4, 2020 13:22

fix check_spelling_pedantic.py errors

50e7300

Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@linux.intel.com>

Merge remote-tracking branch 'upstream/master' into brotli-multi-filt…

31324f2

…ers-v4

fix check_format.py errors

70ef5fc

Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@linux.intel.com>

mattklein123 requested changes Mar 4, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Mar 4, 2020

use deflate instead of brotli in examples

d2c385d

Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@linux.intel.com>

repokitteh-read-only bot removed the waiting label Mar 5, 2020

mattklein123 approved these changes Mar 5, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Mar 5, 2020

mattklein123 merged commit 884bc7d into envoyproxy:master Mar 5, 2020

rojkov deleted the brotli-multi-filters branch March 9, 2020 14:40

rojkov mentioned this pull request Mar 9, 2020

gzip: make use of generalized compression filter #10306

Merged

filters: add generic compressor filter #7057

filters: add generic compressor filter #7057

Conversation

rojkov commented May 23, 2019 • edited Loading

rojkov commented May 23, 2019

repokitteh-read-only bot commented May 23, 2019

dio commented May 24, 2019

azure-pipelines bot commented May 24, 2019

dio commented May 24, 2019

gsagula commented May 24, 2019

dio commented May 27, 2019

gsagula left a comment

Choose a reason for hiding this comment

dio commented May 29, 2019

rojkov commented May 29, 2019

dio commented May 29, 2019 • edited Loading

rojkov commented May 29, 2019

stale bot commented Jun 6, 2019

rojkov commented Jun 10, 2019

gsagula commented Jun 10, 2019

stale bot commented Jun 19, 2019

mattklein123 commented Feb 27, 2020

rojkov commented Feb 27, 2020

rojkov commented Feb 28, 2020

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

rojkov commented Mar 3, 2020

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rojkov commented Mar 4, 2020

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

mattklein123 left a comment

Choose a reason for hiding this comment

htuch commented Mar 9, 2020

rojkov commented May 23, 2019 •

edited

Loading

dio commented May 29, 2019 •

edited

Loading