compression: add generic decompressor filter #11172
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| # DO NOT EDIT. This file is generated by tools/proto_sync.py. | ||
|
|
||
| load("@envoy_api//bazel:api_build_system.bzl", "api_proto_package") | ||
|
|
||
| licenses(["notice"]) # Apache 2 | ||
|
|
||
| api_proto_package( | ||
| deps = [ | ||
| "//envoy/config/core/v3:pkg", | ||
| "@com_github_cncf_udpa//udpa/annotations:pkg", | ||
| ], | ||
| ) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| syntax = "proto3"; | ||
|
|
||
| package envoy.extensions.filters.http.decompressor.v3; | ||
|
|
||
| import "envoy/config/core/v3/base.proto"; | ||
| import "envoy/config/core/v3/extension.proto"; | ||
|
|
||
| import "google/protobuf/any.proto"; | ||
| import "google/protobuf/wrappers.proto"; | ||
|
|
||
| import "udpa/annotations/migrate.proto"; | ||
| import "udpa/annotations/status.proto"; | ||
| import "udpa/annotations/versioning.proto"; | ||
| import "validate/validate.proto"; | ||
|
|
||
| option java_package = "io.envoyproxy.envoy.extensions.filters.http.decompressor.v3"; | ||
| option java_outer_classname = "DecompressorProto"; | ||
| option java_multiple_files = true; | ||
| option (udpa.annotations.file_status).package_version_status = ACTIVE; | ||
|
|
||
| // [#protodoc-title: Decompressor] | ||
| // [#extension: envoy.filters.http.decompressor] | ||
|
|
||
| message Decompressor { | ||
| // Common configuration for filter behavior on both the request and response direction. | ||
| message CommonDirectionConfig { | ||
| // Runtime flag that controls whether the filter is enabled for decompression or not. If set to false, the | ||
| // filter will operate as a pass-through filter. If the message is unspecified, the filter will be enabled. | ||
| config.core.v3.RuntimeFeatureFlag enabled = 1; | ||
| } | ||
|
|
||
| // Configuration for filter behavior on the request direction. | ||
| message RequestDirectionConfig { | ||
| CommonDirectionConfig common_config = 1; | ||
|
|
||
| // If set to true, and response decompression is enabled, the filter modifies the Accept-Encoding | ||
| // request header by appending the decompressor_library's encoding. Defaults to true. | ||
| google.protobuf.BoolValue advertise_accept_encoding = 2; | ||
| } | ||
|
|
||
| // Configuration for filter behavior on the response direction. | ||
| message ResponseDirectionConfig { | ||
| CommonDirectionConfig common_config = 1; | ||
| } | ||
|
|
||
| // A decompressor library to use for both request and response decompression. Currently only | ||
| // :ref:`envoy.compression.gzip.compressor<envoy_api_msg_extensions.compression.gzip.decompressor.v3.Gzip>` | ||
| // is included in Envoy. | ||
| config.core.v3.TypedExtensionConfig decompressor_library = 1 | ||
| [(validate.rules).message = {required: true}]; | ||
|
|
||
| // Configuration for request decompression. Decompression is enabled by default if left empty. | ||
| RequestDirectionConfig request_direction_config = 2; | ||
|
|
||
| // Configuration for response decompression. Decompression is enabled by default if left empty. | ||
| ResponseDirectionConfig response_direction_config = 3; | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,112 @@ | ||
| .. _config_http_filters_decompressor: | ||
|
|
||
| Decompressor | ||
| ============ | ||
| Decompressor is an HTTP filter which enables Envoy to bidirectionally decompress data. | ||
|
|
||
|
|
||
| Configuration | ||
| ------------- | ||
| * :ref:`v3 API reference <envoy_v3_api_msg_extensions.filters.http.decompressor.v3.Decompressor>` | ||
|
|
||
| How it works | ||
| ------------ | ||
| When the decompressor filter is enabled, headers are inspected to | ||
| determine whether or not the content should be decompressed. The content is | ||
| decompressed and passed on to the rest of the filter chain. Note that decompression happens | ||
| independently for request and responses based on the rules described below. | ||
|
|
||
| Currently the filter supports :ref:`gzip compression <envoy_v3_api_msg_extensions.compression.gzip.decompressor.v3.Gzip>` | ||
| only. Other compression libraries can be supported as extensions. | ||
|
|
||
| An example configuration of the filter may look like the following: | ||
|
|
||
| .. code-block:: yaml | ||
|
|
||
| http_filters: | ||
| - name: decompressor | ||
| typed_config: | ||
| "@type": type.googleapis.com/envoy.extensions.filters.http.decompressor.v3.Decompressor | ||
| decompressor_library: | ||
| name: basic | ||
| typed_config: | ||
| "@type": type.googleapis.com/envoy.extensions.compression.gzip.decompressor.v3.Gzip | ||
| window_bits: 10 | ||
|
|
||
| By *default* decompression will be *skipped* when: | ||
|
|
||
| - A request/response does NOT contain *content-encoding* header. | ||
| - A request/response includes *content-encoding* header, but it does not contain the configured | ||
| decompressor's content-encoding. | ||
| - A request/response contains a *cache-control* header whose value includes "no-transform". | ||
|
|
||
| When decompression is *applied*: | ||
|
|
||
| - The *content-length* is removed from headers. | ||
|
There was a problem hiding this comment. Is this going to break some uses cases? Is it a future work item that we might want to buffer the decompressed data and rewrite content-length? There was a problem hiding this comment. Ah, sorry @mattklein123 , this context got dropped because github didn't allow me to move my original PR over. We discussed here: https://github.com/junr03/envoy/pull/1#discussion_r411669576 And then I had this idea: #11172 (comment) tl;dr: I originally implemented buffering here. But then decided to remove (and depend on the buffer filter) to keep this filter simpler. wdyt? There was a problem hiding this comment. Does the buffer filter reset content length? Anyway I think that's fine but I might make that clear in the documentation that the user might want to couple this with the buffer filter. There was a problem hiding this comment.
Yep, it sets it if not present, which made it ideal for this arrangement. Yeah I can add a section in the docs about it 👌 There was a problem hiding this comment. Ultimately the issue is that the setup would still be a bit kludgy because of filter ordering, and the fact that the buffer filter can't be disabled independently for requests and responses. For Lyft's use case it'll be fine because we don't need to preserve content-length on responses. So at the end of the day it might make sense to have this filter pay the price and do buffering itself. My proposal is to leave as is (with the new docs I added) to get this PR in, and then I can open an issue with a link to the commit that had buffering, it would just need to be updated a bit, and integration tests written (which shouldn't be hard at all). I can get to it as a low-pri. There was a problem hiding this comment. I think it's fine to document the limitation and we can revisit later. |
||
|
|
||
| .. note:: | ||
|
|
||
| If an updated *content-length* header is desired, the buffer filter can be installed as part | ||
| of the filter chain to buffer decompressed frames, and ultimately update the header. Due to | ||
| :ref:`filter ordering <arch_overview_http_filters_ordering>` a buffer filter needs to be | ||
| installed after the decompressor for requests and prior to the decompressor for responses. | ||
|
|
||
| - The *content-encoding* header is modified to remove the decompression that was applied. | ||
|
|
||
| .. _decompressor-statistics: | ||
|
|
||
| Using different decompressors for requests and responses | ||
| -------------------------------------------------------- | ||
|
|
||
| If different compression libraries are desired for requests and responses, it is possible to install | ||
| multiple decompressor filters enabled only for requests or responses. For instance: | ||
|
|
||
| .. code-block:: yaml | ||
|
|
||
| http_filters: | ||
| # This filter is only enabled for requests. | ||
| - name: envoy.filters.http.decompressor | ||
| typed_config: | ||
| "@type": type.googleapis.com/envoy.extensions.filters.http.decompressor.v3.Decompressor | ||
| decompressor_library: | ||
| name: small | ||
| typed_config: | ||
| "@type": "type.googleapis.com/envoy.extensions.compression.gzip.decompressor.v3.Gzip" | ||
| window_bits: 9 | ||
| chunk_size: 8192 | ||
| response_direction_config: | ||
| common_config: | ||
| enabled: | ||
| default_value: false | ||
| runtime_key: response_decompressor_enabled | ||
| # This filter is only enabled for responses. | ||
| - name: envoy.filters.http.decompressor | ||
| typed_config: | ||
| "@type": type.googleapis.com/envoy.extensions.filters.http.decompressor.v3.Decompressor | ||
| decompressor_library: | ||
| name: large | ||
| typed_config: | ||
| "@type": "type.googleapis.com/envoy.extensions.compression.gzip.decompressor.v3.Gzip" | ||
| window_bits: 12 | ||
| chunk_size: 16384 | ||
| request_direction_config: | ||
| common_config: | ||
| enabled: | ||
| default_value: false | ||
| runtime_key: request_decompressor_enabled | ||
|
|
||
| Statistics | ||
| ---------- | ||
|
|
||
| Every configured Deompressor filter has statistics rooted at | ||
| <stat_prefix>.decompressor.<decompressor_library.name>.<decompressor_library_stat_prefix>.<request/response>* | ||
| with the following: | ||
|
|
||
| .. csv-table:: | ||
| :header: Name, Type, Description | ||
| :widths: 1, 1, 2 | ||
|
|
||
| decompressed, Counter, Number of request/responses compressed. | ||
| not_decompressed, Counter, Number of request/responses not compressed. | ||
| total_uncompressed_bytes, Counter, The total uncompressed bytes of all the request/responses that were marked for decompression. | ||
| total_compressed_bytes, Counter, The total compressed bytes of all the request/responses that were marked for decompression. | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we configure bufferization separately for requests and responses shouldn't we have separate configs for the decompressor library as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could do that. And the stats would work as well.... I am not sure if that is increasing the cognitive load of someone trying to configure more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was my concern too: that might surprise if some options are configured separately and some are not. I'd probably prefer to configure two separate decompressor filters configured differently for different directions. But now with bufferization moved out it looks much better. I like it.
Perhaps the message's hierarchy could be flatter with
request_direction_config.common_config.advertise_accept_encodingchanged to justrequest_advertise_accept_encoding. Though I don't have strong opinion.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'll leave as is to future proof in case people want to add more complexity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW it seemed odd to me that we don't allow this to be configured differently in each direction? Can you at least clarify this applies to both the request and response configuration depending on whether they are configured or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's trivial to configure them independently, i.e allow different decompressors on each direction. If both you and @rojkov like the idea of allowing that, I can make the change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mattklein123 do you mean this thread? Sorry, I was waiting for you and/or @rojkov to vote one way or another. But I guess both of you have already expressed interest in it, so I will go ahead and add.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO it's a question of composability vs monolithic design. As a proponent of the former I'd prefer two sequential decompressor filters configured differently for different directions and only consider the latter if performance became an issue.
The same applies to buffering: coupled with the buffer filters we can get the same functionality with less code complexity. If the buffer filter is limited to requests only then it makes sense to extend it (and only it) - people might want to buffer responses in scenarios other than decompression (e.g. compression).
So, for now I vote for a comment on how to couple filters for various use cases instead of complicating the filter. Later we can reconsider.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine either way as long as the current state is well documented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, I had added documentation in the proto, but I have expanded with an example. The current proto would lend itself pretty easily to adding per direction config in the future is someone wants it.
lmk if you think this is clear @mattklein123 @rojkov 858c279