http connection manager proto: raise max_request_headers_kb limit to 8192 KiB (8MiB) from 96 KiB#15921
Conversation
… KiB in http connection manager proto. Add/Update relevant unit tests and integration tests. Signed-off-by: Anirudha Singh <ps.anirudha@gmail.com>
|
Hi @anirudhaps, welcome and thank you for your contribution. We will try to review your Pull Request as quickly as possible. In the meantime, please take a look at the contribution guidelines if you have not done so already. |
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
|
cc @auni53 can you remind me what the implementation constraints were? I thought one of the parsers didn't support larger headers so maybe it has been addressed? |
…aders case. Signed-off-by: Anirudha Singh <ps.anirudha@gmail.com>
I thought there was a limitation in the H1 parser but e2e tests do suggest that this works e2e for H1 and H2. |
alyssawilk
left a comment
alyssawilk
left a comment
There was a problem hiding this comment.
Looks good - just one (maybe two pending CI) nits :-)
api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto
Show resolved
Hide resolved
|
Will this work with llhttp for H1 parser? CC @asraa |
…aders rejected test case. Add release notes for these changes. Signed-off-by: Anirudha Singh <ps.anirudha@gmail.com>
Signed-off-by: Anirudha Singh <ps.anirudha@gmail.com>
alyssawilk
left a comment
alyssawilk
left a comment
There was a problem hiding this comment.
LGTM pending happy CI, but I'm going to add @asraa per Harvey's point about the other library
Yes! Should be fine. We handle it in the codec code, and the parser knows that nodejs/llhttp#2 |
…eaders_kb_limit Signed-off-by: Anirudha Singh <ps.anirudha@gmail.com>
…atest upstream main Signed-off-by: Anirudha Singh <ps.anirudha@gmail.com>
|
Updated docs/root/version_history/current.rst as per latest main to avoid any cause of merge conflict |
alyssawilk
left a comment
alyssawilk
left a comment
There was a problem hiding this comment.
@htuch looks like this needs another API stamp, though I assume the v2 changes be reverted for that?
|
Sorry to comment late, yes when I was trying to raise the limits the barrier was in the HTTP1 codec library that we use. |
…iB (envoyproxy#15921) Commit Message: Raise max configurable max_request_headers_kb limit to 8192 KiB (8MiB) from 96 KiB in http connection manager protobuf. Additional Description: Added/Updated relevant unit tests and integration tests. This change will allow increasing max_request_headers_kb to 8MiB from http connection manager's configuration. The change is mainly updating the limit in a validation check in the protobuf. Also, the old (merged) PR envoyproxy#5859 is read, no nghttp2 library-related issues are observed on raising the max_request_headers_kb beyond 96 KiB. Risk Level: Low Testing: Unit, Integration, Manual Docs Changes: Inline in proto file. Signed-off-by: Anirudha Singh <ps.anirudha@gmail.com> Signed-off-by: Gokul Nair <gnair@twitter.com>
6 participants
Signed-off-by: Anirudha Singh ps.anirudha@gmail.com
Commit Message:
Raise max configurable
max_request_headers_kblimit to 8192 KiB (8MiB) from 96 KiB in http connection manager protobuf.Additional Description:
Added/Updated relevant unit tests and integration tests. This change will allow increasing
max_request_headers_kbto 8MiB from http connection manager's configuration. The change is mainly updating the limit in a validation check in the protobuf. Also, the old (merged) PR #5859 is read, no nghttp2 library-related issues are observed on raising themax_request_headers_kbbeyond 96 KiB.Risk Level: Low
Testing: Unit, Integration, Manual
Docs Changes: Inline in proto file.