health checker: removing exceptions (#27859)

Risk Level: medium
Testing: new tests
Docs Changes: n/a
Release Notes: n/a
Part of envoyproxy/envoy-mobile#176

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

envoy/common/exception.h

@@ -11,4 +11,19 @@ class EnvoyException : public std::runtime_error {
 public:
   EnvoyException(const std::string& message) : std::runtime_error(message) {}
 };
+
+// Simple macro to handle bridging functions which return absl::StatusOr, and
+// functions which throw errors.
+//
+// The completely unnecessary throw action argument is just so 'throw' appears
+// at the call site, so format checks about use of exceptions are triggered.
+#define THROW_IF_STATUS_NOT_OK(variable, throw_action)                                             \
+  if (!variable.status().ok()) {                                                                   \
+    throw_action EnvoyException(std::string(variable.status().message()));                         \
+  }
+
+#define RETURN_IF_STATUS_NOT_OK(variable)                                                          \
+  if (!variable.status().ok()) {                                                                   \
+    return variable.status();                                                                      \
+  }
 } // namespace Envoy

source/common/upstream/cluster_factory_impl.cc

@@ -105,10 +105,12 @@ ClusterFactoryImplBase::create(const envoy::config::cluster::v3::Cluster& cluste
     if (cluster.health_checks().size() != 1) {
       return absl::InvalidArgumentError("Multiple health checks not supported");
     } else {
-      new_cluster_pair.first->setHealthChecker(HealthCheckerFactory::create(
+      auto checker_or_error = HealthCheckerFactory::create(
           cluster.health_checks()[0], *new_cluster_pair.first, server_context.runtime(),
           server_context.mainThreadDispatcher(), server_context.accessLogManager(),
-          context.messageValidationVisitor(), server_context.api()));
+          context.messageValidationVisitor(), server_context.api());
+      RETURN_IF_STATUS_NOT_OK(checker_or_error);
+      new_cluster_pair.first->setHealthChecker(checker_or_error.value());
     }
   }
 

source/common/upstream/health_checker_event_logger.h

@@ -36,7 +36,6 @@ class HealthCheckEventLoggerImpl : public HealthCheckEventLogger {
       file_ = log_manager.createAccessLog(Filesystem::FilePathAndType{
           Filesystem::DestinationType::File, health_check_config.event_log_path()});
     }
-
     for (const auto& config : health_check_config.event_logger()) {
       auto& factory = Config::Utility::getAndCheckFactory<HealthCheckEventSinkFactory>(config);
       event_sinks_.push_back(factory.createHealthCheckEventSink(config.typed_config(), context));

source/common/upstream/health_checker_impl.cc

@@ -47,7 +47,7 @@ const std::string& HealthCheckerFactory::getHostname(const HostSharedPtr& host,
   return cluster->name();
 }
 
-HealthCheckerSharedPtr HealthCheckerFactory::create(
+absl::StatusOr<HealthCheckerSharedPtr> HealthCheckerFactory::create(
     const envoy::config::core::v3::HealthCheck& health_check_config, Upstream::Cluster& cluster,
     Runtime::Loader& runtime, Event::Dispatcher& dispatcher,
     AccessLog::AccessLogManager& log_manager,
@@ -56,7 +56,7 @@ HealthCheckerSharedPtr HealthCheckerFactory::create(
 
   switch (health_check_config.health_checker_case()) {
   case envoy::config::core::v3::HealthCheck::HealthCheckerCase::HEALTH_CHECKER_NOT_SET:
-    throw EnvoyException("invalid cluster config");
+    return absl::InvalidArgumentError("invalid cluster config");
   case envoy::config::core::v3::HealthCheck::HealthCheckerCase::kHttpHealthCheck:
     factory = &Config::Utility::getAndCheckFactoryByName<
         Server::Configuration::CustomHealthCheckerFactory>("envoy.health_checkers.http");
@@ -67,8 +67,8 @@ HealthCheckerSharedPtr HealthCheckerFactory::create(
     break;
   case envoy::config::core::v3::HealthCheck::HealthCheckerCase::kGrpcHealthCheck:
     if (!(cluster.info()->features() & Upstream::ClusterInfo::Features::HTTP2)) {
-      throw EnvoyException(fmt::format("{} cluster must support HTTP/2 for gRPC healthchecking",
-                                       cluster.info()->name()));
+      return absl::InvalidArgumentError(fmt::format(
+          "{} cluster must support HTTP/2 for gRPC healthchecking", cluster.info()->name()));
     }
     factory = &Config::Utility::getAndCheckFactoryByName<
         Server::Configuration::CustomHealthCheckerFactory>("envoy.health_checkers.grpc");
@@ -94,7 +94,7 @@ HealthCheckerSharedPtr HealthCheckerFactory::create(
   return factory->createCustomHealthChecker(health_check_config, *context);
 }
 
-PayloadMatcher::MatchSegments PayloadMatcher::loadProtoBytes(
+absl::StatusOr<PayloadMatcher::MatchSegments> PayloadMatcher::loadProtoBytes(
     const Protobuf::RepeatedPtrField<envoy::config::core::v3::HealthCheck::Payload>& byte_array) {
   MatchSegments result;
 
@@ -103,7 +103,7 @@ PayloadMatcher::MatchSegments PayloadMatcher::loadProtoBytes(
     if (entry.has_text()) {
       decoded = Hex::decode(entry.text());
       if (decoded.empty()) {
-        throw EnvoyException(fmt::format("invalid hex string '{}'", entry.text()));
+        return absl::InvalidArgumentError(fmt::format("invalid hex string '{}'", entry.text()));
       }
     } else {
       decoded.assign(entry.binary().begin(), entry.binary().end());

source/common/upstream/health_checker_impl.h

@@ -92,17 +92,17 @@ class HealthCheckerFactory : public Logger::Loggable<Logger::Id::health_checker>
                                         const std::string& config_hostname,
                                         const ClusterInfoConstSharedPtr& cluster);
   /**
-   * Create a health checker.
+   * Create a health checker or return an error.
    * @param health_check_config supplies the health check proto.
    * @param cluster supplies the owning cluster.
    * @param runtime supplies the runtime loader.
    * @param dispatcher supplies the dispatcher.
    * @param log_manager supplies the log_manager.
    * @param validation_visitor message validation visitor instance.
    * @param api reference to the Api object
-   * @return a health checker.
+   * @return a health checker or creation error.
    */
-  static HealthCheckerSharedPtr
+  static absl::StatusOr<HealthCheckerSharedPtr>
   create(const envoy::config::core::v3::HealthCheck& health_check_config,
          Upstream::Cluster& cluster, Runtime::Loader& runtime, Event::Dispatcher& dispatcher,
          AccessLog::AccessLogManager& log_manager,
@@ -159,7 +159,7 @@ class PayloadMatcher {
 public:
   using MatchSegments = std::list<std::vector<uint8_t>>;
 
-  static MatchSegments loadProtoBytes(
+  static absl::StatusOr<MatchSegments> loadProtoBytes(
       const Protobuf::RepeatedPtrField<envoy::config::core::v3::HealthCheck::Payload>& byte_array);
   static bool match(const MatchSegments& expected, const Buffer::Instance& buffer);
 };

source/common/upstream/health_discovery_service.cc

@@ -195,10 +195,11 @@ envoy::config::cluster::v3::Cluster HdsDelegate::createClusterConfig(
   return cluster_config;
 }
 
-void HdsDelegate::updateHdsCluster(HdsClusterPtr cluster,
-                                   const envoy::config::cluster::v3::Cluster& cluster_config,
-                                   const envoy::config::core::v3::BindConfig& bind_config) {
-  cluster->update(cluster_config, bind_config, info_factory_, tls_);
+absl::Status
+HdsDelegate::updateHdsCluster(HdsClusterPtr cluster,
+                              const envoy::config::cluster::v3::Cluster& cluster_config,
+                              const envoy::config::core::v3::BindConfig& bind_config) {
+  return cluster->update(cluster_config, bind_config, info_factory_, tls_);
 }
 
 HdsClusterPtr
@@ -216,7 +217,7 @@ HdsDelegate::createHdsCluster(const envoy::config::cluster::v3::Cluster& cluster
   return new_cluster;
 }
 
-void HdsDelegate::processMessage(
+absl::Status HdsDelegate::processMessage(
     std::unique_ptr<envoy::service::health::v3::HealthCheckSpecifier>&& message) {
   ENVOY_LOG(debug, "New health check response message {} ", message->DebugString());
   ASSERT(message);
@@ -239,7 +240,11 @@ void HdsDelegate::processMessage(
       if (cluster_map_pair != hds_clusters_name_map_.end()) {
         // We have a previous cluster with this name, update.
         cluster_ptr = cluster_map_pair->second;
-        updateHdsCluster(cluster_ptr, cluster_config, cluster_health_check.upstream_bind_config());
+        absl::Status status = updateHdsCluster(cluster_ptr, cluster_config,
+                                               cluster_health_check.upstream_bind_config());
+        if (!status.ok()) {
+          return status;
+        }
       } else {
         // There is no cluster with this name previously or its an empty string, so just create a
         // new cluster.
@@ -269,6 +274,7 @@ void HdsDelegate::processMessage(
   hds_clusters_ = std::move(hds_clusters);
 
   // TODO: add stats reporting for number of clusters added, removed, and reused.
+  return absl::OkStatus();
 }
 
 void HdsDelegate::onReceiveMessage(
@@ -301,8 +307,14 @@ void HdsDelegate::onReceiveMessage(
   // Set response
   auto server_response_ms = PROTOBUF_GET_MS_OR_DEFAULT(*message, interval, 1000);
 
-  // Process the HealthCheckSpecifier message.
-  processMessage(std::move(message));
+  /// Process the HealthCheckSpecifier message.
+  absl::Status status = processMessage(std::move(message));
+  if (!status.ok()) {
+    stats_.errors_.inc();
+    ENVOY_LOG(warn, "Unable to validate health check specifier: {}", status.message());
+    // Do not continue processing message
+    return;
+  }
 
   stats_.updates_.inc();
 
@@ -377,9 +389,9 @@ HdsCluster::HdsCluster(Server::Configuration::ServerFactoryContext& server_conte
       std::make_shared<Envoy::Upstream::HostsPerLocalityImpl>(std::move(hosts_by_locality), false);
 }
 
-void HdsCluster::update(envoy::config::cluster::v3::Cluster cluster,
-                        const envoy::config::core::v3::BindConfig& bind_config,
-                        ClusterInfoFactory& info_factory, ThreadLocal::SlotAllocator& tls) {
+absl::Status HdsCluster::update(envoy::config::cluster::v3::Cluster cluster,
+                                const envoy::config::core::v3::BindConfig& bind_config,
+                                ClusterInfoFactory& info_factory, ThreadLocal::SlotAllocator& tls) {
 
   // check to see if the config changed. If it did, update.
   const uint64_t config_hash = MessageUtil::hash(cluster);
@@ -402,11 +414,15 @@ void HdsCluster::update(envoy::config::cluster::v3::Cluster cluster,
     updateHosts(cluster_.load_assignment().endpoints(), update_cluster_info);
 
     // Check to see if any of the health checkers have changed.
-    updateHealthchecks(cluster_.health_checks());
+    absl::Status status = updateHealthchecks(cluster_.health_checks());
+    if (!status.ok()) {
+      return status;
+    }
   }
+  return absl::OkStatus();
 }
 
-void HdsCluster::updateHealthchecks(
+absl::Status HdsCluster::updateHealthchecks(
     const Protobuf::RepeatedPtrField<envoy::config::core::v3::HealthCheck>& health_checks) {
   std::vector<Upstream::HealthCheckerSharedPtr> health_checkers;
   HealthCheckerMap health_checkers_map;
@@ -420,10 +436,12 @@ void HdsCluster::updateHealthchecks(
       health_checkers.push_back(health_checker->second);
     } else {
       // If it does not, create a new one.
-      auto new_health_checker = Upstream::HealthCheckerFactory::create(
+      auto checker_or_error = Upstream::HealthCheckerFactory::create(
           health_check, *this, server_context_.runtime(), server_context_.mainThreadDispatcher(),
           server_context_.accessLogManager(), server_context_.messageValidationVisitor(),
           server_context_.api());
+      RETURN_IF_STATUS_NOT_OK(checker_or_error);
+      auto new_health_checker = checker_or_error.value();
       health_checkers_map.insert({health_check, new_health_checker});
       health_checkers.push_back(new_health_checker);
 
@@ -437,6 +455,7 @@ void HdsCluster::updateHealthchecks(
   health_checkers_map_ = std::move(health_checkers_map);
 
   // TODO: add stats reporting for number of health checkers added, removed, and reused.
+  return absl::OkStatus();
 }
 
 void HdsCluster::updateHosts(
@@ -532,11 +551,13 @@ ProdClusterInfoFactory::createClusterInfo(const CreateClusterInfoParams& params)
 
 void HdsCluster::initHealthchecks() {
   for (auto& health_check : cluster_.health_checks()) {
-    auto health_checker = Upstream::HealthCheckerFactory::create(
+    auto health_checker_or_error = Upstream::HealthCheckerFactory::create(
         health_check, *this, server_context_.runtime(), server_context_.mainThreadDispatcher(),
         server_context_.accessLogManager(), server_context_.messageValidationVisitor(),
         server_context_.api());
+    THROW_IF_STATUS_NOT_OK(health_checker_or_error, throw);
 
+    auto health_checker = health_checker_or_error.value();
     health_checkers_.push_back(health_checker);
     health_checkers_map_.insert({health_check, health_checker});
     health_checker->start();

source/common/upstream/health_discovery_service.h

@@ -64,9 +64,9 @@ class HdsCluster : public Cluster, Logger::Loggable<Logger::Id::upstream> {
   const Outlier::Detector* outlierDetector() const override { return outlier_detector_.get(); }
   void initialize(std::function<void()> callback) override;
   // Compare changes in the cluster proto, and update parts of the cluster as needed.
-  void update(envoy::config::cluster::v3::Cluster cluster,
-              const envoy::config::core::v3::BindConfig& bind_config,
-              ClusterInfoFactory& info_factory, ThreadLocal::SlotAllocator& tls);
+  absl::Status update(envoy::config::cluster::v3::Cluster cluster,
+                      const envoy::config::core::v3::BindConfig& bind_config,
+                      ClusterInfoFactory& info_factory, ThreadLocal::SlotAllocator& tls);
   // Creates healthcheckers and adds them to the list, then does initial start.
   void initHealthchecks();
 
@@ -98,7 +98,7 @@ class HdsCluster : public Cluster, Logger::Loggable<Logger::Id::upstream> {
   HealthCheckerMap health_checkers_map_;
   TimeSource& time_source_;
 
-  void updateHealthchecks(
+  absl::Status updateHealthchecks(
       const Protobuf::RepeatedPtrField<envoy::config::core::v3::HealthCheck>& health_checks);
   void
   updateHosts(const Protobuf::RepeatedPtrField<envoy::config::endpoint::v3::LocalityLbEndpoints>&
@@ -157,12 +157,13 @@ class HdsDelegate : Grpc::AsyncStreamCallbacks<envoy::service::health::v3::Healt
   void handleFailure();
   // Establishes a connection with the management server
   void establishNewStream();
-  void processMessage(std::unique_ptr<envoy::service::health::v3::HealthCheckSpecifier>&& message);
+  absl::Status
+  processMessage(std::unique_ptr<envoy::service::health::v3::HealthCheckSpecifier>&& message);
   envoy::config::cluster::v3::Cluster
   createClusterConfig(const envoy::service::health::v3::ClusterHealthCheck& cluster_health_check);
-  void updateHdsCluster(HdsClusterPtr cluster,
-                        const envoy::config::cluster::v3::Cluster& cluster_health_check,
-                        const envoy::config::core::v3::BindConfig& bind_config);
+  absl::Status updateHdsCluster(HdsClusterPtr cluster,
+                                const envoy::config::cluster::v3::Cluster& cluster_health_check,
+                                const envoy::config::core::v3::BindConfig& bind_config);
   HdsClusterPtr createHdsCluster(const envoy::config::cluster::v3::Cluster& cluster_health_check,
                                  const envoy::config::core::v3::BindConfig& bind_config);
   HdsDelegateStats stats_;

source/extensions/health_checkers/http/health_checker_impl.cc

@@ -63,7 +63,6 @@ HttpHealthCheckerImpl::HttpHealthCheckerImpl(const Cluster& cluster,
                                              HealthCheckEventLoggerPtr&& event_logger)
     : HealthCheckerImplBase(cluster, config, dispatcher, runtime, random, std::move(event_logger)),
       path_(config.http_health_check().path()), host_value_(config.http_health_check().host()),
-      receive_bytes_(PayloadMatcher::loadProtoBytes(config.http_health_check().receive())),
       method_(getMethod(config.http_health_check().method())),
       response_buffer_size_(PROTOBUF_GET_WRAPPED_OR_DEFAULT(
           config.http_health_check(), response_buffer_size, kDefaultMaxBytesInBuffer)),
@@ -75,6 +74,9 @@ HttpHealthCheckerImpl::HttpHealthCheckerImpl(const Cluster& cluster,
                            static_cast<uint64_t>(Http::Code::OK)),
       codec_client_type_(codecClientType(config.http_health_check().codec_client_type())),
       random_generator_(random) {
+  auto bytes_or_error = PayloadMatcher::loadProtoBytes(config.http_health_check().receive());
+  THROW_IF_STATUS_NOT_OK(bytes_or_error, throw);
+  receive_bytes_ = bytes_or_error.value();
   if (config.http_health_check().has_service_name_matcher()) {
     service_name_matcher_.emplace(config.http_health_check().service_name_matcher());
   }

source/extensions/health_checkers/http/health_checker_impl.h

@@ -164,7 +164,7 @@ class HttpHealthCheckerImpl : public HealthCheckerImplBase {
 
   const std::string path_;
   const std::string host_value_;
-  const PayloadMatcher::MatchSegments receive_bytes_;
+  PayloadMatcher::MatchSegments receive_bytes_;
   const envoy::config::core::v3::RequestMethod method_;
   uint64_t response_buffer_size_;
   absl::optional<Matchers::StringMatcherImpl<envoy::type::matcher::v3::StringMatcher>>

source/extensions/health_checkers/tcp/health_checker_impl.cc

@@ -53,9 +53,14 @@ TcpHealthCheckerImpl::TcpHealthCheckerImpl(const Cluster& cluster,
         if (!config.tcp_health_check().send().text().empty()) {
           send_repeated.Add()->CopyFrom(config.tcp_health_check().send());
         }
-        return PayloadMatcher::loadProtoBytes(send_repeated);
-      }()),
-      receive_bytes_(PayloadMatcher::loadProtoBytes(config.tcp_health_check().receive())) {}
+        auto bytes_or_error = PayloadMatcher::loadProtoBytes(send_repeated);
+        THROW_IF_STATUS_NOT_OK(bytes_or_error, throw);
+        return bytes_or_error.value();
+      }()) {
+  auto bytes_or_error = PayloadMatcher::loadProtoBytes(config.tcp_health_check().receive());
+  THROW_IF_STATUS_NOT_OK(bytes_or_error, throw);
+  receive_bytes_ = bytes_or_error.value();
+}
 
 TcpHealthCheckerImpl::TcpActiveHealthCheckSession::~TcpActiveHealthCheckSession() {
   ASSERT(client_ == nullptr);

source/extensions/health_checkers/tcp/health_checker_impl.h

@@ -101,7 +101,7 @@ class TcpHealthCheckerImpl : public HealthCheckerImplBase {
   }
 
   const PayloadMatcher::MatchSegments send_bytes_;
-  const PayloadMatcher::MatchSegments receive_bytes_;
+  PayloadMatcher::MatchSegments receive_bytes_;
 };
 
 } // namespace Upstream