http: fix datadog and squash handling of responses without bod (envoy…

…proxy#13328) Commit Message: Fixing bugs in datadog and sqaush where unexpected bodyless responses would crash Envoy Risk Level: low Testing: new unit tests Docs Changes: n/a Release Notes: inline Signed-off-by: Alyssa Wilk <alyssar@chromium.org> Signed-off-by: alyssawilk <alyssar@chromium.org> Signed-off-by: Christoph Pakulski <christoph@tetrate.io>
cpakulski · Jan 11, 2021 · af02808 · af02808
1 parent b586086
commit af02808
Show file tree

Hide file tree

Showing 4 changed files with 56 additions and 4 deletions.
diff --git a/source/extensions/filters/http/squash/squash_filter.cc b/source/extensions/filters/http/squash/squash_filter.cc
@@ -308,9 +308,7 @@ void SquashFilter::cleanup() {
 }
 
 Json::ObjectSharedPtr SquashFilter::getJsonBody(Http::ResponseMessagePtr&& m) {
-  Buffer::InstancePtr& data = m->body();
-  std::string jsonbody = data->toString();
-  return Json::Factory::loadFromString(jsonbody);
+  return Json::Factory::loadFromString(m->bodyAsString());
 }
 
 } // namespace Squash

diff --git a/source/extensions/tracers/datadog/datadog_tracer_impl.cc b/source/extensions/tracers/datadog/datadog_tracer_impl.cc
@@ -138,7 +138,7 @@ void TraceReporter::onSuccess(const Http::AsyncClient::Request& request,
   } else {
     ENVOY_LOG(debug, "traces successfully submitted to datadog agent");
     driver_.tracerStats().reports_sent_.inc();
-    encoder_->handleResponse(http_response->body()->toString());
+    encoder_->handleResponse(http_response->bodyAsString());
   }
 }
 

diff --git a/test/extensions/filters/http/squash/squash_filter_test.cc b/test/extensions/filters/http/squash/squash_filter_test.cc
@@ -412,6 +412,19 @@ TEST_F(SquashFilterTest, InvalidJsonForGetAttachment) {
   completeRequest("200", "This is not a JSON object");
 }
 
+TEST_F(SquashFilterTest, InvalidResponseWithNoBody) {
+  doDownstreamRequest();
+  // Expect the get attachment request
+  expectAsyncClientSend();
+  completeCreateRequest();
+
+  auto retry_timer = new NiceMock<Envoy::Event::MockTimer>(&filter_callbacks_.dispatcher_);
+  EXPECT_CALL(*retry_timer, enableTimer(config_->attachmentPollPeriod(), _));
+  Http::ResponseMessagePtr msg(new Http::ResponseMessageImpl(Http::ResponseHeaderMapPtr{
+      new Http::TestResponseHeaderMapImpl{{":status", "200"}, {"content-length", "0"}}}));
+  popPendingCallback()->onSuccess(request_, std::move(msg));
+}
+
 TEST_F(SquashFilterTest, DestroyedInFlight) {
   doDownstreamRequest();
 

diff --git a/test/extensions/tracers/datadog/datadog_tracer_impl_test.cc b/test/extensions/tracers/datadog/datadog_tracer_impl_test.cc
@@ -185,6 +185,47 @@ TEST_F(DatadogDriverTest, FlushSpansTimer) {
   EXPECT_EQ(0U, stats_.counter("tracing.datadog.reports_failed").value());
 }
 
+TEST_F(DatadogDriverTest, NoBody) {
+  setupValidDriver();
+
+  Http::MockAsyncClientRequest request(&cm_.async_client_);
+  Http::AsyncClient::Callbacks* callback;
+  const absl::optional<std::chrono::milliseconds> timeout(std::chrono::seconds(1));
+  EXPECT_CALL(cm_.async_client_,
+              send_(_, _, Http::AsyncClient::RequestOptions().setTimeout(timeout)))
+      .WillOnce(
+          Invoke([&](Http::RequestMessagePtr& message, Http::AsyncClient::Callbacks& callbacks,
+                     const Http::AsyncClient::RequestOptions&) -> Http::AsyncClient::Request* {
+            callback = &callbacks;
+
+            EXPECT_EQ("fake_cluster", message->headers().getHostValue());
+            EXPECT_EQ("application/msgpack", message->headers().getContentTypeValue());
+
+            return &request;
+          }));
+
+  Tracing::SpanPtr span = driver_->startSpan(config_, request_headers_, operation_name_,
+                                             start_time_, {Tracing::Reason::Sampling, true});
+  span->finishSpan();
+
+  // Timer should be re-enabled.
+  EXPECT_CALL(*timer_, enableTimer(std::chrono::milliseconds(900), _));
+
+  timer_->invokeCallback();
+
+  EXPECT_EQ(1U, stats_.counter("tracing.datadog.timer_flushed").value());
+  EXPECT_EQ(1U, stats_.counter("tracing.datadog.traces_sent").value());
+
+  Http::ResponseMessagePtr msg(new Http::ResponseMessageImpl(Http::ResponseHeaderMapPtr{
+      new Http::TestResponseHeaderMapImpl{{":status", "200"}, {"content-length", "0"}}}));
+  callback->onSuccess(request, std::move(msg));
+
+  EXPECT_EQ(0U, stats_.counter("tracing.datadog.reports_skipped_no_cluster").value());
+  EXPECT_EQ(1U, stats_.counter("tracing.datadog.reports_sent").value());
+  EXPECT_EQ(0U, stats_.counter("tracing.datadog.reports_dropped").value());
+  EXPECT_EQ(0U, stats_.counter("tracing.datadog.reports_failed").value());
+}
+
 TEST_F(DatadogDriverTest, SkipReportIfCollectorClusterHasBeenRemoved) {
   Upstream::ClusterUpdateCallbacks* cluster_update_callbacks;
   EXPECT_CALL(cm_, addThreadLocalClusterUpdateCallbacks_(_))