Add rudimentary fuzz test for service control filter #55
Conversation
Notes: - Fuzz the entire service control filter config and upstream/downstream/sidestream requests. - Add some initial valid values to the corpus to kick-start OSS-Fuzz. After running locally for 5 minutes, 66% of lines are covered. - This test is very rudimentary and has some items that can be improved (see inline TODOs and FIXMEs). Currently, this is very slow when running locally (4 exec/s). Signed-off-by: Teju Nareddy <[email protected]>
Signed-off-by: Teju Nareddy <[email protected]>
| TestStreamInfo stream_info = | ||
| Envoy::Fuzz::fromStreamInfo(input.stream_info()); | ||
| EXPECT_CALL(mock_decoder_callbacks, streamInfo()) | ||
| .WillRepeatedly(ReturnRef(stream_info)); |
There was a problem hiding this comment.
Instead of setting expectation (slow-down), can you set mock_decoder_callbacks.stream_info_ = stream_info, the expectation to return stream_info_ is already made in initialization. Does this speed-up?
There was a problem hiding this comment.
Unfortunately, this didn't work due to mocks vs fakes.
src/envoy/http/service_control/filter_fuzz_test.cc:182:41: error: no match for 'operator=' (operand types are 'testing::NiceMock<Envoy::StreamInfo::MockStreamInfo>' and 'Envoy::TestStreamInfo')
182 | mock_decoder_callbacks.stream_info_ = stream_info;
There was a problem hiding this comment.
Ahhhh. Gotcha. Didn't realize stream_info_ was a mock. Boo.
I think this is fine as is as a start, and you can iterat for speed ups. I think it'll be possible to make some of the mocks and expectations static and updating/cleaning up per-run, but it's a good task to iterate on.
There was a problem hiding this comment.
Good point, could you share an example of this in Envoy? I'm not familiar with how this will speed up execution.
Yup, I can iterate on this next week. I'd like to get this merged first so OSS-Fuzz will pick it up and display some initial fuzz stats.
There was a problem hiding this comment.
As discussed offline: envoyproxy/envoy#8397
Signed-off-by: Teju Nareddy <[email protected]>
Signed-off-by: Teju Nareddy <[email protected]>
| !input.downstream_request().has_trailers()) { | ||
| end_stream = true; | ||
| } | ||
| filter.decodeHeaders(downstream_headers, end_stream); |
There was a problem hiding this comment.
And then similarly, is there any reason to consider the filter status in case there's a reason to stop decoding?
There was a problem hiding this comment.
Not for this filter, we only ever StopIteration or Continue, and that will probably never change. Our filter should support all 3 being called.
There was a problem hiding this comment.
why do we need to make sure end_stream is correct? Sometime, due to bug, end_stream may not called correctly. we still need to fuzz these data flow.
There was a problem hiding this comment.
Are you saying it's possible to execute decodeHeaders(_, true) and then decodeData(_, _) somewhere?
There was a problem hiding this comment.
@qiwzhang I think it's reasonable to expect end_stream is set correctly, this is part of the Envoy filter API. If it's set incorrectly, some upstream filters would not function. I don't think this will ever happen, but we can fuzz it if you believe it can.
Signed-off-by: Teju Nareddy <[email protected]>
|
@asraa: changing LGTM is restricted to collaborators In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: asraa, nareddyt, qiwzhang The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: Teju Nareddy [email protected]