compressor: expose generic compressor filter to users by rojkov · Pull Request #10553 · envoyproxy/envoy

rojkov · 2020-03-27T11:15:53Z

Description:

Currently the generic HTTP compressor filter isn't exposed to users
even though it's used internally by envoy.filters.http.gzip and can be
used by external filter extensions.

Expose the compressor's config API to users. For example the filter
can be configured as follows:

    ...
    filter_chains:
      filters:
      - name: envoy.http_connection_manager
        config:
           http_filters:
           - name: envoy.filters.http.compressor
             config:
               disable_on_etag_header: true
               content_length: 100
               content_type:
                 - text/html
                 - application/json
               compressor_library:
                 name: text_optimized
                 typed_config:
                   "@type": type.googleapis.com/envoy.extensions.filters.http.compressor.gzip.v3.Gzip
                   memory_level: 3
                   window_bits: 10
                   compression_level: best
                   compression_strategy: default_strategy
    ...

Multiple compressor filters using different compressor libraries,
e.g. gzip and brotli, can be stacked in one filter chain.

Risk Level: Medium
Testing: unit tests, manual testing
Docs Changes: a page for the new compressor filter is added, also comments in compressor.proto were updated.
Release Notes: added a link to compressor's doc to version_history.rst

Currently the generic HTTP compressor filter isn't exposed to users even though it's used internally by `envoy.filters.http.gzip` and can be used by external filter extensions. Expose the compressor's config API to users. For example the filter can be configured as follows: ... filter_chains: filters: - name: envoy.http_connection_manager config: http_filters: - name: envoy.filters.http.compressor config: disable_on_etag_header: true content_length: 100 content_type: - text/html - application/json compressor_library: name: envoy.filters.http.compressor.gzip config: memory_level: 3 window_bits: 10 compression_level: best compression_strategy: rle ... Multiple compressor filters using different compressor libraries, e.g. gzip and brotli, can be stacked in one filter chain. Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@linux.intel.com>

repokitteh-read-only · 2020-03-27T11:15:59Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #10553 was opened by rojkov.

see: more, trace.

rojkov · 2020-03-27T11:24:40Z

/cc @junr03, @rebello95, @mattklein123

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

htuch · 2020-03-30T15:24:16Z

api/envoy/config/filter/http/compressor/gzip/v2/gzip.proto

@@ -0,0 +1,62 @@
+syntax = "proto3";


How does this relate to https://github.com/envoyproxy/envoy/blob/master/api/envoy/config/filter/http/gzip/v2/gzip.proto? Which one should service operators configure and when? Is the former deprecated?

Yes, exactly, the former is deprecated. We need to make that clear as @htuch points out, have text in deprecated.rst, and also make sure the deprecated feature warning/stat is incremented so that operators know thy need to upgrade.

docs/root/configuration/http/http_filters/compressor_filter.rst

mattklein123

Thanks a ton for working on this. Per @htuch can we nail the deprecation story? Also, please check clang-tidy. It works now and there may be existing and new issues you need to fix. Thank you!

/wait

mattklein123 · 2020-03-30T20:47:29Z

api/envoy/config/filter/http/compressor/gzip/v2/gzip.proto

@@ -0,0 +1,62 @@
+syntax = "proto3";


Yes, exactly, the former is deprecated. We need to make that clear as @htuch points out, have text in deprecated.rst, and also make sure the deprecated feature warning/stat is incremented so that operators know thy need to upgrade.

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

rojkov · 2020-03-31T14:27:23Z

Thank you! I've just marked the old gzip filter explicitly deprecated.

But since it warns users now I wonder if we should revert #10306 and make gzip use its old implementation.

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

htuch

Looks good, a few more API comments..

htuch · 2020-04-02T17:13:01Z

api/envoy/config/filter/http/compressor/gzip/v2/gzip.proto

+// [#next-free-field: 10]
+message Gzip {
+  enum CompressionStrategy {
+    DEFAULT = 0;


Can you comment on what the DEFAULTs are here and below? These should be fixed in the API.

api/envoy/config/filter/http/compressor/gzip/v2/gzip.proto

api/envoy/config/filter/http/compressor/v2/compressor.proto

docs/root/configuration/http/http_filters/compressor_filter.rst

mattklein123

Thanks in general this is awesome. Let's nail all of the API comments and then we can do the in-depth review. Thank you!

/wait

mattklein123 · 2020-04-02T22:12:30Z

api/envoy/config/filter/http/compressor/v2/compressor.proto

+  //
+  // This field is ignored when used in the context of the deprecated "envoy.filters.http.gzip"
+  // filter.
+  CompressorLibrary compressor_library = 6;


I'm not sure it makes sense to have a default here. Can we require this and then make the gzip config explicit for clarity?

Sure, this field is required now.

Can we make it required via annotation?

Hm... This message is used also in the deprecated envoy.filters.http.gzip filter. I guess there are two options:

revert gzip: make use of generalized compression filter #10306 completely or partially (drop the compressor field from the Gzip message);

duplicate compressor.proto for envoy.filters.http.gzip.

docs/root/configuration/http/http_filters/compressor_filter.rst

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

mattklein123

In general looks awesome, thanks. Just a few more high level API comments. Thank you!

/wait

mattklein123 · 2020-04-06T22:34:40Z

api/envoy/config/filter/http/compressor/v2/compressor.proto

+  // The name of the compressor library to instantiate for the filter.
+  string name = 1 [(validate.rules).string = {min_bytes: 1}];


We can remove this for new extensions as the extension will be looked up via the typed_config.

Yeap, removed.

mattklein123 · 2020-04-06T22:34:50Z

api/envoy/config/filter/http/compressor/v2/compressor.proto

+  //
+  // This field is ignored when used in the context of the deprecated "envoy.filters.http.gzip"
+  // filter.
+  CompressorLibrary compressor_library = 6;


Can we make it required via annotation?

Since Envoy::Compressor::ZlibCompressorImpl::CompressionStrategy is simply static_cast'ed to uint64_t the Standard strategy (4) becomes Z_FIXED (4 as well). This basically disables the use of dynamic Huffman codes when the gzip filter is configured to use default values. Make the Standard strategy equal to 0 to translate to Z_DEFAULT_STRATEGY. Contributes to envoyproxy#8448 Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@linux.intel.com>

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

junr03 · 2020-05-04T19:46:29Z

@rojkov awesome work! Seems like you got @htuch's comment resolved. Any thoughts on #10553 (comment)?

Alternatively, we could add a "name" or "stat_prefix" field to the compressor filter config, and use that.

The more I think about it, the more I like this option. It would be a pretty unobtrusive way to disambiguate stats trees from different compression filters without affecting the filter interface.

rojkov · 2020-05-05T08:06:54Z

The more I think about it, the more I like this option. It would be a pretty unobtrusive way to disambiguate stats trees from different compression filters without affecting the filter interface.

Agree. I like "stats_prefix" as more explicit, but would prefer it to be optional to keep minimal configs light. Are you ok with that?

junr03

would prefer it to be optional to keep minimal configs light

That sounds good to me. Include it if present.

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

repokitteh-read-only · 2020-05-06T08:53:30Z

CC @envoyproxy/api-watchers: FYI only for changes made to api/.

🐱

Caused by: #10553 was synchronize by rojkov.

see: more, trace.

rojkov · 2020-05-06T09:01:29Z

That sounds good to me. Include it if present.

Done!
I decided not to mandate a separator at the end of the prefix, e.g. one might want to have an underscore instead of a dot.

junr03 · 2020-05-06T21:45:53Z

api/envoy/extensions/filters/http/compressor/v3/compressor.proto

+  // Prefix added to the names of stats generated by this filter. It can be used to disambiguate stats
+  // trees from different compression filters. For example stats of a compressor filter limited to compress
+  // binaries blobs and tweaked accordingly can be prefixed with "bins.". And another compressor filter
+  // tweaked co compress text content only can be prefixed with "text.".


Suggested change

// tweaked co compress text content only can be prefixed with "text.".

// tweaked to compress text content only can be prefixed with "text.".

super small typo. I can fix in my PR.

junr03

AWESOME! Amazing work @rojkov. Thank you for working with us on iterating and getting this landed. I can't wait to see all the compression extensions we build on top of this!

junr03 · 2020-05-06T21:47:30Z

@htuch @mattklein123 can either of you do a final pass to get the api-shepherds approval?

htuch · 2020-05-07T13:04:34Z

api/envoy/extensions/compression/common/compressor/v3/compressor.proto

+message CompressorLibrary {
+  // Compressor library specific configuration. See the supported libraries for further
+  // documentation.
+  google.protobuf.Any typed_config = 1;


Can we move this one to the new TypedConfig that will appear in #10908?

Sure, I've subscribed to that PR.

I presume it's about TypedExtensionConfig, not TypedConfig?

@htuch If we are moving all the typed_configs across the project to the new TypedExtenstionConfig message that is being created in #10908, this could be updated then. I'd like to merge this and come back and update when the discussion in #10908 settles, and gets merged. We have been working on this PR for a while, and have a chain of a few other PRs that depend on this one.

I don't think we can move them all without a deprecation and major version change. The idea is to make all new ones TypedExtensionConfig. Would it satisfy your concern if I submit a quick PR to introduce this independent of #10908?

I don't think we can move them all without a deprecation and major version change.

Yep, I was suggesting we would be doing the config in this PR as part of the deprecation that all other extensions would have to go through. But I see the want to wait a bit to land the correct thing that would avoid adding more volume to the deprecation cycle. Thanks for #11105, hopefully we can get that in soon, and move this PR along :)

junr03 · 2020-05-07T20:50:47Z

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

+    Compression::Compressor::CompressorFactoryPtr compressor_factory)
+    : Common::Compressors::CompressorFilterConfig(
+          generic_compressor,
+          stats_prefix + "compressor." + generic_compressor.stats_prefix() +


now that #11105 merged, we can delete the stats_prefix field, and use generic_compressor.compressor_library().typed_config().name().

mattklein123

Nice, LGTM other than remaining merge and field replacement.

/wait

This reverts commit 4b4cfda. Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@linux.intel.com>

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

rojkov · 2020-05-08T09:19:18Z

Do we really need the CompressorLibrary message now when there is this standard Envoy-specific TypedExtensionConfig? I remember I had to introduce the new message to accommodate config and typed_config, but then we moved to typed_configs only and now the message looks redundant.

I've replaced CompressorLibrary with TypedExtensionConfig and the configuration of compressors still looks sane imho.

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

junr03

@htuch any opinion around this? Otherwise, LGTM

junr03 · 2020-05-08T17:24:58Z

api/envoy/extensions/compression/common/compressor/v3/compressor.proto

-
-// [#protodoc-title: Compressor Library]
-
-message CompressorLibrary {


I guess the point of this message was to potentially have fields that were common to all compressor extensions. However, if we believe there won't be a case for this, I am happy to delete.

mattklein123 · 2020-05-08T18:03:24Z

@rojkov friendly request to please never force push as it makes reviews more difficult, thank you!

mattklein123

Thank you! Agreed on the message simplification.

rojkov requested a review from dio as a code owner March 27, 2020 11:15

repokitteh-read-only bot added the api label Mar 27, 2020

fix naming conflict in compressor_filter_integration_test.cc

4bc9660

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

rojkov force-pushed the compress branch from 5b30003 to b7cfac0 Compare March 27, 2020 14:43

fix clang-tidy findings

b7cfac0

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

mattklein123 self-assigned this Mar 27, 2020

Dmitry Rozhkov added 2 commits March 30, 2020 10:36

coverage: fix naming conflict

fd7fab2

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

fix bazel compile_time_options build

7bfd623

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

htuch reviewed Mar 30, 2020

View reviewed changes

docs/root/configuration/http/http_filters/compressor_filter.rst Show resolved Hide resolved

mattklein123 requested changes Mar 30, 2020

View reviewed changes

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

Dmitry Rozhkov added 2 commits March 31, 2020 12:22

Merge remote-tracking branch 'upstream/master' into compress

153f97e

explicitly deprecate gzip filter

09f10a9

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

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

fix bazel compile_time_options build

d9b33cc

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

htuch reviewed Apr 2, 2020

View reviewed changes

mattklein123 requested changes Apr 2, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Apr 2, 2020

htuch mentioned this pull request Apr 3, 2020

gzip filter: allow setting zlib compressor's chunk size #10508

Merged

address review comments

90d02d5

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

repokitteh-read-only bot removed the waiting label Apr 6, 2020

mattklein123 previously requested changes Apr 6, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Apr 6, 2020

Dmitry Rozhkov added 2 commits April 7, 2020 14:42

gzip.proto: rework CompressionStrategy and CompressionLevel enums

c3c26b6

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

Dmitry Rozhkov added 2 commits May 4, 2020 12:13

avoid changing frozen API in gzip.proto

786ab77

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

Merge remote-tracking branch 'upstream/master' into compress

6a9b477

junr03 reviewed May 5, 2020

View reviewed changes

compressor: add stats_prefix to compressor filter

4b4cfda

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

junr03 reviewed May 6, 2020

View reviewed changes

junr03 previously approved these changes May 6, 2020

View reviewed changes

mattklein123 self-assigned this May 6, 2020

htuch reviewed May 7, 2020

View reviewed changes

htuch mentioned this pull request May 7, 2020

router: Create InternalRedirectPolicy in side RouteAction and extend it with pluggable predicates #10908

Merged

junr03 reviewed May 7, 2020

View reviewed changes

mattklein123 reviewed May 7, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label May 7, 2020

rojkov dismissed junr03’s stale review via 15b51ea May 8, 2020 08:59

repokitteh-read-only bot removed the waiting label May 8, 2020

Dmitry Rozhkov added 3 commits May 8, 2020 12:04

Revert "compressor: add stats_prefix to compressor filter"

00fc997

This reverts commit 4b4cfda. Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@linux.intel.com>

Merge remote-tracking branch 'upstream/master' into compress

8c37ff1

replace CompressorLibrary with TypedExtensionConfig

152a6b7

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

rojkov force-pushed the compress branch from 15b51ea to 152a6b7 Compare May 8, 2020 09:05

reflect config changes in documentation

6b23035

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

junr03 approved these changes May 8, 2020

View reviewed changes

mattklein123 approved these changes May 8, 2020

View reviewed changes

repokitteh-read-only bot removed the api label May 8, 2020

mattklein123 merged commit 49efb98 into envoyproxy:master May 8, 2020

		// The name of the compressor library to instantiate for the filter.
		string name = 1 [(validate.rules).string = {min_bytes: 1}];

	// tweaked co compress text content only can be prefixed with "text.".
	// tweaked to compress text content only can be prefixed with "text.".


		// [#protodoc-title: Compressor Library]

		message CompressorLibrary {

Conversation

rojkov commented Mar 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Mar 27, 2020

Uh oh!

rojkov commented Mar 27, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rojkov commented Mar 31, 2020

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

junr03 commented May 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rojkov commented May 5, 2020

Uh oh!

junr03 left a comment

Choose a reason for hiding this comment

Uh oh!

repokitteh-read-only bot commented May 6, 2020

Uh oh!

rojkov commented May 6, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

junr03 left a comment

Choose a reason for hiding this comment

Uh oh!

junr03 commented May 6, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rojkov commented Mar 27, 2020 •

edited

Loading

junr03 commented May 4, 2020 •

edited

Loading

junr03 May 7, 2020 •

edited

Loading