filter_state: remove checks for read-only

api/envoy/extensions/filters/common/set_filter_state/v3/value.proto

@@ -91,9 +91,8 @@ message FilterStateValue {
     config.core.v3.SubstitutionFormatString format_string = 2;
   }
 
-  // If marked as read-only, the filter state key value is locked, and cannot
-  // be overridden by any filter, including this filter.
-  bool read_only = 3;
+  // This field is deprecated and its value has no effect.
+  bool read_only = 3 [deprecated = true];
 
   // Configures the object to be shared with the upstream internal connections. See :ref:`internal upstream
   // transport <config_internal_upstream_transport>` for more details on the filter state sharing with

docs/root/configuration/http/http_filters/lua_filter.rst

@@ -1358,7 +1358,7 @@ Sets a filter state object by name using a registered :ref:`object factory <well
 * ``factoryKey`` is a string that specifies the registered ``ObjectFactory`` name used to create the object. See :ref:`well-known filter state objects <well_known_filter_state>` for the list of available factory keys.
 * ``payload`` is a string passed to the factory's ``createFromBytes`` method.
 
-The object is stored as read-only with filter chain lifespan and no upstream sharing.
+The object is stored with filter chain lifespan and no upstream sharing.
 
 Raises a Lua error if the factory key is not registered or if the factory fails to create an object from the given payload.
 

docs/root/intro/arch_overview/advanced/data_sharing_between_filters.rst

@@ -87,8 +87,8 @@ connection and HTTP stream (i.e., HTTP request/response pair)
 respectively. ``StreamInfo`` contains a set of fixed attributes as part of
 the class definition (e.g., HTTP protocol, requested server name, etc.). In
 addition, it provides a facility to store typed objects in a map
-(``map<string, FilterState::Object>``). The state stored per filter can be
-either write-once (immutable), or write-many (mutable).
+(``map<string, FilterState::Object>``). The state stored per filter is
+write-many (mutable).
 
 See :ref:`the well-known dynamic metadata <well_known_dynamic_metadata>` and
 :ref:`the well-known filter state <well_known_filter_state>` for the reference

envoy/stream_info/filter_state.h

@@ -136,7 +136,6 @@ class FilterState {
 
   struct FilterObject {
     std::shared_ptr<Object> data_;
-    StateType state_type_{StateType::ReadOnly};
     StreamSharingMayImpactPooling stream_sharing_{StreamSharingMayImpactPooling::None};
     std::string name_;
   };
@@ -149,24 +148,28 @@ class FilterState {
   /**
    * @param data_name the name of the data being set.
    * @param data an owning pointer to the data to be stored.
-   * @param state_type indicates whether the object is mutable or not.
    * @param life_span indicates the life span of the object: bound to the filter chain, a
    * request, or a connection.
    *
-   * Note that it is an error to call setData() twice with the same
-   * data_name, if the existing object is immutable. Similarly, it is an
-   * error to call setData() with same data_name but different state_types
-   * (mutable and readOnly, or readOnly and mutable) or different life_span.
-   * This is to enforce a single authoritative source for each piece of
-   * data stored in FilterState.
+   * Note that it is an error to call setData() twice with the same data_name, but different
+   * life_span.
    */
   virtual void
-  setData(absl::string_view data_name, std::shared_ptr<Object> data, StateType state_type,
+  setData(absl::string_view data_name, std::shared_ptr<Object> data,
           LifeSpan life_span = LifeSpan::FilterChain,
           StreamSharingMayImpactPooling stream_sharing = StreamSharingMayImpactPooling::None) PURE;
 
   /**
-   * @param data_name the name of the data being looked up (mutable/readonly).
+   * Deprecated version of setData that takes the no longer used `StateType`.
+   */
+  void setData(absl::string_view data_name, std::shared_ptr<Object> data, StateType /*state_type*/,
+               LifeSpan life_span = LifeSpan::FilterChain,
+               StreamSharingMayImpactPooling stream_sharing = StreamSharingMayImpactPooling::None) {
+    return setData(data_name, std::move(data), life_span, stream_sharing);
+  }
+
+  /**
+   * @param data_name the name of the data being looked up.
    * @return a typed pointer to the stored data or nullptr if the data does not exist or the data
    * type does not match the expected type.
    */
@@ -175,13 +178,13 @@ class FilterState {
   }
 
   /**
-   * @param data_name the name of the data being looked up (mutable/readonly).
+   * @param data_name the name of the data being looked up.
    * @return a const pointer to the stored data or nullptr if the data does not exist.
    */
   virtual const Object* getDataReadOnlyGeneric(absl::string_view data_name) const PURE;
 
   /**
-   * @param data_name the name of the data being looked up (mutable/readonly).
+   * @param data_name the name of the data being looked up.
    * @return a typed pointer to the stored data or nullptr if the data does not exist or the data
    * type does not match the expected type.
    */
@@ -190,13 +193,13 @@ class FilterState {
   }
 
   /**
-   * @param data_name the name of the data being looked up (mutable/readonly).
+   * @param data_name the name of the data being looked up.
    * @return a pointer to the stored data or nullptr if the data does not exist.
    */
   virtual Object* getDataMutableGeneric(absl::string_view data_name) PURE;
 
   /**
-   * @param data_name the name of the data being looked up (mutable/readonly).
+   * @param data_name the name of the data being looked up.
    * @return a shared pointer to the stored data or nullptr if the data does not exist.
    */
   virtual std::shared_ptr<Object> getDataSharedMutableGeneric(absl::string_view data_name) PURE;

source/common/network/connection_impl.cc

@@ -1139,7 +1139,7 @@ ClientConnectionImpl::ClientConnectionImpl(
   if (transport_options) {
     for (const auto& object : transport_options->downstreamSharedFilterStateObjects()) {
       // This does not throw as all objects are distinctly named and the stream info is empty.
-      stream_info_.filterState()->setData(object.name_, object.data_, object.state_type_,
+      stream_info_.filterState()->setData(object.name_, object.data_,
                                           StreamInfo::FilterState::LifeSpan::Connection,
                                           object.stream_sharing_);
     }

source/common/stream_info/filter_state_impl.cc

@@ -32,7 +32,7 @@ void FilterStateImpl::maybeCreateParent(FilterStateSharedPtr ancestor) {
 }
 
 void FilterStateImpl::setData(absl::string_view data_name, std::shared_ptr<Object> data,
-                              FilterState::StateType state_type, FilterState::LifeSpan life_span,
+                              FilterState::LifeSpan life_span,
                               StreamSharingMayImpactPooling stream_sharing) {
   if (life_span > life_span_) {
     if (hasDataWithNameInternally(data_name)) {
@@ -46,7 +46,7 @@ void FilterStateImpl::setData(absl::string_view data_name, std::shared_ptr<Objec
     // construction directly and this call will be a no-op.
     // So we only need to consider the case where ancestor is nullptr.
     maybeCreateParent(nullptr);
-    parent_->setData(data_name, data, state_type, life_span, stream_sharing);
+    parent_->setData(data_name, data, life_span, stream_sharing);
     return;
   }
   if (parent_ && parent_->hasDataWithName(data_name)) {
@@ -56,30 +56,9 @@ void FilterStateImpl::setData(absl::string_view data_name, std::shared_ptr<Objec
                     data_name));
     return;
   }
-  const auto& it = data_storage_.find(data_name);
-  if (it != data_storage_.end()) {
-    // We have another object with same data_name. Check for mutability
-    // violations namely: readonly data cannot be overwritten, mutable data
-    // cannot be overwritten by readonly data.
-    const FilterStateImpl::FilterObject* current = it->second.get();
-    if (current->state_type_ == FilterState::StateType::ReadOnly) {
-      IS_ENVOY_BUG(fmt::format("FilterStateAccessViolation: FilterState::setData<T> "
-                               "called twice on same ReadOnly state: {}.",
-                               data_name));
-      return;
-    }
-
-    if (current->state_type_ != state_type) {
-      IS_ENVOY_BUG(fmt::format("FilterStateAccessViolation: FilterState::setData<T> "
-                               "called twice with different state types: {}.",
-                               data_name));
-      return;
-    }
-  }
 
   std::unique_ptr<FilterStateImpl::FilterObject> filter_object(new FilterStateImpl::FilterObject());
   filter_object->data_ = data;
-  filter_object->state_type_ = state_type;
   filter_object->stream_sharing_ = stream_sharing;
   data_storage_[data_name] = std::move(filter_object);
 }
@@ -119,14 +98,6 @@ FilterStateImpl::getDataSharedMutableGeneric(absl::string_view data_name) {
   }
 
   FilterStateImpl::FilterObject* current = it->second.get();
-  if (current->state_type_ == FilterState::StateType::ReadOnly) {
-    IS_ENVOY_BUG(fmt::format("FilterStateAccessViolation: FilterState accessed "
-                             "immutable data as mutable: {}.",
-                             data_name));
-    // To reduce the chances of a crash, allow the mutation in this case instead of returning a
-    // nullptr.
-  }
-
   return current->data_;
 }
 
@@ -143,11 +114,10 @@ FilterState::ObjectsPtr FilterStateImpl::objectsSharedWithUpstreamConnection() c
   for (const auto& [name, object] : data_storage_) {
     switch (object->stream_sharing_) {
     case StreamSharingMayImpactPooling::SharedWithUpstreamConnection:
-      objects->push_back({object->data_, object->state_type_, object->stream_sharing_, name});
+      objects->push_back({object->data_, object->stream_sharing_, name});
       break;
     case StreamSharingMayImpactPooling::SharedWithUpstreamConnectionOnce:
-      objects->push_back(
-          {object->data_, object->state_type_, StreamSharingMayImpactPooling::None, name});
+      objects->push_back({object->data_, StreamSharingMayImpactPooling::None, name});
       break;
     default:
       break;

source/common/stream_info/filter_state_impl.h

@@ -34,7 +34,7 @@ class FilterStateImpl : public FilterState {
 
   // FilterState
   void setData(
-      absl::string_view data_name, std::shared_ptr<Object> data, FilterState::StateType state_type,
+      absl::string_view data_name, std::shared_ptr<Object> data,
       FilterState::LifeSpan life_span = FilterState::LifeSpan::FilterChain,
       StreamSharingMayImpactPooling stream_sharing = StreamSharingMayImpactPooling::None) override;
   bool hasDataWithName(absl::string_view) const override;

source/common/upstream/transport_socket_match_impl.cc

@@ -143,7 +143,7 @@ TransportSocketMatcher::MatchData TransportSocketMatcherImpl::resolveUsingMatche
       filter_state = std::make_shared<StreamInfo::FilterStateImpl>(
           StreamInfo::FilterState::LifeSpan::Connection);
       for (const auto& object : shared_objects) {
-        filter_state->setData(object.name_, object.data_, object.state_type_,
+        filter_state->setData(object.name_, object.data_,
                               StreamInfo::FilterState::LifeSpan::Connection,
                               object.stream_sharing_);
       }

test/common/stream_info/filter_state_impl_test.cc

@@ -157,44 +157,6 @@ TEST_F(FilterStateImplTest, SimpleTypeMutable) {
   EXPECT_EQ(200, filterState().getDataReadOnly<SimpleType>("test_2")->access());
 }
 
-TEST_F(FilterStateImplTest, NameConflictReadOnly) {
-  // read only data cannot be overwritten (by any state type)
-  filterState().setData("test_1", std::make_unique<SimpleType>(1), FilterState::StateType::ReadOnly,
-                        FilterState::LifeSpan::FilterChain);
-  EXPECT_ENVOY_BUG(filterState().setData("test_1", std::make_unique<SimpleType>(2),
-                                         FilterState::StateType::ReadOnly,
-                                         FilterState::LifeSpan::FilterChain),
-                   "FilterStateAccessViolation: FilterState::setData<T> called twice on "
-                   "same ReadOnly state: test_1.");
-  EXPECT_ENVOY_BUG(filterState().setData("test_1", std::make_unique<SimpleType>(2),
-                                         FilterState::StateType::Mutable,
-                                         FilterState::LifeSpan::FilterChain),
-                   "FilterStateAccessViolation: FilterState::setData<T> called twice on "
-                   "same ReadOnly state: test_1.");
-  EXPECT_EQ(1, filterState().getDataReadOnly<SimpleType>("test_1")->access());
-}
-
-TEST_F(FilterStateImplTest, NameConflictDifferentTypesReadOnly) {
-  filterState().setData("test_1", std::make_unique<SimpleType>(1), FilterState::StateType::ReadOnly,
-                        FilterState::LifeSpan::FilterChain);
-  EXPECT_ENVOY_BUG(
-      filterState().setData("test_1", std::make_unique<TestStoredTypeTracking>(2, nullptr, nullptr),
-                            FilterState::StateType::ReadOnly, FilterState::LifeSpan::FilterChain),
-      "FilterStateAccessViolation: FilterState::setData<T> called twice on "
-      "same ReadOnly state: test_1.");
-}
-
-TEST_F(FilterStateImplTest, NameConflictMutableAndReadOnly) {
-  // Mutable data cannot be overwritten by read only data.
-  filterState().setData("test_1", std::make_unique<SimpleType>(1), FilterState::StateType::Mutable,
-                        FilterState::LifeSpan::FilterChain);
-  EXPECT_ENVOY_BUG(filterState().setData("test_1", std::make_unique<SimpleType>(2),
-                                         FilterState::StateType::ReadOnly,
-                                         FilterState::LifeSpan::FilterChain),
-                   "FilterStateAccessViolation: FilterState::setData<T> called twice with "
-                   "different state types: test_1.");
-}
-
 TEST_F(FilterStateImplTest, NoNameConflictMutableAndMutable) {
   // Mutable data can be overwritten by another mutable data of same or different type.
 
@@ -226,18 +188,6 @@ TEST_F(FilterStateImplTest, WrongTypeGet) {
   EXPECT_EQ(nullptr, filterState().getDataReadOnly<SimpleType>("test_name"));
 }
 
-TEST_F(FilterStateImplTest, ErrorAccessingReadOnlyAsMutable) {
-  // Accessing read only data as mutable should throw error
-  filterState().setData("test_name", std::make_unique<TestStoredTypeTracking>(5, nullptr, nullptr),
-                        FilterState::StateType::ReadOnly, FilterState::LifeSpan::FilterChain);
-  EXPECT_ENVOY_BUG(filterState().getDataMutable<TestStoredTypeTracking>("test_name"),
-                   "FilterStateAccessViolation: FilterState accessed immutable "
-                   "data as mutable: test_name.");
-  EXPECT_ENVOY_BUG(filterState().getDataSharedMutableGeneric("test_name"),
-                   "FilterStateAccessViolation: FilterState accessed "
-                   "immutable data as mutable: test_name.");
-}
-
 namespace {
 
 class A : public FilterState::Object {};
@@ -294,16 +244,9 @@ TEST_F(FilterStateImplTest, LifeSpanInitFromParent) {
   EXPECT_TRUE(new_filter_state.hasDataWithName("test_4"));
   EXPECT_TRUE(new_filter_state.hasDataWithName("test_5"));
   EXPECT_TRUE(new_filter_state.hasDataWithName("test_6"));
-  EXPECT_ENVOY_BUG(new_filter_state.getDataMutable<SimpleType>("test_3"),
-                   "FilterStateAccessViolation: FilterState accessed "
-                   "immutable data as mutable: test_3.");
-
+  EXPECT_EQ(3, new_filter_state.getDataMutable<SimpleType>("test_3")->access());
   EXPECT_EQ(4, new_filter_state.getDataMutable<SimpleType>("test_4")->access());
-
-  EXPECT_ENVOY_BUG(new_filter_state.getDataMutable<SimpleType>("test_5"),
-                   "FilterStateAccessViolation: FilterState accessed "
-                   "immutable data as mutable: test_5.");
-
+  EXPECT_EQ(5, new_filter_state.getDataMutable<SimpleType>("test_5")->access());
   EXPECT_EQ(6, new_filter_state.getDataMutable<SimpleType>("test_6")->access());
 }
 
@@ -329,9 +272,7 @@ TEST_F(FilterStateImplTest, LifeSpanInitFromGrandparent) {
   EXPECT_FALSE(new_filter_state.hasDataWithName("test_4"));
   EXPECT_TRUE(new_filter_state.hasDataWithName("test_5"));
   EXPECT_TRUE(new_filter_state.hasDataWithName("test_6"));
-  EXPECT_ENVOY_BUG(new_filter_state.getDataMutable<SimpleType>("test_5"),
-                   "FilterStateAccessViolation: FilterState accessed "
-                   "immutable data as mutable: test_5.");
+  EXPECT_EQ(5, new_filter_state.getDataMutable<SimpleType>("test_5")->access());
   EXPECT_EQ(6, new_filter_state.getDataMutable<SimpleType>("test_6")->access());
 }
 
@@ -383,20 +324,16 @@ TEST_F(FilterStateImplTest, SharedWithUpstream) {
   std::sort(objects->begin(), objects->end(),
             [](const auto& lhs, const auto& rhs) -> bool { return lhs.name_ < rhs.name_; });
   EXPECT_EQ(objects->at(0).name_, "shared_1");
-  EXPECT_EQ(objects->at(0).state_type_, FilterState::StateType::ReadOnly);
   EXPECT_EQ(objects->at(0).stream_sharing_,
             StreamSharingMayImpactPooling::SharedWithUpstreamConnection);
   EXPECT_EQ(objects->at(0).data_.get(), shared.get());
   EXPECT_EQ(objects->at(1).name_, "shared_4");
-  EXPECT_EQ(objects->at(1).state_type_, FilterState::StateType::Mutable);
   EXPECT_EQ(objects->at(1).stream_sharing_,
             StreamSharingMayImpactPooling::SharedWithUpstreamConnection);
   EXPECT_EQ(objects->at(2).name_, "shared_5");
-  EXPECT_EQ(objects->at(2).state_type_, FilterState::StateType::ReadOnly);
   EXPECT_EQ(objects->at(2).stream_sharing_,
             StreamSharingMayImpactPooling::SharedWithUpstreamConnection);
   EXPECT_EQ(objects->at(3).name_, "shared_7");
-  EXPECT_EQ(objects->at(3).state_type_, FilterState::StateType::ReadOnly);
   EXPECT_EQ(objects->at(3).stream_sharing_, StreamSharingMayImpactPooling::None);
 }