Skip to content

http/2: use hpack_table_size to control both encoder and decoder.#3659

Merged
mattklein123 merged 4 commits intoenvoyproxy:masterfrom
PiotrSikora:http2_table_size
Jul 19, 2018
Merged

http/2: use hpack_table_size to control both encoder and decoder.#3659
mattklein123 merged 4 commits intoenvoyproxy:masterfrom
PiotrSikora:http2_table_size

Conversation

@PiotrSikora
Copy link
Contributor

@PiotrSikora PiotrSikora commented Jun 19, 2018

Previously, hpack_table_size was used to configure maximum table size used by
the local endpoint for HPACK decoding, however, there was no way to configure
table size used for HPACK enoding.

Since this option is mostly used to disable header compression by setting the
size to 0, it means that Envoy only asked the remote endpoint not to compress
headers, but it was still compressing them itself (unless asked not to by the
remote endpoint).

Re-using hpack_table_size instead of adding a new option, since both: encoder
and decoder will usually use the same value anyway.

Level: Medium (some broken libraries don't support header table updates)
Testing: bazel test //test/...
Docs Changes: n/a
Release Notes: Added

Signed-off-by: Piotr Sikora piotrsikora@google.com

Previously, hpack_table_size was used to configure maximum table size used by
the local endpoint for HPACK decoding, however, there was no way to configure
table size used for HPACK enoding.

Since this option is mostly used to disable header compression by setting the
size to 0, it means that Envoy only asked the remote endpoint not to compress
headers, but it was still compressing them itself (unless asked not to by the
remote endpoint).

Re-using hpack_table_size instead of adding a new option, since both: encoder
and decoder will usually use the same value anyway.

*Level*: Medium (some broken libraries don't support header table updates)
*Testing*: bazel test //test/...
*Docs Changes*: n/a
*Release Notes*: n/a

Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora
Copy link
Contributor Author

@mattklein123 mattklein123 self-assigned this Jun 19, 2018
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems reasonable to me. @alyssawilk any objections?

@alyssawilk
Copy link
Contributor

SGTM. Also LGTM if there's a release note added (I think it's a worthwhile change to note)

@stale
Copy link

stale bot commented Jun 26, 2018

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!

@stale stale bot added the stale stalebot believes this issue/PR has not been touched recently label Jun 26, 2018
@stale
Copy link

stale bot commented Jul 3, 2018

This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot closed this Jul 3, 2018
@lizan lizan reopened this Jul 13, 2018
@stale stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Jul 13, 2018
@mattklein123
Copy link
Member

@PiotrSikora can you add the release note per @alyssawilk and then we can merge this?

While there, fix typo and ordering of others.

Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora
Copy link
Contributor Author

@mattklein123 Added.

@mattklein123
Copy link
Member

@PiotrSikora sorry needs another master merge for your other commit.

mattklein123
mattklein123 previously approved these changes Jul 17, 2018
…o_compression

Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@mattklein123 mattklein123 merged commit a8fa0c6 into envoyproxy:master Jul 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants