Revert "HTTP2: Add DumpState support for HTTP2 (envoyproxy#14923)"

This reverts commit 3f740bc. Because it breaks builds on RHEL 8 and Ubuntu 18. See: envoyproxy#15093 Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
rgs1 · Feb 23, 2021 · 7819a9d · 7819a9d
1 parent 5e7eb11
commit 7819a9d
Show file tree

Hide file tree

Showing 11 changed files with 5 additions and 302 deletions.
diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst
@@ -90,7 +90,7 @@ New Features
 * ext_authz: added :ref:`allowed_client_headers_on_success <envoy_v3_api_field_extensions.filters.http.ext_authz.v3.AuthorizationResponse.allowed_client_headers_on_success>` to support sending response headers to downstream clients on OK external authorization checks via HTTP.
 * grpc_json_transcoder: added :ref:`request_validation_options <envoy_v3_api_field_extensions.filters.http.grpc_json_transcoder.v3.GrpcJsonTranscoder.request_validation_options>` to reject invalid requests early.
 * grpc_json_transcoder: filter can now be configured on per-route/per-vhost level as well. Leaving empty list of services in the filter configuration disables transcoding on the specific route.
-* http: added support for `Envoy::ScopeTrackedObject` for HTTP/1 and HTTP/2 dispatching. Crashes while inside the dispatching loop should dump debug information.
+* http: added support for `Envoy::ScopeTrackedObject` for HTTP/1 dispatching. Crashes while inside the dispatching loop should dump debug information.
 * http: added support for :ref:`preconnecting <envoy_v3_api_msg_config.cluster.v3.Cluster.PreconnectPolicy>`. Preconnecting is off by default, but recommended for clusters serving latency-sensitive traffic, especially if using HTTP/1.1.
 * http: change frame flood and abuse checks to the upstream HTTP/2 codec to ON by default. It can be disabled by setting the `envoy.reloadable_features.upstream_http2_flood_checks` runtime key to false.
 * json: introduced new JSON parser (https://github.com/nlohmann/json) to replace RapidJSON. The new parser is disabled by default. To test the new RapidJSON parser, enable the runtime feature `envoy.reloadable_features.remove_legacy_json`.

diff --git a/source/common/common/dump_state_utils.h b/source/common/common/dump_state_utils.h
@@ -21,7 +21,6 @@ namespace Envoy {
 // Macro assumes local member variables
 // os (ostream)
 // indent_level (int)
-// spaces (const char *)
 #define DUMP_DETAILS(member)                                                                       \
   do {                                                                                             \
     os << spaces << #member ": ";                                                                  \
@@ -36,7 +35,6 @@ namespace Envoy {
 // Macro assumes local member variables
 // os (ostream)
 // indent_level (int)
-// spaces (const char *)
 #define DUMP_OPT_REF_DETAILS(member)                                                               \
   do {                                                                                             \
     os << spaces << #member ": ";                                                                  \

diff --git a/source/common/common/utility.cc b/source/common/common/utility.cc
@@ -476,12 +476,6 @@ void StringUtil::escapeToOstream(std::ostream& os, absl::string_view view) {
     case '\t':
       os << "\\t";
       break;
-    case '\v':
-      os << "\\v";
-      break;
-    case '\0':
-      os << "\\0";
-      break;
     case '"':
       os << "\\\"";
       break;

diff --git a/source/common/http/http2/BUILD b/source/common/http/http2/BUILD
@@ -126,7 +126,6 @@ envoy_cc_library(
         ":codec_stats_lib",
         "//include/envoy/network:connection_interface",
         "//source/common/common:assert_lib",
-        "//source/common/common:dump_state_utils",
         "//source/common/http:status_lib",
         "@envoy_api//envoy/config/core/v3:pkg_cc_proto",
     ],

diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc
@@ -1,9 +1,7 @@
 #include "common/http/http2/codec_impl.h"
 
-#include <algorithm>
 #include <cstdint>
 #include <memory>
-#include <ostream>
 #include <vector>
 
 #include "envoy/event/dispatcher.h"
@@ -13,10 +11,8 @@
 
 #include "common/common/assert.h"
 #include "common/common/cleanup.h"
-#include "common/common/dump_state_utils.h"
 #include "common/common/enum_to_int.h"
 #include "common/common/fmt.h"
-#include "common/common/scope_tracker.h"
 #include "common/common/utility.h"
 #include "common/http/codes.h"
 #include "common/http/exception.h"
@@ -645,17 +641,12 @@ Http::Status ConnectionImpl::dispatch(Buffer::Instance& data) {
 }
 
 Http::Status ConnectionImpl::innerDispatch(Buffer::Instance& data) {
-  ScopeTrackerScopeState scope(this, connection_.dispatcher());
   ENVOY_CONN_LOG(trace, "dispatching {} bytes", connection_, data.length());
   // Make sure that dispatching_ is set to false after dispatching, even when
   // ConnectionImpl::dispatch returns early or throws an exception (consider removing if there is a
   // single return after exception removal (#10878)).
-  Cleanup cleanup([this]() {
-    dispatching_ = false;
-    current_slice_ = nullptr;
-  });
+  Cleanup cleanup([this]() { dispatching_ = false; });
   for (const Buffer::RawSlice& slice : data.getRawSlices()) {
-    current_slice_ = &slice;
     dispatching_ = true;
     ssize_t rc =
         nghttp2_session_mem_recv(session_, static_cast<const uint8_t*>(slice.mem_), slice.len_);
@@ -674,7 +665,6 @@ Http::Status ConnectionImpl::innerDispatch(Buffer::Instance& data) {
       return codecProtocolError(nghttp2_strerror(rc));
     }
 
-    current_slice_ = nullptr;
     dispatching_ = false;
   }
 
@@ -1396,74 +1386,6 @@ ConnectionImpl::ClientHttp2Options::ClientHttp2Options(
       options_, ::Envoy::Http2::Utility::OptionsLimits::DEFAULT_MAX_CONCURRENT_STREAMS);
 }
 
-void ConnectionImpl::dumpState(std::ostream& os, int indent_level) const {
-  const char* spaces = spacesForLevel(indent_level);
-  os << spaces << "Http2::ConnectionImpl " << this << DUMP_MEMBER(max_headers_kb_)
-     << DUMP_MEMBER(max_headers_count_) << DUMP_MEMBER(per_stream_buffer_limit_)
-     << DUMP_MEMBER(allow_metadata_) << DUMP_MEMBER(stream_error_on_invalid_http_messaging_)
-     << DUMP_MEMBER(is_outbound_flood_monitored_control_frame_)
-     << DUMP_MEMBER(skip_encoding_empty_trailers_) << DUMP_MEMBER(dispatching_)
-     << DUMP_MEMBER(raised_goaway_) << DUMP_MEMBER(pending_deferred_reset_) << '\n';
-
-  // Dump the protocol constraints
-  DUMP_DETAILS(&protocol_constraints_);
-
-  os << spaces << "Number of active streams: " << active_streams_.size() << " Active Streams:\n";
-  std::for_each_n(active_streams_.begin(), std::min<size_t>(active_streams_.size(), 100),
-                  [&](auto& stream) { DUMP_DETAILS(stream); });
-
-  // Dump the active slice
-  if (current_slice_ == nullptr) {
-    // No current slice, use macro for consistent formatting.
-    os << spaces << "current_slice_: null\n";
-  } else {
-    auto slice_view =
-        absl::string_view(static_cast<const char*>(current_slice_->mem_), current_slice_->len_);
-
-    os << spaces << "current slice length: " << slice_view.length() << " contents: \"";
-    StringUtil::escapeToOstream(os, slice_view);
-    os << "\"\n";
-  }
-}
-
-void ConnectionImpl::StreamImpl::dumpState(std::ostream& os, int indent_level) const {
-  const char* spaces = spacesForLevel(indent_level);
-  os << spaces << "ConnectionImpl::StreamImpl " << this << DUMP_MEMBER(stream_id_)
-     << DUMP_MEMBER(unconsumed_bytes_) << DUMP_MEMBER(read_disable_count_)
-     << DUMP_MEMBER(local_end_stream_sent_) << DUMP_MEMBER(remote_end_stream_)
-     << DUMP_MEMBER(data_deferred_) << DUMP_MEMBER(received_noninformational_headers_)
-     << DUMP_MEMBER(pending_receive_buffer_high_watermark_called_)
-     << DUMP_MEMBER(pending_send_buffer_high_watermark_called_)
-     << DUMP_MEMBER(reset_due_to_messaging_error_)
-     << DUMP_MEMBER_AS(cookies_, cookies_.getStringView());
-
-  DUMP_DETAILS(pending_trailers_to_encode_);
-}
-
-void ConnectionImpl::ClientStreamImpl::dumpState(std::ostream& os, int indent_level) const {
-  const char* spaces = spacesForLevel(indent_level);
-  StreamImpl::dumpState(os, indent_level);
-
-  // Dump header map
-  if (absl::holds_alternative<ResponseHeaderMapPtr>(headers_or_trailers_)) {
-    DUMP_DETAILS(absl::get<ResponseHeaderMapPtr>(headers_or_trailers_));
-  } else {
-    DUMP_DETAILS(absl::get<ResponseTrailerMapPtr>(headers_or_trailers_));
-  }
-}
-
-void ConnectionImpl::ServerStreamImpl::dumpState(std::ostream& os, int indent_level) const {
-  const char* spaces = spacesForLevel(indent_level);
-  StreamImpl::dumpState(os, indent_level);
-
-  // Dump header map
-  if (absl::holds_alternative<RequestHeaderMapPtr>(headers_or_trailers_)) {
-    DUMP_DETAILS(absl::get<RequestHeaderMapPtr>(headers_or_trailers_));
-  } else {
-    DUMP_DETAILS(absl::get<RequestTrailerMapPtr>(headers_or_trailers_));
-  }
-}
-
 ClientConnectionImpl::ClientConnectionImpl(
     Network::Connection& connection, Http::ConnectionCallbacks& callbacks, CodecStats& stats,
     Random::RandomGenerator& random_generator,

diff --git a/source/common/http/http2/codec_impl.h b/source/common/http/http2/codec_impl.h
@@ -4,12 +4,10 @@
 #include <functional>
 #include <list>
 #include <memory>
-#include <ostream>
 #include <string>
 #include <vector>
 
 #include "envoy/common/random_generator.h"
-#include "envoy/common/scope_tracker.h"
 #include "envoy/config/core/v3/protocol.pb.h"
 #include "envoy/event/deferred_deletable.h"
 #include "envoy/http/codec.h"
@@ -92,9 +90,7 @@ class ProdNghttp2SessionFactory : public Nghttp2SessionFactory {
 /**
  * Base class for HTTP/2 client and server codecs.
  */
-class ConnectionImpl : public virtual Connection,
-                       protected Logger::Loggable<Logger::Id::http2>,
-                       public ScopeTrackedObject {
+class ConnectionImpl : public virtual Connection, protected Logger::Loggable<Logger::Id::http2> {
 public:
   ConnectionImpl(Network::Connection& connection, CodecStats& stats,
                  Random::RandomGenerator& random_generator,
@@ -133,9 +129,6 @@ class ConnectionImpl : public virtual Connection,
    */
   virtual Http::Status innerDispatch(Buffer::Instance& data);
 
-  // ScopeTrackedObject
-  void dumpState(std::ostream& os, int indent_level) const override;
-
 protected:
   friend class ProdNghttp2SessionFactory;
 
@@ -179,8 +172,7 @@ class ConnectionImpl : public virtual Connection,
                       public Stream,
                       public LinkedObject<StreamImpl>,
                       public Event::DeferredDeletable,
-                      public StreamCallbackHelper,
-                      public ScopeTrackedObject {
+                      public StreamCallbackHelper {
 
     StreamImpl(ConnectionImpl& parent, uint32_t buffer_limit);
     ~StreamImpl() override;
@@ -235,9 +227,6 @@ class ConnectionImpl : public virtual Connection,
       stream_idle_timeout_ = timeout;
     }
 
-    // ScopeTrackedObject
-    void dumpState(std::ostream& os, int indent_level) const override;
-
     // This code assumes that details is a static string, so that we
     // can avoid copying it.
     void setDetails(absl::string_view details) {
@@ -369,9 +358,6 @@ class ConnectionImpl : public virtual Connection,
     }
     void enableTcpTunneling() override {}
 
-    // ScopeTrackedObject
-    void dumpState(std::ostream& os, int indent_level) const override;
-
     ResponseDecoder& response_decoder_;
     absl::variant<ResponseHeaderMapPtr, ResponseTrailerMapPtr> headers_or_trailers_;
     std::string upgrade_type_;
@@ -414,9 +400,6 @@ class ConnectionImpl : public virtual Connection,
       encodeTrailersBase(trailers);
     }
 
-    // ScopeTrackedObject
-    void dumpState(std::ostream& os, int indent_level) const override;
-
     RequestDecoder* request_decoder_{};
     absl::variant<RequestHeaderMapPtr, RequestTrailerMapPtr> headers_or_trailers_;
 
@@ -548,8 +531,6 @@ class ConnectionImpl : public virtual Connection,
   void onKeepaliveResponse();
   void onKeepaliveResponseTimeout();
 
-  // Tracks the current slice we're processing in the dispatch loop.
-  const Buffer::RawSlice* current_slice_ = nullptr;
   bool dispatching_ : 1;
   bool raised_goaway_ : 1;
   bool pending_deferred_reset_ : 1;

diff --git a/source/common/http/http2/protocol_constraints.cc b/source/common/http/http2/protocol_constraints.cc
@@ -1,7 +1,6 @@
 #include "common/http/http2/protocol_constraints.h"
 
 #include "common/common/assert.h"
-#include "common/common/dump_state_utils.h"
 
 namespace Envoy {
 namespace Http {
@@ -113,20 +112,6 @@ Status ProtocolConstraints::checkInboundFrameLimits() {
   return okStatus();
 }
 
-void ProtocolConstraints::dumpState(std::ostream& os, int indent_level) const {
-  const char* spaces = spacesForLevel(indent_level);
-
-  os << spaces << "ProtocolConstraints " << this << DUMP_MEMBER(outbound_frames_)
-     << DUMP_MEMBER(max_outbound_frames_) << DUMP_MEMBER(outbound_control_frames_)
-     << DUMP_MEMBER(max_outbound_control_frames_)
-     << DUMP_MEMBER(consecutive_inbound_frames_with_empty_payload_)
-     << DUMP_MEMBER(max_consecutive_inbound_frames_with_empty_payload_)
-     << DUMP_MEMBER(opened_streams_) << DUMP_MEMBER(inbound_priority_frames_)
-     << DUMP_MEMBER(max_inbound_priority_frames_per_stream_)
-     << DUMP_MEMBER(inbound_window_update_frames_) << DUMP_MEMBER(outbound_data_frames_)
-     << DUMP_MEMBER(max_inbound_window_update_frames_per_data_frame_sent_) << '\n';
-}
-
 } // namespace Http2
 } // namespace Http
 } // namespace Envoy
diff --git a/source/common/http/http2/protocol_constraints.h b/source/common/http/http2/protocol_constraints.h
@@ -22,7 +22,7 @@ namespace Http2 {
 //  2. detection of outbound DATA or HEADER frame floods.
 //  4. zero length, PRIORITY and WINDOW_UPDATE floods.
 
-class ProtocolConstraints : public ScopeTrackedObject {
+class ProtocolConstraints {
 public:
   using ReleasorProc = std::function<void()>;
 
@@ -54,9 +54,6 @@ class ProtocolConstraints : public ScopeTrackedObject {
 
   Status checkOutboundFrameLimits();
 
-  // ScopeTrackedObject
-  void dumpState(std::ostream& os, int indent_level) const override;
-
 private:
   void releaseOutboundFrame();
   void releaseOutboundControlFrame();

diff --git a/test/common/common/utility_test.cc b/test/common/common/utility_test.cc
@@ -317,21 +317,6 @@ TEST(StringUtil, escapeToOstream) {
     StringUtil::escapeToOstream(ostream, R"(\\)");
     EXPECT_EQ(ostream.contents(), R"(\\\\)");
   }
-
-  {
-    std::array<char, 64> buffer;
-    OutputBufferStream ostream{buffer.data(), buffer.size()};
-    StringUtil::escapeToOstream(ostream, "vertical\vtab");
-    EXPECT_EQ(ostream.contents(), "vertical\\vtab");
-  }
-
-  {
-    using namespace std::string_literals;
-    std::array<char, 64> buffer;
-    OutputBufferStream ostream{buffer.data(), buffer.size()};
-    StringUtil::escapeToOstream(ostream, "null\0char"s);
-    EXPECT_EQ(ostream.contents(), "null\\0char");
-  }
 }
 
 TEST(StringUtil, toUpper) {