RFC: Streamlining stream info usage, init well known attribute sets

[rshriram]

Nuking perConnectionState. Instead, introduce StreamInfo which has dynamicAttributes.
Initialize two well known dynamic attribute lists apriori (one for network level attributes, one for hcm level attributes).

I was hoping to read the info from envoy.connection.attributes in Istio mixer filter and push it to mixer.
At the same time, use in-house filters to generate additional attributes into this list.

Same goals for RBAC as well (wean off DynamicMetadata and source all the properties(?) from envoy.connection.attributes or envoy.hcm.attributes). This model allows arbitrary filters to stick data into StreamInfo.DynamicAttributes (e.g., http.inHouseFilter.AuthToken=abc). With some changes, RBAC filter can then use these dynamic attributes when it does its thing. (cc @rodaine - we talked about this earlier. cc @yangminzhu ).

Implementation is incomplete. Secondly, the hasString/hasInt etc. seems a bit clunky but I don't see a nicer way of encapsulating the whole thing into one shared list (e.g., envoy.connection.attributes).

Thoughts (@htuch / @ggreenway ) ?
If this is too much to bite off, I can split this PR into different pieces. My main goal is to initialize the two lists (envoy.connection.attributes, envoy.hcm.attributes) so that we can have multiple filters piling up info into these, and use the generated info in RBAC/telemetry.

Signed-off-by: Shriram Rajagopalan shriramr@vmware.com

[htuch]

I think I need a refresher on why we need this to be dynamic vs. using fixed per-class types. I'm so confused now between filter state, dynamic metadata, this PR and #4723.

We desperately need something like https://github.com/envoyproxy/envoy/pull/4723/files#diff-4ec21d29489e6427acb0e0e102cc6883 to make sense of this.

Can you take https://github.com/envoyproxy/envoy/pull/4723/files#diff-4ec21d29489e6427acb0e0e102cc6883 and create a doc, then explain in that doc how your new design evolves this state?

CC @stevenzzzz

[mattklein123]

Can you take https://github.com/envoyproxy/envoy/pull/4723/files#diff-4ec21d29489e6427acb0e0e102cc6883 and create a doc, then explain in that doc how your new design evolves this state?

+1, thanks for asking for this @htuch. As I just mentioned to @htuch offline, I'm also super confused about the entire landscape of metadata now. It would be really useful to have a single MD (or gdoc first -> MD later) that explains the lanscape. I would prefer that we pause work until this exists and we can discuss and make sure we are all on the same page.

[rshriram]

To clarify, here is whats happening in the PR:

• state that the dynamic metadata stuff is deprecated [or heck remove it??]
• rename perRequestState (badly named) into dynamicAttributes() [but can’t remove perRequestState due to code level backward compatibility issues??]
• rename connection level perConnectionState to StreamInfo (i.e. adding StreamInfo to Connection. Need to slowly phase out streamInfo from TCP proxy)
• Creating two well known dynamicAttribute bags (envoy.connection.attributes, envoy.hcm.attributes) - populated by Mongo/Redis/various http filters, etc. — consumed by RBAC, access logs(file/streaming), and other policy filters, etc.

[lizan]

@rshriram It seems to me that you want to do is adding dynamicMetadata alike stuff for L4 connection, and kill kind of unrelated per{Connection,Request}State. See #4100 for dynamicMetadata vs perRequestData, they are not to be combined together and not only attributes.

cc @curiouserrandy

[rshriram]

I think I should drop this effort.. Looking at the code git grep DynamicMetadata

source/common/access_log/access_log_formatter.cc:        formatters.emplace_back(new DynamicMetadataFormatter(filter_namespace, path, max_length));
source/common/access_log/access_log_formatter.cc:DynamicMetadataFormatter::DynamicMetadataFormatter(const std::string& filter_namespace,
source/common/access_log/access_log_formatter.cc:std::string DynamicMetadataFormatter::format(const Http::HeaderMap&, const Http::HeaderMap&,
source/common/access_log/access_log_formatter.h: * Formatter based on the DynamicMetadata from StreamInfo.
source/common/access_log/access_log_formatter.h:class DynamicMetadataFormatter : public Formatter, MetadataFormatter {
source/common/access_log/access_log_formatter.h:  DynamicMetadataFormatter(const std::string& filter_namespace,
source/common/stream_info/stream_info_impl.h:  void setDynamicMetadata(const std::string& name, const ProtobufWkt::Struct& value) override {
source/extensions/filters/http/header_to_metadata/header_to_metadata_filter.cc:      callbacks.streamInfo().setDynamicMetadata(entry.first, entry.second);
source/extensions/filters/http/lua/lua_filter.cc:  lua_state_.registerType<DynamicMetadataMapWrapper>();
source/extensions/filters/http/lua/lua_filter.cc:  lua_state_.registerType<DynamicMetadataMapIterator>();
source/extensions/filters/http/lua/lua_filter.h:   * accommodate write API e.g. setDynamicMetadata().
source/extensions/filters/http/lua/wrappers.cc:int StreamInfoWrapper::luaDynamicMetadata(lua_State* state) {
source/extensions/filters/http/lua/wrappers.cc:    dynamic_metadata_wrapper_.reset(DynamicMetadataMapWrapper::create(state, *this), true);
source/extensions/filters/http/lua/wrappers.cc:DynamicMetadataMapIterator::DynamicMetadataMapIterator(DynamicMetadataMapWrapper& parent)
source/extensions/filters/http/lua/wrappers.cc:StreamInfo::StreamInfo& DynamicMetadataMapWrapper::streamInfo() { return parent_.stream_info_; }
source/extensions/filters/http/lua/wrappers.cc:int DynamicMetadataMapIterator::luaPairsIterator(lua_State* state) {
source/extensions/filters/http/lua/wrappers.cc:int DynamicMetadataMapWrapper::luaGet(lua_State* state) {
source/extensions/filters/http/lua/wrappers.cc:int DynamicMetadataMapWrapper::luaSet(lua_State* state) {
source/extensions/filters/http/lua/wrappers.cc:  streamInfo().setDynamicMetadata(filter_name, MessageUtil::keyValueStruct(key, value));
source/extensions/filters/http/lua/wrappers.cc:int DynamicMetadataMapWrapper::luaPairs(lua_State* state) {
source/extensions/filters/http/lua/wrappers.cc:  iterator_.reset(DynamicMetadataMapIterator::create(state, *this), true);
source/extensions/filters/http/lua/wrappers.cc:  lua_pushcclosure(state, DynamicMetadataMapIterator::static_luaPairsIterator, 1);
source/extensions/filters/http/lua/wrappers.h:class DynamicMetadataMapWrapper;
source/extensions/filters/http/lua/wrappers.h:class DynamicMetadataMapIterator
source/extensions/filters/http/lua/wrappers.h:    : public Filters::Common::Lua::BaseLuaObject<DynamicMetadataMapIterator> {
source/extensions/filters/http/lua/wrappers.h:  DynamicMetadataMapIterator(DynamicMetadataMapWrapper& parent);
source/extensions/filters/http/lua/wrappers.h:  DECLARE_LUA_CLOSURE(DynamicMetadataMapIterator, luaPairsIterator);
source/extensions/filters/http/lua/wrappers.h:  DynamicMetadataMapWrapper& parent_;
source/extensions/filters/http/lua/wrappers.h:class DynamicMetadataMapWrapper
source/extensions/filters/http/lua/wrappers.h:    : public Filters::Common::Lua::BaseLuaObject<DynamicMetadataMapWrapper> {
source/extensions/filters/http/lua/wrappers.h:  DynamicMetadataMapWrapper(StreamInfoWrapper& parent) : parent_{parent} {}
source/extensions/filters/http/lua/wrappers.h:  DECLARE_LUA_FUNCTION(DynamicMetadataMapWrapper, luaGet);
source/extensions/filters/http/lua/wrappers.h:  DECLARE_LUA_FUNCTION(DynamicMetadataMapWrapper, luaSet);
source/extensions/filters/http/lua/wrappers.h:  DECLARE_LUA_FUNCTION(DynamicMetadataMapWrapper, luaPairs);
source/extensions/filters/http/lua/wrappers.h:  Filters::Common::Lua::LuaDeathRef<DynamicMetadataMapIterator> iterator_;
source/extensions/filters/http/lua/wrappers.h:  friend class DynamicMetadataMapIterator;
source/extensions/filters/http/lua/wrappers.h:    return {{"protocol", static_luaProtocol}, {"dynamicMetadata", static_luaDynamicMetadata}};
source/extensions/filters/http/lua/wrappers.h:   * @return DynamicMetadataMapWrapper representation of StreamInfo dynamic metadata.
source/extensions/filters/http/lua/wrappers.h:  DECLARE_LUA_FUNCTION(StreamInfoWrapper, luaDynamicMetadata);
source/extensions/filters/http/lua/wrappers.h:  Filters::Common::Lua::LuaDeathRef<DynamicMetadataMapWrapper> dynamic_metadata_wrapper_;
source/extensions/filters/http/lua/wrappers.h:  friend class DynamicMetadataMapWrapper;
source/extensions/filters/http/rbac/rbac_filter.cc:    callbacks_->streamInfo().setDynamicMetadata(HttpFilterNames::get().Rbac, metrics);

😑 !!! Dynamic metadata is like the all purpose data structure in Lua, access log, header to metadata. And of all things, its Protobuf Struct!

Which brings me back to the question I asked on slack: why even have this per-request-state or per-connection-state (my fault for adding this based on perRequestState) when we already have this multi-purpose-thingy?

Can't we stick with the metadata galore and get rid of the additional types?
It seems like we are trying really hard to fit all the different things and justify their purpose via docs like this: https://github.com/envoyproxy/envoy/pull/4723/files#diff-4ec21d29489e6427acb0e0e102cc6883, when in reality you could certainly make do with just one form.

[mattklein123]

I just put this in slack but can we please do the following:

1. Lift the docs that were added into this PR into a gdoc
2. Link to the gdoc from this PR and reconcile (I think you basically did this, just link it): Add typedMetadata for RouteEntry&ClusterInfo #4723 cc @stevenzzzz
3. Then send to envoy-dev@ and we can have everyone comment on it? Feel free to put in your concerns/recommendations, etc.

Then if we can't figure it out in the doc we can discuss at a community meeting. Thank you!

[rshriram]

https://docs.google.com/document/d/101Rcggh7flniYKgsr2T07yHXApywbnXiKj1hIpxX6yQ

[curiouserrandy]

@rshriram : Could you allow comments on that doc? I'm getting it view only.

[rshriram]

Oh sorry. Just noticed it.. Updated.

[rgs1]

It's

[rshriram]

Closing as @venilnoronha broke this into smaller pieces and merged the clean up parts.. will send separate PR for write-many filter state.