-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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. |
b479de8
to
c531e89
Compare
Signed-off-by: G Vamshi Krishna Reddy <vamshikrishna.gunreddy@gmail.com>
c531e89
to
56a9981
Compare
Signed-off-by: G Vamshi Krishna Reddy <vamshikrishna.gunreddy@gmail.com>
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that necessary to always do this heap allocation if this is an NoOp callabcks instance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: add a '.' at the end
2804933
to
16173e3
Compare
Signed-off-by: G Vamshi Krishna Reddy <vamshikrishna.gunreddy@gmail.com>
16173e3
to
d50fab5
Compare
<!-- !!!ATTENTION!!! If you are fixing *any* crash or *any* potential security issue, *do not* open a pull request in this repo. Please report the issue via emailing envoy-security@googlegroups.com where the issue will be triaged appropriately. Thank you in advance for helping to keep Envoy secure. !!!ATTENTION!!! For an explanation of how to fill out the fields, please see the relevant section in [PULL_REQUESTS.md](https://github.com/envoyproxy/envoy/blob/main/PULL_REQUESTS.md) --> 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 `LegacyHttpParserImpl` as the first step in transitioning to the Balsa parser (part of envoyproxy#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](https://github.com/envoyproxy/envoy/blob/main/api/review_checklist.md):] --------- Signed-off-by: G Vamshi Krishna Reddy <vamshikrishna.gunreddy@gmail.com> Signed-off-by: Gustavo <grnmeira@gmail.com>
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
LegacyHttpParserImpl
as 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:]