compression: add brotli compressor and decompressor by rojkov · Pull Request #12998 · envoyproxy/envoy

rojkov · 2020-09-07T11:15:52Z

Commit Message: compression: add brotli compressor and decompressor
Additional Description: Add new brotli compression extensions in addition to gzip.
Risk Level: Low, no existing functionality is touched
Testing: uni tests, manual tests with curl.
Docs Changes: updated docs for compression and decompression HTTP filters to refer the new available encoder/decoder.
Release Notes: updated current.rst
Fixes #4429

The PR adds a new dependency on https://github.com/google/brotli. Here's the current criteria answers:

Criteria	Answer
Cloud Native Computing Foundation (CNCF) approved license	MIT
Dependencies must not substantially increase the binary size unless they are optional	brotli's binary size built with `-c opt` is 752K
No duplication of existing dependencies	no other dep provides Brotli
Hosted on a git repository and the archive fetch must directly reference this repository.	https://github.com/google/brotli
CVE history appears reasonable, no pathological CVE arcs	so far 4 CVEs related to brotli have been registered
Code review (ideally PRs) before merge	PRs are reviewed before merge
Security vulnerability process exists, with contact details and reporting/disclosure process	no policy exists, submitted google/brotli#878
> 1 contributor responsible for a non-trivial number of commits	75 contributors
Tests run in CI	CI set up with AppVeyor and Github actions
High test coverage (also static/dynamic analysis, fuzzing)	Fuzzers are run in CI
Envoy can obtain advanced notification of vulnerabilities or of security releases	brotli is registered in CPE
Do other significant projects have shared fate by using this dependency?	Google Chrome is using the library
Releases (with release notes)	https://github.com/google/brotli/releases
Commits/releases in last 90 days	last commit 9 days ago

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

repokitteh-read-only · 2020-09-07T11:15:59Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.
CC @envoyproxy/dependency-watchers: FYI only for changes made to (bazel/repository_locations\.bzl)|(api/bazel/repository_locations\.bzl)|(.*/requirements\.txt).

🐱

Caused by: #12998 was opened by rojkov.

see: more, trace.

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

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

rojkov · 2020-09-09T09:13:36Z

/retest

repokitteh-read-only · 2020-09-09T09:13:41Z

Retrying Azure Pipelines, to retry CircleCI checks, use /retest-circle.
Retried failed jobs in: envoy-presubmit

🐱

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

see: more, trace.

jmarantz

flushing comments

jmarantz · 2020-09-09T13:39:51Z

source/extensions/compression/brotli/compressor/brotli_compressor_impl.cc

+                                    Envoy::Compression::Compressor::State state) {
+  BrotliContext ctx(chunk_size_);
+
+  for (const Buffer::RawSlice& input_slice : buffer.getRawSlices()) {


i'm a little confused about the buffer management here. It looks like we are appending the compress bytes to the buffer in process() while we are looping over buffer.getRawSlices().

Could we use a separate output buffer -- for clarity at least -- and then swap them at the end if want compress() to be a mutation on buffer?

I think what you have is basically correct but it feels a little queezy to be modifying the buffer while iterating over it.

I stole this approach from the gzip encoder since it seems to consume a bit less memory. I guess gzip needs to be updated too.

I wonder if it would be better to drain the buffer after every slice, so that the output buffer could potentially re-use the memory from the drained slices from the input buffer.

Thanks! Updated to drain the buffer as soon as possible.

jmarantz · 2020-09-09T13:41:20Z

source/extensions/compression/brotli/compressor/brotli_compressor_impl.cc

+  if (ctx.avail_out == 0) {
+    // update output and reset context
+    output_buffer.add(static_cast<void*>(ctx.chunk_ptr.get()), chunk_size_);
+    ctx.chunk_ptr = std::make_unique<uint8_t[]>(chunk_size_);


why do we need to reallocate chunk_ptr on every call to process()? It looks like we are copying the bytes out of it above, can't we just re-use the same buffer and just define

void reset() { next_out = chunk_ptr.get(); }

Oops, that was a debug leftover copy-pasted in two places. Thank you!

jmarantz · 2020-09-09T13:45:29Z

source/extensions/compression/brotli/decompressor/brotli_decompressor_impl.h

+  void decompress(const Buffer::Instance& input_buffer, Buffer::Instance& output_buffer) override;
+
+private:
+  struct BrotliContext {


this is almost identical to the compressor's BrotliContext; the only difference is whether chunk_size is taken as a size_t or uint32_t. Can we share the struct definition?

and anyway let's probably make it a class and factor out the common management of the member variables (e.g. reset() per above comment)

For now I've moved the struct to a common module. But probably it makes sense to turn it into a base class for the implementations in the similar fashion it's done for gzip. WDYT?

source/extensions/compression/brotli/compressor/brotli_compressor_impl.h

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

junr03 · 2020-09-11T17:27:41Z

Thanks for the initial pass, will take a look today

junr03

initial pass. Super excited for this!

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

junr03 · 2020-09-13T18:52:37Z

api/envoy/extensions/compression/brotli/decompressor/v3/brotli.proto

+  // Ring buffer is allocated according to window size, despite the real size of the content.
+  bool disable_ring_buffer_reallocation = 1;
+
+  // Value for encoder's next output buffer. If not set, defaults to 4096.


Suggested change

// Value for encoder's next output buffer. If not set, defaults to 4096.

// Value for decoder's next output buffer. If not set, defaults to 4096.

Although should we just use compressor/decompressor in these comments?

Agree, replaced with compressor/decompressor.

docs/root/api-v3/config/compression/compression.rst

source/extensions/compression/brotli/compressor/brotli_compressor_impl.h

junr03 · 2020-09-13T19:44:26Z

source/extensions/compression/brotli/compressor/brotli_compressor_impl.cc

+    process(ctx, buffer,
+            state == Envoy::Compression::Compressor::State::Finish ? BROTLI_OPERATION_FINISH
+                                                                   : BROTLI_OPERATION_FLUSH);
+  } while (BrotliEncoderHasMoreOutput(state_.get()) && !BrotliEncoderIsFinished(state_.get()));


What does it mean if the encoder gets to a point where it still has more output but is finished? Or conversely if there is no more output but it hasn't finished? Is there something to be done at this layer?

Seems like the expected final state is for there to be no more output and for the encoder to have finished? Do we only expect reaching that at the same time that this function gets state == Envoy::Compression::Compressor::State::Finish?

I looked at Brotli's source code and it turned out BrotliEncoderIsFinished() internally just calls BrotliEncoderHasMoreOutput() and checks if the current operation is FINISH. So it's not really needed here and we should care only about the encoder's empty output.

inline comment here would also be good

junr03 · 2020-09-13T19:44:47Z

source/extensions/compression/brotli/compressor/config.cc

+// Default compression window size.
+const uint32_t DefaultWindowBits = 22;
+
+// Default quality. The actual value passed to the compressor is decremented by


Added a clarification. But perhaps there is a better way to deal with zero values in omitted fields. I can only come up with making the field mandatory.

Ah, yeah you can use google.protobuf.UInt32Value so there is a difference between omitted and default.

Oh, I didn't know that. Thanks! I dropped the decrement.

junr03 · 2020-09-13T19:48:51Z

source/extensions/compression/brotli/compressor/brotli_compressor_impl.cc

+                                   const BrotliEncoderOperation op) {
+  BROTLI_BOOL result = BrotliEncoderCompressStream(state_.get(), op, &ctx.avail_in_, &ctx.next_in_,
+                                                   &ctx.avail_out_, &ctx.next_out_, nullptr);
+  RELEASE_ASSERT(result == BROTLI_TRUE, "unable to compress");


How come the compressor asserts while the decompressor increases an error stat?

For the compressor it must be an internal error or OOM. But in case of decompression it usually means the input data is garbage.

Ah I see, so for the compressor you want to crash because there is nothing the user can do. But for the decompressor it could be user error (garbage input) so you want to alert via telemetry.

Yes, exactly. The gzip extensions has the same behavior.

junr03 · 2020-09-13T19:58:04Z

source/extensions/compression/brotli/decompressor/brotli_decompressor_impl.cc

+    ctx.next_in_ = static_cast<uint8_t*>(input_slice.mem_);
+
+    while (ctx.avail_in_ > 0) {
+      if (!process(ctx, output_buffer)) {


Maybe I should just read the brotli documentation. But I wonder if we should write some documentation inline about this.

In compression I could, at least at the surface see that we had different operations, and there was first a process pass, and then a flush/finish pass. Decompression doesn't seem to have an explicit operation? Why do we still need two passes?

The first loop runs until the input is not empty. But there might be situations when small input fully consumed by the decompressor unfolds into output not fitting the output chunk. Hence the second loop runs until there's no more output in the decompressor.

Would such a comment help?

Yeah, inline comment would help. That way future readers can understand the interaction without needing to go to brotli source from the get go.

added a comment

source/extensions/compression/brotli/decompressor/brotli_decompressor_impl.h

junr03 · 2020-09-13T20:01:01Z

/wait

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

htuch

/wait

bazel/brotli.patch

rojkov · 2020-12-30T11:07:02Z

Merged the latest master to resolve a merge conflict in current.rst.

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

rojkov · 2021-01-21T09:05:21Z

/retest

repokitteh-read-only · 2021-01-21T09:05:26Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

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

see: more, trace.

rojkov · 2021-01-21T13:49:05Z

@lizan could you take a look?

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

rojkov · 2021-02-02T12:28:51Z

/retest

repokitteh-read-only · 2021-02-02T12:28:55Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

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

see: more, trace.

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

htuch

/lgtm deps

Thanks so much for the detailed scorecard!

rojkov · 2021-02-05T16:11:49Z

/retest

repokitteh-read-only · 2021-02-05T16:11:53Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

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

see: more, trace.

moderation · 2021-02-08T21:37:03Z

I'm getting build errors as there isn't a category specified in https://github.com/envoyproxy/envoy/blob/main/source/extensions/compression/brotli/compressor/BUILD#L24-L35

ERROR: Traceback (most recent call last):
        File "~/Library/envoyproxy/envoy/source/extensions/compression/brotli/compressor/BUILD", line 24, column 19, in <toplevel>
                envoy_cc_extension(
        File "~/Library/envoyproxy/envoy/bazel/envoy_library.bzl", line 124, column 13, in envoy_cc_extension
                fail("Category not set for %s" % name)
Error in fail: Category not set for config
ERROR: Traceback (most recent call last):
        File "~/Library/envoyproxy/envoy/source/extensions/compression/brotli/decompressor/BUILD", line 26, column 19, in <toplevel>
                envoy_cc_extension(
        File "~/Library/envoyproxy/envoy/bazel/envoy_library.bzl", line 124, column 13, in envoy_cc_extension
                fail("Category not set for %s" % name)
Error in fail: Category not set for config
ERROR: ~/Library/envoyproxy/envoy/source/exe/BUILD:28:17: no such target '//source/extensions/compression/brotli/compressor:config_envoy_extension': target 'config_envoy_extension' not declared in package 'source/extensions/compression/brotli/compressor' defined by ~/Library/envoyproxy/envoy/source/extensions/compression/brotli/compressor/BUILD and referenced by '//source/exe:envoy_common_lib'
ERROR: ~/Library/envoyproxy/envoy/source/exe/BUILD:28:17: no such target '//source/extensions/compression/brotli/decompressor:config_envoy_extension': target 'config_envoy_extension' not declared in package 'source/extensions/compression/brotli/decompressor' defined by ~/Library/envoyproxy/envoy/source/extensions/compression/brotli/decompressor/BUILD and referenced by '//source/exe:envoy_common_lib'
ERROR: Analysis of target '//source/exe:envoy-static.stripped' failed; build aborted: Analysis failed
INFO: Elapsed time: 0.842s
INFO: 0 processes.
FAILED: Build did NOT complete successfully (0 packages loaded, 0 targets configured)

@rojkov @lizan

moderation · 2021-02-08T21:41:40Z

diff --git a/source/extensions/compression/brotli/compressor/BUILD b/source/extensions/compression/brotli/compressor/BUILD
index efd1739e0..93f13d8fe 100644
--- a/source/extensions/compression/brotli/compressor/BUILD
+++ b/source/extensions/compression/brotli/compressor/BUILD
@@ -25,6 +25,7 @@ envoy_cc_extension(
     name = "config",
     srcs = ["config.cc"],
     hdrs = ["config.h"],
+    category = "envoy.compression.decompressor",
     security_posture = "robust_to_untrusted_downstream",
     deps = [
         ":compressor_lib",
diff --git a/source/extensions/compression/brotli/decompressor/BUILD b/source/extensions/compression/brotli/decompressor/BUILD
index 3cc016e2f..3667300a8 100644
--- a/source/extensions/compression/brotli/decompressor/BUILD
+++ b/source/extensions/compression/brotli/decompressor/BUILD
@@ -27,6 +27,7 @@ envoy_cc_extension(
     name = "config",
     srcs = ["config.cc"],
     hdrs = ["config.h"],
+    category = "envoy.compression.decompressor",
     security_posture = "robust_to_untrusted_downstream",
     deps = [
         ":decompressor_lib",

compression: add brotli compressor and decompressor

eb1f837

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

repokitteh-read-only bot added the api label Sep 7, 2020

Dmitry Rozhkov added 4 commits September 7, 2020 16:09

run check_spelling_pedantic.py

95e0251

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

speed up testing by reducing default compression quality

6b8c9e4

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

fix modernize-use-nullptr check

7400a45

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

run check_format.py

162eee4

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

rojkov marked this pull request as draft September 8, 2020 13:00

test Windows build

f10361b

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

htuch assigned jmarantz Sep 8, 2020

htuch requested a review from jmarantz September 8, 2020 13:15

add reference to pull request to brotli repo

0a579d9

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

rojkov marked this pull request as ready for review September 8, 2020 15:13

remove commented out leftovers

7166d94

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

htuch requested a review from junr03 September 8, 2020 18:30

htuch assigned junr03 and unassigned jmarantz Sep 8, 2020

jmarantz self-assigned this Sep 9, 2020

jmarantz reviewed Sep 9, 2020

View reviewed changes

address review comments

9f5d5f3

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

junr03 reviewed Sep 13, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Sep 13, 2020

rojkov added 2 commits September 14, 2020 10:50

free memory as soon as possible

b407c3a

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

address review comments

d3d47fe

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

repokitteh-read-only bot removed the waiting label Sep 14, 2020

htuch suggested changes Sep 14, 2020

View reviewed changes

bazel/brotli.patch Outdated Show resolved Hide resolved

repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Dec 30, 2020

rojkov added 2 commits January 13, 2021 11:36

Merge remote-tracking branch 'upstream/master' into brotli-v2

2baf168

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

adjust current.rst

e8494ca

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

Base automatically changed from master to main January 15, 2021 23:01

rojkov added 2 commits January 20, 2021 16:32

Merge remote-tracking branch 'upstream/main' into brotli-v2

6e6e207

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

update current.rst to have features in alphabetical order

30cec33

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

rojkov added 2 commits February 2, 2021 09:44

Merge remote-tracking branch 'upstream/main' into brotli-v2

4ffacc6

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

fix current.rst after resolving merge conflict

e6fb711

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

Merge remote-tracking branch 'upstream/main' into brotli-v2

2101920

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

htuch reviewed Feb 5, 2021

View reviewed changes

repokitteh-read-only bot removed the deps Approval required for changes to Envoy's external dependencies label Feb 5, 2021

lizan approved these changes Feb 8, 2021

View reviewed changes

lizan merged commit 127aa55 into envoyproxy:main Feb 8, 2021

moderation mentioned this pull request Feb 8, 2021

brotoli: fix CI by adding category field to bazel BUILD files #14987

Merged

rainingmaster mentioned this pull request Mar 14, 2022

compression: add zstd compressor and decompressor rainingmaster/envoy#1

Closed

rainingmaster mentioned this pull request Mar 21, 2022

compression: add zstd compressor and decompressor #20434

Merged

This was referenced Feb 27, 2026

🌱 CNCF mission generation 2026-02-27 kubestellar/console-kb#6

Closed

🌱 CNCF mission generation 2026-02-27 kubestellar/console-kb#10

Merged

	// Value for encoder's next output buffer. If not set, defaults to 4096.
	// Value for decoder's next output buffer. If not set, defaults to 4096.

Conversation

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

Uh oh!

Uh oh!

repokitteh-read-only bot commented Sep 7, 2020

Uh oh!

rojkov commented Sep 9, 2020

Uh oh!

repokitteh-read-only bot commented Sep 9, 2020

Uh oh!

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

Uh oh!

Uh oh!

junr03 commented Sep 11, 2020

Uh oh!

junr03 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

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!

junr03 commented Sep 13, 2020

Uh oh!

htuch left a comment

rojkov commented Sep 7, 2020 •

edited

Loading

moderation commented Feb 8, 2021 •

edited

Loading