use StopAllIterationAndWatermark in geoip filter

Signed-off-by: Haowei Yuan <hyuan@wustl.edu>
envoyproxy · Mar 27, 2024 · fd50b89 · fd50b89
1 parent 6a6ad95
commit fd50b89
Show file tree

Hide file tree

Showing 3 changed files with 13 additions and 8 deletions.
diff --git a/changelogs/current.yaml b/changelogs/current.yaml
@@ -199,6 +199,10 @@ bug_fixes:
 - area: admin
  change: |
  Removing the ECDS config entry from the config dump when it does not exist (or has expired).
+- area: geoip
+ change: |
+ Switch to use StopAllIterationAndWatermark instead of StopIteration in geoip filter to fix an issue where large
+ POST request body may get corrupted when geoip is enabled.
 
 removed_config_or_runtime:
 # *Normally occurs at the end of the* :ref:`deprecation period <deprecated>`

diff --git a/source/extensions/filters/http/geoip/geoip_filter.cc b/source/extensions/filters/http/geoip/geoip_filter.cc
@@ -63,8 +63,9 @@ Http::FilterHeadersStatus GeoipFilter::decodeHeaders(Http::RequestHeaderMap& hea
  });
  });
 
- // Stop the iteration for headers for the current filter and the filters following.
- return Http::FilterHeadersStatus::StopIteration;
+ // Stop the iteration for headers and data (POST request) for the current filter and the filters
+ // following.
+ return Http::FilterHeadersStatus::StopAllIterationAndWatermark;
 }
 
 Http::FilterDataStatus GeoipFilter::decodeData(Buffer::Instance&, bool) {

diff --git a/test/extensions/filters/http/geoip/geoip_filter_test.cc b/test/extensions/filters/http/geoip/geoip_filter_test.cc
@@ -101,7 +101,7 @@ TEST_F(GeoipFilterTest, NoXffSuccessfulLookup) {
  EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->decodeData(data_, false));
  Http::TestRequestTrailerMapImpl request_trailers;
  EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_->decodeTrailers(request_trailers));
- EXPECT_EQ(Http::FilterHeadersStatus::StopIteration,
+ EXPECT_EQ(Http::FilterHeadersStatus::StopAllIterationAndWatermark,
  filter_->decodeHeaders(request_headers, false));
  captured_cb_(Geolocation::LookupResult{{"x-geo-city", "dummy-city"}});
  EXPECT_CALL(filter_callbacks_, continueDecoding());
@@ -134,7 +134,7 @@ TEST_F(GeoipFilterTest, UseXffSuccessfulLookup) {
  DoAll(SaveArg<0>(&captured_rq_), SaveArg<1>(&captured_cb_), Invoke([this]() {
  captured_cb_(Geolocation::LookupResult{{"x-geo-region", "dummy-region"}});
  })));
- EXPECT_EQ(Http::FilterHeadersStatus::StopIteration,
+ EXPECT_EQ(Http::FilterHeadersStatus::StopAllIterationAndWatermark,
  filter_->decodeHeaders(request_headers, false));
  EXPECT_CALL(filter_callbacks_, continueDecoding());
  dispatcher_->run(Event::Dispatcher::RunType::Block);
@@ -167,7 +167,7 @@ TEST_F(GeoipFilterTest, GeoHeadersOverridenForIncomingRequest) {
  captured_cb_(Geolocation::LookupResult{
  {"x-geo-city", "dummy-city"}, {"x-geo-region", "dummy-region"}});
  })));
- EXPECT_EQ(Http::FilterHeadersStatus::StopIteration,
+ EXPECT_EQ(Http::FilterHeadersStatus::StopAllIterationAndWatermark,
  filter_->decodeHeaders(request_headers, false));
  EXPECT_CALL(filter_callbacks_, continueDecoding());
  dispatcher_->run(Event::Dispatcher::RunType::Block);
@@ -214,7 +214,7 @@ TEST_F(GeoipFilterTest, AllHeadersPropagatedCorrectly) {
  {"x-geo-anon-tor", "true"},
  {"x-geo-anon-proxy", "true"}});
  })));
- EXPECT_EQ(Http::FilterHeadersStatus::StopIteration,
+ EXPECT_EQ(Http::FilterHeadersStatus::StopAllIterationAndWatermark,
  filter_->decodeHeaders(request_headers, false));
  EXPECT_CALL(filter_callbacks_, continueDecoding());
  dispatcher_->run(Event::Dispatcher::RunType::Block);
@@ -252,7 +252,7 @@ TEST_F(GeoipFilterTest, GeoHeaderNotAppendedOnEmptyLookup) {
  captured_cb_(Geolocation::LookupResult{
  {"x-geo-city", ""}, {"x-geo-region", "dummy-region"}});
  })));
- EXPECT_EQ(Http::FilterHeadersStatus::StopIteration,
+ EXPECT_EQ(Http::FilterHeadersStatus::StopAllIterationAndWatermark,
  filter_->decodeHeaders(request_headers, false));
  EXPECT_CALL(filter_callbacks_, continueDecoding());
  dispatcher_->run(Event::Dispatcher::RunType::Block);
@@ -279,7 +279,7 @@ TEST_F(GeoipFilterTest, NoCrashIfFilterDestroyedBeforeCallbackCalled) {
  .WillRepeatedly(DoAll(SaveArg<0>(&captured_rq_), SaveArg<1>(&captured_cb_), Invoke([this]() {
  captured_cb_(Geolocation::LookupResult{{"x-geo-city", "dummy-city"}});
  })));
- EXPECT_EQ(Http::FilterHeadersStatus::StopIteration,
+ EXPECT_EQ(Http::FilterHeadersStatus::StopAllIterationAndWatermark,
  filter_->decodeHeaders(request_headers, false));
  filter_.reset();
  dispatcher_->run(Event::Dispatcher::RunType::Block);