refactor http parser implementation in http_inspector #36430
Conversation
|
Hi @vamshi177, 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. |
vamshi177
force-pushed
the
update-http-inspector
branch
from
b479de8
to
c531e89
Compare
Signed-off-by: G Vamshi Krishna Reddy <vamshikrishna.gunreddy@gmail.com>
vamshi177
force-pushed
the
update-http-inspector
branch
from
c531e89
to
56a9981
Compare
vamshi177
changed the title
vamshi177
changed the title
vamshi177
changed the title
Signed-off-by: G Vamshi Krishna Reddy <vamshikrishna.gunreddy@gmail.com>
vamshi177
force-pushed
the
update-http-inspector
branch
from
8c80588
to
29075aa
Compare
|
@yanavlasov @wbpcode Could you provide feedback on this change? Recently, we encountered a use case that required support for custom methods. However, using To allow custom methods in http_inspector, as a first step, I am refactoring the parser code in http_inspector to use the Parser interface with |
|
/wait on CI |
|
/retest |
|
FWIW we had a permafail on main so this will almost certainly need a main merge now |
Thanks for the info, will merge the changes from main now. |
Signed-off-by: G Vamshi Krishna Reddy <vamshikrishna.gunreddy@gmail.com>
Signed-off-by: G Vamshi Krishna Reddy <vamshikrishna.gunreddy@gmail.com>
Hi, please check the coverage issue of CI. Thanks. The code LGTM, and thanks for this contribution! /wait |
| @@ -24,13 +24,12 @@ Config::Config(Stats::Scope& scope) | |||
| const absl::string_view Filter::HTTP2_CONNECTION_PREFACE = "PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n"; | |||
|
|
|||
| Filter::Filter(const ConfigSharedPtr config) : config_(config) { | |||
| http_parser_init(&parser_, HTTP_REQUEST); | |||
| // Filter for only Request Message types with NoOp callbacks | |||
| no_op_callbacks_ = std::make_unique<NoOpParserCallbacks>(); | |||
There was a problem hiding this comment.
Is that necessary to always do this heap allocation if this is an NoOp callabcks instance?
There was a problem hiding this comment.
yes, As this is a NoOp callback instance, there is no need for heap allocation.
updated the PR.
Signed-off-by: G Vamshi Krishna Reddy <vamshikrishna.gunreddy@gmail.com>
working on adding more tests to increase coverage for http_inspector. will update the PR shortly. |
|
@vamshi177 Thanks for your hard working. But we recently will cut a major release version, we could land the PR after that release. So, take it easy, it have enough time. /wait |
| Filter::Filter(const ConfigSharedPtr config) : config_(config) { | ||
| http_parser_init(&parser_, HTTP_REQUEST); | ||
| Filter::Filter(const ConfigSharedPtr config) : config_(config), no_op_callbacks_() { | ||
| // Filter for only Request Message types with NoOp callbacks |
There was a problem hiding this comment.
nit: add a '.' at the end
Commit Message: refactor http parser implementation in http_inspector
Additional Description:
This update replaces the usage of the legacy http_parser in http_inspector with the
LegacyHttpParserImplas the first step in transitioning to the Balsa parser (part of #36433). The Parser interface is used with LegacyHttpParserImpl to preserve the current behaviour of http_parser.Risk Level: Low (no behavioural change)
Testing: All the http_inspector tests have passed (
bazel test //test/extensions/filters/listener/http_inspector/...)Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]