Add Metadata to codec #4796

soya3129 · 2018-10-19T22:52:44Z

Signed-off-by: Yang Song [email protected]

Description: Add MetadataEncoder and MetadataDecoder to codec. I also corrected some of the design decisions in MetadataEnocder and MetadataDecoder so that they are more suitable for codec. Those corrections includes: 1. let StreamImpl owns the decoded metadata, not the decoder. 2. Remove stream_id field in decoder and encoder since stream id is stored in StreamImpl. 3. Remove set/getMaxMetadataSize(), because nghttp2 uses default max frame size.

This is the second step towards supporting metadata in envoy. The next step would be add filters to add/delete/modify metadata.

Risk Level: Low. Not used.
Testing: Unit test.
Docs Changes:
Release Notes:
#2394

alyssawilk

Looking good!

alyssawilk · 2018-10-22T14:24:06Z

include/envoy/http/codec.h

+   * Encode METADATA.
+   * @param metadata_map is the METADATA to encode.
+   */
+  virtual void encodeMetadata(const MetadataMap& metadata_map) PURE;


I think this is a good PR to start landing docs with, even if we have them tagged as work in progress.
Let's talk about when you can encodeMetadata, how often you can encode it, when it will be passed up to filters (we can edit the docs if they change when that lands) etc.

Still working on the doc.

alyssawilk · 2018-10-22T14:32:26Z

source/common/http/conn_manager_impl.h

@@ -303,6 +303,7 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
    void decodeHeaders(HeaderMapPtr&& headers, bool end_stream) override;
    void decodeData(Buffer::Instance& data, bool end_stream) override;
    void decodeTrailers(HeaderMapPtr&& trailers) override;
+    void decodeMetadata(MetadataMap&) override { NOT_REACHED_GCOVR_EXCL_LINE; }


Do we think there should be anything done if an HTTP/1.1 codec is asked to encode metadata? If nothing else maybe error reporting stats so folks know their metadata is being swallowed?

Good call! I added an error log in Http1::StreamEncoderImpl::encodeHeader().

alyssawilk · 2018-10-22T14:34:11Z

source/common/http/http2/codec_impl.cc

@@ -48,8 +48,8 @@ template <typename T> static T* remove_const(const void* object) {
 }

 ConnectionImpl::StreamImpl::StreamImpl(ConnectionImpl& parent, uint32_t buffer_limit)
-    : parent_(parent), headers_(new HeaderMapImpl()), local_end_stream_sent_(false),
-      remote_end_stream_(false), data_deferred_(false),
+    : parent_(parent), headers_(new HeaderMapImpl()), metadata_map_(new MetadataMap()),


What do we think about doing lazy creation here? I think it's worth the complexity to not create a map for the vast majority of users who don't need this functionality.

Also if there's one metadata_map_ (at a time) per stream and one decoder per stream I'm not sure I follow why you moved ownership out of the decoder.

Right! Moved metadata_map_ back to decoder. I thought it would be consistent to let StreamImpl hold metadata since it holds headers. But it is definitely not necessary.

alyssawilk · 2018-10-22T14:43:43Z

source/common/http/http2/codec_impl.cc

+  encoder.releasePayload(size_to_copy);
+
+  // Keep submitting extension frames if there is payload left in the encoder.
+  if (encoder.hasNextFrame()) {


so when the caller calls encodeMetadata with a large frame, encodeMetadata calls submitMetadata, submitMetadata results in nghttp2 calling packMetadata, which may result in another submitMetadata?

Any chance we can remove the recursion by moving where we check for hasNextFrame and calling submitMetadata multiple times instead?

You are right. The old code let packMetadata call submitMetadata until there is no metadata left. I can see it is very hard to understand. Changed according to the suggestion.

alyssawilk · 2018-10-22T14:51:50Z

source/common/router/router.h

@@ -288,6 +288,7 @@ class Filter : Logger::Loggable<Logger::Id::router>,
    void decodeHeaders(Http::HeaderMapPtr&& headers, bool end_stream) override;
    void decodeData(Buffer::Instance& data, bool end_stream) override;
    void decodeTrailers(Http::HeaderMapPtr&& trailers) override;
+    void decodeMetadata(Http::MetadataMap&) override { NOT_REACHED_GCOVR_EXCL_LINE; }


I think we're going to need to implement something here. Maybe a TODO instead of marking it as not reached?

alyssawilk · 2018-10-22T14:52:13Z

source/common/upstream/health_checker_impl.h

@@ -280,6 +281,7 @@ class GrpcHealthCheckerImpl : public HealthCheckerImplBase {
    void decodeHeaders(Http::HeaderMapPtr&& headers, bool end_stream) override;
    void decodeData(Buffer::Instance&, bool end_stream) override;
    void decodeTrailers(Http::HeaderMapPtr&&) override;
+    void decodeMetadata(Http::MetadataMap&) override {}


These ones we're probably never going to really use, but we could unit test the functions to avoid a coverage decrease

Good idea! Test added.

alyssawilk · 2018-10-22T14:55:10Z

include/envoy/http/metadata_interface.h

@@ -22,10 +21,13 @@ namespace Http2 {
 const uint8_t METADATA_FRAME_TYPE = 0x4d;
 const uint8_t END_METADATA_FLAG = 0x4;

+// NGHTTP2_MAX_PAYLOADLEN in nghttp2.
+const uint64_t METADATA_MAX_PAYLOAD_SIZE = 16384;


As a sanity check, if nghttp2 changed their NGHTTP2_MAX_PAYLOADLEN to be lower, they'd reject large data chunks and your large metadata frame test would fail when we imported the change, right?

Great question! I increase METADATA_MAX_PAYLOAD_SIZE to 20000, and the test fails at:
source/common/http/http2/codec_impl.cc:623
ASSERT(len >= size_to_copy);

Added a comment to check NGHTTP2_MAX_PAYLOADLEN in case of the assert failure.

Make this as constexpr (perhaps for all constants), and do static_assert against NGHTTP2_MAX_PAYLOADLEN? It will fail at compile time then.

It's a good idea! But I don't think nghttp2 exposes NGHTTP2_MAX_PAYLOADLEN. I changed const to constexpr. Thanks!

soya3129 · 2018-10-24T16:31:20Z

Thanks for the review! I am still working on the doc part. Will update the PR when the doc is ready.

I changed two if conditions to assert since the if condition should not happen, and it increases the test coverage. I am having trouble making tests hit the if condition in onMetadataReceived():
if (!stream) {
return 0
}
It can't be changed to ASSERT in case envoy receives metadata with invalid stream id. I also notice the existing function saveHeader() function has the same problem. I will leave it for now if there is no objection.

Signed-off-by: Yang Song <[email protected]>

alyssawilk

Oh argh, I had draft comments and failed to actually send them out. So sorry for review lag!

alyssawilk · 2018-10-24T17:15:42Z

source/common/http/http2/codec_impl.cc

+    int result = submitMetadata();
+    ASSERT(result == 0);
+    parent_.sendPendingFrames();
+  }


Can we have an ASSERT that hasNextFrame is false, or if failure is possible should there be error handling?

Good idea! Done.

alyssawilk · 2018-10-24T17:21:36Z

source/common/http/http2/codec_impl.cc

+  // Keep submitting extension frames if there is payload left in the encoder.
+  while (metadata_encoder_->hasNextFrame() && count++ <= frame_count) {
+    int result = submitMetadata();
+    ASSERT(result == 0);


Looking at nghttp2_submit_extension the failure modes are specifying a non-registered type (I agree shouldn't happen if metadata is on), insufficient memory (I agree shouldn't happen) and the pack_extension callback not being set (shouldn't happen if metadata is on).

But then it occurred to me (which I should have caught on first pass - sorry!) we're adding H2 Metadata support by default. I think we should probably make this an opt-in feature and add it to the Http2ProtocolOptions. I'd hope you could crib where we added allow_connect support for e2e config examples when you get there.

Also it does strike me as a bit weird we have a return which we assert is always 0. I'd be inclined to keep the assert lower down and keep the function void, or if we think it might legit fail, do error handling.

Made metadata as an opt-in feature (will add to Http2ProtocolOptions later. TODO added). Made submitMetadata a void function, and added assert outside the loop. Thanks for the suggestions!

alyssawilk · 2018-10-24T17:29:18Z

source/common/http/http1/codec_impl.h

@@ -37,6 +37,9 @@ class StreamEncoderImpl : public StreamEncoder,
  void encodeHeaders(const HeaderMap& headers, bool end_stream) override;
  void encodeData(Buffer::Instance& data, bool end_stream) override;
  void encodeTrailers(const HeaderMap& trailers) override;
+  void encodeMetadata(const MetadataMap&) override {
+    ENVOY_LOG(error, "HTTP/1.1 doesn't support METADATA. METADATA is ignored.");


Unit test, just for coverage? :-)

Ah, right! Done.

Personally I would just do NOT_IMPLEMENTED and have this crash but I don't feel strongly about it.

alyssawilk · 2018-10-24T17:35:12Z

test/common/http/http2/codec_impl_test.cc

+  response_encoder_->encodeMetadata(metadata_map);
+}
+
+TEST_P(Http2CodecImplTest, LargeMetadataTest) {


I don't think we have examples of this, but could we maybe have a test of error conditions / bad data? I think you could tweak setupDefaultConnectionMocks to corrupt the data to result in some of the calls failing and make sure nghtttp2 ends up killing the connection.

Add a test with corrupted metadata. Verified exception is thrown.

Signed-off-by: Yang Song <[email protected]>

alyssawilk

Docs are a great addition - thanks for adding them!

alyssawilk · 2018-10-29T17:57:23Z

source/docs/h2_metadata.md

+information is represented by a string key-value pair. For example, users can
+pass along RTT information associated with a stream using a key of "rtt info", and a value of
+"100ms". In Envoy, we call this type of information metadata.
+A stream can be associated with multiple metadatas, and the multiple metadatas


I think the plural of metdata is metadata, so s/metadatas/metadata/ here and below?

I see! Thanks for correcting my English!

alyssawilk · 2018-10-29T17:57:42Z

source/docs/h2_metadata.md

+
+### Limitation
+
+For the ease of implementation and compatibility purpose, metadata will only be


For the ease of implementation and compatibility purpose -> For ease of implementation and compatibility purposes

Done. Thanks!

alyssawilk · 2018-10-29T17:58:22Z

source/docs/h2_metadata.md

+errors or be ignored.
+
+Envoy can consume or add metadata. But for one request (or response), we expect at most one of the
+two operations to be performed.


You think? I'm thinking we had a 3 layer Envoy set up, the middle Envoy could consume metadata from upstream and send metadata downstream.

Sorry I didn't think the limitation through. I would like to avoid the case where metadata added by one filter is consumed by another filter on the same stream. But now I think how to use the filters is up to the users. Removed this limitation.

alyssawilk · 2018-10-29T18:05:47Z

source/docs/h2_metadata.md

+
+Envoy provides the functionality to proxy, process and add metadatas.
+
+## Proxy metadata


Let's have the headers consistent
Proxying meadata
Consuming metadata
Inserting metadata

They are better headers! Fixed.

alyssawilk · 2018-10-29T18:07:05Z

source/docs/h2_metadata.md

+
+(To be implemented)
+
+If Envoy needs to take actions when a metadata is received, users should


s/a metadata/metadata/
or "a metadata frame"?

Done. Thanks!

alyssawilk · 2018-10-29T18:07:25Z

source/docs/h2_metadata.md

+it untouched.
+
+If Envoy needs to parse a metadata sent on a response from upstream to downstream, a
+StreamEncodeFilter should be created. The interface to override is


Oops! Fixed.

alyssawilk · 2018-10-29T18:08:11Z

source/docs/h2_metadata.md

+The METADATA frame payload is not subject to HTTP/2 flow control, but the size
+of the payload is bounded by the maximum frame size negotiated in SETTINGS.
+There are no restrictions on the set of octets that may be used in keys or values.
+TODO(soya3129): decide if we allow METADATA frame to terminate a stream.


I'd say no for now. Folks can send 0 length data frame.

alyssawilk · 2018-10-29T18:09:40Z

source/docs/h2_metadata.md

+If not specified, all the metadata received by Envoy is proxied to the next hop
+unmodified. Note that, we do not guarantee the same frame order will be preserved from
+hop by hop. That is, metadata from upstream at the beginning of a stream can be
+received by the downstream at the end of the stream.


So I agree that we don't guarantee anything with respect to body order (and doccing that up is an excellent idea!) but I think we can and probably should make sure we don't mess with end stream. For example if we forward end-stream ahead of metadata in a response headed downstream that stream may be torn down before the metadata is received. I think we have to make sure that if we've read metadata we defer end stream until metadata is flushed through the filter chain. What do you think?

I totally agree! I will follow the suggestion in the implementation.

Signed-off-by: Yang Song <[email protected]>

soya3129 · 2018-10-29T19:00:54Z

Please hold the review until I fix the check failure. Sorry about the noisy notification! I will address the check failure and the documentation comments together.

alyssawilk

Cool, getting there!

I think I'm pretty good on the code/test front (may do one more pass on the docs) so may be worth @mattklein123 doing a pass if he has time.

alyssawilk · 2018-10-29T18:51:11Z

source/common/http/http2/codec_impl.cc

@@ -863,8 +870,10 @@ ConnectionImpl::Http2Options::Http2Options(const Http2Settings& http2_settings)
  if (http2_settings.hpack_table_size_ != NGHTTP2_DEFAULT_HEADER_TABLE_SIZE) {
    nghttp2_option_set_max_deflate_dynamic_table_size(options_, http2_settings.hpack_table_size_);
  }
-
-  nghttp2_option_set_user_recv_extension_type(options_, METADATA_FRAME_TYPE);
+  // TODO(soya3129): change to ENABLE_METADATA when it's available.


AFIK nghttp2 won't take this until we make it an rfc, which is probably 1-2 years out given we haven't started anything IETF-side and it takes a while. If it were me I wouldn't TODO this (I did it for CONNECT as it was about 3 months from landing) but it's totally your call :-)

Got it! Removed.

alyssawilk · 2018-10-29T18:58:09Z

test/common/http/http2/codec_impl_test.cc

+class Http2CodecImplTestEnableMetadata : public Http2CodecImplTest {
+public:
+  Http2CodecImplTestEnableMetadata() : Http2CodecImplTest(true) {}
+};


super nitty - I'd rather defer client_http2settings_ initialization to initialize() than subclass to set a boolean, but I'll leave it up to you.

I see! Done. It's definitely cleaner.

mattklein123

In general LGTM, a few small comments. Thank you!

mattklein123 · 2018-10-30T20:25:59Z

include/envoy/http/codec.h

+   * Called with decoded METADATA.
+   * @param decoded METADATA.
+   */
+  virtual void decodeMetadata(MetadataMap& metadata_map) PURE;


Do we want to pass this as a unique_ptr with move semantics so that the callee can take ownership if they desire? I suspect this is what you want, much like the other header decode APIs.

Fixed as suggested. Thanks!

mattklein123 · 2018-10-30T20:27:28Z

source/common/http/http1/codec_impl.h

@@ -37,6 +37,9 @@ class StreamEncoderImpl : public StreamEncoder,
  void encodeHeaders(const HeaderMap& headers, bool end_stream) override;
  void encodeData(Buffer::Instance& data, bool end_stream) override;
  void encodeTrailers(const HeaderMap& trailers) override;
+  void encodeMetadata(const MetadataMap&) override {
+    ENVOY_LOG(error, "HTTP/1.1 doesn't support METADATA. METADATA is ignored.");


Personally I would just do NOT_IMPLEMENTED and have this crash but I don't feel strongly about it.

mattklein123 · 2018-10-30T20:29:30Z

source/common/http/http2/codec_impl.cc

+
+void ConnectionImpl::StreamImpl::encodeMetadata(const MetadataMap& metadata_map) {
+  if (!parent_.allow_metadata_) {
+    ENVOY_CONN_LOG(error, "Connection doesn't allow metadata", parent_.connection_);


When do we envision this error happening? I'm mainly wondering if this should be a stat? Or should it just crash? Or just be a debug statement? I'm leery of error logs being useful to people.

Good point! I thought it could stop encoding metadata in case metadata is not allowed. Now I realize the check should be done in ActiveStream or UpstreamRequest. Changed to ASSERT.

Correct my comments above: we don't need to check allow_metadata_ not because of the reasons I stated. If allow_metadata_ is false, nghttp2 will not pass metadata to codec. We don't need to check it in envoy code anywhere. Sorry about the wrong information!

mattklein123 · 2018-10-30T20:30:25Z

source/common/http/http2/codec_impl.cc

@@ -194,6 +215,15 @@ void ConnectionImpl::StreamImpl::submitTrailers(const HeaderMap& trailers) {
  ASSERT(rc == 0);
 }

+void ConnectionImpl::StreamImpl::submitMetadata() {
+  ASSERT(stream_id_ > -1);


We should never encode metadata on stream 0/connection either, right? I think this can be > 0?

Right, we don't have connection usage case for now. Changed to > 0.

mattklein123 · 2018-10-30T20:31:48Z

source/common/http/http2/codec_impl.cc

+MetadataDecoder& ConnectionImpl::StreamImpl::getMetadataDecoder() {
+  if (metadata_decoder_ == nullptr) {
+    auto cb =
+        std::bind(&ConnectionImpl::StreamImpl::onMetadataDecoded, this, std::placeholders::_1);


nit: I find bind hard to read, can you just pass an inline lambda / std::function?

Changed to inline lambda.

mattklein123 · 2018-10-30T20:32:57Z

source/common/http/http2/codec_impl.cc

+  // NGHTTP2_MAX_PAYLOADLEN is consistent with METADATA_MAX_PAYLOAD_SIZE.
+  ASSERT(len >= size_to_copy);
+
+  Buffer::OwnedImpl& p = reinterpret_cast<Buffer::OwnedImpl&>(encoder.payload());


Doing this type of cast is a bit of an anti-pattern. Why is this needed?

Oops! Fixed.

mattklein123 · 2018-10-30T20:34:04Z

source/common/http/http2/codec_impl.h

@@ -210,6 +221,8 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable<Log
        [this]() -> void { this->pendingSendBufferLowWatermark(); },
        [this]() -> void { this->pendingSendBufferHighWatermark(); }};
    HeaderMapPtr pending_trailers_;
+    std::unique_ptr<MetadataDecoder> metadata_decoder_{nullptr};


nit: {nullptr} not needed. Default init will do this. Same next line.

Signed-off-by: Yang Song <[email protected]>

mattklein123 · 2018-11-03T23:47:52Z

Marking as waiting.

/wait

Signed-off-by: Yang Song <[email protected]>

soya3129 · 2018-11-04T20:21:04Z

Very sorry about delay!

Signed-off-by: Yang Song <[email protected]>

mattklein123

Generally LGTM. Will defer to @alyssawilk for the rest of the review. Thank you!

mattklein123 · 2018-11-07T20:41:59Z

include/envoy/http/codec.h

+   * Called with decoded METADATA.
+   * @param decoded METADATA.
+   */
+  virtual void decodeMetadata(std::unique_ptr<MetadataMap> metadata_map) PURE;


I would pass as std::unique_ptr<MetadataMap>&& (see style doc)

Signed-off-by: Yang Song <[email protected]>

soya3129 · 2018-11-07T23:08:41Z

Thanks for the review!

Signed-off-by: Yang Song <[email protected]>

lizan · 2018-11-08T00:59:58Z

source/docs/h2_metadata.md

+If Envoy needs to parse a metadata sent on a request from downstream to upstream, a
+StreamDecodeFilter should be created. The interface to override is
+
+FilterMetadataStatus StreamDecoderFilter::decodeMetadata(std::unique\_ptr\<MetadataMap\> metadata);


MetadataMapPtr&&

Oops! Fixed.

lizan · 2018-11-08T01:01:24Z

source/common/http/http2/metadata_decoder.h

@@ -58,18 +59,15 @@ class MetadataDecoder : Logger::Loggable<Logger::Id::http2> {
   */
  bool decodeMetadataPayloadUsingNghttp2(bool end_metadata);

+  // Metadata that is currently under decoding.
+  std::unique_ptr<MetadataMap> metadata_map_;


nit: MetadataMapPtr

+1, plus "is currently under decoding." -> "is currently being decoded."?

Fixed the code and English. Thanks!

lizan · 2018-11-08T01:03:58Z

include/envoy/http/metadata_interface.h

@@ -22,10 +21,13 @@ namespace Http2 {
 const uint8_t METADATA_FRAME_TYPE = 0x4d;
 const uint8_t END_METADATA_FLAG = 0x4;

+// NGHTTP2_MAX_PAYLOADLEN in nghttp2.
+const uint64_t METADATA_MAX_PAYLOAD_SIZE = 16384;


Make this as constexpr (perhaps for all constants), and do static_assert against NGHTTP2_MAX_PAYLOADLEN? It will fail at compile time then.

alyssawilk

Looking great! just a few more nits and you're good to go.

alyssawilk · 2018-11-08T14:34:38Z

source/common/http/http2/metadata_decoder.h

@@ -19,7 +20,10 @@ class MetadataEncoderDecoderTest_VerifyEncoderDecoderOnMultipleMetadataMaps_Test
 // map of string key value pairs.
 class MetadataDecoder : Logger::Loggable<Logger::Id::http2> {
 public:
-  MetadataDecoder(uint64_t stream_id, MetadataCallback cb);
+  /**
+   * @param metadata_map is where to save the decoded metadata.


update comment?

Oops! Fixed.

alyssawilk · 2018-11-08T14:35:13Z

source/common/http/http2/metadata_decoder.h

@@ -58,18 +59,15 @@ class MetadataDecoder : Logger::Loggable<Logger::Id::http2> {
   */
  bool decodeMetadataPayloadUsingNghttp2(bool end_metadata);

+  // Metadata that is currently under decoding.
+  std::unique_ptr<MetadataMap> metadata_map_;


+1, plus "is currently under decoding." -> "is currently being decoded."?

alyssawilk · 2018-11-08T14:37:29Z

source/docs/h2_metadata.md

+If not specified, all the metadata received by Envoy is proxied to the next hop
+unmodified. Note that, we do not guarantee the same frame order will be preserved from
+hop by hop. That is, metadata from upstream at the beginning of a stream can be
+received by the downstream at the end of the stream.


Worth commenting somewhere that empty maps will not be serialized out, i.e. if you clear the data it will functionally result in metadata being removed for the next hop?

Good idea! Done.

Signed-off-by: Yang Song <[email protected]>

soya3129 · 2018-11-08T17:28:03Z

Thanks for the reviews!

alyssawilk

@lizan any final comments?

soya3129 · 2018-11-15T16:32:23Z

Cc Biren the metadata change.
@birenroy

Description: Add MetadataEncoder and MetadataDecoder to codec. I also corrected some of the design decisions in MetadataEnocder and MetadataDecoder so that they are more suitable for codec. Those corrections includes: 1. let StreamImpl owns the decoded metadata, not the decoder. 2. Remove stream_id field in decoder and encoder since stream id is stored in StreamImpl. 3. Remove set/getMaxMetadataSize(), because nghttp2 uses default max frame size. This is the second step towards supporting metadata in envoy. The next step would be add filters to add/delete/modify metadata. Risk Level: Low. Not used. Testing: Unit test. Docs Changes: Release Notes: envoyproxy#2394 Signed-off-by: Yang Song <[email protected]> Signed-off-by: Fred Douglas <[email protected]>

mattklein123 assigned alyssawilk and mattklein123 Oct 20, 2018

alyssawilk reviewed Oct 22, 2018

View reviewed changes

soya3129 changed the title ~~Metadata codec new~~ Add Metadata to codec Oct 23, 2018

Yang Song added 4 commits October 24, 2018 15:29

Add metadata encoder and decoder to codec

24b078c

Signed-off-by: Yang Song <[email protected]>

delete old metadata_interface.h

baf9bbb

Signed-off-by: Yang Song <[email protected]>

remove unused variable

7162720

Signed-off-by: Yang Song <[email protected]>

address review comments

85f34ef

Signed-off-by: Yang Song <[email protected]>

alyssawilk reviewed Oct 26, 2018

View reviewed changes

proof reading

e7646a0

Signed-off-by: Yang Song <[email protected]>

soya3129 requested a review from lizan as a code owner October 26, 2018 15:38

soya3129 force-pushed the metadata_codec_new branch from ff05613 to e7646a0 Compare October 26, 2018 16:03

merge conflict

9e0f77c

Signed-off-by: Yang Song <[email protected]>

alyssawilk reviewed Oct 29, 2018

View reviewed changes

address review comments

25cee11

Signed-off-by: Yang Song <[email protected]>

alyssawilk reviewed Oct 29, 2018

View reviewed changes

mattklein123 reviewed Oct 30, 2018

View reviewed changes

Address comments from alyssawilk@

7100557

Signed-off-by: Yang Song <[email protected]>

repokitteh-read-only bot added the waiting label Nov 3, 2018

address review comments

f8ee7b5

Signed-off-by: Yang Song <[email protected]>

repokitteh-read-only bot removed the waiting label Nov 4, 2018

Yang Song added 3 commits November 4, 2018 15:23

fix format

9ae2f9f

Signed-off-by: Yang Song <[email protected]>

fix format

48e26b6

Signed-off-by: Yang Song <[email protected]>

fix coverage&format

e713dc8

Signed-off-by: Yang Song <[email protected]>

soya3129 force-pushed the metadata_codec_new branch from e713dc8 to 48e26b6 Compare November 5, 2018 14:37

merge conflict: remove source/common/http/http2/metadata_interface.h

10699aa

fix format

a56daa3

Signed-off-by: Yang Song <[email protected]>

soya3129 force-pushed the metadata_codec_new branch from 2f2d2f5 to 48e26b6 Compare November 5, 2018 15:36

fix clang_tidy and coverage failure

31f0838

Signed-off-by: Yang Song <[email protected]>

soya3129 force-pushed the metadata_codec_new branch from 6fd54da to 31f0838 Compare November 5, 2018 16:43

Yang Song added 3 commits November 6, 2018 11:04

clang tidy fix

5bda379

Signed-off-by: Yang Song <[email protected]>

merge conflict: remove metadata_interface.h

2da119c

Signed-off-by: Yang Song <[email protected]>

Fix wrong return value in metadata docs

3fb8980

Signed-off-by: Yang Song <[email protected]>

mattklein123 reviewed Nov 7, 2018

View reviewed changes

mattklein123 added the waiting label Nov 7, 2018

Yang Song added 2 commits November 7, 2018 18:02

Change unique_ptr to comply style guide

33e65a0

Signed-off-by: Yang Song <[email protected]>

Make decodeMetadata_ consistent with other decoding functions

5bdf989

Signed-off-by: Yang Song <[email protected]>

repokitteh-read-only bot removed the waiting label Nov 7, 2018

fix format

0bd1d21

Signed-off-by: Yang Song <[email protected]>

lizan reviewed Nov 8, 2018

View reviewed changes

alyssawilk reviewed Nov 8, 2018

View reviewed changes

Address review comments

90786a4

Signed-off-by: Yang Song <[email protected]>

alyssawilk approved these changes Nov 8, 2018

View reviewed changes

lizan approved these changes Nov 8, 2018

View reviewed changes

alyssawilk merged commit 7b96648 into envoyproxy:master Nov 8, 2018


		### Limitation

		For the ease of implementation and compatibility purpose, metadata will only be


		Envoy provides the functionality to proxy, process and add metadatas.

		## Proxy metadata


		(To be implemented)

		If Envoy needs to take actions when a metadata is received, users should

Add Metadata to codec #4796

Add Metadata to codec #4796

Conversation

soya3129 commented Oct 19, 2018

alyssawilk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soya3129 commented Oct 24, 2018

alyssawilk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alyssawilk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soya3129 commented Oct 29, 2018

alyssawilk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 commented Nov 3, 2018

soya3129 commented Nov 4, 2018

mattklein123 left a comment

Choose a reason for hiding this comment