changing xDS APIs to take abs::Satus (envoyproxy#29439)

Unlike many of the throw->status PRs I'm doing xDS in two parts - one is a pure API change, and the follow-up(s) will change the throw over to returning non-Ok stauts. This PR is large enough already I didn't want to do them all at once. Risk Level: low (but will cause downstream churn, sorry) Testing: updated tests Docs Changes: n/a Release Notes: n/a envoyproxy/envoy-mobile#176 Signed-off-by: Alyssa Wilk <[email protected]>
ohadvano · Sep 7, 2023 · e6ee1cf · e6ee1cf
1 parent ffddd03
commit e6ee1cf
Show file tree

Hide file tree

Showing 72 changed files with 715 additions and 490 deletions.
diff --git a/contrib/generic_proxy/filters/network/test/config_test.cc b/contrib/generic_proxy/filters/network/test/config_test.cc
@@ -124,8 +124,10 @@ version_info: "1"
       TestUtility::parseYaml<envoy::service::discovery::v3::DiscoveryResponse>(response_yaml);
   const auto decoded_resources = TestUtility::decodeResources<
       envoy::extensions::filters::network::generic_proxy::v3::RouteConfiguration>(response);
-  factory_context.server_factory_context_.cluster_manager_.subscription_factory_.callbacks_
-      ->onConfigUpdate(decoded_resources.refvec_, response.version_info());
+  EXPECT_TRUE(
+      factory_context.server_factory_context_.cluster_manager_.subscription_factory_.callbacks_
+          ->onConfigUpdate(decoded_resources.refvec_, response.version_info())
+          .ok());
   auto message_ptr =
       factory_context.admin_.config_tracker_.config_tracker_callbacks_["genericrds_routes"](
           universal_name_matcher);

diff --git a/contrib/generic_proxy/filters/network/test/proxy_test.cc b/contrib/generic_proxy/filters/network/test/proxy_test.cc
@@ -38,7 +38,7 @@ class MockRouteConfigProvider : public Rds::RouteConfigProvider {
   MOCK_METHOD(Rds::ConfigConstSharedPtr, config, (), (const));
   MOCK_METHOD(const absl::optional<Rds::RouteConfigProvider::ConfigInfo>&, configInfo, (), (const));
   MOCK_METHOD(SystemTime, lastUpdated, (), (const));
-  MOCK_METHOD(void, onConfigUpdate, ());
+  MOCK_METHOD(absl::Status, onConfigUpdate, ());
 
   std::shared_ptr<NiceMock<MockRouteMatcher>> route_config_{new NiceMock<MockRouteMatcher>()};
 };

diff --git a/contrib/sxg/filters/http/test/filter_test.cc b/contrib/sxg/filters/http/test/filter_test.cc
@@ -395,7 +395,7 @@ name: certificate
   TestUtility::loadFromYaml(yaml_client, typed_secret);
   const auto decoded_resources_client = TestUtility::decodeResources({typed_secret});
 
-  certificate_callback->onConfigUpdate(decoded_resources_client.refvec_, "");
+  EXPECT_TRUE(certificate_callback->onConfigUpdate(decoded_resources_client.refvec_, "").ok());
   EXPECT_EQ(secret_reader.certificate(), "certificate_test");
   EXPECT_EQ(secret_reader.privateKey(), "");
 
@@ -408,7 +408,7 @@ name: private_key
   TestUtility::loadFromYaml(yaml_token, typed_secret);
   const auto decoded_resources_token = TestUtility::decodeResources({typed_secret});
 
-  private_key_callback->onConfigUpdate(decoded_resources_token.refvec_, "");
+  EXPECT_TRUE(private_key_callback->onConfigUpdate(decoded_resources_token.refvec_, "").ok());
   EXPECT_EQ(secret_reader.certificate(), "certificate_test");
   EXPECT_EQ(secret_reader.privateKey(), "private_key_test");
 
@@ -421,7 +421,8 @@ name: certificate
   TestUtility::loadFromYaml(yaml_client_recheck, typed_secret);
   const auto decoded_resources_client_recheck = TestUtility::decodeResources({typed_secret});
 
-  certificate_callback->onConfigUpdate(decoded_resources_client_recheck.refvec_, "");
+  EXPECT_TRUE(
+      certificate_callback->onConfigUpdate(decoded_resources_client_recheck.refvec_, "").ok());
   EXPECT_EQ(secret_reader.certificate(), "certificate_test_recheck");
   EXPECT_EQ(secret_reader.privateKey(), "private_key_test");
 }

diff --git a/envoy/common/exception.h b/envoy/common/exception.h
@@ -12,6 +12,14 @@ class EnvoyException : public std::runtime_error {
   EnvoyException(const std::string& message) : std::runtime_error(message) {}
 };
 
+#define THROW_IF_NOT_OK(status_fn)                                                                 \
+  {                                                                                                \
+    absl::Status status = status_fn;                                                               \
+    if (!status.ok()) {                                                                            \
+      throw EnvoyException(std::string(status.message()));                                         \
+    }                                                                                              \
+  }
+
 // Simple macro to handle bridging functions which return absl::StatusOr, and
 // functions which throw errors.
 //

diff --git a/envoy/config/dynamic_extension_config_provider.h b/envoy/config/dynamic_extension_config_provider.h
@@ -26,9 +26,11 @@ class DynamicExtensionConfigProviderBase {
    * validated.
    * @param version_info is the version of the new extension configuration.
    * @param cb the continuation callback for a completed configuration application on all threads.
+   * @return an absl status indicating if a non-exception-throwing error was encountered.
    */
-  virtual void onConfigUpdate(const Protobuf::Message& config, const std::string& version_info,
-                              ConfigAppliedCb applied_on_all_threads) PURE;
+  virtual absl::Status onConfigUpdate(const Protobuf::Message& config,
+                                      const std::string& version_info,
+                                      ConfigAppliedCb applied_on_all_threads) PURE;
 
   /**
    * Removes the current configuration from the provider.

diff --git a/envoy/config/subscription.h b/envoy/config/subscription.h
@@ -103,28 +103,30 @@ class SubscriptionCallbacks {
    * everything other than delta gRPC - filesystem, HTTP, non-delta gRPC).
    * @param resources vector of fetched resources corresponding to the configuration update.
    * @param version_info supplies the version information as supplied by the xDS discovery response.
-   * @throw EnvoyException with reason if the configuration is rejected. Otherwise the configuration
-   *        is accepted. Accepted configurations have their version_info reflected in subsequent
-   *        requests.
+   * @return an absl status indicating if a non-exception-throwing error was encountered.
+   * @throw EnvoyException with reason if the configuration is rejected for legacy reasons,
+   *        Accepted configurations have their version_info reflected in subsequent requests.
    */
-  virtual void onConfigUpdate(const std::vector<DecodedResourceRef>& resources,
-                              const std::string& version_info) PURE;
+  virtual absl::Status onConfigUpdate(const std::vector<DecodedResourceRef>& resources,
+                                      const std::string& version_info) PURE;
 
   /**
    * Called when a delta configuration update is received.
    * @param added_resources resources newly added since the previous fetch.
    * @param removed_resources names of resources that this fetch instructed to be removed.
    * @param system_version_info aggregate response data "version", for debugging.
-   * @throw EnvoyException with reason if the config changes are rejected. Otherwise the changes
-   *        are accepted. Accepted changes have their version_info reflected in subsequent requests.
+   * @return an absl status indicating if a non-exception-throwing error was encountered.
+   * @throw EnvoyException with reason if the configuration is rejected for legacy reasons,
+   *        Accepted configurations have their version_info reflected in subsequent requests.
    */
-  virtual void onConfigUpdate(const std::vector<DecodedResourceRef>& added_resources,
-                              const Protobuf::RepeatedPtrField<std::string>& removed_resources,
-                              const std::string& system_version_info) PURE;
+  virtual absl::Status
+  onConfigUpdate(const std::vector<DecodedResourceRef>& added_resources,
+                 const Protobuf::RepeatedPtrField<std::string>& removed_resources,
+                 const std::string& system_version_info) PURE;
 
   /**
    * Called when either the Subscription is unable to fetch a config update or when onConfigUpdate
-   * invokes an exception.
+   * returns a failure or invokes an exception.
    * @param reason supplies the update failure reason.
    * @param e supplies any exception data on why the fetch failed. May be nullptr.
    */

diff --git a/envoy/rds/route_config_provider.h b/envoy/rds/route_config_provider.h
@@ -50,8 +50,9 @@ class RouteConfigProvider {
 
   /**
    * Callback used to notify RouteConfigProvider about configuration changes.
+   * @return Status indicating if the call was successful or had graceful error handling.
    */
-  virtual void onConfigUpdate() PURE;
+  virtual absl::Status onConfigUpdate() PURE;
 };
 
 using RouteConfigProviderPtr = std::unique_ptr<RouteConfigProvider>;

diff --git a/source/common/config/config_provider_impl.h b/source/common/config/config_provider_impl.h
@@ -172,9 +172,10 @@ class ConfigSubscriptionCommonBase : protected Logger::Loggable<Logger::Id::conf
    * Must be called by derived classes when the onConfigUpdate() callback associated with the
    * underlying subscription is issued.
    */
-  void onConfigUpdate() {
+  absl::Status onConfigUpdate() {
     setLastUpdated();
     local_init_target_.ready();
+    return absl::OkStatus();
   }
 
   /**

diff --git a/source/common/config/xds_context_params.cc b/source/common/config/xds_context_params.cc
@@ -53,7 +53,7 @@ void mergeMetadataJson(Protobuf::Map<std::string, std::string>& params,
   UNREFERENCED_PARAMETER(params);
   UNREFERENCED_PARAMETER(metadata);
   UNREFERENCED_PARAMETER(prefix);
-  throw EnvoyException("JSON/YAML support compiled out");
+  IS_ENVOY_BUG("JSON/YAML support compiled out");
 #endif
 }
 

diff --git a/source/common/filter/config_discovery_impl.cc b/source/common/filter/config_discovery_impl.cc
@@ -17,12 +17,14 @@ namespace Envoy {
 namespace Filter {
 
 namespace {
-void validateTypeUrlHelper(const std::string& type_url,
-                           const absl::flat_hash_set<std::string> require_type_urls) {
+absl::Status validateTypeUrlHelper(const std::string& type_url,
+                                   const absl::flat_hash_set<std::string> require_type_urls) {
   if (!require_type_urls.contains(type_url)) {
-    throw EnvoyException(fmt::format("Error: filter config has type URL {} but expect {}.",
-                                     type_url, absl::StrJoin(require_type_urls, ", ")));
+    return absl::InvalidArgumentError(
+        fmt::format("Error: filter config has type URL {} but expect {}.", type_url,
+                    absl::StrJoin(require_type_urls, ", ")));
   }
+  return absl::OkStatus();
 }
 
 } // namespace
@@ -50,8 +52,9 @@ DynamicFilterConfigProviderImplBase::~DynamicFilterConfigProviderImplBase() {
   subscription_->filter_config_providers_.erase(this);
 }
 
-void DynamicFilterConfigProviderImplBase::validateTypeUrl(const std::string& type_url) const {
-  validateTypeUrlHelper(type_url, require_type_urls_);
+absl::Status
+DynamicFilterConfigProviderImplBase::validateTypeUrl(const std::string& type_url) const {
+  return validateTypeUrlHelper(type_url, require_type_urls_);
 }
 
 const std::string& DynamicFilterConfigProviderImplBase::name() { return subscription_->name(); }
@@ -86,33 +89,37 @@ void FilterConfigSubscription::start() {
   }
 }
 
-void FilterConfigSubscription::onConfigUpdate(
-    const std::vector<Config::DecodedResourceRef>& resources, const std::string& version_info) {
+absl::Status
+FilterConfigSubscription::onConfigUpdate(const std::vector<Config::DecodedResourceRef>& resources,
+                                         const std::string& version_info) {
   ConfigVersionSharedPtr next =
       std::make_shared<ConfigVersion>(version_info, factory_context_.timeSource().systemTime());
   if (resources.size() != 1) {
-    throw EnvoyException(fmt::format(
+    return absl::InvalidArgumentError(fmt::format(
         "Unexpected number of resources in ExtensionConfigDS response: {}", resources.size()));
   }
   const auto& filter_config = dynamic_cast<const envoy::config::core::v3::TypedExtensionConfig&>(
       resources[0].get().resource());
   if (filter_config.name() != filter_config_name_) {
-    throw EnvoyException(fmt::format("Unexpected resource name in ExtensionConfigDS response: {}",
-                                     filter_config.name()));
+    return absl::InvalidArgumentError(fmt::format(
+        "Unexpected resource name in ExtensionConfigDS response: {}", filter_config.name()));
   }
   // Skip update if hash matches
   next->config_hash_ = MessageUtil::hash(filter_config.typed_config());
   if (next->config_hash_ == last_->config_hash_) {
     // Initial hash is 0, so this branch happens only after a config was already applied, and
     // there is no need to mark the init target ready.
-    return;
+    return absl::OkStatus();
   }
   // Ensure that the filter config is valid in the filter chain context once the proto is
   // processed. Validation happens before updating to prevent a partial update application. It
   // might be possible that the providers have distinct type URL constraints.
   next->type_url_ = Config::Utility::getFactoryType(filter_config.typed_config());
   for (auto* provider : filter_config_providers_) {
-    provider->validateTypeUrl(next->type_url_);
+    absl::Status status = provider->validateTypeUrl(next->type_url_);
+    if (!status.ok()) {
+      return status;
+    }
   }
   std::tie(next->config_, next->factory_name_) =
       filter_config_provider_manager_.getMessage(filter_config, factory_context_);
@@ -127,16 +134,18 @@ void FilterConfigSubscription::onConfigUpdate(
       filter_config_providers_,
       [last = last_](DynamicFilterConfigProviderImplBase* provider,
                      std::shared_ptr<Cleanup> cleanup) {
-        provider->onConfigUpdate(*last->config_, last->version_info_, [cleanup] {});
+        THROW_IF_NOT_OK(
+            provider->onConfigUpdate(*last->config_, last->version_info_, [cleanup] {}));
       },
       [me = shared_from_this()]() { me->updateComplete(); });
   // The filter configs are created and published to worker queues at this point, so it
   // is safe to mark the subscription as ready and publish the warmed parent resources.
   ENVOY_LOG(debug, "Updated filter config {} created, warming done", filter_config_name_);
   init_target_.ready();
+  return absl::OkStatus();
 }
 
-void FilterConfigSubscription::onConfigUpdate(
+absl::Status FilterConfigSubscription::onConfigUpdate(
     const std::vector<Config::DecodedResourceRef>& added_resources,
     const Protobuf::RepeatedPtrField<std::string>& removed_resources, const std::string&) {
   if (!removed_resources.empty()) {
@@ -151,8 +160,9 @@ void FilterConfigSubscription::onConfigUpdate(
         [me = shared_from_this()]() { me->updateComplete(); });
   } else if (!added_resources.empty()) {
     ASSERT(added_resources.size() == 1);
-    onConfigUpdate(added_resources, added_resources[0].get().version());
+    return onConfigUpdate(added_resources, added_resources[0].get().version());
   }
+  return absl::OkStatus();
 }
 
 void FilterConfigSubscription::onConfigUpdateFailed(Config::ConfigUpdateFailureReason reason,
@@ -215,7 +225,7 @@ void FilterConfigProviderManagerImplBase::applyLastOrDefaultConfig(
   bool last_config_valid = false;
   if (subscription->lastConfig()) {
     TRY_ASSERT_MAIN_THREAD {
-      provider.validateTypeUrl(subscription->lastTypeUrl());
+      THROW_IF_NOT_OK(provider.validateTypeUrl(subscription->lastTypeUrl()))
       provider.validateMessage(filter_config_name, *subscription->lastConfig(),
                                subscription->lastFactoryName());
       last_config_valid = true;
@@ -227,8 +237,8 @@ void FilterConfigProviderManagerImplBase::applyLastOrDefaultConfig(
     });
 
     if (last_config_valid) {
-      provider.onConfigUpdate(*subscription->lastConfig(), subscription->lastVersionInfo(),
-                              nullptr);
+      THROW_IF_NOT_OK(provider.onConfigUpdate(*subscription->lastConfig(),
+                                              subscription->lastVersionInfo(), nullptr));
     }
   }
 
@@ -250,7 +260,7 @@ void FilterConfigProviderManagerImplBase::validateProtoConfigDefaultFactory(
 
 void FilterConfigProviderManagerImplBase::validateProtoConfigTypeUrl(
     const std::string& type_url, const absl::flat_hash_set<std::string>& require_type_urls) const {
-  validateTypeUrlHelper(type_url, require_type_urls);
+  THROW_IF_NOT_OK(validateTypeUrlHelper(type_url, require_type_urls));
 }
 
 } // namespace Filter

diff --git a/source/common/filter/config_discovery_impl.h b/source/common/filter/config_discovery_impl.h
@@ -44,7 +44,7 @@ class DynamicFilterConfigProviderImplBase : public Config::DynamicExtensionConfi
   ~DynamicFilterConfigProviderImplBase() override;
   const Init::Target& initTarget() const { return init_target_; }
 
-  void validateTypeUrl(const std::string& type_url) const;
+  absl::Status validateTypeUrl(const std::string& type_url) const;
   virtual void validateMessage(const std::string& config_name, const Protobuf::Message& message,
                                const std::string& factory_name) const PURE;
 
@@ -108,10 +108,11 @@ class DynamicFilterConfigProviderImpl : public DynamicFilterConfigProviderImplBa
   }
 
   // Config::DynamicExtensionConfigProviderBase
-  void onConfigUpdate(const Protobuf::Message& message, const std::string&,
-                      Config::ConfigAppliedCb applied_on_all_threads) override {
+  absl::Status onConfigUpdate(const Protobuf::Message& message, const std::string&,
+                              Config::ConfigAppliedCb applied_on_all_threads) override {
     const FactoryCb config = instantiateFilterFactory(message);
     update(config, applied_on_all_threads);
+    return absl::OkStatus();
   }
 
   void onConfigRemoved(Config::ConfigAppliedCb applied_on_all_threads) override {
@@ -124,7 +125,10 @@ class DynamicFilterConfigProviderImpl : public DynamicFilterConfigProviderImplBa
 
   void applyDefaultConfiguration() override {
     if (default_configuration_) {
-      onConfigUpdate(*default_configuration_, "", nullptr);
+      auto status = onConfigUpdate(*default_configuration_, "", nullptr);
+      if (!status.ok()) {
+        throw EnvoyException(std::string(status.message()));
+      }
     }
   }
   const Network::ListenerFilterMatcherSharedPtr& getListenerFilterMatcher() override {
@@ -406,11 +410,11 @@ class FilterConfigSubscription
   void start();
 
   // Config::SubscriptionCallbacks
-  void onConfigUpdate(const std::vector<Config::DecodedResourceRef>& resources,
-                      const std::string& version_info) override;
-  void onConfigUpdate(const std::vector<Config::DecodedResourceRef>& added_resources,
-                      const Protobuf::RepeatedPtrField<std::string>& removed_resources,
-                      const std::string&) override;
+  absl::Status onConfigUpdate(const std::vector<Config::DecodedResourceRef>& resources,
+                              const std::string& version_info) override;
+  absl::Status onConfigUpdate(const std::vector<Config::DecodedResourceRef>& added_resources,
+                              const Protobuf::RepeatedPtrField<std::string>& removed_resources,
+                              const std::string&) override;
   void onConfigUpdateFailed(Config::ConfigUpdateFailureReason reason,
                             const EnvoyException*) override;
   void updateComplete();

diff --git a/source/common/rds/rds_route_config_provider_impl.cc b/source/common/rds/rds_route_config_provider_impl.cc
@@ -29,9 +29,10 @@ RdsRouteConfigProviderImpl::configInfo() const {
   return config_update_info_->configInfo();
 }
 
-void RdsRouteConfigProviderImpl::onConfigUpdate() {
+absl::Status RdsRouteConfigProviderImpl::onConfigUpdate() {
   tls_.runOnAllThreads([new_config = config_update_info_->parsedConfiguration()](
                            OptRef<ThreadLocalConfig> tls) { tls->config_ = new_config; });
+  return absl::OkStatus();
 }
 
 } // namespace Rds

diff --git a/source/common/rds/rds_route_config_provider_impl.h b/source/common/rds/rds_route_config_provider_impl.h
@@ -32,7 +32,7 @@ class RdsRouteConfigProviderImpl : public RouteConfigProvider,
 
   const absl::optional<ConfigInfo>& configInfo() const override;
   SystemTime lastUpdated() const override { return config_update_info_->lastUpdated(); }
-  void onConfigUpdate() override;
+  absl::Status onConfigUpdate() override;
 
 private:
   struct ThreadLocalConfig : public ThreadLocal::ThreadLocalObject {