compression: add generic decompressor filter by junr03 · Pull Request #11172 · envoyproxy/envoy

junr03 · 2020-05-12T21:01:08Z

Commit Message: add generic decompressor filter
Additional Description: this PR takes the design of the compressor filter, and uses the decompressor extension point to build a generic decompressor http filter. This filter can be used to decompress compressed payloads both on the request and response direction.
Risk Level: low - low as it is an extension, med - for users as this is a brand new filter.
Testing: unit tests, integration tests
Docs Changes: added docs
Release Notes: added release notes

Signed-off-by: Jose Nino <jnino@lyft.com>

…atted Signed-off-by: Jose Nino <jnino@lyft.com>

Signed-off-by: Jose Nino <jnino@lyft.com>

rojkov

Yes, it definitely makes sense to buffer streams in a dedicated filter. I missed there's such filter.

rojkov · 2020-05-15T08:12:49Z

+  // A decompressor library to use for 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


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_encoding changed to just request_advertise_accept_encoding. Though I don't have strong opinion.

rebello95

A few comments, but LGTM overall!

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 · 2020-05-19T05:01:34Z

@rojkov, @rebello95 updated if you can take a look

rebello95

LGTM

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 · 2020-05-20T18:50:44Z

@mattklein123 this is now ready for you! Thanks

mattklein123

Nice work! Just a few high level comments/questions. Thank you!

/wait

mattklein123 · 2020-05-21T21:27:48Z

+  RequestDirectionConfig request_direction_config = 2;
+
+  ResponseDirectionConfig response_direction_config = 3;


Can you add docs here to explain what this config does and what happens if it is not set? Presumably if empty it doesn't decompress in the given direction?

Yep, bears repeating here what I wrote in the runtime field.

The default is actually to decompress if empty. Do you prefer to not decompress if empty?

I think it's fine either way I would just clearly document it as it wasn't clear to me on first read through.

mattklein123 · 2020-05-21T21:28:57Z

+  // A decompressor library to use for 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


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?

mattklein123 · 2020-05-21T21:30:42Z

+
+When decompression is *applied*:
+
+- The *content-length* is removed from headers.


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?

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?

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.

Does the buffer filter reset content length?

Yep, it sets it if not present, which made it ideal for this arrangement.

envoy/source/extensions/filters/http/buffer/buffer_filter.cc

Line 100 in a18611c

void BufferFilter::maybeAddContentLength() {

Yeah I can add a section in the docs about it 👌

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.

I think it's fine to document the limitation and we can revisit later.

mattklein123 · 2020-05-21T21:32:23Z

 namespace {
 const uint32_t DefaultWindowBits = 12;
 const uint32_t DefaultChunkSize = 4096;
+// When summed to window bits, this tells zlib library to decompress gzip data per:


nit: s/summed/logical ORd or something like that (even though sum is technically correct I think this is more clear)

Signed-off-by: Jose Nino <jnino@lyft.com>

mattklein123

LGTM modulo the comment about the compression config. Not sure if you are still planning on fixing that or not.

Signed-off-by: Jose Nino <jnino@lyft.com>

mattklein123

I think this is fine to ship and we can reevaluate the config later depending on feedback. Thank you!

Jose Nino added 30 commits April 10, 2020 11:44

dont delete my proto files

120e412

Signed-off-by: Jose Nino <jnino@lyft.com>

versioning stuff

ac30003

Signed-off-by: Jose Nino <jnino@lyft.com>

option

de750ce

Signed-off-by: Jose Nino <jnino@lyft.com>

proto: final state approved by formatter

9df2154

Signed-off-by: Jose Nino <jnino@lyft.com>

create new extension point in api

ab9a8f6

Signed-off-by: Jose Nino <jnino@lyft.com>

CHECKPOINT: moved protos to decompressors extension point

6b37345

Signed-off-by: Jose Nino <jnino@lyft.com>

decompressors extension point and gzip implementation. built and form…

b02ba42

…atted Signed-off-by: Jose Nino <jnino@lyft.com>

spelling

7ecc87a

Signed-off-by: Jose Nino <jnino@lyft.com>

move protos again

68929fe

Signed-off-by: Jose Nino <jnino@lyft.com>

new tree

222be60

Signed-off-by: Jose Nino <jnino@lyft.com>

done up to decompressor

4be8a3e

Signed-off-by: Jose Nino <jnino@lyft.com>

fmt

33c7ba1

Signed-off-by: Jose Nino <jnino@lyft.com>

name

9f86578

Signed-off-by: Jose Nino <jnino@lyft.com>

minor

f6d4737

Signed-off-by: Jose Nino <jnino@lyft.com>

name

21b62e0

Signed-off-by: Jose Nino <jnino@lyft.com>

wip

a4a02bb

Signed-off-by: Jose Nino <jnino@lyft.com>

wip

4df55ff

Signed-off-by: Jose Nino <jnino@lyft.com>

move back to mac

6e1415f

Signed-off-by: Jose Nino <jnino@lyft.com>

more tests

9de0fd0

Signed-off-by: Jose Nino <jnino@lyft.com>

passing

3ab576d

Signed-off-by: Jose Nino <jnino@lyft.com>

segfault

87fc130

Signed-off-by: Jose Nino <jnino@lyft.com>

pass

a26ae04

Signed-off-by: Jose Nino <jnino@lyft.com>

segfault

8faedf3

Signed-off-by: Jose Nino <jnino@lyft.com>

passed

b45e113

Signed-off-by: Jose Nino <jnino@lyft.com>

tests complete, look at coverage

1a981cf

Signed-off-by: Jose Nino <jnino@lyft.com>

all code covered

4830068

Signed-off-by: Jose Nino <jnino@lyft.com>

fmt

1043607

Signed-off-by: Jose Nino <jnino@lyft.com>

Merge branch 'master' into decompressor-filter

4bca431

Signed-off-by: Jose Nino <jnino@lyft.com>

update

9366f99

Signed-off-by: Jose Nino <jnino@lyft.com>

comments

b37b163

Signed-off-by: Jose Nino <jnino@lyft.com>

fmt

6fe0828

Signed-off-by: Jose Nino <jnino@lyft.com>

rojkov reviewed May 15, 2020

View reviewed changes

rebello95 reviewed May 16, 2020

View reviewed changes

Jose Nino added 4 commits May 18, 2020 17:13

some comments

bbb8370

Signed-off-by: Jose Nino <jnino@lyft.com>

comments

a3521a2

Signed-off-by: Jose Nino <jnino@lyft.com>

spelling

9eeba7a

Signed-off-by: Jose Nino <jnino@lyft.com>

spelling

513fcff

Signed-off-by: Jose Nino <jnino@lyft.com>

rebello95 previously approved these changes May 19, 2020

View reviewed changes

Comment thread source/extensions/filters/http/decompressor/decompressor_filter.h Outdated

Comment thread test/extensions/filters/http/decompressor/decompressor_filter_test.cc

Jose Nino added 2 commits May 20, 2020 08:06

finishing details: docs, release, integration test

4862341

Signed-off-by: Jose Nino <jnino@lyft.com>

spelling

df7f12f

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 dismissed rebello95’s stale review via df7f12f May 20, 2020 15:11

junr03 assigned mattklein123 May 20, 2020

mattklein123 marked this pull request as ready for review May 20, 2020 20:56

mattklein123 requested changes May 21, 2020

View reviewed changes

repokitteh-read-only Bot added the waiting label May 21, 2020

Jose Nino added 2 commits May 21, 2020 15:30

comments

8127a66

Signed-off-by: Jose Nino <jnino@lyft.com>

fmt

1bf8156

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 requested a review from dio as a code owner May 21, 2020 22:37

repokitteh-read-only Bot removed the waiting label May 21, 2020

mattklein123 previously approved these changes May 21, 2020

View reviewed changes

repokitteh-read-only Bot removed the api label May 21, 2020

update docs

858c279

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 dismissed mattklein123’s stale review via 858c279 May 25, 2020 18:03

repokitteh-read-only Bot added the api label May 25, 2020

mattklein123 approved these changes May 25, 2020

View reviewed changes

repokitteh-read-only Bot removed the api label May 25, 2020

junr03 merged commit 48a5b21 into envoyproxy:master May 26, 2020

junr03 mentioned this pull request Jun 1, 2020

decompressor: fix logs and docs #11393

Merged

		RequestDirectionConfig request_direction_config = 2;

		ResponseDirectionConfig response_direction_config = 3;


		When decompression is applied:

		- The content-length is removed from headers.

Conversation

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

Uh oh!

Uh oh!

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

rebello95 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

junr03 commented May 19, 2020

Uh oh!

rebello95 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

junr03 commented May 20, 2020

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!

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!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

junr03 commented May 12, 2020 •

edited

Loading