request_info: initial implementation of dynamic metadata object #3918

curiouserrandy · 2018-07-20T21:18:07Z

Description: Add functionality for setting and getting arbitrarily typed information for a request by filters associated with that request. This is the initial PR to implement https://docs.google.com/document/d/1GNawM59Pp09WZ34zqJYiO_qmERjuGbOUeAqF1GG6v9Q/edit. Following this PR I intend to switch all current uses of dynamic metadata over to this method, then implement the %DYNAMIC_METADATA% substitution template for user defined headers.

Risk Level: Low (no functionality change in this PR).

Testing: Unit tests.

Docs Changes: None.

Release Notes: None.

Fixes #151

Signed-off-by: Randy Smith <rdsmith@google.com>

zuercher

Thanks for taking this on. I think the overall structure makes sense, but I have some questions on the details.

zuercher · 2018-07-23T18:42:27Z

test/common/request_info/dynamic_metadata_impl_test.cc

+  EXPECT_THROW(
+      dynamic_metadata().setData("test_1", std::make_unique<TestStoredType>(2, nullptr, nullptr)),
+      EnvoyException);
+}


Please add tests for getData with an unknown name and getData with the wrong type.

Good catches; thanks. Done.

zuercher · 2018-07-23T18:45:32Z

include/envoy/request_info/dynamic_metadata.h

+  virtual ~DynamicMetadata(){};
+
+  // May only be called once for a particular |data_name|
+  template <typename T> void setData(absl::string_view data_name, std::unique_ptr<T>&& data) {


Please add doxygen-style comments for these methods per the style guide. This method in particular should probably get a note that setData with a duplicate name will destroy data.

Done (sorry to have missed this).

In terms of a duplicate name destroying data: If you ask me again I'll do it, but to my mind calling it with a duplicate name is an interface violation, so it feels a little funny to me to say "Don't do this. Oh, and if you do, the owning pointer you passed in will be destructed, resulting in deallocation of the data you passed."

(It occurs to me that this interface is incomplete, in that it's not possible for a consumer that doesn't have prior knowledge to be sure it isn't going to do something that be an interface contract violation. I've added a hasData<> method to solve that problem. Let me know if you want other changes/reversions in this space.)

That's fine. I suppose the pointer has to be create inline with the call or else moved, so it shouldn't be surprising that it's freed on error.

zuercher · 2018-07-23T18:48:15Z

include/envoy/request_info/request_info.h

+   * More general dynamic metadata object.
+   * TODO(rdsmith): Replace uses of above with this object.
+   */
+  virtual DynamicMetadata& dynamicMetadata2() PURE;


Perhaps leave this out for now and replace the appropriate methods when you're ready to swap them in?

zuercher · 2018-07-23T18:55:32Z

include/envoy/request_info/string_accessor.h

+namespace Envoy {
+namespace RequestInfo {
+
+class StringAccessor {


This class should get doxygen comments.

zuercher · 2018-07-23T18:57:18Z

source/common/request_info/string_accessor_impl.h

+namespace Envoy {
+namespace RequestInfo {
+
+class StringAccessorImpl : public StringAccessor {


How do you expect this to be used? I would imagine that dynamic metadata types would normally just extend StringAccessor and provide an asString() based on some internal data.

Fair point, and if you'd like I'm happy to nuke it--it certainly isn't going to cause anyone grief to create the equivalent. My thought was that for many such situations, the source of the data might not want to deal with the ownership/lifetime issues involved in owning the string data (or whatever is used to create the string data) and hence might simple want to hand over to DynamicMetadata an accessor that can be used for user-defined headers and forget about it.

My inclination would be to leave it out, but let's see what other people think.

zuercher · 2018-07-23T19:03:11Z

include/envoy/request_info/dynamic_metadata.h

+  template <typename T> class Traits {
+  public:
+    static size_t getTypeId() {
+      static const size_t type_id = ++type_id_index_;


I don't think this access to type_id_index_ is thread safe.

Right, sorry, I haven't yet gotten re-used to doing multi-threaded programming. Done.

Signed-off-by: Randy Smith <rdsmith@google.com>

curiouserrandy · 2018-07-25T14:56:54Z

Thanks for the review! I think it's ready for another round (only trivial changes since all auto-tests passed on my last push), but feel free to wait until the auto-tests pass on this commit before diving back in.

curiouserrandy · 2018-07-25T14:57:19Z

Actually, my last changes included a merge to ToT, so maybe it's worth waiting :-}.

curiouserrandy · 2018-07-25T16:01:34Z

Ready for next round of review.

zuercher · 2018-07-25T17:41:21Z

@htuch and/or @mattklein123 ptal

rshriram · 2018-07-26T13:43:12Z

is requestInfo specific to HTTP? I remember seeing folks pushing it up so that TCP filters could log similar info (for access logs, etc.). If so, then can this be used as a way to share info across filters in TCP stack as well?

curiouserrandy · 2018-07-26T15:58:59Z

For anyone following along: I just responded to this question on the doc @ https://docs.google.com/document/d/1GNawM59Pp09WZ34zqJYiO_qmERjuGbOUeAqF1GG6v9Q/edit?disco=AAAACB6UHU4. TL;DR: No basic reason it couldn't, though stream multiplexing needs to be considered, but I'm not currently planning to do that work.

Signed-off-by: Randy Smith <rdsmith@google.com>

curiouserrandy · 2018-07-26T19:34:37Z

The newly added probe functions are to allow for better error handling when implementing user-defined headers using this mechanism. I also added some doxygen comments and const markers on methods.

htuch

Overall, this looks really clean, nice!

htuch · 2018-07-27T01:42:39Z

include/envoy/request_info/BUILD

+    hdrs = ["dynamic_metadata.h"],
+    external_deps = ["abseil_optional"],
+    deps = [
+    ],


Nit: No need for empty deps here and below.

htuch · 2018-07-27T03:40:54Z

include/envoy/request_info/dynamic_metadata.cc

+namespace Envoy {
+namespace RequestInfo {
+
+std::atomic<size_t> DynamicMetadata::type_id_index_(0u);


I think I'm a bit skeptical about state living in include/envoy. But, I don't see a good alternative, since I gather that other parts of the interface depend on this.

Agreed on both counts. Happy to brainstorm with you on this--I couldn't come up with a different way to do it with the templated interface.

The other alternative is to put it in source/common/request_info. There is precedent here; we have various deps on assert.h, protobuf.h in source/common/ already in include/envoy, but no .cc. I think it would be least bad to do this than add a .cc to include/envoy. But, this is bike shedding a bit. @mattklein123 ?

I don't have a strong opinion. I guess what @htuch proposes is more inline w/ what we already have. I might drop a comment in here explaining why this is the way it is if we keep it this way since it's so unlike anything else we have.

I have no feelings at all about how to implement this, so I moved the .cc file over to source/common/request_info and re-jiggered the dependencies. I do want a review on that move before landing, LGTM or no :-}.

Do you need to push? I don't see any changes.

Nope, sorry, still working on other changes so haven't pushed yet. We got into a bit of a bikeshed internally around one of Harvey's other comments.

Tangent question on github comment use: I usually respond to these comments as I make changes in my local checkout, then flag that I'm ready for a new review after I push and the auto tests pass. Is there a better pattern that would confuse reviewers less? Or is it just that there was a long (not yet complete) gap between comment and push?

There is no defined pattern, I just noticed that you said "I do want a review" and there was nothing to review. :) No problem.

This request is now moot--the re-implementation requested by Harvey elsewhere means that we no longer need the typeid jiggery.

htuch · 2018-07-27T03:43:28Z

include/envoy/request_info/dynamic_metadata.h

+  /**
+   * @param data_name the name of the data being set.
+   * @param data an owning pointer to the data to be stored.
+   * Note that it is an error to call setData() twice with the same data_name.


Is this limitation going to be an issue anywhere?

I presume you mean in the currently existing uses of dynamic metadata?

The simple answer is "I don't know--haven't really started on the conversion yet". The more complex answer is:

This was put in as a way to keep the use of dynamic metadata under control, specifically, to make sure without dependency management (since that implementation comes later) that only one "source" sets any particular piece of data.

The structure that allows for lazy evaluation also allows for the dynamic metadata source to change DM entries without that needing to go through the DM interface. So if there are single sources that want to change DM over the lifetime of the request, that can still be implemented within this restriction. The only thing this really forbids is something setting a piece of data and then something else overriding it.

As we discussed offline, I expanded the comment here to give more of an explanation for this choice. I looked at STYLE.md to see if adding something there made sense, but it didn't seem to me that it would. I scanned the names of the other .md files in that directory and didn't see an obvious place to put something like this. Willing to take a look and see if there's some specific place you think I should put more information?

OK, as long as this is covered in the expanded comment, all good.

htuch · 2018-07-27T03:45:58Z

include/envoy/request_info/dynamic_metadata.h

+  }
+
+  /**
+   * @param data_name the name of the data being set.


This method doesn't set..

Oops. Done.

htuch · 2018-07-27T03:46:06Z

include/envoy/request_info/dynamic_metadata.h

+  }
+
+  /**
+   * @param data_name the name of the data being set.


This method doesn't set..

htuch · 2018-07-27T03:50:44Z

include/envoy/request_info/string_accessor.h

+ * Contains a string in a form which is usable with DynamicMetadata and
+ * allows lazy evaluation if needed.
+ */
+class StringAccessor {


Not sure I fully grok how this is used from the comments here and the rest of the PR.

So the basic idea here is that "StringAccessor" is how user defined headers (working on the PR now) get access to DynamicMetadata. That may mean I should pull it into that PR, and (?) move its definition over to include/envoy/router. Or maybe I should just expand the comment here to say "Intended to be a globally known interface for accessing string data on DynamicMetadata".

What's your preference?

As discussed offline, pushing to next PR.

htuch · 2018-07-27T03:51:53Z

source/common/request_info/dynamic_metadata_impl.cc

+                                         void (*destructor)(void*)) {
+  if (data_storage_.find(data_name) != data_storage_.end()) {
+    (*destructor)(data);
+    throw EnvoyException("DynamicMetadata::setData<T> called twice with same name.");


Should this just be an ASSERT?

My inclination is against, but there may be issues I'm not thinking about.

My argument would be that this can occur because of a configuration error (imagine two filters that are variants of each other that set the same output data, and someone configures them in the same filter chain), and I believe that throws result in cleanly handled configuration errors where asserts always crash. But happy to take your guidance.

I agree, that is a valid use case.

htuch · 2018-07-27T03:53:00Z

source/common/request_info/dynamic_metadata_impl.cc

+    throw EnvoyException("DynamicMetadata::setData<T> called twice with same name.");
+  }
+
+  data_storage_[static_cast<std::string>(data_name)] = {type_id, data, destructor};


You can static cast string view to string?

Yep. https://github.com/abseil/abseil-cpp/blob/master/absl/strings/string_view.h#L328

TIL. I'm not sure it's the clearest thing to me as a reader, but I don't objective enough to want to ask you to change it :)

htuch · 2018-07-27T03:53:22Z

source/common/request_info/dynamic_metadata_impl.cc

+      it.second.destructor_(it.second.ptr_);
+    }
+  }
+  data_storage_.clear();


Won't the implicit destructor do this for you?

Yeah, this is just my discomfort with leaving pointers into freed memory around for even a little bit of time after they're freed. But agreed, there's really no point. Removed.

htuch · 2018-07-27T03:54:32Z

source/common/request_info/dynamic_metadata_impl.cc

+    throw EnvoyException("DynamicMetadata::setData<T> called twice with same name.");
+  }
+
+  data_storage_[static_cast<std::string>(data_name)] = {type_id, data, destructor};


Why not combine data name + type ID in the key, so that different types have independent namespaces?

So I'm inclined to believe that, philosophically, having a single text based namespace is the right thing. The concrete example is that if someone tries to access data in a UDH that they set in a filter but didn't use StringAccessor, they'll get a "wrong type" error rather than a "data not found" error. And I'm having a hard time coming up with an example where I'd want two pieces of data with different types to have the same reverse DNS name; when would that make sense? Maybe if they were two different ways of saying the same thing? But in that case, why not have a single class with two different accessors? (Other than UDH, which needs StringAccessor.)

As discussed in person, I agree; we're essentially getting dynamic type checks here, which is a good thing.

Signed-off-by: Randy Smith <rdsmith@google.com>

…icMetadata Signed-off-by: Randy Smith <rdsmith@google.com>

curiouserrandy · 2018-07-27T19:52:25Z

Harvey, PTAL? I think this is ready for another round of review.

I'm getting test timeouts occasionally on tsan (different ones running locally than in CI); let me know what I should be doing about that.

htuch · 2018-07-31T02:17:03Z

TSAN timeouts are in @envoy//test/extensions/filters/network/thrift_proxy:filter_integration_test, so unrelated to your PR. I've kicked CI. CC @alyssawilk (Envoy maintainer on-call, integration test deflake Jedi) and @zuercher (Thrift extension owner).

htuch

LGTM modulo remaining comments, this is great.

htuch · 2018-07-31T02:25:14Z

include/envoy/request_info/dynamic_metadata.cc

+namespace Envoy {
+namespace RequestInfo {
+
+std::atomic<size_t> DynamicMetadata::type_id_index_(0u);


The other alternative is to put it in source/common/request_info. There is precedent here; we have various deps on assert.h, protobuf.h in source/common/ already in include/envoy, but no .cc. I think it would be least bad to do this than add a .cc to include/envoy. But, this is bike shedding a bit. @mattklein123 ?

htuch · 2018-07-31T02:30:39Z

include/envoy/request_info/dynamic_metadata.h

+   * Note that it is an error to access data that has not previously been set.
+   */
+  template <typename T> const T& getData(absl::string_view data_name) const {
+    return *static_cast<T*>(getDataGeneric(data_name, Traits<T>::getTypeId()));


Did you consider adding an abstract empty class DynamicMetadataObject that all eligible metadata object inherit from? That way, you could us a safer dynamic_cast here to catch type violations via RTTI. This is what is done for TLS, see https://github.com/envoyproxy/envoy/blob/master/include/envoy/thread_local/thread_local.h#L16 and its uses (https://github.com/envoyproxy/envoy/blob/master/include/envoy/thread_local/thread_local.h#L41).

Done. That's close to a complete rewrite, so may require a fuller re-review. It also resulted in changes to the design doc (https://docs.google.com/document/d/1GNawM59Pp09WZ34zqJYiO_qmERjuGbOUeAqF1GG6v9Q/edit?pli=1); the TL;DR is that setData now just takes a pointer to the abstract empty class and that since dynamic_cast<> is used for retrieval, any type that the actual type of the stored data can be safely cast to will work.

It also made some other changes obsolete; I'll try to mark those individually.

htuch · 2018-07-31T02:32:29Z

source/common/request_info/dynamic_metadata_impl.cc

+}
+
+bool DynamicMetadataImpl::hasDataWithName(absl::string_view data_name) const {
+  return (data_storage_.find(data_name) != data_storage_.end());


Nit: return data_storage_.count(data_name) > 0;

htuch · 2018-07-31T02:32:50Z

source/common/request_info/dynamic_metadata_impl.cc

+bool DynamicMetadataImpl::hasDataGeneric(absl::string_view data_name, size_t type_id) const {
+  const auto& it = data_storage_.find(data_name);
+
+  return (it != data_storage_.end() && it->second.typeid_ == type_id);


NIt: parens not needed.

Moot, but done in the other method.

htuch · 2018-07-31T02:34:53Z

include/envoy/request_info/dynamic_metadata.h

+  template <typename T> class Traits {
+  public:
+    static size_t getTypeId() {
+      static const size_t type_id = type_id_index_.fetch_add(1);


It's a shame we can't just get at RTTI for this BTW, but I'm guessing you've gone down that path..

Nope; all my recent work has been in codebases that avoided RTTI like the plague. But the interface shift you suggest doesn't do us any good without RTTI, and agreed that the C++ internal implementation is likely to be better than mine :-}, so done.

htuch · 2018-07-31T02:35:43Z

source/common/request_info/dynamic_metadata_impl.h

+    void (*destructor_)(void*);
+  };
+
+  std::map<std::string, Data, std::less<>> data_storage_;


Nit: is there a reason this is ordered? Since we're not traversing, I don't know if there is a strong case here.

This turns out to be necessary in order to use an absl::string_view as a find() argument. See https://en.cppreference.com/w/cpp/container/map/find, specifically instances 3 & 4 of the method.

htuch · 2018-07-31T02:36:03Z

test/common/request_info/dynamic_metadata_impl_test.cc

+    }
+  }
+
+  int Access() const {


Nit: s/Access/access/ for Envoy style

Ooops. Done.

htuch · 2018-07-31T02:37:20Z

test/common/request_info/dynamic_metadata_impl_test.cc

+    }
+  }
+
+  int Access() const {


Does this make sense as const given it mutates stuff, or is access count hidden and so it's fine?

So this strikes me as the bitwise const versus conceptually const argument, on which I've always landed on the conceptual side. Also note that the function must be const, as dynamic_metadata() always returns a const reference for stored functions. That + this being in a test file makes me think that this is fine. Let me know if you disagree.

htuch · 2018-07-31T02:37:51Z

test/common/request_info/dynamic_metadata_impl_test.cc

+public:
+  DynamicMetadataImplTest() { ResetDynamicMetadata(); }
+
+  void ResetDynamicMetadata() { dynamic_metadata_ = std::make_unique<DynamicMetadataImpl>(); }


Nit: resetDynamicMetadata.

Signed-off-by: Randy Smith <rdsmith@google.com>

…icMetadata Signed-off-by: Randy Smith <rdsmith@google.com>

Signed-off-by: Randy Smith <rdsmith@google.com>

htuch

Woah, way simpler. I'm good with this one over the previous version, seems easier to grok, thanks for taking the time to iterate on this.

htuch · 2018-08-01T22:43:47Z

include/envoy/request_info/dynamic_metadata.h

+
+class DynamicMetadata {
+public:
+  class DynamicMetadataObject {


Nit: If this is going to be nested in DynamicMetadata, you can just call it Object.

htuch · 2018-08-01T22:46:07Z

source/common/request_info/dynamic_metadata_impl.cc

+  const auto& it = data_storage_.find(data_name);
+
+  if (it == data_storage_.end()) {
+    throw EnvoyException("DynamicMetadata::getData<T> called for unknown data name.");


Nit: getDataGeneric

I intentionally made it getData<> as the receiver of the throw will be above the public API of the DynamicMetadata class, and hence will be more interested in what they did wrong in targeting that public API. Given that, do you still want it spelled getDataGeneric?

htuch · 2018-08-01T22:46:29Z

source/common/request_info/dynamic_metadata_impl.h

+class DynamicMetadataImpl : public DynamicMetadata {
+public:
+  DynamicMetadataImpl() {}
+  ~DynamicMetadataImpl() override{};


Don't think we need either of these empty ones.

htuch · 2018-08-01T22:47:23Z

test/common/request_info/dynamic_metadata_impl_test.cc

+  size_t access_count_1 = 0u;
+  size_t access_count_2 = 0u;
+  size_t destruction_count = 0u;
+  const int ValueOne = 5;


Nit: static const

Signed-off-by: Randy Smith <rdsmith@google.com>

curiouserrandy · 2018-08-02T15:49:50Z

Ok, ready for another round of review. (The test failure on the last round was a timeout that didn't seem to be related to my changes.)

htuch

Rad, LGTM; @zuercher @mattklein123 ?

mattklein123

Looks great. Few small comments.

mattklein123 · 2018-08-03T16:57:09Z

source/common/request_info/dynamic_metadata_impl.h

+  const Object* getDataGeneric(absl::string_view data_name) const override;
+
+private:
+  std::map<std::string, std::unique_ptr<Object>, std::less<>> data_storage_;


q: What's the reason for specifying std::less here? Comment?

Added a comment.

mattklein123 · 2018-08-03T16:58:51Z

source/common/request_info/dynamic_metadata_impl.cc

+  if (data_storage_.find(data_name) != data_storage_.end()) {
+    throw EnvoyException("DynamicMetadata::setData<T> called twice with same name.");
+  }
+  data_storage_[static_cast<std::string>(data_name)] = std::move(data);


nit: using a cast here is a bit strange to me. Is this just to force type coercion when it doesn't happen otherwise?

Yes; absl::string_view doesn't convert to std::string unless you ask it nicely :-}. Added a comment.

mattklein123 · 2018-08-03T16:59:46Z

test/common/request_info/dynamic_metadata_impl_test.cc

+
+TEST_F(DynamicMetadataImplTest, NameConflict) {
+  dynamic_metadata().setData("test_1", std::make_unique<SimpleType>(1));
+  EXPECT_THROW(dynamic_metadata().setData("test_1", std::make_unique<SimpleType>(2)),


Can we do EXPECT_THROW_WITH_MESSAGE for here and below?

Signed-off-by: Randy Smith <rdsmith@google.com>

curiouserrandy · 2018-08-03T18:25:39Z

Ready for next round of review (changes were minor enough that I'm assuming any problems would have been caught with bazel test //test/..., which I did on my workstation).

mattklein123

Awesome, thanks!

curiouserrandy added 8 commits July 19, 2018 16:27

Initial implementation of DynamicMetadata & StringAccessor

de5288a

Signed-off-by: Randy Smith <rdsmith@google.com>

Integrated (I hope) with RequestInfo.

3b1250e

Signed-off-by: Randy Smith <rdsmith@google.com>

Initial dynamic metadata test passes.

ee73e1f

Signed-off-by: Randy Smith <rdsmith@google.com>

All DM tests.

c8066df

Signed-off-by: Randy Smith <rdsmith@google.com>

Trivial request_info test.

653fdd7

Signed-off-by: Randy Smith <rdsmith@google.com>

Format fixed.

36a3594

Signed-off-by: Randy Smith <rdsmith@google.com>

Changes forced by asan testing.

1bfe99c

Signed-off-by: Randy Smith <rdsmith@google.com>

Got test to build.

603c588

Signed-off-by: Randy Smith <rdsmith@google.com>

junr03 assigned zuercher Jul 20, 2018

Fixed format

199a5c8

Signed-off-by: Randy Smith <rdsmith@google.com>

zuercher reviewed Jul 23, 2018

View reviewed changes

curiouserrandy added 6 commits July 24, 2018 18:18

Incorporated most comments.

229414d

Signed-off-by: Randy Smith <rdsmith@google.com>

Made type id thread safe.

48f2246

Signed-off-by: Randy Smith <rdsmith@google.com>

Removed insertion into RequestInfo

ed497c8

Signed-off-by: Randy Smith <rdsmith@google.com>

fit fix format

dbbaec5

Signed-off-by: Randy Smith <rdsmith@google.com>

Cleanup

cedbac4

Signed-off-by: Randy Smith <rdsmith@google.com>

Merge branch 'master' into DynamicMetadata

4207f27

Signed-off-by: Randy Smith <rdsmith@google.com>

curiouserrandy added 3 commits July 26, 2018 14:43

Added doxygen comments and finer grained probe function

6864aa4

Signed-off-by: Randy Smith <rdsmith@google.com>

Added missing const markers

18b3b57

Signed-off-by: Randy Smith <rdsmith@google.com>

dock fix

0df9cfa

Signed-off-by: Randy Smith <rdsmith@google.com>

htuch suggested changes Jul 27, 2018

View reviewed changes

curiouserrandy added 2 commits July 27, 2018 13:16

Incoporated comments.

510368d

Signed-off-by: Randy Smith <rdsmith@google.com>

dock fix

88fade4

Signed-off-by: Randy Smith <rdsmith@google.com>

curiouserrandy added 2 commits July 27, 2018 13:30

Removed StringAccessor.

66d640e

Signed-off-by: Randy Smith <rdsmith@google.com>

Merge remote-tracking branch 'origin/DynamicMetadata_xfer' into Dynam…

5dd51be

…icMetadata Signed-off-by: Randy Smith <rdsmith@google.com>

mattklein123 assigned mattklein123 and htuch Jul 30, 2018

htuch reviewed Jul 31, 2018

View reviewed changes

curiouserrandy added 7 commits August 1, 2018 15:39

Shifted source associated with template MP to source/common

c5f3775

Signed-off-by: Randy Smith <rdsmith@google.com>

Switched implementation over to dynamic_cast<> with all that represents.

4a89207

Signed-off-by: Randy Smith <rdsmith@google.com>

... and finished tests.

d76a5fe

Signed-off-by: Randy Smith <rdsmith@google.com>

Incorporated rest of comments.

e59f4be

Signed-off-by: Randy Smith <rdsmith@google.com>

Fix format

c6ba24c

Signed-off-by: Randy Smith <rdsmith@google.com>

Merge remote-tracking branch 'origin/DynamicMetadata_xfer' into Dynam…

5205592

…icMetadata Signed-off-by: Randy Smith <rdsmith@google.com>

Remove unneeded header.

9a5ba4f

Signed-off-by: Randy Smith <rdsmith@google.com>

htuch reviewed Aug 1, 2018

View reviewed changes

curiouserrandy added 3 commits August 2, 2018 11:20

Changed DynamicMetadataObject to Object

9cb16cc

Signed-off-by: Randy Smith <rdsmith@google.com>

Incorporated comments

65d8e16

Signed-off-by: Randy Smith <rdsmith@google.com>

fix format

e62097f

Signed-off-by: Randy Smith <rdsmith@google.com>

htuch previously approved these changes Aug 2, 2018

View reviewed changes

mattklein123 reviewed Aug 3, 2018

View reviewed changes

Incorporated comments.

d221181

Signed-off-by: Randy Smith <rdsmith@google.com>

curiouserrandy dismissed htuch’s stale review via d221181 August 3, 2018 18:25

mattklein123 approved these changes Aug 3, 2018

View reviewed changes

mattklein123 merged commit fd517b3 into envoyproxy:master Aug 5, 2018

This was referenced Aug 9, 2018

Mechanism for accessing filter-generated data in added user-defined headers #3559

Closed

Determine upgrade path for dynamic metadata #4100

Closed

rshriram mentioned this pull request Sep 20, 2018

Introduce PerConnectionState #4454

Merged

request_info: initial implementation of dynamic metadata object #3918

request_info: initial implementation of dynamic metadata object #3918

Conversation

curiouserrandy commented Jul 20, 2018

zuercher 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

curiouserrandy commented Jul 25, 2018

curiouserrandy commented Jul 25, 2018

curiouserrandy commented Jul 25, 2018

zuercher commented Jul 25, 2018

rshriram commented Jul 26, 2018

curiouserrandy commented Jul 26, 2018

curiouserrandy commented Jul 26, 2018

htuch 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

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

curiouserrandy commented Jul 27, 2018

htuch commented Jul 31, 2018

htuch 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