context_provider: exception free (envoyproxy#37457)

Risk Level: low Testing: updated tests Docs Changes: n/a Release Notes: n/a envoyproxy/envoy-mobile#176 Signed-off-by: Alyssa Wilk <[email protected]>
makesoftwaresafe · Dec 3, 2024 · 602ebb2 · 602ebb2
1 parent 5b1ab35
commit 602ebb2
Show file tree

Hide file tree

Showing 7 changed files with 29 additions and 20 deletions.
diff --git a/envoy/config/context_provider.h b/envoy/config/context_provider.h
@@ -44,17 +44,19 @@ class ContextProvider {
    * @param resource_type_url resource type URL for context parameter.
    * @param key parameter key.
    * @param value parameter value.
+   * @return if the update was successful.
    */
-  virtual void setDynamicContextParam(absl::string_view resource_type_url, absl::string_view key,
-                                      absl::string_view value) PURE;
+  virtual absl::Status setDynamicContextParam(absl::string_view resource_type_url,
+                                              absl::string_view key, absl::string_view value) PURE;
 
   /**
    * Unset a dynamic context parameter.
    * @param resource_type_url resource type URL for context parameter.
    * @param key parameter key.
+   * @return if the update was successful.
    */
-  virtual void unsetDynamicContextParam(absl::string_view resource_type_url,
-                                        absl::string_view key) PURE;
+  virtual absl::Status unsetDynamicContextParam(absl::string_view resource_type_url,
+                                                absl::string_view key) PURE;
 
   /**
    * Register a callback for notification when the dynamic context changes.

diff --git a/source/common/config/context_provider_impl.h b/source/common/config/context_provider_impl.h
@@ -27,20 +27,20 @@ class ContextProviderImpl : public ContextProvider {
     }
     return xds::core::v3::ContextParams::default_instance();
   };
-  void setDynamicContextParam(absl::string_view resource_type_url, absl::string_view key,
-                              absl::string_view value) override {
+  absl::Status setDynamicContextParam(absl::string_view resource_type_url, absl::string_view key,
+                                      absl::string_view value) override {
     ASSERT_IS_MAIN_OR_TEST_THREAD();
     (*dynamic_context_[resource_type_url]
           .mutable_params())[toStdStringView(key)] = // NOLINT(std::string_view)
         toStdStringView(value);                      // NOLINT(std::string_view)
-    THROW_IF_NOT_OK(update_cb_helper_.runCallbacks(resource_type_url));
+    return update_cb_helper_.runCallbacks(resource_type_url);
   }
-  void unsetDynamicContextParam(absl::string_view resource_type_url,
-                                absl::string_view key) override {
+  absl::Status unsetDynamicContextParam(absl::string_view resource_type_url,
+                                        absl::string_view key) override {
     ASSERT_IS_MAIN_OR_TEST_THREAD();
     dynamic_context_[resource_type_url].mutable_params()->erase(
         toStdStringView(key)); // NOLINT(std::string_view)
-    THROW_IF_NOT_OK(update_cb_helper_.runCallbacks(resource_type_url));
+    return update_cb_helper_.runCallbacks(resource_type_url);
   }
   ABSL_MUST_USE_RESULT Common::CallbackHandlePtr
   addDynamicContextUpdateCallback(UpdateNotificationCb callback) const override {

diff --git a/test/common/config/context_provider_impl_test.cc b/test/common/config/context_provider_impl_test.cc
@@ -56,26 +56,26 @@ TEST(ContextProviderTest, DynamicContextParameters) {
   EXPECT_EQ(0, update_count);
 
   // Setting a DCP.
-  context_provider.setDynamicContextParam("some_type", "foo", "bar");
+  ASSERT_TRUE(context_provider.setDynamicContextParam("some_type", "foo", "bar").ok());
   EXPECT_EQ(1, update_count);
   EXPECT_EQ("some_type", last_updated_resource);
   EXPECT_EQ("bar", context_provider.dynamicContext("some_type").params().at("foo"));
 
   // Updating a DCP.
-  context_provider.setDynamicContextParam("some_type", "foo", "baz");
+  ASSERT_TRUE(context_provider.setDynamicContextParam("some_type", "foo", "baz").ok());
   EXPECT_EQ(2, update_count);
   EXPECT_EQ("some_type", last_updated_resource);
   EXPECT_EQ("baz", context_provider.dynamicContext("some_type").params().at("foo"));
 
   // Setting a DCP on an unrelated resource.
-  context_provider.setDynamicContextParam("other_type", "foo", "bar");
+  ASSERT_TRUE(context_provider.setDynamicContextParam("other_type", "foo", "bar").ok());
   EXPECT_EQ(3, update_count);
   EXPECT_EQ("other_type", last_updated_resource);
   EXPECT_EQ("baz", context_provider.dynamicContext("some_type").params().at("foo"));
   EXPECT_EQ("bar", context_provider.dynamicContext("other_type").params().at("foo"));
 
   // Unsetting a DCP
-  context_provider.unsetDynamicContextParam("some_type", "foo");
+  ASSERT_TRUE(context_provider.unsetDynamicContextParam("some_type", "foo").ok());
   EXPECT_EQ(4, update_count);
   EXPECT_EQ("some_type", last_updated_resource);
   EXPECT_EQ(0, context_provider.dynamicContext("some_type").params().count("foo"));

diff --git a/test/common/local_info/local_info_impl_test.cc b/test/common/local_info/local_info_impl_test.cc
@@ -13,7 +13,7 @@ TEST(LocalInfoTest, DynamicContextUpdate) {
   Stats::TestUtil::TestSymbolTable symbol_table;
   LocalInfoImpl local_info(*symbol_table, {}, {}, nullptr, "zone_name", "cluster_name",
                            "node_name");
-  local_info.contextProvider().setDynamicContextParam("foo", "bar", "baz");
+  EXPECT_TRUE(local_info.contextProvider().setDynamicContextParam("foo", "bar", "baz").ok());
   EXPECT_EQ("baz", local_info.node().dynamic_parameters().at("foo").params().at("bar"));
 }
 

diff --git a/test/integration/server.cc b/test/integration/server.cc
@@ -100,14 +100,22 @@ void IntegrationTestServer::waitUntilListenersReady() {
 void IntegrationTestServer::setDynamicContextParam(absl::string_view resource_type_url,
                                                    absl::string_view key, absl::string_view value) {
   server().dispatcher().post([this, resource_type_url, key, value]() {
-    server().localInfo().contextProvider().setDynamicContextParam(resource_type_url, key, value);
+    ASSERT_TRUE(server()
+                    .localInfo()
+                    .contextProvider()
+                    .setDynamicContextParam(resource_type_url, key, value)
+                    .ok());
   });
 }
 
 void IntegrationTestServer::unsetDynamicContextParam(absl::string_view resource_type_url,
                                                      absl::string_view key) {
   server().dispatcher().post([this, resource_type_url, key]() {
-    server().localInfo().contextProvider().unsetDynamicContextParam(resource_type_url, key);
+    ASSERT_TRUE(server()
+                    .localInfo()
+                    .contextProvider()
+                    .unsetDynamicContextParam(resource_type_url, key)
+                    .ok());
   });
 }
 

diff --git a/test/mocks/config/mocks.h b/test/mocks/config/mocks.h
@@ -186,10 +186,10 @@ class MockContextProvider : public ContextProvider {
   MOCK_METHOD(const xds::core::v3::ContextParams&, nodeContext, (), (const));
   MOCK_METHOD(const xds::core::v3::ContextParams&, dynamicContext,
               (absl::string_view resource_type_url), (const));
-  MOCK_METHOD(void, setDynamicContextParam,
+  MOCK_METHOD(absl::Status, setDynamicContextParam,
               (absl::string_view resource_type_url, absl::string_view key,
                absl::string_view value));
-  MOCK_METHOD(void, unsetDynamicContextParam,
+  MOCK_METHOD(absl::Status, unsetDynamicContextParam,
               (absl::string_view resource_type_url, absl::string_view key));
   MOCK_METHOD(Common::CallbackHandlePtr, addDynamicContextUpdateCallback,
               (UpdateNotificationCb callback), (const));

diff --git a/tools/code_format/config.yaml b/tools/code_format/config.yaml
@@ -94,7 +94,6 @@ paths:
     - source/extensions/common/matcher/trie_matcher.h
     - envoy/common/exception.h
     - source/common/upstream/upstream_impl.h
-    - source/common/config/context_provider_impl.h
     - source/common/protobuf/utility.h
     # legacy core files which throw exceptions. We can add to this list but strongly prefer
     # StausOr where possible.