http: codec refactor part 2 (#9918)

This is part 1 in a set of changes to refactor the HTTP codecs
so they can emit typed request headers, request trailers, response
headers, and response trailers. This will help clean up various
interface inconsistencies as well as unlock a greater variety of
header map improvements.

Signed-off-by: Matt Klein <mklein@lyft.com>

include/envoy/http/api_listener.h

@@ -20,14 +20,15 @@ class ApiListener {
    *                         response are backed by the same Stream object.
    * @param is_internally_created indicates if this stream was originated by a
    *   client, or was created by Envoy, by example as part of an internal redirect.
-   * @return StreamDecoder& supplies the decoder callbacks to fire into for stream decoding events.
+   * @return RequestDecoder& supplies the decoder callbacks to fire into for stream
+   *   decoding events.
    */
-  virtual StreamDecoder& newStream(StreamEncoder& response_encoder,
-                                   bool is_internally_created = false) PURE;
+  virtual RequestDecoder& newStream(ResponseEncoder& response_encoder,
+                                    bool is_internally_created = false) PURE;
 };
 
 using ApiListenerPtr = std::unique_ptr<ApiListener>;
 using ApiListenerOptRef = absl::optional<std::reference_wrapper<ApiListener>>;
 
 } // namespace Http
-} // namespace Envoy
+} // namespace Envoy

include/envoy/http/codec.h

@@ -27,89 +27,158 @@ const char MaxResponseHeadersCountOverrideKey[] =
 class Stream;
 
 /**
- * Encodes an HTTP stream.
+ * Encodes an HTTP stream. This interface contains methods common to both the request and response
+ * path.
+ * TODO(mattklein123): Consider removing the StreamEncoder interface entirely and just duplicating
+ * the methods in both the request/response path for simplicity.
  */
 class StreamEncoder {
 public:
   virtual ~StreamEncoder() = default;
 
   /**
-   * Encode 100-Continue headers.
-   * @param headers supplies the 100-Continue header map to encode.
+   * Encode a data frame.
+   * @param data supplies the data to encode. The data may be moved by the encoder.
+   * @param end_stream supplies whether this is the last data frame.
    */
-  virtual void encode100ContinueHeaders(const HeaderMap& headers) PURE;
+  virtual void encodeData(Buffer::Instance& data, bool end_stream) PURE;
+
+  /**
+   * @return Stream& the backing stream.
+   */
+  virtual Stream& getStream() PURE;
 
+  /**
+   * Encode metadata.
+   * @param metadata_map_vector is the vector of metadata maps to encode.
+   */
+  virtual void encodeMetadata(const MetadataMapVector& metadata_map_vector) PURE;
+};
+
+/**
+ * Stream encoder used for sending a request (client to server). Virtual inheritance is required
+ * due to a parallel implementation split between the shared base class and the derived class.
+ * TODO(mattklein123): In a future change the header types will be changed to differentiate from
+ * the response path.
+ */
+class RequestEncoder : public virtual StreamEncoder {
+public:
   /**
    * Encode headers, optionally indicating end of stream. Response headers must
    * have a valid :status set.
    * @param headers supplies the header map to encode.
-   * @param end_stream supplies whether this is a header only request/response.
+   * @param end_stream supplies whether this is a header only request.
    */
   virtual void encodeHeaders(const HeaderMap& headers, bool end_stream) PURE;
 
-  /**
-   * Encode a data frame.
-   * @param data supplies the data to encode. The data may be moved by the encoder.
-   * @param end_stream supplies whether this is the last data frame.
-   */
-  virtual void encodeData(Buffer::Instance& data, bool end_stream) PURE;
-
   /**
    * Encode trailers. This implicitly ends the stream.
    * @param trailers supplies the trailers to encode.
    */
   virtual void encodeTrailers(const HeaderMap& trailers) PURE;
+};
 
+/**
+ * Stream encoder used for sending a response (server to client). Virtual inheritance is required
+ * due to a parallel implementation split between the shared base class and the derived class.
+ * TODO(mattklein123): In a future change the header types will be changed to differentiate from
+ * the request path.
+ */
+class ResponseEncoder : public virtual StreamEncoder {
+public:
   /**
-   * @return Stream& the backing stream.
+   * Encode 100-Continue headers.
+   * @param headers supplies the 100-Continue header map to encode.
    */
-  virtual Stream& getStream() PURE;
+  virtual void encode100ContinueHeaders(const HeaderMap& headers) PURE;
 
   /**
-   * Encode metadata.
-   * @param metadata_map_vector is the vector of metadata maps to encode.
+   * Encode headers, optionally indicating end of stream. Response headers must
+   * have a valid :status set.
+   * @param headers supplies the header map to encode.
+   * @param end_stream supplies whether this is a header only response.
    */
-  virtual void encodeMetadata(const MetadataMapVector& metadata_map_vector) PURE;
+  virtual void encodeHeaders(const HeaderMap& headers, bool end_stream) PURE;
+
+  /**
+   * Encode trailers. This implicitly ends the stream.
+   * @param trailers supplies the trailers to encode.
+   */
+  virtual void encodeTrailers(const HeaderMap& trailers) PURE;
 };
 
 /**
- * Decodes an HTTP stream. These are callbacks fired into a sink.
+ * Decodes an HTTP stream. These are callbacks fired into a sink. This interface contains methods
+ * common to both the request and response path.
+ * TODO(mattklein123): Consider removing the StreamDecoder interface entirely and just duplicating
+ * the methods in both the request/response path for simplicity.
  */
 class StreamDecoder {
 public:
   virtual ~StreamDecoder() = default;
 
   /**
-   * Called with decoded 100-Continue headers.
-   * @param headers supplies the decoded 100-Continue headers map that is moved into the callee.
+   * Called with a decoded data frame.
+   * @param data supplies the decoded data.
+   * @param end_stream supplies whether this is the last data frame.
    */
-  virtual void decode100ContinueHeaders(HeaderMapPtr&& headers) PURE;
+  virtual void decodeData(Buffer::Instance& data, bool end_stream) PURE;
 
   /**
-   * Called with decoded headers, optionally indicating end of stream.
-   * @param headers supplies the decoded headers map that is moved into the callee.
-   * @param end_stream supplies whether this is a header only request/response.
+   * Called with decoded METADATA.
+   * @param decoded METADATA.
    */
-  virtual void decodeHeaders(HeaderMapPtr&& headers, bool end_stream) PURE;
+  virtual void decodeMetadata(MetadataMapPtr&& metadata_map) PURE;
+};
 
+/**
+ * Stream decoder used for receiving a request (client to server). Virtual inheritance is required
+ * due to a parallel implementation split between the shared base class and the derived class.
+ * TODO(mattklein123): In a future change the header types will be changed to differentiate from
+ * the response path.
+ */
+class RequestDecoder : public virtual StreamDecoder {
+public:
   /**
-   * Called with a decoded data frame.
-   * @param data supplies the decoded data.
-   * @param end_stream supplies whether this is the last data frame.
+   * Called with decoded headers, optionally indicating end of stream.
+   * @param headers supplies the decoded headers map.
+   * @param end_stream supplies whether this is a header only request.
    */
-  virtual void decodeData(Buffer::Instance& data, bool end_stream) PURE;
+  virtual void decodeHeaders(HeaderMapPtr&& headers, bool end_stream) PURE;
 
   /**
    * Called with a decoded trailers frame. This implicitly ends the stream.
    * @param trailers supplies the decoded trailers.
    */
   virtual void decodeTrailers(HeaderMapPtr&& trailers) PURE;
+};
 
+/**
+ * Stream decoder used for receiving a response (server to client). Virtual inheritance is required
+ * due to a parallel implementation split between the shared base class and the derived class.
+ * TODO(mattklein123): In a future change the header types will be changed to differentiate from
+ * the request path.
+ */
+class ResponseDecoder : public virtual StreamDecoder {
+public:
   /**
-   * Called with decoded METADATA.
-   * @param decoded METADATA.
+   * Called with decoded 100-Continue headers.
+   * @param headers supplies the decoded 100-Continue headers map.
    */
-  virtual void decodeMetadata(MetadataMapPtr&& metadata_map) PURE;
+  virtual void decode100ContinueHeaders(HeaderMapPtr&& headers) PURE;
+
+  /**
+   * Called with decoded headers, optionally indicating end of stream.
+   * @param headers supplies the decoded headers map.
+   * @param end_stream supplies whether this is a header only response.
+   */
+  virtual void decodeHeaders(HeaderMapPtr&& headers, bool end_stream) PURE;
+
+  /**
+   * Called with a decoded trailers frame. This implicitly ends the stream.
+   * @param trailers supplies the decoded trailers.
+   */
+  virtual void decodeTrailers(HeaderMapPtr&& trailers) PURE;
 };
 
 /**
@@ -198,14 +267,14 @@ class Stream {
    */
   virtual void readDisable(bool disable) PURE;
 
-  /*
+  /**
    * Return the number of bytes this stream is allowed to buffer, or 0 if there is no limit
    * configured.
    * @return uint32_t the stream's configured buffer limits.
    */
   virtual uint32_t bufferLimit() PURE;
 
-  /*
+  /**
    * @return string_view optionally return the reason behind codec level errors.
    *
    * This information is communicated via direct accessor rather than passed with the
@@ -214,7 +283,7 @@ class Stream {
    */
   virtual absl::string_view responseDetails() { return ""; }
 
-  /*
+  /**
    * @return const Address::InstanceConstSharedPtr& the local address of the connection associated
    * with the stream.
    */
@@ -421,17 +490,17 @@ class ServerConnectionCallbacks : public virtual ConnectionCallbacks {
    *                         response are backed by the same Stream object.
    * @param is_internally_created indicates if this stream was originated by a
    *   client, or was created by Envoy, by example as part of an internal redirect.
-   * @return StreamDecoder& supplies the decoder callbacks to fire into for stream decoding events.
+   * @return RequestDecoder& supplies the decoder callbacks to fire into for stream decoding
+   *   events.
    */
-  virtual StreamDecoder& newStream(StreamEncoder& response_encoder,
-                                   bool is_internally_created = false) PURE;
+  virtual RequestDecoder& newStream(ResponseEncoder& response_encoder,
+                                    bool is_internally_created = false) PURE;
 };
 
 /**
  * A server side HTTP connection.
  */
 class ServerConnection : public virtual Connection {};
-
 using ServerConnectionPtr = std::unique_ptr<ServerConnection>;
 
 /**
@@ -442,9 +511,9 @@ class ClientConnection : public virtual Connection {
   /**
    * Create a new outgoing request stream.
    * @param response_decoder supplies the decoder callbacks to fire response events into.
-   * @return StreamEncoder& supplies the encoder to write the request into.
+   * @return RequestEncoder& supplies the encoder to write the request into.
    */
-  virtual StreamEncoder& newStream(StreamDecoder& response_decoder) PURE;
+  virtual RequestEncoder& newStream(ResponseDecoder& response_decoder) PURE;
 };
 
 using ClientConnectionPtr = std::unique_ptr<ClientConnection>;

include/envoy/http/conn_pool.h

@@ -60,8 +60,7 @@ class Callbacks {
    *             connection pools the description may be different each time this is called.
    * @param info supplies the stream info object associated with the upstream connection.
    */
-  virtual void onPoolReady(Http::StreamEncoder& encoder,
-                           Upstream::HostDescriptionConstSharedPtr host,
+  virtual void onPoolReady(RequestEncoder& encoder, Upstream::HostDescriptionConstSharedPtr host,
                            const StreamInfo::StreamInfo& info) PURE;
 };
 
@@ -119,7 +118,8 @@ class Instance : public Event::DeferredDeletable {
    * @warning Do not call cancel() from the callbacks, as the request is implicitly canceled when
    *          the callbacks are called.
    */
-  virtual Cancellable* newStream(Http::StreamDecoder& response_decoder, Callbacks& callbacks) PURE;
+  virtual Cancellable* newStream(Http::ResponseDecoder& response_decoder,
+                                 Callbacks& callbacks) PURE;
 
   /**
    * @return Upstream::HostDescriptionConstSharedPtr the host for which connections are pooled.

source/common/http/codec_client.cc

@@ -58,7 +58,7 @@ void CodecClient::deleteRequest(ActiveRequest& request) {
   }
 }
 
-StreamEncoder& CodecClient::newStream(StreamDecoder& response_decoder) {
+RequestEncoder& CodecClient::newStream(ResponseDecoder& response_decoder) {
   ActiveRequestPtr request(new ActiveRequest(*this, response_decoder));
   request->encoder_ = &codec_->newStream(*request);
   request->encoder_->getStream().addCallbacks(*request);

source/common/http/codec_client.h

@@ -100,7 +100,7 @@ class CodecClient : Logger::Loggable<Logger::Id::client>,
    * @param response_decoder supplies the decoder to use for response callbacks.
    * @return StreamEncoder& the encoder to use for encoding the request.
    */
-  StreamEncoder& newStream(StreamDecoder& response_decoder);
+  RequestEncoder& newStream(ResponseDecoder& response_decoder);
 
   void setConnectionStats(const Network::Connection::ConnectionStats& stats) {
     connection_->setConnectionStats(stats);
@@ -186,9 +186,9 @@ class CodecClient : Logger::Loggable<Logger::Id::client>,
   struct ActiveRequest : LinkedObject<ActiveRequest>,
                          public Event::DeferredDeletable,
                          public StreamCallbacks,
-                         public StreamDecoderWrapper {
-    ActiveRequest(CodecClient& parent, StreamDecoder& inner)
-        : StreamDecoderWrapper(inner), parent_(parent) {}
+                         public ResponseDecoderWrapper {
+    ActiveRequest(CodecClient& parent, ResponseDecoder& inner)
+        : ResponseDecoderWrapper(inner), parent_(parent) {}
 
     // StreamCallbacks
     void onResetStream(StreamResetReason reason, absl::string_view) override {
@@ -201,7 +201,7 @@ class CodecClient : Logger::Loggable<Logger::Id::client>,
     void onPreDecodeComplete() override { parent_.responseDecodeComplete(*this); }
     void onDecodeComplete() override {}
 
-    StreamEncoder* encoder_{};
+    RequestEncoder* encoder_{};
     CodecClient& parent_;
   };
 

source/common/http/codec_wrappers.h

@@ -6,11 +6,11 @@ namespace Envoy {
 namespace Http {
 
 /**
- * Wrapper for StreamDecoder that just forwards to an "inner" decoder.
+ * Wrapper for ResponseDecoder that just forwards to an "inner" decoder.
  */
-class StreamDecoderWrapper : public StreamDecoder {
+class ResponseDecoderWrapper : public ResponseDecoder {
 public:
-  // StreamDecoder
+  // ResponseDecoder
   void decode100ContinueHeaders(HeaderMapPtr&& headers) override {
     inner_.decode100ContinueHeaders(std::move(headers));
   }
@@ -50,7 +50,7 @@ class StreamDecoderWrapper : public StreamDecoder {
   }
 
 protected:
-  StreamDecoderWrapper(StreamDecoder& inner) : inner_(inner) {}
+  ResponseDecoderWrapper(ResponseDecoder& inner) : inner_(inner) {}
 
   /**
    * Consumers of the wrapper generally want to know when a decode is complete. This is called
@@ -59,19 +59,15 @@ class StreamDecoderWrapper : public StreamDecoder {
   virtual void onPreDecodeComplete() PURE;
   virtual void onDecodeComplete() PURE;
 
-  StreamDecoder& inner_;
+  ResponseDecoder& inner_;
 };
 
 /**
- * Wrapper for StreamEncoder that just forwards to an "inner" encoder.
+ * Wrapper for RequestEncoder that just forwards to an "inner" encoder.
  */
-class StreamEncoderWrapper : public StreamEncoder {
+class RequestEncoderWrapper : public RequestEncoder {
 public:
-  // StreamEncoder
-  void encode100ContinueHeaders(const HeaderMap& headers) override {
-    inner_.encode100ContinueHeaders(headers);
-  }
-
+  // RequestEncoder
   void encodeHeaders(const HeaderMap& headers, bool end_stream) override {
     inner_.encodeHeaders(headers, end_stream);
     if (end_stream) {
@@ -98,15 +94,15 @@ class StreamEncoderWrapper : public StreamEncoder {
   Stream& getStream() override { return inner_.getStream(); }
 
 protected:
-  StreamEncoderWrapper(StreamEncoder& inner) : inner_(inner) {}
+  RequestEncoderWrapper(RequestEncoder& inner) : inner_(inner) {}
 
   /**
    * Consumers of the wrapper generally want to know when an encode is complete. This is called at
    * that time and is implemented by derived classes.
    */
   virtual void onEncodeComplete() PURE;
 
-  StreamEncoder& inner_;
+  RequestEncoder& inner_;
 };
 
 } // namespace Http