[fuzz] split http filter logic into a fuzzing class

[asraa]

Signed-off-by: Asra Ali asraa@google.com

Commit Message: This splits out the "filter manager" logic that the HTTP filter fuzzer uses into a lightweight HttpFilterFuzzer class. The logic implemented control the flow for calling decode(Headers/Data/Trailers) and encode(Headers/Data/Trailers). Users of this class now create a filter, set callbacks, and then call HttpFilterFuzzer::runData(filter, fuzzed_data). See example (https://github.com/GoogleCloudPlatform/esp-v2/blob/e86330b106484eb3734dc207a3ab9419f9dcb903/src/envoy/http/path_matcher/filter_fuzz_test.cc#L59). It could be done before, but UberFilterFuzzer held much more state than necessary to apply to a dedicated fuzzer.

The UberFilterFuzzer extends this and holds the logic to generically create and set up filters. It uses this HttpFilterFuzzer library for the headers, data, trailer logic calls. Now all state needed for all filters is held in UberFilterFuzzer.

Risk Level: Low
Testing: filter_fuzz_test still works, this is a no-op

Follow-up PRs:

• [docs/test] Add test/extensions/filters/http/common/fuzz/README.md for devs and build out the corpus for the generic fuzzer
• [test] Apply the library to ext_authz for a dedicated fuzzer.

[asraa]

\cc @nareddyt This is my attempt at cleaning up the design for fuzzing http filters... We have a base HttpFilterFuzzer library that can truly be applied to any filter without extra mocks or any set-up needed. It just holds onto the filter decoding/encoding logic.
The UberFilterFuzzer is basically a user of this, and handles automatic generic filter creation.

If you import this it would mean changing UberFilterFuzzer to HttpFilterFuzzer. It should increase the speed of the fuzzer drastically. WDYT?

I could also implement these outside of a class, but I wouldn't be able to hold onto the headermaps for access logging. Open to suggestions.

[nareddyt]

This is great, thanks @asraa!

I was brainstorming a way of simplifying this a few months ago, but I was trying to use composition instead of inheritance and had some issues with holding onto the header maps. Your solution works really well and it seems clean to me.

I'll be sure to switch over ESPv2 (maybe in the next month) and try collect some metrics on the performance improvement. I'm curious how much improvement we'll see from removing mocks, though we might still have to mock the StreamInfo.

[htuch]

LGTM, thanks! The comment applies to existing code.

[htuch]

Why are these all templated?

[asraa]

The logic of handling filter return status and end_stream values in runData is the common for encoders/decoder, so we meant to share that across both encoder/decoder despite different methods.

There were some alternatives considered here #11209, one of which was adding a lambda in for the correct header/data/trailer funcs (encodeHeaders, decodeHeaders), and having the runData use those lambda HeaderFunc, DataFunc, TrailerFunc depending on if it's encoder or decoder.

If I were to de-template this, it's not too much additional work. The lambdas (Header/Data/Trailer)Funcs are the template send(Header/Data/Trailer) specializations. I guess it also depends on what API is exposed to the fuzzer:
runData(FilterType, data) vs runEncode(encoder, data) and runDecode(decoder, data)
where runEncode(encoder, data) calls runData with the appropriate lambas. I was happy with both.

[asraa]

quick ping?

[nareddyt]

FYI if we don't template these functions, then you have to duplicate all the code in runData, right?

envoy/test/extensions/filters/http/common/fuzz/http_filter_fuzzer.h

Lines 84 to 114 in d60073c

	void HttpFilterFuzzer::runData(FilterType* filter, const test::fuzz::HttpData& data) {
	bool end_stream = false;
	enabled_ = true;
	if (data.body_case() == test::fuzz::HttpData::BODY_NOT_SET && !data.has_trailers()) {
	end_stream = true;
	}
	const auto& headersStatus = sendHeaders(filter, data, end_stream);
	ENVOY_LOG_MISC(debug, "Finished with FilterHeadersStatus: {}", headersStatus);
	if ((headersStatus != Http::FilterHeadersStatus::Continue &&
	headersStatus != Http::FilterHeadersStatus::StopIteration) ||
	!enabled_) {
	return;
	}
	const std::vector<std::string> data_chunks = Fuzz::parseHttpData(data);
	for (size_t i = 0; i < data_chunks.size(); i++) {
	if (!data.has_trailers() && i == data_chunks.size() - 1) {
	end_stream = true;
	}
	Buffer::OwnedImpl buffer(data_chunks[i]);
	const auto& dataStatus = sendData(filter, buffer, end_stream);
	ENVOY_LOG_MISC(debug, "Finished with FilterDataStatus: {}", dataStatus);
	if (dataStatus != Http::FilterDataStatus::Continue || !enabled_) {
	return;
	}
	}
	if (data.has_trailers() && enabled_) {
	sendTrailers(filter, data);
	}
	}

[asraa]

I don't think so. runData's signature could be
runData(FilterType* filter, const test::fuzz::HttpData& data, HeaderFunc header_func, DataFunc data_func, TrailerFunc trailer_func)
and runEncode(encoder, data) would call runData with the right funcs (the specializations of sendHeaders/sendData/sendTrailers)

Edit: maybe because of encoder/decoder typing there'd need to be type downcasting... I'll just give it a shot