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.

Sign up for GitHub

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

Jump to bottom

[balsa] Validate HTTP version string. #24759

Merged
merged 16 commits into from
Jan 26, 2023
Merged

[balsa] Validate HTTP version string. #24759

merged 16 commits into from
Jan 26, 2023

Conversation

bencebeky
Copy link
Contributor

Tracking issue: #21245

Signed-off-by: Bence Béky bnc@google.com

Commit Message: [balsa] Validate HTTP version string.
Additional Description:
Risk Level: low, changed code protected by existing default-false runtime flag.
Testing: //test/common/http/http1:codec_impl_test //test/integration:integration_test
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a
Runtime guard: envoy.reloadable_features.http1_use_balsa_parser

@bnc-google
[balsa] Validate HTTP version string.
0b287e8 
Signed-off-by: Bence Béky <bnc@google.com>
@repokitteh-read-only
Copy link

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #24759 was opened by bencebeky.

see: more, trace.

@bencebeky
Copy link
Contributor Author

/assign @birenroy

@bencebeky
Copy link
Contributor Author

/assign @diannahu

@bencebeky bencebeky closed this Jan 9, 2023
@bencebeky bencebeky reopened this Jan 9, 2023
diannahu
diannahu previously approved these changes Jan 9, 2023
View reviewed changes
Copy link
Contributor

@diannahu diannahu left a comment

Choose a reason for hiding this comment

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

This version is great!

source/common/http/http1/balsa_parser.cc Outdated Show resolved Hide resolved
test/common/http/http1/codec_impl_test.cc Show resolved Hide resolved
source/common/http/http1/balsa_parser.cc Outdated Show resolved Hide resolved
birenroy
birenroy previously approved these changes Jan 10, 2023
View reviewed changes
Copy link
Contributor

@birenroy birenroy left a comment

Choose a reason for hiding this comment

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

Just a couple of minor comments.

source/common/http/http1/balsa_parser.cc Outdated Show resolved Hide resolved
test/common/http/http1/codec_impl_test.cc Show resolved Hide resolved
@bnc-google
Update RFC reference.
18acaad 
Signed-off-by: Bence Béky <bnc@google.com>
@bnc-google
Only compile regex once.
582c210 
Signed-off-by: Bence Béky <bnc@google.com>
@bnc-google
Parametrize tests, add more test cases.
3b2f5a1 
Signed-off-by: Bence Béky <bnc@google.com>
@bnc-google
Merge remote-tracking branch 'upstream/main' into version
c28cc34 
Signed-off-by: Bence Béky <bnc@google.com>
@bencebeky bencebeky dismissed stale reviews from birenroy and diannahu via 3b2f5a1 January 18, 2023 13:20
@bnc-google
Add SPELLCHECKER annotations around HHTTP.
6742aea 
Signed-off-by: Bence Béky <bnc@google.com>
@bnc-google
clang-format
657eb01 
Signed-off-by: Bence Béky <bnc@google.com>
diannahu
diannahu previously approved these changes Jan 18, 2023
View reviewed changes
@bnc-google
Fix universal header validation case.
dc62dec 
When universal header validation is enabled, HTTP_PARSER_STRICT is
defined to be 0, and http-parser gives different error messages.

Signed-off-by: Bence Béky <bnc@google.com>
@bencebeky
Copy link
Contributor Author

Kevin: PTAL.
/assign @KBaichoo

Dianna: thanks for the review.

@bencebeky bencebeky marked this pull request as ready for review January 18, 2023 21:42
KBaichoo
KBaichoo reviewed Jan 20, 2023
View reviewed changes
Copy link
Contributor

@KBaichoo KBaichoo left a comment

Choose a reason for hiding this comment

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

Looks great otherwise! Thank you for adding this.

test/common/http/http1/codec_impl_test.cc Outdated Show resolved Hide resolved
source/common/http/http1/balsa_parser.cc Show resolved Hide resolved
test/common/http/http1/codec_impl_test.cc Outdated Show resolved Hide resolved
@KBaichoo
Copy link
Contributor

/wait

@bnc-google
Add 1.0 and 1.1 test cases.
7d3f0b2 
Signed-off-by: Bence Béky <bnc@google.com>
@bnc-google
Add _ suffix to struct member names.
9a7d7f2 
Signed-off-by: Bence Béky <bnc@google.com>
@bnc-google
Move struct definition to anonymous namespace and test case definitio…
00fa987 
…ns to tests.

Signed-off-by: Bence Béky <bnc@google.com>
@bnc-google
Merge remote-tracking branch 'upstream/main' into version
93958a0 
Signed-off-by: Bence Béky <bnc@google.com>
KBaichoo
KBaichoo previously approved these changes Jan 23, 2023
View reviewed changes
Copy link
Contributor

@KBaichoo KBaichoo left a comment

Choose a reason for hiding this comment

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

Thank you! LGTM

@KBaichoo KBaichoo enabled auto-merge (squash) January 23, 2023 16:08
@bnc-google
Merge remote-tracking branch 'upstream/main' into version
93c4d83 
Signed-off-by: Bence Béky <bnc@google.com>
@bnc-google
Re-add accidentally removed SPELLCHECKER.
444a58e 
Signed-off-by: Bence Béky <bnc@google.com>
auto-merge was automatically disabled January 24, 2023 18:38

Head branch was pushed to by a user without write access

@bencebeky
Copy link
Contributor Author

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #24759 (comment) was created by @bencebeky.

see: more, trace.

@KBaichoo
Copy link
Contributor

/wait

Coverage failer:
Code coverage for source/common/api is lower than limit of 82.5 (82.4)

/coverage

@repokitteh-read-only
Copy link

Coverage for this Pull Request will be rendered here:

https://storage.googleapis.com/envoy-pr/24759/coverage/index.html

The coverage results are (re-)rendered each time the CI envoy-presubmit (check linux_x64 coverage) job completes.

🐱

Caused by: a #24759 (comment) was created by @KBaichoo.

see: more, trace.

@KBaichoo
Copy link
Contributor

actually just saw: #25122 (comment) since this doesn't touch this, might just need a merge from main. Sorry about that.

@bnc-google
Merge remote-tracking branch 'upstream/main' into version
39c8648 
Signed-off-by: Bence Béky <bnc@google.com>
@bnc-google
Merge remote-tracking branch 'upstream/main' into version
a7b27b2 
Signed-off-by: Bence Béky <bnc@google.com>
@bencebeky
Copy link
Contributor Author

Tests failed due to an issue that was fixed at #25169. Merging to retry.

@KBaichoo KBaichoo enabled auto-merge (squash) January 26, 2023 18:11
@KBaichoo KBaichoo merged commit ac91620 into envoyproxy:main Jan 26, 2023
@bencebeky bencebeky deleted the version branch January 27, 2023 16:33
VishalDamgude pushed a commit to freshworks/envoy that referenced this pull request Feb 2, 2023
@bencebeky @VishalDamgude
[balsa] Validate HTTP version string. (envoyproxy#24759)
a68a3b4 
* [balsa] Validate HTTP version string.

Signed-off-by: Bence Béky <bnc@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KBaichoo KBaichoo KBaichoo approved these changes

@diannahu diannahu diannahu left review comments

@birenroy birenroy birenroy left review comments

Assignees

@KBaichoo KBaichoo

@diannahu diannahu

@birenroy birenroy

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@bencebeky @KBaichoo @diannahu @birenroy @bnc-google