Skip to content
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

Skip http method matching in http_parser with UHV #36245

Closed
wants to merge 4 commits into from

Conversation

vamshi177
Copy link

@vamshi177 vamshi177 commented Sep 20, 2024

Commit Message: Skip http method matching in http_parser with UHV
Additional Description: When the UHV compile-time flag is enabled, strict HTTP parsing is disabled by default. However, there is currently no support for custom methods in http_parser.

There is an option available to enable runtime http_protocol_options.allow_custom_methods feature that allows custom methods in BalsaParser, which is the current default HTTP/1 codec. But the http_inspector still uses http_parser which currently doesn't support custom methods.

As a result, even with the allow_custom_methods runtime feature enabled, it doesn't fully resolve the issue, as calls are still rejected by the client-side Envoy at the http_inspector stage. This change introduces support for custom HTTP methods in http_parser using the existing HTTP_PARSER_STRICT compile-time flag.

Risk Level: Low
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

Signed-off-by: G Vamshi Krishna Reddy <[email protected]>
Copy link

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.

🐱

Caused by: #36245 was opened by vamshi177.

see: more, trace.

@vamshi177 vamshi177 marked this pull request as draft September 20, 2024 07:17
@vamshi177 vamshi177 changed the title Skip strict http method matching Skip strict http method matching when HTTP_PARSER_STRICT is disabled Sep 23, 2024
@vamshi177 vamshi177 changed the title Skip strict http method matching when HTTP_PARSER_STRICT is disabled Skip http method matching when http_parser_strict is disabled Sep 23, 2024
@vamshi177 vamshi177 marked this pull request as ready for review September 23, 2024 15:43
Signed-off-by: G Vamshi Krishna Reddy <[email protected]>
@vamshi177 vamshi177 changed the title Skip http method matching when http_parser_strict is disabled Skip http method matching in http_parser with UHV Sep 24, 2024
@Pawan-Bishnoi
Copy link
Contributor

/retest failed

@vamshi177
Copy link
Author

/retest

@bencebeky
Copy link
Contributor

Hi, just a quick heads up that I do not work on Envoy any more. Consider contacting @yanavlasov for further assistance with custom methods.

@wbpcode
Copy link
Member

wbpcode commented Oct 1, 2024

Hi, thanks for trying to resolve this problem. But could we just switch the parser in the http_inspector to our new parser and add an option to the http_inspector to enable the custom methods support?

Considering we finally may remove the legacy parser, I think updating the http_inspector would be better than to hacking the legacy http_parser.c.

Thanks again for you contribution. :)

/wait

@ramaraochavali
Copy link
Contributor

Hi, thanks for trying to resolve this problem. But could we just switch the parser in the http_inspector to our new parser and add an option to the http_inspector to enable the custom methods support?

So you are suggesting to switch the parser in http_inspector and add a new [#not-implemented-hide:] flag like allow_custom_method similar to what we for HCM?

@wbpcode
Copy link
Member

wbpcode commented Oct 1, 2024

Hi, thanks for trying to resolve this problem. But could we just switch the parser in the http_inspector to our new parser and add an option to the http_inspector to enable the custom methods support?

So you are suggesting to switch the parser in http_inspector and add a new [#not-implemented-hide:] flag like allow_custom_method similar to what we for HCM?

Not sure if the #not-implemented-hide annotation is necessary or not. Because we basically won't provide another UHV in the http_inspector.

@vamshi177
Copy link
Author

Hi, thanks for trying to resolve this problem. But could we just switch the parser in the http_inspector to our new parser and add an option to the http_inspector to enable the custom methods support?

Regarding updating the parser in the http_inspector, I'm planning to do this in two steps.

  1. Update the http_parser usage with the Parser interface and use the LegacyHttpParserImpl implementation for the current http_parser behaviour.
  2. Introduce a new runtime flag that will switch the usage to the BalsaParser.

Does this approach seeem fine?

@wbpcode
Copy link
Member

wbpcode commented Oct 1, 2024

Hi, thanks for trying to resolve this problem. But could we just switch the parser in the http_inspector to our new parser and add an option to the http_inspector to enable the custom methods support?

Regarding updating the parser in the http_inspector, I'm planning to do this in two steps.

  1. Update the http_parser usage with the Parser interface and use the LegacyHttpParserImpl implementation for the current http_parser behaviour.
  2. Introduce a new runtime flag that will switch the usage to the BalsaParser.

Does this approach seeem fine?

SGTM.

@vamshi177
Copy link
Author

Created a new issue #36433 to update the parser in http_inspector.

@vamshi177
Copy link
Author

@wbpcode created a new PR #36430 to update the parser code in the http_inspector. Can you review the changes?
Also, closing the current PR for updating the legacy http_parser.

@vamshi177 vamshi177 closed this Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants