diff --git a/api/envoy/extensions/filters/common/set_filter_state/v3/BUILD b/api/envoy/extensions/filters/common/set_filter_state/v3/BUILD index 504c6c70514ac..a216e596f0ddb 100644 --- a/api/envoy/extensions/filters/common/set_filter_state/v3/BUILD +++ b/api/envoy/extensions/filters/common/set_filter_state/v3/BUILD @@ -6,6 +6,7 @@ licenses(["notice"]) # Apache 2 api_proto_package( deps = [ + "//envoy/annotations:pkg", "//envoy/config/core/v3:pkg", "@xds//udpa/annotations:pkg", ], diff --git a/api/envoy/extensions/filters/common/set_filter_state/v3/value.proto b/api/envoy/extensions/filters/common/set_filter_state/v3/value.proto index 054529c546f37..7f2578e876451 100644 --- a/api/envoy/extensions/filters/common/set_filter_state/v3/value.proto +++ b/api/envoy/extensions/filters/common/set_filter_state/v3/value.proto @@ -4,6 +4,7 @@ package envoy.extensions.filters.common.set_filter_state.v3; import "envoy/config/core/v3/substitution_format_string.proto"; +import "envoy/annotations/deprecation.proto"; import "udpa/annotations/status.proto"; import "validate/validate.proto"; @@ -91,9 +92,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, (envoy.annotations.deprecated_at_minor_version) = "3.0"]; // Configures the object to be shared with the upstream internal connections. See :ref:`internal upstream // transport ` for more details on the filter state sharing with diff --git a/docs/root/configuration/http/http_filters/lua_filter.rst b/docs/root/configuration/http/http_filters/lua_filter.rst index 261896b6442ec..4b29cc3f09444 100644 --- a/docs/root/configuration/http/http_filters/lua_filter.rst +++ b/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 ` 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. diff --git a/docs/root/intro/arch_overview/advanced/data_sharing_between_filters.rst b/docs/root/intro/arch_overview/advanced/data_sharing_between_filters.rst index 9e39057fabd53..50456356cd439 100644 --- a/docs/root/intro/arch_overview/advanced/data_sharing_between_filters.rst +++ b/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``). The state stored per filter can be -either write-once (immutable), or write-many (mutable). +(``map``). The state stored per filter is +write-many (mutable). See :ref:`the well-known dynamic metadata ` and :ref:`the well-known filter state ` for the reference diff --git a/envoy/stream_info/filter_state.h b/envoy/stream_info/filter_state.h index 55bc1f21e945f..6fc9650500321 100644 --- a/envoy/stream_info/filter_state.h +++ b/envoy/stream_info/filter_state.h @@ -136,7 +136,6 @@ class FilterState { struct FilterObject { std::shared_ptr 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 data, StateType state_type, + setData(absl::string_view data_name, std::shared_ptr 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 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 getDataSharedMutableGeneric(absl::string_view data_name) PURE; diff --git a/source/common/network/connection_impl.cc b/source/common/network/connection_impl.cc index 7afe5e577696a..be8422604f871 100644 --- a/source/common/network/connection_impl.cc +++ b/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_); } diff --git a/source/common/stream_info/filter_state_impl.cc b/source/common/stream_info/filter_state_impl.cc index 0b608b5b5eed6..5d3776e3dedfb 100644 --- a/source/common/stream_info/filter_state_impl.cc +++ b/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 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_ptrsetData(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_ptrsecond.get(); - if (current->state_type_ == FilterState::StateType::ReadOnly) { - IS_ENVOY_BUG(fmt::format("FilterStateAccessViolation: FilterState::setData " - "called twice on same ReadOnly state: {}.", - data_name)); - return; - } - - if (current->state_type_ != state_type) { - IS_ENVOY_BUG(fmt::format("FilterStateAccessViolation: FilterState::setData " - "called twice with different state types: {}.", - data_name)); - return; - } - } std::unique_ptr 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; diff --git a/source/common/stream_info/filter_state_impl.h b/source/common/stream_info/filter_state_impl.h index 47390432719ab..e94cba83bdcd8 100644 --- a/source/common/stream_info/filter_state_impl.h +++ b/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 data, FilterState::StateType state_type, + absl::string_view data_name, std::shared_ptr data, FilterState::LifeSpan life_span = FilterState::LifeSpan::FilterChain, StreamSharingMayImpactPooling stream_sharing = StreamSharingMayImpactPooling::None) override; bool hasDataWithName(absl::string_view) const override; diff --git a/source/common/upstream/transport_socket_match_impl.cc b/source/common/upstream/transport_socket_match_impl.cc index 205feeb1ab58a..0ba3a4a669e86 100644 --- a/source/common/upstream/transport_socket_match_impl.cc +++ b/source/common/upstream/transport_socket_match_impl.cc @@ -143,7 +143,7 @@ TransportSocketMatcher::MatchData TransportSocketMatcherImpl::resolveUsingMatche filter_state = std::make_shared( 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_); } diff --git a/source/extensions/filters/common/set_filter_state/filter_config.cc b/source/extensions/filters/common/set_filter_state/filter_config.cc index a6cb1a6557cdf..0cbf0ec52ac44 100644 --- a/source/extensions/filters/common/set_filter_state/filter_config.cc +++ b/source/extensions/filters/common/set_filter_state/filter_config.cc @@ -57,7 +57,6 @@ Config::parse(const Protobuf::RepeatedPtrField& proto_val if (value.factory_ == nullptr) { throw EnvoyException(fmt::format("'{}' does not have an object factory", value.key_)); } - value.state_type_ = proto_value.read_only() ? StateType::ReadOnly : StateType::Mutable; switch (proto_value.shared_with_upstream()) { case FilterStateValueProto::ONCE: value.stream_sharing_ = StreamSharing::SharedWithUpstreamConnectionOnce; @@ -92,7 +91,7 @@ void Config::updateFilterState(const Formatter::Context& context, continue; } ENVOY_LOG(debug, "Created the filter state '{}' from value '{}'", value.key_, bytes_value); - info.filterState()->setData(value.key_, std::move(object), value.state_type_, life_span_, + info.filterState()->setData(value.key_, std::move(object), life_span_, value.stream_sharing_); } } diff --git a/source/extensions/filters/common/set_filter_state/filter_config.h b/source/extensions/filters/common/set_filter_state/filter_config.h index 25e048e0cbed0..bcd65288882d5 100644 --- a/source/extensions/filters/common/set_filter_state/filter_config.h +++ b/source/extensions/filters/common/set_filter_state/filter_config.h @@ -22,7 +22,6 @@ using FilterStateValueProto = struct Value { std::string key_; const StreamInfo::FilterState::ObjectFactory* factory_; - StateType state_type_{StateType::ReadOnly}; StreamSharing stream_sharing_{StreamSharing::None}; bool skip_if_empty_; Formatter::FormatterConstSharedPtr value_; diff --git a/source/extensions/io_socket/user_space/io_handle_impl.cc b/source/extensions/io_socket/user_space/io_handle_impl.cc index f5aef8bb84421..706e548bcf2a0 100644 --- a/source/extensions/io_socket/user_space/io_handle_impl.cc +++ b/source/extensions/io_socket/user_space/io_handle_impl.cc @@ -385,7 +385,7 @@ void PassthroughStateImpl::mergeInto(envoy::config::core::v3::Metadata& metadata } for (const auto& object : filter_state_objects_) { // This should not throw as stream info is new and filter objects are uniquely named. - filter_state.setData(object.name_, object.data_, object.state_type_, + filter_state.setData(object.name_, object.data_, StreamInfo::FilterState::LifeSpan::Connection, object.stream_sharing_); } metadata_ = nullptr; diff --git a/test/common/stream_info/filter_state_impl_test.cc b/test/common/stream_info/filter_state_impl_test.cc index ed6ab46a82b0d..87ad739327cab 100644 --- a/test/common/stream_info/filter_state_impl_test.cc +++ b/test/common/stream_info/filter_state_impl_test.cc @@ -157,44 +157,6 @@ TEST_F(FilterStateImplTest, SimpleTypeMutable) { EXPECT_EQ(200, filterState().getDataReadOnly("test_2")->access()); } -TEST_F(FilterStateImplTest, NameConflictReadOnly) { - // read only data cannot be overwritten (by any state type) - filterState().setData("test_1", std::make_unique(1), FilterState::StateType::ReadOnly, - FilterState::LifeSpan::FilterChain); - EXPECT_ENVOY_BUG(filterState().setData("test_1", std::make_unique(2), - FilterState::StateType::ReadOnly, - FilterState::LifeSpan::FilterChain), - "FilterStateAccessViolation: FilterState::setData called twice on " - "same ReadOnly state: test_1."); - EXPECT_ENVOY_BUG(filterState().setData("test_1", std::make_unique(2), - FilterState::StateType::Mutable, - FilterState::LifeSpan::FilterChain), - "FilterStateAccessViolation: FilterState::setData called twice on " - "same ReadOnly state: test_1."); - EXPECT_EQ(1, filterState().getDataReadOnly("test_1")->access()); -} - -TEST_F(FilterStateImplTest, NameConflictDifferentTypesReadOnly) { - filterState().setData("test_1", std::make_unique(1), FilterState::StateType::ReadOnly, - FilterState::LifeSpan::FilterChain); - EXPECT_ENVOY_BUG( - filterState().setData("test_1", std::make_unique(2, nullptr, nullptr), - FilterState::StateType::ReadOnly, FilterState::LifeSpan::FilterChain), - "FilterStateAccessViolation: FilterState::setData 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(1), FilterState::StateType::Mutable, - FilterState::LifeSpan::FilterChain); - EXPECT_ENVOY_BUG(filterState().setData("test_1", std::make_unique(2), - FilterState::StateType::ReadOnly, - FilterState::LifeSpan::FilterChain), - "FilterStateAccessViolation: FilterState::setData 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("test_name")); } -TEST_F(FilterStateImplTest, ErrorAccessingReadOnlyAsMutable) { - // Accessing read only data as mutable should throw error - filterState().setData("test_name", std::make_unique(5, nullptr, nullptr), - FilterState::StateType::ReadOnly, FilterState::LifeSpan::FilterChain); - EXPECT_ENVOY_BUG(filterState().getDataMutable("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("test_3"), - "FilterStateAccessViolation: FilterState accessed " - "immutable data as mutable: test_3."); - + EXPECT_EQ(3, new_filter_state.getDataMutable("test_3")->access()); EXPECT_EQ(4, new_filter_state.getDataMutable("test_4")->access()); - - EXPECT_ENVOY_BUG(new_filter_state.getDataMutable("test_5"), - "FilterStateAccessViolation: FilterState accessed " - "immutable data as mutable: test_5."); - + EXPECT_EQ(5, new_filter_state.getDataMutable("test_5")->access()); EXPECT_EQ(6, new_filter_state.getDataMutable("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("test_5"), - "FilterStateAccessViolation: FilterState accessed " - "immutable data as mutable: test_5."); + EXPECT_EQ(5, new_filter_state.getDataMutable("test_5")->access()); EXPECT_EQ(6, new_filter_state.getDataMutable("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); } diff --git a/test/extensions/filters/common/set_filter_state/filter_config_test.cc b/test/extensions/filters/common/set_filter_state/filter_config_test.cc index bd4fd4b9a9450..e8d42e76e586b 100644 --- a/test/extensions/filters/common/set_filter_state/filter_config_test.cc +++ b/test/extensions/filters/common/set_filter_state/filter_config_test.cc @@ -253,7 +253,6 @@ TEST_F(ConfigTest, SetValueUpstreamSharedOnce) { const auto objects = info_.filterState()->objectsSharedWithUpstreamConnection(); EXPECT_EQ(1, objects->size()); EXPECT_EQ(StreamSharing::None, objects->at(0).stream_sharing_); - EXPECT_EQ(StateType::Mutable, objects->at(0).state_type_); EXPECT_EQ("foo", objects->at(0).name_); EXPECT_EQ(foo, objects->at(0).data_.get()); } @@ -275,7 +274,6 @@ TEST_F(ConfigTest, SetValueUpstreamSharedTransitive) { const auto objects = info_.filterState()->objectsSharedWithUpstreamConnection(); EXPECT_EQ(1, objects->size()); EXPECT_EQ(StreamSharing::SharedWithUpstreamConnection, objects->at(0).stream_sharing_); - EXPECT_EQ(StateType::ReadOnly, objects->at(0).state_type_); EXPECT_EQ("foo", objects->at(0).name_); EXPECT_EQ(foo, objects->at(0).data_.get()); }