Large request payload makes geoip filter wrongly trim decodeData

[JuniorHsu]

Title: Large request payload makes geoip filter wrongly trim decodeData

Description:
While request payload is large, the upstream service could received truncated bytes, i.e., the remaining filters behind geoip could miss somedecodeData events.

Here's what we think:
While the filter does async lookup, it asks filter manager to stop iteration but not stop the world.

envoy/source/extensions/filters/http/geoip/geoip_filter.cc

Line 67 in dbbe285

return Http::FilterHeadersStatus::StopIteration;

However, decodeDatas probably come after and we return Continue

envoy/source/extensions/filters/http/geoip/geoip_filter.cc

Line 71 in dbbe285

return Http::FilterDataStatus::Continue;

by the comments which unlock decodeHeaders to the remaining filters

envoy/envoy/http/filter.h

Line 127 in 667e963

// will be sent first via decodeHeaders()/encodeHeaders(). If data has previously been buffered,

Therefore, we have two side effect
(a) the header is not set before decodeData sent out

envoy/source/extensions/filters/http/geoip/geoip_filter.cc

Line 88 in dbbe285

request_headers_->setCopy(Http::LowerCaseString(geo_header), lookup_result);

(b) while we code continueDecoding again, filter manager fails to handle the events correctly, which triggers the bug we observed.

envoy/source/extensions/filters/http/geoip/geoip_filter.cc

Line 93 in dbbe285

decoder_callbacks_->continueDecoding();

Un-equipping geoip filter resolved the issue.

--

So we have two bugs I think
(i)

envoy/source/extensions/filters/http/geoip/geoip_filter.cc

Line 67 in dbbe285

return Http::FilterHeadersStatus::StopIteration;

we should use StopAllIterationAndWatermark in geoip filter to stop the world so we could set headers ready

(ii) although this might be only side effect of filter event mishandling but continueDecoding could make the data truncated and IIRC this is not the first time we hit the similar issue. Might need filter management expert to weigh in.

[htuch]

@alyssawilk @ravenblackx @nezdolik

[JuniorHsu]

we've verified that StopAllIterationAndWatermark work as expected

[alyssawilk]

yeah that sounds like a reasonable fix to me. cc @nezdolik @ravenblackx as geoip owners.

[JuniorHsu]

cool we can work on a patch and let's go from there. thanks alyssa