Skip to content

deps: integrate and use local copy of http-parser library#19786

Merged
phlax merged 4 commits intoenvoyproxy:mainfrom
trail-of-forks:local-http-parser
Feb 11, 2022
Merged

deps: integrate and use local copy of http-parser library#19786
phlax merged 4 commits intoenvoyproxy:mainfrom
trail-of-forks:local-http-parser

Conversation

@ameily
Copy link
Copy Markdown
Contributor

@ameily ameily commented Feb 2, 2022

Commit Message: deps: integrate and use local copy of http-parser library

Additional Description: As part of the unified header validation component, Envoy will need to eventually modify the http-parser behavior based on the active configuration (see #19750 and #19757). This PR covers the first step of that goal by bringing http-parser into the Envoy repository and get it building with Bazel. This PR contains the http-parser code, without any modifications, from the commit nodejs/http-parser@4f15b7d.

Risk Level: Low

Testing: Build Envoy and run the HTTP protocol integration tests.

Docs Changes: None

Release Notes: None

Platform Specific Features: None

Fixes #19749

Signed-off-by: Adam Meily <adam.meily@trailofbits.com>
Signed-off-by: Adam Meily <adam.meily@trailofbits.com>
@repokitteh-read-only repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Feb 2, 2022
@repokitteh-read-only
Copy link
Copy Markdown

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).
envoyproxy/dependency-shepherds assignee is @phlax

🐱

Caused by: #19786 was opened by ameily.

see: more, trace.

@ameily
Copy link
Copy Markdown
Contributor Author

ameily commented Feb 2, 2022

For context, we are supporting the unified header validation component and have been coordinating our work with @yanavlasov

@phlax
Copy link
Copy Markdown
Member

phlax commented Feb 2, 2022

just from pov of cve noise from nodejs this would be good

@ameily you need to do some format fixing - diff is available in the artefacts here https://dev.azure.com/cncf/envoy/_build/results?buildId=100606&view=results

cc @htuch for approval on including this in Envoy code base

Signed-off-by: Adam Meily <adam.meily@trailofbits.com>
@ameily
Copy link
Copy Markdown
Contributor Author

ameily commented Feb 2, 2022

you need to do some format fixing - diff is available in the artefacts here https://dev.azure.com/cncf/envoy/_build/results?buildId=100606&view=results

Sorry about that, I apparently didn't have my virtualenv setup properly so the test wasn't running locally. It should be fixed now.

@htuch
Copy link
Copy Markdown
Member

htuch commented Feb 3, 2022

I'm on the fence on this one. I think we decided not to fork http-parser and we're pursuing a strategy of moving to a QUICHE-based parser. @yanavlasov do you have an ETA for when we will have a replacement parser? I'd feel more comfortable if we could say "this is just for 4 months and then we will switch to default QUICHE.

@yanavlasov yanavlasov self-assigned this Feb 9, 2022
@yanavlasov
Copy link
Copy Markdown
Contributor

@htuch balsa integration into Envoy will not start until H2. We will need to make some changes to http-parser in the interim to move forward with the unified header validation. Hence this fork. This is temporary until http-parser can be ditched for balsa.

@htuch
Copy link
Copy Markdown
Member

htuch commented Feb 10, 2022

Ack, no objection then.

Copy link
Copy Markdown
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

/lgtm deps

@repokitteh-read-only repokitteh-read-only bot removed the deps Approval required for changes to Envoy's external dependencies label Feb 10, 2022
@phlax
Copy link
Copy Markdown
Member

phlax commented Feb 10, 2022

@ameily
Copy link
Copy Markdown
Contributor Author

ameily commented Feb 10, 2022

@phlax This is third-party code so I added it to the exclusion list in check_format.py. Should I revert that and then fix the format/tidy findings? My initial assumption was that this is non-Envoy code that we know works within Envoy so don't change it.

@phlax
Copy link
Copy Markdown
Member

phlax commented Feb 10, 2022

My initial assumption was that this is non-Envoy code that we know works within Envoy so don't change it.

yep, agreed

Signed-off-by: Adam Meily <adam.meily@trailofbits.com>
Copy link
Copy Markdown
Member

@phlax phlax left a comment

Choose a reason for hiding this comment

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

lgtm, thanks @ameily

@phlax phlax enabled auto-merge (squash) February 11, 2022 13:52
@phlax phlax merged commit d3ecdce into envoyproxy:main Feb 11, 2022
joshperry pushed a commit to joshperry/envoy that referenced this pull request Feb 13, 2022
…#19786)

* integrate and use local copy of http-parser; refs envoyproxy#19749
* ignore third party dependency: http-parser
* exclude http-parser third-party code from clang-tidy

Signed-off-by: Adam Meily <adam.meily@trailofbits.com>
Signed-off-by: Josh Perry <josh.perry@mx.com>
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.

uhv: move http-parser to envoy

4 participants