diff --git a/envoy/common/exception.h b/envoy/common/exception.h index 5734df1f6628..7b74e2094b39 100644 --- a/envoy/common/exception.h +++ b/envoy/common/exception.h @@ -30,8 +30,8 @@ class EnvoyException : public std::runtime_error { }; #define SET_AND_RETURN_IF_NOT_OK(check_status, set_status) \ - if (!check_status.ok()) { \ - set_status = check_status; \ + if (const absl::Status temp_status = check_status; !temp_status.ok()) { \ + set_status = temp_status; \ return; \ } diff --git a/source/common/listener_manager/listener_impl.cc b/source/common/listener_manager/listener_impl.cc index 23350a0c2270..ed1585afe452 100644 --- a/source/common/listener_manager/listener_impl.cc +++ b/source/common/listener_manager/listener_impl.cc @@ -72,12 +72,27 @@ bool shouldBindToPort(const envoy::config::listener::v3::Listener& config) { } } // namespace +absl::StatusOr> ListenSocketFactoryImpl::create( + ListenerComponentFactory& factory, Network::Address::InstanceConstSharedPtr address, + Network::Socket::Type socket_type, const Network::Socket::OptionsSharedPtr& options, + const std::string& listener_name, uint32_t tcp_backlog_size, + ListenerComponentFactory::BindType bind_type, + const Network::SocketCreationOptions& creation_options, uint32_t num_sockets) { + absl::Status creation_status = absl::OkStatus(); + auto ret = std::unique_ptr(new ListenSocketFactoryImpl( + factory, address, socket_type, options, listener_name, tcp_backlog_size, bind_type, + creation_options, num_sockets, creation_status)); + RETURN_IF_NOT_OK(creation_status); + return ret; +} + ListenSocketFactoryImpl::ListenSocketFactoryImpl( ListenerComponentFactory& factory, Network::Address::InstanceConstSharedPtr address, Network::Socket::Type socket_type, const Network::Socket::OptionsSharedPtr& options, const std::string& listener_name, uint32_t tcp_backlog_size, ListenerComponentFactory::BindType bind_type, - const Network::SocketCreationOptions& creation_options, uint32_t num_sockets) + const Network::SocketCreationOptions& creation_options, uint32_t num_sockets, + absl::Status& creation_status) : factory_(factory), local_address_(address), socket_type_(socket_type), options_(options), listener_name_(listener_name), tcp_backlog_size_(tcp_backlog_size), bind_type_(bind_type), socket_creation_options_(creation_options) { @@ -100,8 +115,9 @@ ListenSocketFactoryImpl::ListenSocketFactoryImpl( } } - sockets_.push_back(THROW_OR_RETURN_VALUE( - createListenSocketAndApplyOptions(factory, socket_type, 0), Network::SocketSharedPtr)); + auto socket_or_error = createListenSocketAndApplyOptions(factory, socket_type, 0); + SET_AND_RETURN_IF_NOT_OK(socket_or_error.status(), creation_status); + sockets_.push_back(*socket_or_error); if (sockets_[0] != nullptr && local_address_->ip() && local_address_->ip()->port() == 0) { local_address_ = sockets_[0]->connectionInfoProvider().localAddress(); @@ -114,8 +130,9 @@ ListenSocketFactoryImpl::ListenSocketFactoryImpl( if (bind_type_ != ListenerComponentFactory::BindType::ReusePort && sockets_[0] != nullptr) { sockets_.push_back(sockets_[0]->duplicate()); } else { - sockets_.push_back(THROW_OR_RETURN_VALUE( - createListenSocketAndApplyOptions(factory, socket_type, i), Network::SocketSharedPtr)); + auto socket_or_error = createListenSocketAndApplyOptions(factory, socket_type, i); + SET_AND_RETURN_IF_NOT_OK(socket_or_error.status(), creation_status); + sockets_.push_back(*socket_or_error); } } ASSERT(sockets_.size() == num_sockets); @@ -165,12 +182,7 @@ absl::StatusOr ListenSocketFactoryImpl::createListenSo fmt::format("{}: Setting socket options {}", listener_name_, ok ? "succeeded" : "failed"); if (!ok) { ENVOY_LOG(warn, "{}", message); -#ifdef ENVOY_DISABLE_EXCEPTIONS - PANIC(message); -#else - throw Network::SocketOptionException(message); -#endif - + return absl::InvalidArgumentError(message); } else { ENVOY_LOG(debug, "{}", message); } @@ -236,8 +248,11 @@ std::string listenerStatsScope(const envoy::config::listener::v3::Listener& conf return absl::StrCat("envoy_internal_", config.name()); } auto address_or_error = Network::Address::resolveProtoAddress(config.address()); - THROW_IF_NOT_OK_REF(address_or_error.status()); - return address_or_error.value()->asString(); + if (address_or_error.status().ok()) { + return address_or_error.value()->asString(); + } + // Listener creation will fail shortly when the address is used. + return absl::StrCat("invalid_address_listener"); } } // namespace @@ -254,10 +269,22 @@ Network::DrainDecision& ListenerFactoryContextBaseImpl::drainDecision() { return Server::DrainManager& ListenerFactoryContextBaseImpl::drainManager() { return *drain_manager_; } Init::Manager& ListenerFactoryContextBaseImpl::initManager() { return server_.initManager(); } +absl::StatusOr> +ListenerImpl::create(const envoy::config::listener::v3::Listener& config, + const std::string& version_info, ListenerManagerImpl& parent, + const std::string& name, bool added_via_api, bool workers_started, + uint64_t hash) { + absl::Status creation_status = absl::OkStatus(); + auto ret = std::unique_ptr(new ListenerImpl( + config, version_info, parent, name, added_via_api, workers_started, hash, creation_status)); + RETURN_IF_NOT_OK(creation_status); + return ret; +} + ListenerImpl::ListenerImpl(const envoy::config::listener::v3::Listener& config, const std::string& version_info, ListenerManagerImpl& parent, const std::string& name, bool added_via_api, bool workers_started, - uint64_t hash) + uint64_t hash, absl::Status& creation_status) : parent_(parent), socket_type_(config.has_internal_listener() ? Network::Socket::Type::Stream @@ -324,25 +351,28 @@ ListenerImpl::ListenerImpl(const envoy::config::listener::v3::Listener& config, // All the addresses should be same socket type, so get the first address's socket type is // enough. auto address_or_error = Network::Address::resolveProtoAddress(config.address()); - THROW_IF_NOT_OK_REF(address_or_error.status()); + SET_AND_RETURN_IF_NOT_OK(address_or_error.status(), creation_status); auto address = std::move(address_or_error.value()); - THROW_IF_NOT_OK(checkIpv4CompatAddress(address, config.address())); + SET_AND_RETURN_IF_NOT_OK(checkIpv4CompatAddress(address, config.address()), creation_status); addresses_.emplace_back(address); address_opts_list.emplace_back(std::ref(config.socket_options())); for (auto i = 0; i < config.additional_addresses_size(); i++) { if (socket_type_ != Network::Utility::protobufAddressSocketType(config.additional_addresses(i).address())) { - throwEnvoyExceptionOrPanic( + creation_status = absl::InvalidArgumentError( fmt::format("listener {}: has different socket type. The listener only " "support same socket type for all the addresses.", name_)); + return; } auto addresses_or_error = Network::Address::resolveProtoAddress(config.additional_addresses(i).address()); - THROW_IF_NOT_OK_REF(addresses_or_error.status()); + SET_AND_RETURN_IF_NOT_OK(addresses_or_error.status(), creation_status); auto additional_address = std::move(addresses_or_error.value()); - THROW_IF_NOT_OK(checkIpv4CompatAddress(address, config.additional_addresses(i).address())); + SET_AND_RETURN_IF_NOT_OK( + checkIpv4CompatAddress(address, config.additional_addresses(i).address()), + creation_status); addresses_.emplace_back(additional_address); if (config.additional_addresses(i).has_socket_options()) { address_opts_list.emplace_back( @@ -367,20 +397,21 @@ ListenerImpl::ListenerImpl(const envoy::config::listener::v3::Listener& config, addresses_, listener_factory_context_->parentFactoryContext(), initManager()), buildAccessLog(config); - THROW_IF_NOT_OK(validateConfig()); + SET_AND_RETURN_IF_NOT_OK(validateConfig(), creation_status); // buildUdpListenerFactory() must come before buildListenSocketOptions() because the UDP // listener factory can provide additional options. - THROW_IF_NOT_OK(buildUdpListenerFactory(config, parent_.server_.options().concurrency())); + SET_AND_RETURN_IF_NOT_OK(buildUdpListenerFactory(config, parent_.server_.options().concurrency()), + creation_status); buildListenSocketOptions(config, address_opts_list); - THROW_IF_NOT_OK(createListenerFilterFactories(config)); - THROW_IF_NOT_OK(validateFilterChains(config)); - THROW_IF_NOT_OK(buildFilterChains(config)); + SET_AND_RETURN_IF_NOT_OK(createListenerFilterFactories(config), creation_status); + SET_AND_RETURN_IF_NOT_OK(validateFilterChains(config), creation_status); + SET_AND_RETURN_IF_NOT_OK(buildFilterChains(config), creation_status); if (socket_type_ != Network::Socket::Type::Datagram) { buildSocketOptions(config); buildOriginalDstListenerFilter(config); buildProxyProtocolListenerFilter(config); - THROW_IF_NOT_OK(buildInternalListener(config)); + SET_AND_RETURN_IF_NOT_OK(buildInternalListener(config), creation_status); } if (!workers_started_) { // Initialize dynamic_init_manager_ from Server's init manager if it's not initialized. @@ -395,7 +426,7 @@ ListenerImpl::ListenerImpl(ListenerImpl& origin, const envoy::config::listener::v3::Listener& config, const std::string& version_info, ListenerManagerImpl& parent, const std::string& name, bool added_via_api, bool workers_started, - uint64_t hash) + uint64_t hash, absl::Status& creation_status) : parent_(parent), addresses_(origin.addresses_), socket_type_(origin.socket_type_), bind_to_port_(shouldBindToPort(config)), mptcp_enabled_(config.enable_mptcp()), hand_off_restored_destination_connections_( @@ -443,11 +474,11 @@ ListenerImpl::ListenerImpl(ListenerImpl& origin, missing_listener_config_stats_({ALL_MISSING_LISTENER_CONFIG_STATS( POOL_COUNTER(listener_factory_context_->listenerScope()))}) { buildAccessLog(config); - THROW_IF_NOT_OK(validateConfig()); - THROW_IF_NOT_OK(createListenerFilterFactories(config)); - THROW_IF_NOT_OK(validateFilterChains(config)); - THROW_IF_NOT_OK(buildFilterChains(config)); - THROW_IF_NOT_OK(buildInternalListener(config)); + SET_AND_RETURN_IF_NOT_OK(validateConfig(), creation_status); + SET_AND_RETURN_IF_NOT_OK(createListenerFilterFactories(config), creation_status); + SET_AND_RETURN_IF_NOT_OK(validateFilterChains(config), creation_status); + SET_AND_RETURN_IF_NOT_OK(buildFilterChains(config), creation_status); + SET_AND_RETURN_IF_NOT_OK(buildInternalListener(config), creation_status); if (socket_type_ == Network::Socket::Type::Stream) { // Apply the options below only for TCP. buildSocketOptions(config); @@ -1020,14 +1051,18 @@ bool ListenerImpl::supportUpdateFilterChain(const envoy::config::listener::v3::L return false; } -ListenerImplPtr +absl::StatusOr ListenerImpl::newListenerWithFilterChain(const envoy::config::listener::v3::Listener& config, bool workers_started, uint64_t hash) { + + absl::Status creation_status = absl::OkStatus(); // Use WrapUnique since the constructor is private. - return absl::WrapUnique(new ListenerImpl(*this, config, version_info_, parent_, name_, - added_via_api_, - /* new new workers started state */ workers_started, - /* use new hash */ hash)); + auto ret = absl::WrapUnique(new ListenerImpl(*this, config, version_info_, parent_, name_, + added_via_api_, + /* new new workers started state */ workers_started, + /* use new hash */ hash, creation_status)); + RETURN_IF_NOT_OK(creation_status); + return ret; } void ListenerImpl::diffFilterChain(const ListenerImpl& another_listener, diff --git a/source/common/listener_manager/listener_impl.h b/source/common/listener_manager/listener_impl.h index f7bd70f47a3c..2157b5eac533 100644 --- a/source/common/listener_manager/listener_impl.h +++ b/source/common/listener_manager/listener_impl.h @@ -63,14 +63,12 @@ class ListenerManagerImpl; class ListenSocketFactoryImpl : public Network::ListenSocketFactory, protected Logger::Loggable { public: - ListenSocketFactoryImpl(ListenerComponentFactory& factory, - Network::Address::InstanceConstSharedPtr address, - Network::Socket::Type socket_type, - const Network::Socket::OptionsSharedPtr& options, - const std::string& listener_name, uint32_t tcp_backlog_size, - ListenerComponentFactory::BindType bind_type, - const Network::SocketCreationOptions& creation_options, - uint32_t num_sockets); + static absl::StatusOr> + create(ListenerComponentFactory& factory, Network::Address::InstanceConstSharedPtr address, + Network::Socket::Type socket_type, const Network::Socket::OptionsSharedPtr& options, + const std::string& listener_name, uint32_t tcp_backlog_size, + ListenerComponentFactory::BindType bind_type, + const Network::SocketCreationOptions& creation_options, uint32_t num_sockets); // Network::ListenSocketFactory Network::Socket::Type socketType() const override { return socket_type_; } @@ -89,6 +87,15 @@ class ListenSocketFactoryImpl : public Network::ListenSocketFactory, absl::Status doFinalPreWorkerInit() override; private: + ListenSocketFactoryImpl(ListenerComponentFactory& factory, + Network::Address::InstanceConstSharedPtr address, + Network::Socket::Type socket_type, + const Network::Socket::OptionsSharedPtr& options, + const std::string& listener_name, uint32_t tcp_backlog_size, + ListenerComponentFactory::BindType bind_type, + const Network::SocketCreationOptions& creation_options, + uint32_t num_sockets, absl::Status& creation_status); + ListenSocketFactoryImpl(const ListenSocketFactoryImpl& factory_to_clone); absl::StatusOr @@ -205,9 +212,10 @@ class ListenerImpl final : public Network::ListenerConfig, * @param hash supplies the hash to use for duplicate checking. * @param concurrency is the number of listeners instances to be created. */ - ListenerImpl(const envoy::config::listener::v3::Listener& config, const std::string& version_info, - ListenerManagerImpl& parent, const std::string& name, bool added_via_api, - bool workers_started, uint64_t hash); + static absl::StatusOr> + create(const envoy::config::listener::v3::Listener& config, const std::string& version_info, + ListenerManagerImpl& parent, const std::string& name, bool added_via_api, + bool workers_started, uint64_t hash); ~ListenerImpl() override; // TODO(lambdai): Explore using the same ListenerImpl object to execute in place filter chain @@ -216,7 +224,7 @@ class ListenerImpl final : public Network::ListenerConfig, * Execute in place filter chain update. The filter chain update is less expensive than full * listener update because connections may not need to be drained. */ - std::unique_ptr + absl::StatusOr> newListenerWithFilterChain(const envoy::config::listener::v3::Listener& config, bool workers_started, uint64_t hash); /** @@ -344,6 +352,9 @@ class ListenerImpl final : public Network::ListenerConfig, SystemTime last_updated_; private: + ListenerImpl(const envoy::config::listener::v3::Listener& config, const std::string& version_info, + ListenerManagerImpl& parent, const std::string& name, bool added_via_api, + bool workers_started, uint64_t hash, absl::Status& creation_status); struct UdpListenerConfigImpl : public Network::UdpListenerConfig { UdpListenerConfigImpl(const envoy::config::listener::v3::UdpListenerConfig config) : config_(config) {} @@ -384,7 +395,8 @@ class ListenerImpl final : public Network::ListenerConfig, */ ListenerImpl(ListenerImpl& origin, const envoy::config::listener::v3::Listener& config, const std::string& version_info, ListenerManagerImpl& parent, - const std::string& name, bool added_via_api, bool workers_started, uint64_t hash); + const std::string& name, bool added_via_api, bool workers_started, uint64_t hash, + absl::Status& creation_status); // Helpers for constructor. void buildAccessLog(const envoy::config::listener::v3::Listener& config); absl::Status buildInternalListener(const envoy::config::listener::v3::Listener& config); diff --git a/source/common/listener_manager/listener_manager_impl.cc b/source/common/listener_manager/listener_manager_impl.cc index 1683caf51b80..8de7a7da3360 100644 --- a/source/common/listener_manager/listener_manager_impl.cc +++ b/source/common/listener_manager/listener_manager_impl.cc @@ -580,13 +580,17 @@ absl::StatusOr ListenerManagerImpl::addOrUpdateListenerInternal( (*existing_active_listener)->supportUpdateFilterChain(config, workers_started_)) { ENVOY_LOG(debug, "use in place update filter chain update path for listener name={} hash={}", name, hash); - new_listener = + auto listener_or_error = (*existing_active_listener)->newListenerWithFilterChain(config, workers_started_, hash); + RETURN_IF_NOT_OK_REF(listener_or_error.status()); + new_listener = std::move(*listener_or_error); stats_.listener_in_place_updated_.inc(); } else { ENVOY_LOG(debug, "use full listener update path for listener name={} hash={}", name, hash); - new_listener = std::make_unique(config, version_info, *this, name, added_via_api, + auto listener_or_error = ListenerImpl::create(config, version_info, *this, name, added_via_api, workers_started_, hash); + RETURN_IF_NOT_OK_REF(listener_or_error.status()); + new_listener = std::move(*listener_or_error); } ListenerImpl& new_listener_ref = *new_listener; @@ -1197,10 +1201,15 @@ absl::Status ListenerManagerImpl::createListenSocketFactory(ListenerImpl& listen creation_options.mptcp_enabled_ = listener.mptcpEnabled(); for (std::vector::size_type i = 0; i < listener.addresses().size(); i++) { - socket_status = listener.addSocketFactory(std::make_unique( + auto factory_or_error = ListenSocketFactoryImpl::create( *factory_, listener.addresses()[i], socket_type, listener.listenSocketOptions(i), listener.name(), listener.tcpBacklogSize(), bind_type, creation_options, - server_.options().concurrency())); + server_.options().concurrency()); + if (!factory_or_error.status().ok()) { + socket_status = factory_or_error.status(); + } else { + socket_status = listener.addSocketFactory(std::move(*factory_or_error)); + } if (!socket_status.ok()) { break; } diff --git a/test/common/listener_manager/listener_manager_impl_test.cc b/test/common/listener_manager/listener_manager_impl_test.cc index d86093344ffc..cd7a7f658fe6 100644 --- a/test/common/listener_manager/listener_manager_impl_test.cc +++ b/test/common/listener_manager/listener_manager_impl_test.cc @@ -1008,11 +1008,12 @@ TEST_P(ListenerManagerImplTest, RejectTcpOptionsWithInternalListenerConfig) { for (const auto& f : listener_mutators) { auto new_listener = listener; f(new_listener); - EXPECT_THROW_WITH_MESSAGE(new ListenerImpl(new_listener, "version", *manager_, "foo", true, - false, /*hash=*/static_cast(0)), - EnvoyException, - "error adding listener named 'foo': has " - "unsupported tcp listener feature"); + EXPECT_EQ(ListenerImpl::create(new_listener, "version", *manager_, "foo", true, false, + /*hash=*/static_cast(0)) + .status() + .message(), + "error adding listener named 'foo': has " + "unsupported tcp listener feature"); } { auto new_listener = listener; @@ -7809,6 +7810,24 @@ TEST(ListenerMessageUtilTest, ListenerMessageHaveDifferentFilterChainsAreEquival EXPECT_TRUE(Server::ListenerMessageUtil::filterChainOnlyChange(listener1, listener2)); } +TEST_P(ListenerManagerImplForInPlaceFilterChainUpdateTest, InvalidAddress) { + // Worker is not started yet. + envoy::config::listener::v3::Listener listener_proto; + Protobuf::TextFormat::ParseFromString(R"EOF( + name: "foo" + address: { + socket_address: { + address: "127.0.0.1.0" + port_value: 1234 + } + } + filter_chains: {} + )EOF", + &listener_proto); + EXPECT_EQ(manager_->addOrUpdateListener(listener_proto, "", true).status().message(), + "malformed IP address: 127.0.0.1.0"); +} + TEST_P(ListenerManagerImplForInPlaceFilterChainUpdateTest, TraditionalUpdateIfWorkerNotStarted) { // Worker is not started yet. auto listener_proto = createDefaultListener(); @@ -8034,13 +8053,13 @@ TEST_P(ListenerManagerImplWithRealFiltersTest, InvalidExtendConnectionBalanceCon extend_balance_config->mutable_typed_config()->set_type_url( "type.googleapis.com/google.protobuf.test"); - auto listener_impl = ListenerImpl(listener, "version", *manager_, "foo", true, false, - /*hash=*/static_cast(0)); + auto listener_impl = *ListenerImpl::create(listener, "version", *manager_, "foo", true, false, + /*hash=*/static_cast(0)); auto socket_factory = std::make_unique(); Network::Address::InstanceConstSharedPtr address( new Network::Address::Ipv4Instance("192.168.0.1", 80, nullptr)); EXPECT_CALL(*socket_factory, localAddress()).WillOnce(ReturnRef(address)); - EXPECT_EQ(listener_impl.addSocketFactory(std::move(socket_factory)).message(), + EXPECT_EQ(listener_impl->addSocketFactory(std::move(socket_factory)).message(), "Didn't find a registered implementation for type: 'google.protobuf.test'"); #endif } @@ -8051,13 +8070,13 @@ TEST_P(ListenerManagerImplWithRealFiltersTest, EmptyConnectionBalanceConfig) { auto listener = createIPv4Listener("TCPListener"); listener.mutable_connection_balance_config(); - auto listener_impl = ListenerImpl(listener, "version", *manager_, "foo", true, false, - /*hash=*/static_cast(0)); + auto listener_impl = *ListenerImpl::create(listener, "version", *manager_, "foo", true, false, + /*hash=*/static_cast(0)); auto socket_factory = std::make_unique(); Network::Address::InstanceConstSharedPtr address( new Network::Address::Ipv4Instance("192.168.0.1", 80, nullptr)); EXPECT_CALL(*socket_factory, localAddress()).WillOnce(ReturnRef(address)); - EXPECT_EQ(listener_impl.addSocketFactory(std::move(socket_factory)).message(), + EXPECT_EQ(listener_impl->addSocketFactory(std::move(socket_factory)).message(), "No valid balance type for connection balance"); #endif } diff --git a/tools/code_format/config.yaml b/tools/code_format/config.yaml index 3d117d0f634e..b453bb1cb842 100644 --- a/tools/code_format/config.yaml +++ b/tools/code_format/config.yaml @@ -100,7 +100,6 @@ paths: # legacy core files which throw exceptions. We can add to this list but strongly prefer # StausOr where possible. - source/common/router/route_config_update_receiver_impl.cc - - source/common/listener_manager/listener_impl.cc - source/common/upstream/cluster_manager_impl.cc - source/common/upstream/upstream_impl.cc - source/common/network/listen_socket_impl.cc