ext_authz: Avoid calling check multiple times (#13288)

This patch makes sure the filter sends exactly only one check request when the buffer is full, while there might be more data to be decoded. Risk Level: Low Testing: Unit test Docs Changes: N/A Release Notes: N/A Fixes #13260 Signed-off-by: Dhi Aurrahman <[email protected]>
envoyproxy · Oct 16, 2020 · 2196214 · 2196214
1 parent 7b62e16
commit 2196214
Show file tree

Hide file tree

Showing 2 changed files with 59 additions and 0 deletions.
diff --git a/source/extensions/filters/http/ext_authz/ext_authz.cc b/source/extensions/filters/http/ext_authz/ext_authz.cc
@@ -323,6 +323,9 @@ bool Filter::isBufferFull() const {
 }
 
 void Filter::continueDecoding() {
+  // After sending the check request, we don't need to buffer the data anymore.
+  buffer_data_ = false;
+
   filter_return_ = FilterReturn::ContinueDecoding;
   if (!initiating_call_) {
     callbacks_->continueDecoding();

diff --git a/test/extensions/filters/http/ext_authz/ext_authz_test.cc b/test/extensions/filters/http/ext_authz/ext_authz_test.cc
@@ -489,6 +489,62 @@ TEST_F(HttpFilterTest, RequestDataWithPartialMessage) {
   EXPECT_EQ(Http::FilterTrailersStatus::StopIteration, filter_->decodeTrailers(request_trailers_));
 }
 
+// Checks that the filter initiates an authorization request when the buffer reaches maximum
+// request bytes and allow_partial_message is set to true. In addition to that, after the filter
+// sends the check request, data decoding continues.
+TEST_F(HttpFilterTest, RequestDataWithPartialMessageThenContinueDecoding) {
+  InSequence s;
+
+  initialize(R"EOF(
+  grpc_service:
+    envoy_grpc:
+      cluster_name: "ext_authz_server"
+  failure_mode_allow: false
+  with_request_body:
+    max_request_bytes: 10
+    allow_partial_message: true
+  )EOF");
+
+  ON_CALL(filter_callbacks_, connection()).WillByDefault(Return(&connection_));
+  ON_CALL(filter_callbacks_, decodingBuffer()).WillByDefault(Return(&data_));
+  EXPECT_CALL(filter_callbacks_, setDecoderBufferLimit(_)).Times(0);
+  EXPECT_CALL(connection_, remoteAddress()).WillOnce(ReturnRef(addr_));
+  EXPECT_CALL(connection_, localAddress()).WillOnce(ReturnRef(addr_));
+
+  // The check call should only be called once.
+  EXPECT_CALL(*client_, check(_, _, _, testing::A<Tracing::Span&>(), _))
+      .WillOnce(
+          WithArgs<0>(Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks) -> void {
+            request_callbacks_ = &callbacks;
+          })));
+
+  EXPECT_CALL(filter_callbacks_, continueDecoding());
+
+  EXPECT_EQ(Http::FilterHeadersStatus::StopIteration,
+            filter_->decodeHeaders(request_headers_, false));
+
+  data_.add("foo");
+  EXPECT_EQ(Http::FilterDataStatus::StopIterationAndBuffer, filter_->decodeData(data_, false));
+
+  data_.add("bar");
+  EXPECT_EQ(Http::FilterDataStatus::StopIterationAndBuffer, filter_->decodeData(data_, false));
+
+  data_.add("barfoo");
+  EXPECT_EQ(Http::FilterDataStatus::StopIterationAndWatermark, filter_->decodeData(data_, false));
+
+  data_.add("more data after watermark is set is possible");
+  EXPECT_EQ(Http::FilterDataStatus::StopIterationAndWatermark, filter_->decodeData(data_, false));
+
+  Filters::Common::ExtAuthz::Response response{};
+  response.status = Filters::Common::ExtAuthz::CheckStatus::OK;
+  request_callbacks_->onComplete(std::make_unique<Filters::Common::ExtAuthz::Response>(response));
+
+  data_.add("more data after calling check request");
+  EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->decodeData(data_, true));
+
+  EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_->decodeTrailers(request_trailers_));
+}
+
 // Checks that the filter initiates the authorization process only when the filter decode trailers
 // is called.
 TEST_F(HttpFilterTest, RequestDataWithSmallBuffer) {