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

Adding an HTTP/2 integration fuzzer #10321

Merged
merged 20 commits into from
Jun 7, 2020

Conversation

adisuissa
Copy link
Contributor

Adding an HTTP/2 integration fuzzer that checks different kind of frames handling in both Downstream and Upstream

Signed-off-by: Adi Suissa-Peleg [email protected]

Description: A new fuzzer to test the HTTP/2.0 integration. At the moment it is slow, and should be improved.
Risk Level: Low - a new test
Testing: It is a new fuzz test

…mes handling in both Downstream and Upstream

Signed-off-by: Adi Suissa-Peleg <[email protected]>
@adisuissa
Copy link
Contributor Author

cc @asraa @htuch

Copy link
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.

Thanks for adding this @adisuissa, it's great to have e2e coverage of H2 happening. A few comments below, I think mostly if we can get a future proof data model (see enum vs. oneof) then we can grow this even further over time.
/wait

test/integration/h2_capture_fuzz.proto Outdated Show resolved Hide resolved
test/integration/h2_capture_fuzz.proto Outdated Show resolved Hide resolved
test/integration/h2_fuzz.h Outdated Show resolved Hide resolved
test/integration/h2_fuzz.h Outdated Show resolved Hide resolved
test/integration/h2_fuzz.h Outdated Show resolved Hide resolved
test/integration/h2_fuzz.cc Outdated Show resolved Hide resolved
Copy link
Contributor

@asraa asraa left a comment

Choose a reason for hiding this comment

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

This is great :) Thank you for taking this on!

test/integration/h2_capture_fuzz.proto Outdated Show resolved Hide resolved
test/integration/h2_fuzz.cc Outdated Show resolved Hide resolved
@stale
Copy link

stale bot commented Mar 18, 2020

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot added the stale stalebot believes this issue/PR has not been touched recently label Mar 18, 2020
… for each type of frame in the proto file, and adding their possible inner fields

Signed-off-by: Adi Suissa-Peleg <[email protected]>
@stale stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Mar 25, 2020
Copy link
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.

Looks good, just a few more comments, I think this is ready to ship once we figure out the timeout issue.
/wait

test/integration/h2_capture_fuzz.proto Outdated Show resolved Hide resolved
test/integration/h2_capture_fuzz_test.cc Show resolved Hide resolved
test/integration/h2_fuzz.cc Outdated Show resolved Hide resolved
test/integration/h2_fuzz.h Outdated Show resolved Hide resolved
test/integration/h2_fuzz.cc Outdated Show resolved Hide resolved
test/integration/h2_fuzz.h Outdated Show resolved Hide resolved
test/integration/h2_capture_fuzz.proto Outdated Show resolved Hide resolved
test/integration/h2_capture_fuzz.proto Outdated Show resolved Hide resolved
@stale
Copy link

stale bot commented Apr 2, 2020

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot added the stale stalebot believes this issue/PR has not been touched recently label Apr 2, 2020
@stale
Copy link

stale bot commented Apr 9, 2020

This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot closed this Apr 9, 2020
@htuch htuch reopened this May 18, 2020
@stale stale bot removed the stale stalebot believes this issue/PR has not been touched recently label May 18, 2020
Copy link
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.

Looks good, mostly style nits.
/wait

test/integration/h2_capture_fuzz.proto Outdated Show resolved Hide resolved
test/integration/h2_capture_fuzz.proto Outdated Show resolved Hide resolved
test/common/http/http2/http2_frame.cc Outdated Show resolved Hide resolved
test/integration/h2_fuzz.cc Outdated Show resolved Hide resolved
test/integration/h2_fuzz.cc Outdated Show resolved Hide resolved
test/integration/h2_fuzz.cc Outdated Show resolved Hide resolved
test/integration/h2_fuzz.h Outdated Show resolved Hide resolved
message H2TestFrame {
// These values map to the frame creation methods in:
// test/common/http/http2/http2_frame.h
oneof frame_type {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is awesome, I'm excited to see metadata frames to this later. @AndresGuedez

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Haven't found a METADATA frame in the RFC, but would be glad to add it if there's a spec.

@adisuissa
Copy link
Contributor Author

@htuch Please take a look, and let me know if there are other things that should be added/modified

Copy link
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 modulo a few final minor comments.
/wait

test/common/http/http2/http2_frame.cc Outdated Show resolved Hide resolved
test/common/http/http2/http2_frame.cc Outdated Show resolved Hide resolved
// an additional zero length header (making this a malformed request)
message H2FrameMalformedRequestWithZerolenHeader {
uint32 stream_index = 1;
bytes host = 2;
Copy link
Member

Choose a reason for hiding this comment

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

How come these are bytes and not string?

test/integration/h2_fuzz.cc Outdated Show resolved Hide resolved
test/integration/h2_fuzz.cc Outdated Show resolved Hide resolved
test/integration/h2_fuzz.cc Outdated Show resolved Hide resolved
@adisuissa
Copy link
Contributor Author

@htuch I've addressed the latest comments, please take a look. Thanks!

Copy link
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, thanks!

@htuch htuch merged commit 5dfa0f5 into envoyproxy:master Jun 7, 2020
songhu pushed a commit to songhu/envoy that referenced this pull request Jun 25, 2020
Adding an HTTP/2 integration fuzzer that checks different kind of frames handling in both Downstream and Upstream

Risk Level: Low - a new test
Testing: It is a new fuzz test

Signed-off-by: Adi Suissa-Peleg <[email protected]>
yashwant121 pushed a commit to yashwant121/envoy that referenced this pull request Jul 24, 2020
Adding an HTTP/2 integration fuzzer that checks different kind of frames handling in both Downstream and Upstream

Risk Level: Low - a new test
Testing: It is a new fuzz test

Signed-off-by: Adi Suissa-Peleg <[email protected]>
Signed-off-by: yashwant121 <[email protected]>
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.

3 participants