From d4b04fce3bc32d81fa97173f6376e7132874e968 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Mon, 5 Jun 2023 15:54:06 -0400 Subject: [PATCH 1/5] build: adding an disable_exceptions which does not yet work Signed-off-by: Alyssa Wilk --- bazel/BUILD | 7 ++ bazel/envoy_build_system.bzl | 2 + bazel/envoy_internal.bzl | 3 +- bazel/envoy_mobile_defines.bzl | 3 +- bazel/envoy_select.bzl | 7 ++ mobile/library/common/engine.cc | 10 +-- source/common/common/thread.h | 19 ++++- source/common/filter/config_discovery_impl.cc | 18 ++--- source/common/router/header_parser_utils.cc | 5 +- source/common/runtime/runtime_impl.cc | 11 +-- source/common/secret/sds_api.cc | 6 +- source/common/stats/tag_extractor_impl.cc | 5 +- source/common/upstream/cds_api_helper.cc | 6 +- .../upstream/health_discovery_service.cc | 4 +- source/common/upstream/upstream_impl.cc | 37 +++++----- source/exe/stripped_main_base.cc | 4 +- .../file_based_metadata/config.cc | 2 +- .../network/dns_resolver/cares/dns_impl.cc | 2 +- source/server/options_impl.cc | 11 ++- source/server/server.cc | 69 +++++++++---------- 20 files changed, 132 insertions(+), 99 deletions(-) diff --git a/bazel/BUILD b/bazel/BUILD index ca8db723eb3e..1ad963a26072 100644 --- a/bazel/BUILD +++ b/bazel/BUILD @@ -375,6 +375,13 @@ config_setting( values = {"define": "envoy_yaml=disabled"}, ) +# The goal here is to allow Envoy to build with this option but it is not yet +# complete. See https://github.com/envoyproxy/envoy/issues/27412 +config_setting( + name = "disable_exceptions", + values = {"define": "envoy_exceptions=disabled"}, +) + config_setting( name = "disable_envoy_mobile_listener", values = {"define": "envoy_mobile_listener=disabled"}, diff --git a/bazel/envoy_build_system.bzl b/bazel/envoy_build_system.bzl index da025768f034..8f05c9fcb8ff 100644 --- a/bazel/envoy_build_system.bzl +++ b/bazel/envoy_build_system.bzl @@ -22,6 +22,7 @@ load( _envoy_select_admin_html = "envoy_select_admin_html", _envoy_select_admin_no_html = "envoy_select_admin_no_html", _envoy_select_boringssl = "envoy_select_boringssl", + _envoy_select_disable_exceptions = "envoy_select_disable_exceptions", _envoy_select_disable_logging = "envoy_select_disable_logging", _envoy_select_enable_http3 = "envoy_select_enable_http3", _envoy_select_enable_http_datagrams = "envoy_select_enable_http_datagrams", @@ -242,6 +243,7 @@ envoy_select_disable_logging = _envoy_select_disable_logging envoy_select_google_grpc = _envoy_select_google_grpc envoy_select_enable_http3 = _envoy_select_enable_http3 envoy_select_enable_yaml = _envoy_select_enable_yaml +envoy_select_disable_exceptions = _envoy_select_disable_exceptions envoy_select_hot_restart = _envoy_select_hot_restart envoy_select_enable_http_datagrams = _envoy_select_enable_http_datagrams envoy_select_signal_trace = _envoy_select_signal_trace diff --git a/bazel/envoy_internal.bzl b/bazel/envoy_internal.bzl index 472dfd2fa483..a1f8f1dc6e50 100644 --- a/bazel/envoy_internal.bzl +++ b/bazel/envoy_internal.bzl @@ -1,6 +1,6 @@ # DO NOT LOAD THIS FILE. Targets from this file should be considered private # and not used outside of the @envoy//bazel package. -load(":envoy_select.bzl", "envoy_select_admin_html", "envoy_select_disable_logging", "envoy_select_google_grpc", "envoy_select_hot_restart", "envoy_select_signal_trace", "envoy_select_static_extension_registration") +load(":envoy_select.bzl", "envoy_select_admin_html", "envoy_select_disable_exceptions", "envoy_select_disable_logging", "envoy_select_google_grpc", "envoy_select_hot_restart", "envoy_select_signal_trace", "envoy_select_static_extension_registration") # Compute the final copts based on various options. def envoy_copts(repository, test = False): @@ -119,6 +119,7 @@ def envoy_copts(repository, test = False): repository + "//bazel:uhv_enabled": ["-DENVOY_ENABLE_UHV"], "//conditions:default": [], }) + envoy_select_hot_restart(["-DENVOY_HOT_RESTART"], repository) + \ + envoy_select_disable_exceptions(["-fno-unwind-tables", "-fno-exceptions"], repository) + \ envoy_select_admin_html(["-DENVOY_ADMIN_HTML"], repository) + \ envoy_select_static_extension_registration(["-DENVOY_STATIC_EXTENSION_REGISTRATION"], repository) + \ envoy_select_disable_logging(["-DENVOY_DISABLE_LOGGING"], repository) + \ diff --git a/bazel/envoy_mobile_defines.bzl b/bazel/envoy_mobile_defines.bzl index df2fd7a7cdbf..5bce8ebef8bf 100644 --- a/bazel/envoy_mobile_defines.bzl +++ b/bazel/envoy_mobile_defines.bzl @@ -1,11 +1,12 @@ # DO NOT LOAD THIS FILE. Load envoy_build_system.bzl instead. -load(":envoy_select.bzl", "envoy_select_admin_functionality", "envoy_select_enable_http3", "envoy_select_enable_http_datagrams", "envoy_select_enable_yaml", "envoy_select_envoy_mobile_listener", "envoy_select_envoy_mobile_request_compression", "envoy_select_envoy_mobile_stats_reporting", "envoy_select_google_grpc") +load(":envoy_select.bzl", "envoy_select_admin_functionality", "envoy_select_disable_exceptions", "envoy_select_enable_http3", "envoy_select_enable_http_datagrams", "envoy_select_enable_yaml", "envoy_select_envoy_mobile_listener", "envoy_select_envoy_mobile_request_compression", "envoy_select_envoy_mobile_stats_reporting", "envoy_select_google_grpc") # Compute the defines needed for Envoy Mobile libraries that don't use Envoy's main library wrappers. def envoy_mobile_defines(repository): return envoy_select_admin_functionality(["ENVOY_ADMIN_FUNCTIONALITY"], repository) + \ envoy_select_enable_http3(["ENVOY_ENABLE_QUIC"], repository) + \ envoy_select_enable_yaml(["ENVOY_ENABLE_YAML"], repository) + \ + envoy_select_disable_exceptions(["ENVOY_DISABLE_EXCEPTIONS"], repository) + \ envoy_select_enable_http_datagrams(["ENVOY_ENABLE_HTTP_DATAGRAMS"], repository) + \ envoy_select_envoy_mobile_listener(["ENVOY_MOBILE_ENABLE_LISTENER"], repository) + \ envoy_select_envoy_mobile_stats_reporting(["ENVOY_MOBILE_STATS_REPORTING"], repository) + \ diff --git a/bazel/envoy_select.bzl b/bazel/envoy_select.bzl index 7fee86b3eeab..7cd774bd460e 100644 --- a/bazel/envoy_select.bzl +++ b/bazel/envoy_select.bzl @@ -94,6 +94,13 @@ def envoy_select_enable_yaml(xs, repository = ""): "//conditions:default": xs, }) +# Selects the given values if exceptions are disabled in the current build. +def envoy_select_disable_exceptions(xs, repository = ""): + return select({ + repository + "//bazel:disable_exceptions": xs, + "//conditions:default": [], + }) + # Selects the given values if HTTP datagram support is enabled in the current build. def envoy_select_enable_http_datagrams(xs, repository = ""): return select({ diff --git a/mobile/library/common/engine.cc b/mobile/library/common/engine.cc index a21c37c80ac5..125b443728fe 100644 --- a/mobile/library/common/engine.cc +++ b/mobile/library/common/engine.cc @@ -49,7 +49,7 @@ envoy_status_t Engine::main(std::unique_ptr&& options) { std::unique_ptr main_common; { Thread::LockGuard lock(mutex_); - try { + TRY_NEEDS_AUDIT { if (event_tracker_.track != nullptr) { assert_handler_registration_ = Assert::addDebugAssertionFailureRecordAction([this](const char* location) { @@ -79,13 +79,9 @@ envoy_status_t Engine::main(std::unique_ptr&& options) { event_dispatcher_ = &server_->dispatcher(); cv_.notifyAll(); - } catch (const Envoy::NoServingException& e) { - PANIC(e.what()); - } catch (const Envoy::MalformedArgvException& e) { - PANIC(e.what()); - } catch (const Envoy::EnvoyException& e) { - PANIC(e.what()); } + END_TRY + CATCH(const Envoy::EnvoyException& e, { PANIC(e.what()); }); // Note: We're waiting longer than we might otherwise to drain to the main thread's dispatcher. // This is because we're not simply waiting for its availability and for it to have started, but diff --git a/source/common/common/thread.h b/source/common/common/thread.h index b3f2fa17652c..ead02e6bf06b 100644 --- a/source/common/common/thread.h +++ b/source/common/common/thread.h @@ -237,9 +237,22 @@ class MainThread { #define END_TRY } +#ifdef ENVOY_DISABLE_EXCEPTIONS +#define TRY_NEEDS_AUDIT { +#else // TODO(chaoqinli-1123): Remove this macros after we have removed all the exceptions from data // plane. -#define TRY_NEEDS_AUDIT try +#define TRY_NEEDS_AUDIT try { +#endif + +#ifdef ENVOY_DISABLE_EXCEPTIONS +#define CATCH(ExceptionType, Handler) +#else +#define CATCH(Exception, Handler) \ + catch (Exception) { \ + Handler \ + } +#endif // These convenience macros assert properties of the threading system, when // feasible. There is a platform-specific mechanism for determining whether the @@ -277,6 +290,9 @@ class MainThread { #endif +#ifdef ENVOY_DISABLE_EXCEPTIONS +#define TRY_ASSERT_MAIN_THREAD { +#else /** * To improve exception safety in data plane, we plan to forbid the use of raw * try in the core code base. This macros uses main thread assertion to make @@ -285,6 +301,7 @@ class MainThread { #define TRY_ASSERT_MAIN_THREAD \ try { \ ASSERT_IS_MAIN_OR_TEST_THREAD(); +#endif /** * RAII class to override thread assertions checks in the macros: diff --git a/source/common/filter/config_discovery_impl.cc b/source/common/filter/config_discovery_impl.cc index c08e0d146383..ae1966ea3037 100644 --- a/source/common/filter/config_discovery_impl.cc +++ b/source/common/filter/config_discovery_impl.cc @@ -21,7 +21,7 @@ void validateTypeUrlHelper(const std::string& type_url, const absl::flat_hash_set 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, ", "))); + type_url, absl::StrJoin(require_type_urls, ", "))); } } @@ -98,8 +98,8 @@ void FilterConfigSubscription::onConfigUpdate( const auto& filter_config = dynamic_cast( resources[0].get().resource()); if (filter_config.name() != filter_config_name_) { - throw EnvoyException(fmt::format("Unexpected resource name in ExtensionConfigDS response: {}", - filter_config.name())); + throw EnvoyException(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()); @@ -220,11 +220,12 @@ void FilterConfigProviderManagerImplBase::applyLastOrDefaultConfig( subscription->lastFactoryName()); last_config_valid = true; } - END_TRY catch (const EnvoyException& e) { + END_TRY CATCH(const EnvoyException& e, { ENVOY_LOG(debug, "ECDS subscription {} is invalid in a listener context: {}.", filter_config_name, e.what()); subscription->incrementConflictCounter(); - } + }); + if (last_config_valid) { provider.onConfigUpdate(*subscription->lastConfig(), subscription->lastVersionInfo(), nullptr); @@ -241,9 +242,10 @@ void FilterConfigProviderManagerImplBase::validateProtoConfigDefaultFactory( const bool null_default_factory, const std::string& filter_config_name, absl::string_view type_url) const { if (null_default_factory) { - throw EnvoyException(fmt::format("Error: cannot find filter factory {} for default filter " - "configuration with type URL {}.", - filter_config_name, type_url)); + throw EnvoyException( + fmt::format("Error: cannot find filter factory {} for default filter " + "configuration with type URL {}.", + filter_config_name, type_url)); } } diff --git a/source/common/router/header_parser_utils.cc b/source/common/router/header_parser_utils.cc index 39136cfce837..d278e04de445 100644 --- a/source/common/router/header_parser_utils.cc +++ b/source/common/router/header_parser_utils.cc @@ -51,10 +51,7 @@ std::string HeaderParser::translateMetadataFormat(const std::string& header_valu int subs = absl::StrReplaceAll({{matches[0].as_string(), new_format}}, &new_header_value); ASSERT(subs > 0); } - END_TRY - catch (Json::Exception& e) { - return header_value; - } + END_TRY CATCH(Json::Exception & e, { return header_value; }); } return new_header_value; diff --git a/source/common/runtime/runtime_impl.cc b/source/common/runtime/runtime_impl.cc index 423493601583..5b6130fa197b 100644 --- a/source/common/runtime/runtime_impl.cc +++ b/source/common/runtime/runtime_impl.cc @@ -599,9 +599,10 @@ void RtdsSubscription::validateUpdateSize(uint32_t added_resources_num, uint32_t removed_resources_num) { if (added_resources_num + removed_resources_num != 1) { init_target_.ready(); - throw EnvoyException(fmt::format("Unexpected RTDS resource length, number of added recources " - "{}, number of removed recources {}", - added_resources_num, removed_resources_num)); + throw EnvoyException( + fmt::format("Unexpected RTDS resource length, number of added recources " + "{}, number of removed recources {}", + added_resources_num, removed_resources_num)); } } @@ -690,13 +691,13 @@ SnapshotImplPtr LoaderImpl::createNewSnapshot() { ++disk_layers; } END_TRY - catch (EnvoyException& e) { + CATCH(EnvoyException & e, { // TODO(htuch): Consider latching here, rather than ignoring the // layer. This would be consistent with filesystem RTDS. ++error_layers; ENVOY_LOG(debug, "error loading runtime values for layer {} from disk: {}", layer.DebugString(), e.what()); - } + }); } break; } diff --git a/source/common/secret/sds_api.cc b/source/common/secret/sds_api.cc index e20ca282f22e..581251f65cee 100644 --- a/source/common/secret/sds_api.cc +++ b/source/common/secret/sds_api.cc @@ -47,8 +47,8 @@ void SdsApi::resolveDataSource(const FileContentMap& files, void SdsApi::onWatchUpdate() { // Filesystem reads and update callbacks can fail if the key material is missing or bad. We're not // under an onConfigUpdate() context, so we need to catch these cases explicitly here. + // Obtain a stable set of files. If a rotation happens while we're reading, TRY_ASSERT_MAIN_THREAD { - // Obtain a stable set of files. If a rotation happens while we're reading, // then we need to try again. uint64_t prev_hash = 0; FileContentMap files = loadFiles(); @@ -73,10 +73,10 @@ void SdsApi::onWatchUpdate() { } } END_TRY - catch (const EnvoyException& e) { + CATCH(const EnvoyException& e, { ENVOY_LOG_MISC(warn, fmt::format("Failed to reload certificates: {}", e.what())); sds_api_stats_.key_rotation_failed_.inc(); - } + }); } void SdsApi::onConfigUpdate(const std::vector& resources, diff --git a/source/common/stats/tag_extractor_impl.cc b/source/common/stats/tag_extractor_impl.cc index 2ac20ee1648f..75efdcf73017 100644 --- a/source/common/stats/tag_extractor_impl.cc +++ b/source/common/stats/tag_extractor_impl.cc @@ -21,9 +21,8 @@ namespace { std::regex parseStdRegex(const std::string& regex) { TRY_ASSERT_MAIN_THREAD { return std::regex(regex, std::regex::optimize); } END_TRY - catch (const std::regex_error& e) { - throw EnvoyException(fmt::format("Invalid regex '{}': {}", regex, e.what())); - } + CATCH(const std::regex_error& e, + { throw EnvoyException(fmt::format("Invalid regex '{}': {}", regex, e.what())); }); } } // namespace diff --git a/source/common/upstream/cds_api_helper.cc b/source/common/upstream/cds_api_helper.cc index 6bd1655d4a5e..306d6c0fe1e9 100644 --- a/source/common/upstream/cds_api_helper.cc +++ b/source/common/upstream/cds_api_helper.cc @@ -54,10 +54,10 @@ CdsApiHelper::onConfigUpdate(const std::vector& adde } } END_TRY - catch (const EnvoyException& e) { - exception_msgs.push_back(fmt::format("{}: {}", cluster.name(), e.what())); - } + CATCH(const EnvoyException& e, + { exception_msgs.push_back(fmt::format("{}: {}", cluster.name(), e.what())); }); } + for (const auto& resource_name : removed_resources) { if (cm_.removeCluster(resource_name)) { any_applied = true; diff --git a/source/common/upstream/health_discovery_service.cc b/source/common/upstream/health_discovery_service.cc index 30f16ac78084..08815cb4acdd 100644 --- a/source/common/upstream/health_discovery_service.cc +++ b/source/common/upstream/health_discovery_service.cc @@ -289,14 +289,14 @@ void HdsDelegate::onReceiveMessage( server_context_.messageValidationContext().dynamicValidationVisitor()); } END_TRY - catch (const ProtoValidationException& ex) { + CATCH(const ProtoValidationException& ex, { // Increment error count stats_.errors_.inc(); ENVOY_LOG(warn, "Unable to validate health check specifier: {}", ex.what()); // Do not continue processing message return; - } + }); // Set response auto server_response_ms = PROTOBUF_GET_MS_OR_DEFAULT(*message, interval, 1000); diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 5bda8581a491..eb78b4c5801b 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -94,9 +94,10 @@ createProtocolOptionsConfig(const std::string& name, const ProtobufWkt::Any& typ } if (factory == nullptr) { - throw EnvoyException(fmt::format("Didn't find a registered network or http filter or protocol " - "options implementation for name: '{}'", - name)); + throw EnvoyException( + fmt::format("Didn't find a registered network or http filter or protocol " + "options implementation for name: '{}'", + name)); } ProtobufTypes::MessagePtr proto_config = factory->createEmptyProtocolOptionsProto(); @@ -954,8 +955,9 @@ createOptions(const envoy::config::cluster::v3::Cluster& config, if (config.protocol_selection() == envoy::config::cluster::v3::Cluster::USE_CONFIGURED_PROTOCOL) { // Make sure multiple protocol configurations are not present if (config.has_http_protocol_options() && config.has_http2_protocol_options()) { - throw EnvoyException(fmt::format("cluster: Both HTTP1 and HTTP2 options may only be " - "configured with non-default 'protocol_selection' values")); + throw EnvoyException( + fmt::format("cluster: Both HTTP1 and HTTP2 options may only be " + "configured with non-default 'protocol_selection' values")); } } @@ -1093,15 +1095,16 @@ ClusterInfoImpl::ClusterInfoImpl( added_via_api_(added_via_api), has_configured_http_filters_(false) { #ifdef WIN32 if (set_local_interface_name_on_upstream_connections_) { - throw EnvoyException("set_local_interface_name_on_upstream_connections_ cannot be set to true " - "on Windows platforms"); + throw EnvoyException( + "set_local_interface_name_on_upstream_connections_ cannot be set to true " + "on Windows platforms"); } #endif if (config.has_max_requests_per_connection() && http_protocol_options_->common_http_protocol_options_.has_max_requests_per_connection()) { throw EnvoyException("Only one of max_requests_per_connection from Cluster or " - "HttpProtocolOptions can be specified"); + "HttpProtocolOptions can be specified"); } // If load_balancing_policy is set we will use it directly, ignoring lb_policy. @@ -1144,8 +1147,8 @@ ClusterInfoImpl::ClusterInfoImpl( if (config.lb_subset_config().locality_weight_aware() && !config.common_lb_config().has_locality_weighted_lb_config()) { throw EnvoyException(fmt::format("Locality weight aware subset LB requires that a " - "locality_weighted_lb_config be set in {}", - name_)); + "locality_weighted_lb_config be set in {}", + name_)); } // Use default (1h) or configured `idle_timeout`, unless it's set to 0, indicating that no @@ -1253,7 +1256,8 @@ void ClusterInfoImpl::configureLbPolicies(const envoy::config::cluster::v3::Clus } if (config.has_lb_subset_config()) { - throw EnvoyException("cluster: load_balancing_policy cannot be combined with lb_subset_config"); + throw EnvoyException( + "cluster: load_balancing_policy cannot be combined with lb_subset_config"); } if (config.has_common_lb_config()) { @@ -1286,9 +1290,10 @@ void ClusterInfoImpl::configureLbPolicies(const envoy::config::cluster::v3::Clus } if (load_balancer_factory_ == nullptr) { - throw EnvoyException(fmt::format("cluster: didn't find a registered load balancer factory " - "implementation for cluster: '{}' with names from [{}]", - name_, absl::StrJoin(missing_policies, ", "))); + throw EnvoyException( + fmt::format("cluster: didn't find a registered load balancer factory " + "implementation for cluster: '{}' with names from [{}]", + name_, absl::StrJoin(missing_policies, ", "))); } lb_type_ = LoadBalancerType::LoadBalancingPolicyConfig; @@ -1623,7 +1628,7 @@ const Network::Address::InstanceConstSharedPtr ClusterImplBase::resolveProtoAddress(const envoy::config::core::v3::Address& address) { TRY_ASSERT_MAIN_THREAD { return Network::Address::resolveProtoAddress(address); } END_TRY - catch (EnvoyException& e) { + CATCH(EnvoyException & e, { if (info_->type() == envoy::config::cluster::v3::Cluster::STATIC || info_->type() == envoy::config::cluster::v3::Cluster::EDS) { throw EnvoyException(fmt::format("{}. Consider setting resolver_name or setting cluster type " @@ -1631,7 +1636,7 @@ ClusterImplBase::resolveProtoAddress(const envoy::config::core::v3::Address& add e.what())); } throw e; - } + }); } void ClusterImplBase::validateEndpointsForZoneAwareRouting( diff --git a/source/exe/stripped_main_base.cc b/source/exe/stripped_main_base.cc index 9d532a993b90..878e72a75735 100644 --- a/source/exe/stripped_main_base.cc +++ b/source/exe/stripped_main_base.cc @@ -129,10 +129,10 @@ void StrippedMainBase::configureHotRestarter(Random::RandomGenerator& random_gen options_.socketMode()); } END_TRY - catch (Server::HotRestartDomainSocketInUseException& ex) { + CATCH(Server::HotRestartDomainSocketInUseException & ex, { // No luck, try again. ENVOY_LOG_MISC(debug, "dynamic base id: {}", ex.what()); - } + }); } if (restarter == nullptr) { diff --git a/source/extensions/grpc_credentials/file_based_metadata/config.cc b/source/extensions/grpc_credentials/file_based_metadata/config.cc index aebba3b17150..a0a9db5e6617 100644 --- a/source/extensions/grpc_credentials/file_based_metadata/config.cc +++ b/source/extensions/grpc_credentials/file_based_metadata/config.cc @@ -74,7 +74,7 @@ FileBasedMetadataAuthenticator::GetMetadata(grpc::string_ref, grpc::string_ref, TRY_NEEDS_AUDIT { std::string header_value = Envoy::Config::DataSource::read(config_.secret_data(), true, api_); metadata->insert(std::make_pair(header_key, header_prefix + header_value)); - } + } END_TRY catch (const EnvoyException& e) { return grpc::Status(grpc::StatusCode::NOT_FOUND, e.what()); } diff --git a/source/extensions/network/dns_resolver/cares/dns_impl.cc b/source/extensions/network/dns_resolver/cares/dns_impl.cc index 1c4bd6fa6a51..cff6fb7f42bf 100644 --- a/source/extensions/network/dns_resolver/cares/dns_impl.cc +++ b/source/extensions/network/dns_resolver/cares/dns_impl.cc @@ -286,7 +286,7 @@ void DnsResolverImpl::PendingResolution::finishResolve() { // exception in fuzz tests. TRY_NEEDS_AUDIT { callback_(pending_response_.status_, std::move(pending_response_.address_list_)); - } + } END_TRY catch (const EnvoyException& e) { ENVOY_LOG(critical, "EnvoyException in c-ares callback: {}", e.what()); dispatcher_.post([s = std::string(e.what())] { throw EnvoyException(s); }); diff --git a/source/server/options_impl.cc b/source/server/options_impl.cc index 6345c789e897..633fe423d7be 100644 --- a/source/server/options_impl.cc +++ b/source/server/options_impl.cc @@ -166,20 +166,19 @@ OptionsImpl::OptionsImpl(std::vector args, count_ = cmd.getArgList().size(); } END_TRY - catch (TCLAP::ArgException& e) { + CATCH (TCLAP::ArgException& e, { TRY_ASSERT_MAIN_THREAD { cmd.getOutput()->failure(cmd, e); } END_TRY - catch (const TCLAP::ExitException&) { + CATCH(const TCLAP::ExitException&, { // failure() has already written an informative message to stderr, so all that's left to do // is throw our own exception with the original message. throw MalformedArgvException(e.what()); - } - } - catch (const TCLAP::ExitException& e) { + }); + }) CATCH (const TCLAP::ExitException& e, { // parse() throws an ExitException with status 0 after printing the output for --help and // --version. throw NoServingException(); - } + }); hot_restart_disabled_ = disable_hot_restart.getValue(); mutex_tracing_enabled_ = enable_mutex_tracing.getValue(); diff --git a/source/server/server.cc b/source/server/server.cc index 8ed78eb8b139..5095a72eede2 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -104,42 +104,41 @@ InstanceImpl::InstanceImpl( grpc_context_(store.symbolTable()), http_context_(store.symbolTable()), router_context_(store.symbolTable()), process_context_(std::move(process_context)), hooks_(hooks), quic_stat_names_(store.symbolTable()), server_contexts_(*this), - enable_reuse_port_default_(true), stats_flush_in_progress_(false) { - TRY_ASSERT_MAIN_THREAD { - if (!options.logPath().empty()) { - TRY_ASSERT_MAIN_THREAD { - file_logger_ = std::make_unique( - options.logPath(), access_log_manager_, Logger::Registry::getSink()); - } - END_TRY - catch (const EnvoyException& e) { - throw EnvoyException( - fmt::format("Failed to open log-file '{}'. e.what(): {}", options.logPath(), e.what())); - } - } + enable_reuse_port_default_(true), + stats_flush_in_progress_(false){ + TRY_ASSERT_MAIN_THREAD{if (!options.logPath().empty()){TRY_ASSERT_MAIN_THREAD{ + file_logger_ = std::make_unique( + options.logPath(), access_log_manager_, Logger::Registry::getSink()); +} // namespace Server +END_TRY +CATCH(const EnvoyException& e, { + throw EnvoyException( + fmt::format("Failed to open log-file '{}'. e.what(): {}", options.logPath(), e.what())); +}); +} // namespace Envoy - restarter_.initialize(*dispatcher_, *this); - drain_manager_ = component_factory.createDrainManager(*this); - initialize(std::move(local_address), component_factory); - } - END_TRY - catch (const EnvoyException& e) { - ENVOY_LOG(critical, "error initializing config '{} {} {}': {}", - options.configProto().DebugString(), options.configYaml(), options.configPath(), - e.what()); - terminate(); - throw; - } - catch (const std::exception& e) { - ENVOY_LOG(critical, "error initializing due to unexpected exception: {}", e.what()); - terminate(); - throw; - } - catch (...) { - ENVOY_LOG(critical, "error initializing due to unknown exception"); - terminate(); - throw; - } +restarter_.initialize(*dispatcher_, *this); +drain_manager_ = component_factory.createDrainManager(*this); +initialize(std::move(local_address), component_factory); +} +END_TRY +CATCH(const EnvoyException& e, { + ENVOY_LOG(critical, "error initializing config '{} {} {}': {}", + options.configProto().DebugString(), options.configYaml(), options.configPath(), + e.what()); + terminate(); + throw; +}) +CATCH(const std::exception& e, { + ENVOY_LOG(critical, "error initializing due to unexpected exception: {}", e.what()); + terminate(); + throw; +}) +CATCH(..., { + ENVOY_LOG(critical, "error initializing due to unknown exception"); + terminate(); + throw; +}) } InstanceImpl::~InstanceImpl() { From 39e8c46533b6f517d2cdc2044f1fbad7a366e82a Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Mon, 5 Jun 2023 16:43:38 -0400 Subject: [PATCH 2/5] format Signed-off-by: Alyssa Wilk --- source/common/filter/config_discovery_impl.cc | 13 ++++---- source/common/runtime/runtime_impl.cc | 7 ++-- source/common/upstream/upstream_impl.cc | 33 ++++++++----------- .../file_based_metadata/config.cc | 3 +- .../network/dns_resolver/cares/dns_impl.cc | 3 +- source/server/options_impl.cc | 20 ++++++----- 6 files changed, 38 insertions(+), 41 deletions(-) diff --git a/source/common/filter/config_discovery_impl.cc b/source/common/filter/config_discovery_impl.cc index ae1966ea3037..3f6c40cce4b4 100644 --- a/source/common/filter/config_discovery_impl.cc +++ b/source/common/filter/config_discovery_impl.cc @@ -21,7 +21,7 @@ void validateTypeUrlHelper(const std::string& type_url, const absl::flat_hash_set 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, ", "))); + type_url, absl::StrJoin(require_type_urls, ", "))); } } @@ -98,8 +98,8 @@ void FilterConfigSubscription::onConfigUpdate( const auto& filter_config = dynamic_cast( resources[0].get().resource()); if (filter_config.name() != filter_config_name_) { - throw EnvoyException(fmt::format( - "Unexpected resource name in ExtensionConfigDS response: {}", filter_config.name())); + throw EnvoyException(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()); @@ -242,10 +242,9 @@ void FilterConfigProviderManagerImplBase::validateProtoConfigDefaultFactory( const bool null_default_factory, const std::string& filter_config_name, absl::string_view type_url) const { if (null_default_factory) { - throw EnvoyException( - fmt::format("Error: cannot find filter factory {} for default filter " - "configuration with type URL {}.", - filter_config_name, type_url)); + throw EnvoyException(fmt::format("Error: cannot find filter factory {} for default filter " + "configuration with type URL {}.", + filter_config_name, type_url)); } } diff --git a/source/common/runtime/runtime_impl.cc b/source/common/runtime/runtime_impl.cc index 5b6130fa197b..bce5ae8e16a7 100644 --- a/source/common/runtime/runtime_impl.cc +++ b/source/common/runtime/runtime_impl.cc @@ -599,10 +599,9 @@ void RtdsSubscription::validateUpdateSize(uint32_t added_resources_num, uint32_t removed_resources_num) { if (added_resources_num + removed_resources_num != 1) { init_target_.ready(); - throw EnvoyException( - fmt::format("Unexpected RTDS resource length, number of added recources " - "{}, number of removed recources {}", - added_resources_num, removed_resources_num)); + throw EnvoyException(fmt::format("Unexpected RTDS resource length, number of added recources " + "{}, number of removed recources {}", + added_resources_num, removed_resources_num)); } } diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index eb78b4c5801b..1a09f4e6fb86 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -94,10 +94,9 @@ createProtocolOptionsConfig(const std::string& name, const ProtobufWkt::Any& typ } if (factory == nullptr) { - throw EnvoyException( - fmt::format("Didn't find a registered network or http filter or protocol " - "options implementation for name: '{}'", - name)); + throw EnvoyException(fmt::format("Didn't find a registered network or http filter or protocol " + "options implementation for name: '{}'", + name)); } ProtobufTypes::MessagePtr proto_config = factory->createEmptyProtocolOptionsProto(); @@ -955,9 +954,8 @@ createOptions(const envoy::config::cluster::v3::Cluster& config, if (config.protocol_selection() == envoy::config::cluster::v3::Cluster::USE_CONFIGURED_PROTOCOL) { // Make sure multiple protocol configurations are not present if (config.has_http_protocol_options() && config.has_http2_protocol_options()) { - throw EnvoyException( - fmt::format("cluster: Both HTTP1 and HTTP2 options may only be " - "configured with non-default 'protocol_selection' values")); + throw EnvoyException(fmt::format("cluster: Both HTTP1 and HTTP2 options may only be " + "configured with non-default 'protocol_selection' values")); } } @@ -1095,16 +1093,15 @@ ClusterInfoImpl::ClusterInfoImpl( added_via_api_(added_via_api), has_configured_http_filters_(false) { #ifdef WIN32 if (set_local_interface_name_on_upstream_connections_) { - throw EnvoyException( - "set_local_interface_name_on_upstream_connections_ cannot be set to true " - "on Windows platforms"); + throw EnvoyException("set_local_interface_name_on_upstream_connections_ cannot be set to true " + "on Windows platforms"); } #endif if (config.has_max_requests_per_connection() && http_protocol_options_->common_http_protocol_options_.has_max_requests_per_connection()) { throw EnvoyException("Only one of max_requests_per_connection from Cluster or " - "HttpProtocolOptions can be specified"); + "HttpProtocolOptions can be specified"); } // If load_balancing_policy is set we will use it directly, ignoring lb_policy. @@ -1147,8 +1144,8 @@ ClusterInfoImpl::ClusterInfoImpl( if (config.lb_subset_config().locality_weight_aware() && !config.common_lb_config().has_locality_weighted_lb_config()) { throw EnvoyException(fmt::format("Locality weight aware subset LB requires that a " - "locality_weighted_lb_config be set in {}", - name_)); + "locality_weighted_lb_config be set in {}", + name_)); } // Use default (1h) or configured `idle_timeout`, unless it's set to 0, indicating that no @@ -1256,8 +1253,7 @@ void ClusterInfoImpl::configureLbPolicies(const envoy::config::cluster::v3::Clus } if (config.has_lb_subset_config()) { - throw EnvoyException( - "cluster: load_balancing_policy cannot be combined with lb_subset_config"); + throw EnvoyException("cluster: load_balancing_policy cannot be combined with lb_subset_config"); } if (config.has_common_lb_config()) { @@ -1290,10 +1286,9 @@ void ClusterInfoImpl::configureLbPolicies(const envoy::config::cluster::v3::Clus } if (load_balancer_factory_ == nullptr) { - throw EnvoyException( - fmt::format("cluster: didn't find a registered load balancer factory " - "implementation for cluster: '{}' with names from [{}]", - name_, absl::StrJoin(missing_policies, ", "))); + throw EnvoyException(fmt::format("cluster: didn't find a registered load balancer factory " + "implementation for cluster: '{}' with names from [{}]", + name_, absl::StrJoin(missing_policies, ", "))); } lb_type_ = LoadBalancerType::LoadBalancingPolicyConfig; diff --git a/source/extensions/grpc_credentials/file_based_metadata/config.cc b/source/extensions/grpc_credentials/file_based_metadata/config.cc index a0a9db5e6617..ccd753761a67 100644 --- a/source/extensions/grpc_credentials/file_based_metadata/config.cc +++ b/source/extensions/grpc_credentials/file_based_metadata/config.cc @@ -74,7 +74,8 @@ FileBasedMetadataAuthenticator::GetMetadata(grpc::string_ref, grpc::string_ref, TRY_NEEDS_AUDIT { std::string header_value = Envoy::Config::DataSource::read(config_.secret_data(), true, api_); metadata->insert(std::make_pair(header_key, header_prefix + header_value)); - } END_TRY + } + END_TRY catch (const EnvoyException& e) { return grpc::Status(grpc::StatusCode::NOT_FOUND, e.what()); } diff --git a/source/extensions/network/dns_resolver/cares/dns_impl.cc b/source/extensions/network/dns_resolver/cares/dns_impl.cc index cff6fb7f42bf..12cf91e9c33f 100644 --- a/source/extensions/network/dns_resolver/cares/dns_impl.cc +++ b/source/extensions/network/dns_resolver/cares/dns_impl.cc @@ -286,7 +286,8 @@ void DnsResolverImpl::PendingResolution::finishResolve() { // exception in fuzz tests. TRY_NEEDS_AUDIT { callback_(pending_response_.status_, std::move(pending_response_.address_list_)); - } END_TRY + } + END_TRY catch (const EnvoyException& e) { ENVOY_LOG(critical, "EnvoyException in c-ares callback: {}", e.what()); dispatcher_.post([s = std::string(e.what())] { throw EnvoyException(s); }); diff --git a/source/server/options_impl.cc b/source/server/options_impl.cc index 633fe423d7be..75e54f753b64 100644 --- a/source/server/options_impl.cc +++ b/source/server/options_impl.cc @@ -166,15 +166,17 @@ OptionsImpl::OptionsImpl(std::vector args, count_ = cmd.getArgList().size(); } END_TRY - CATCH (TCLAP::ArgException& e, { - TRY_ASSERT_MAIN_THREAD { cmd.getOutput()->failure(cmd, e); } - END_TRY - CATCH(const TCLAP::ExitException&, { - // failure() has already written an informative message to stderr, so all that's left to do - // is throw our own exception with the original message. - throw MalformedArgvException(e.what()); - }); - }) CATCH (const TCLAP::ExitException& e, { + CATCH(TCLAP::ArgException & e, + { + TRY_ASSERT_MAIN_THREAD { cmd.getOutput()->failure(cmd, e); } + END_TRY + CATCH(const TCLAP::ExitException&, { + // failure() has already written an informative message to stderr, so all that's left to + // do is throw our own exception with the original message. + throw MalformedArgvException(e.what()); + }); + }) + CATCH(const TCLAP::ExitException& e, { // parse() throws an ExitException with status 0 after printing the output for --help and // --version. throw NoServingException(); From 59c46cff5aafeb913889b37198d3088777d1ccf9 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Tue, 6 Jun 2023 11:05:32 -0400 Subject: [PATCH 3/5] format Signed-off-by: Alyssa Wilk --- source/common/common/thread.h | 12 ++++++++++++ source/common/secret/sds_api.cc | 2 +- source/server/options_impl.cc | 28 +++++++++++++--------------- source/server/server.cc | 33 +++++++++++++++------------------ 4 files changed, 41 insertions(+), 34 deletions(-) diff --git a/source/common/common/thread.h b/source/common/common/thread.h index ead02e6bf06b..a1d651efe679 100644 --- a/source/common/common/thread.h +++ b/source/common/common/thread.h @@ -254,6 +254,18 @@ class MainThread { } #endif +#ifdef ENVOY_DISABLE_EXCEPTIONS +#define MULTI_CATCH(ExceptionType, Handler) +#else +#define MULTI_CATCH(Exception, Handler, Handler2) \ + catch (Exception) { \ + Handler \ + } \ + catch (...) { \ + Handler2 \ + } +#endif + // These convenience macros assert properties of the threading system, when // feasible. There is a platform-specific mechanism for determining whether the // current thread is from main(), which we call the "test thread", and if that diff --git a/source/common/secret/sds_api.cc b/source/common/secret/sds_api.cc index 581251f65cee..effcac5a1a62 100644 --- a/source/common/secret/sds_api.cc +++ b/source/common/secret/sds_api.cc @@ -47,8 +47,8 @@ void SdsApi::resolveDataSource(const FileContentMap& files, void SdsApi::onWatchUpdate() { // Filesystem reads and update callbacks can fail if the key material is missing or bad. We're not // under an onConfigUpdate() context, so we need to catch these cases explicitly here. - // Obtain a stable set of files. If a rotation happens while we're reading, TRY_ASSERT_MAIN_THREAD { + // Obtain a stable set of files. If a rotation happens while we're reading, // then we need to try again. uint64_t prev_hash = 0; FileContentMap files = loadFiles(); diff --git a/source/server/options_impl.cc b/source/server/options_impl.cc index 75e54f753b64..81d7490c095e 100644 --- a/source/server/options_impl.cc +++ b/source/server/options_impl.cc @@ -161,26 +161,24 @@ OptionsImpl::OptionsImpl(std::vector args, false, "string", cmd); cmd.setExceptionHandling(false); + + std::function failure_function = [&](TCLAP::ArgException& e) { + TRY_ASSERT_MAIN_THREAD { cmd.getOutput()->failure(cmd, e); } + END_TRY + CATCH(const TCLAP::ExitException&, { + // failure() has already written an informative message to stderr, so all that's left to do + // is throw our own exception with the original message. + throw MalformedArgvException(e.what()); + }); + }; + TRY_ASSERT_MAIN_THREAD { cmd.parse(args); count_ = cmd.getArgList().size(); } END_TRY - CATCH(TCLAP::ArgException & e, - { - TRY_ASSERT_MAIN_THREAD { cmd.getOutput()->failure(cmd, e); } - END_TRY - CATCH(const TCLAP::ExitException&, { - // failure() has already written an informative message to stderr, so all that's left to - // do is throw our own exception with the original message. - throw MalformedArgvException(e.what()); - }); - }) - CATCH(const TCLAP::ExitException& e, { - // parse() throws an ExitException with status 0 after printing the output for --help and - // --version. - throw NoServingException(); - }); + MULTI_CATCH( + TCLAP::ArgException & e, { failure_function(e); }, { throw NoServingException(); }); hot_restart_disabled_ = disable_hot_restart.getValue(); mutex_tracing_enabled_ = enable_mutex_tracing.getValue(); diff --git a/source/server/server.cc b/source/server/server.cc index 5095a72eede2..bab1bb347f96 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -115,30 +115,27 @@ CATCH(const EnvoyException& e, { throw EnvoyException( fmt::format("Failed to open log-file '{}'. e.what(): {}", options.logPath(), e.what())); }); -} // namespace Envoy restarter_.initialize(*dispatcher_, *this); drain_manager_ = component_factory.createDrainManager(*this); initialize(std::move(local_address), component_factory); +} // namespace Envoy } END_TRY -CATCH(const EnvoyException& e, { - ENVOY_LOG(critical, "error initializing config '{} {} {}': {}", - options.configProto().DebugString(), options.configYaml(), options.configPath(), - e.what()); - terminate(); - throw; -}) -CATCH(const std::exception& e, { - ENVOY_LOG(critical, "error initializing due to unexpected exception: {}", e.what()); - terminate(); - throw; -}) -CATCH(..., { - ENVOY_LOG(critical, "error initializing due to unknown exception"); - terminate(); - throw; -}) +MULTI_CATCH( + const EnvoyException& e, + { + ENVOY_LOG(critical, "error initializing config '{} {} {}': {}", + options.configProto().DebugString(), options.configYaml(), options.configPath(), + e.what()); + terminate(); + throw; + }, + { + ENVOY_LOG(critical, "error initializing due to unknown exception"); + terminate(); + throw; + }) } InstanceImpl::~InstanceImpl() { From d4dc36a291e3bb839a0f26ac4b85c0ce656fe24c Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Tue, 6 Jun 2023 12:57:23 -0400 Subject: [PATCH 4/5] fix Signed-off-by: Alyssa Wilk --- source/server/server.cc | 68 ++++++++++++++++++++++------------------- 1 file changed, 36 insertions(+), 32 deletions(-) diff --git a/source/server/server.cc b/source/server/server.cc index bab1bb347f96..91c72e529197 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -104,38 +104,42 @@ InstanceImpl::InstanceImpl( grpc_context_(store.symbolTable()), http_context_(store.symbolTable()), router_context_(store.symbolTable()), process_context_(std::move(process_context)), hooks_(hooks), quic_stat_names_(store.symbolTable()), server_contexts_(*this), - enable_reuse_port_default_(true), - stats_flush_in_progress_(false){ - TRY_ASSERT_MAIN_THREAD{if (!options.logPath().empty()){TRY_ASSERT_MAIN_THREAD{ - file_logger_ = std::make_unique( - options.logPath(), access_log_manager_, Logger::Registry::getSink()); -} // namespace Server -END_TRY -CATCH(const EnvoyException& e, { - throw EnvoyException( - fmt::format("Failed to open log-file '{}'. e.what(): {}", options.logPath(), e.what())); -}); - -restarter_.initialize(*dispatcher_, *this); -drain_manager_ = component_factory.createDrainManager(*this); -initialize(std::move(local_address), component_factory); -} // namespace Envoy -} -END_TRY -MULTI_CATCH( - const EnvoyException& e, - { - ENVOY_LOG(critical, "error initializing config '{} {} {}': {}", - options.configProto().DebugString(), options.configYaml(), options.configPath(), - e.what()); - terminate(); - throw; - }, - { - ENVOY_LOG(critical, "error initializing due to unknown exception"); - terminate(); - throw; - }) + enable_reuse_port_default_(true), stats_flush_in_progress_(false) { + std::function set_up_logger = [&] { + TRY_ASSERT_MAIN_THREAD { + file_logger_ = std::make_unique( + options.logPath(), access_log_manager_, Logger::Registry::getSink()); + } + END_TRY + CATCH(const EnvoyException& e, { + throw EnvoyException( + fmt::format("Failed to open log-file '{}'. e.what(): {}", options.logPath(), e.what())); + }); + }; + + TRY_ASSERT_MAIN_THREAD { + if (!options.logPath().empty()) { + set_up_logger(); + } + restarter_.initialize(*dispatcher_, *this); + drain_manager_ = component_factory.createDrainManager(*this); + initialize(std::move(local_address), component_factory); + } + END_TRY + MULTI_CATCH( + const EnvoyException& e, + { + ENVOY_LOG(critical, "error initializing config '{} {} {}': {}", + options.configProto().DebugString(), options.configYaml(), options.configPath(), + e.what()); + terminate(); + throw; + }, + { + ENVOY_LOG(critical, "error initializing due to unknown exception"); + terminate(); + throw; + }); } InstanceImpl::~InstanceImpl() { From 087b389a95831cd3890484835d2c56ff7de23883 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Tue, 6 Jun 2023 16:56:16 -0400 Subject: [PATCH 5/5] legacy tidy Signed-off-by: Alyssa Wilk --- .../extensions/grpc_credentials/file_based_metadata/config.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/extensions/grpc_credentials/file_based_metadata/config.cc b/source/extensions/grpc_credentials/file_based_metadata/config.cc index ccd753761a67..1fa300a2f711 100644 --- a/source/extensions/grpc_credentials/file_based_metadata/config.cc +++ b/source/extensions/grpc_credentials/file_based_metadata/config.cc @@ -77,7 +77,7 @@ FileBasedMetadataAuthenticator::GetMetadata(grpc::string_ref, grpc::string_ref, } END_TRY catch (const EnvoyException& e) { - return grpc::Status(grpc::StatusCode::NOT_FOUND, e.what()); + return {grpc::StatusCode::NOT_FOUND, e.what()}; } return grpc::Status::OK; }