From a0c1ef2bdefe3d082917ddb684a0fc9e6f45b9e0 Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Mon, 9 Oct 2017 18:01:12 -0400 Subject: [PATCH 01/16] Add logicalName() to Network::Address interface This will be used to support resolved addresses, where the resolved (physical) name is different from the unresolved (logical) name. Signed-off-by: Alex Konradi --- include/envoy/network/address.h | 12 +++++++++++- source/common/network/address_impl.h | 2 ++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/include/envoy/network/address.h b/include/envoy/network/address.h index 0aab5e84deb9..87c8439d136f 100644 --- a/include/envoy/network/address.h +++ b/include/envoy/network/address.h @@ -101,7 +101,8 @@ class Instance { bool operator!=(const Instance& rhs) const { return !operator==(rhs); } /** - * @return a human readable string for the address. + * @return a human readable string for the address that represents the + * physical/resolved address. * * This string will be in the following format: * For IPv4 addresses: "1.2.3.4:80" @@ -110,6 +111,15 @@ class Instance { */ virtual const std::string& asString() const PURE; + /** + * @return a human readable string for the address that represents the + * logical/unresolved name. + * + * This string has a source-dependent format and should preserve the original + * name for Address::Instances resolved by a Network::AddressResolver. + */ + virtual const std::string& logicalName() const PURE; + /** * Bind a socket to this address. The socket should have been created with a call to socket() on * an Instance of the same address family. diff --git a/source/common/network/address_impl.h b/source/common/network/address_impl.h index b6b8a4fa282a..517e207b8eff 100644 --- a/source/common/network/address_impl.h +++ b/source/common/network/address_impl.h @@ -48,6 +48,8 @@ class InstanceBase : public Instance { // Network::Address::Instance bool operator==(const Instance& rhs) const override { return asString() == rhs.asString(); } const std::string& asString() const override { return friendly_name_; } + // Default logical name is the human-readable name + const std::string& logicalName() const override { return asString(); } Type type() const override { return type_; } protected: From b3ea3675023ecadeb08b618c10a93844a85faee2 Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Tue, 10 Oct 2017 10:14:00 -0400 Subject: [PATCH 02/16] Add Address::Resolver interface Used for address translation. This supports a completely synchronous interface, so all translation must be done inline without any network access. DNS resolution is thus explicitly forbidden, and must be done through the already-existing DNS machinery. Signed-off-by: Alex Konradi --- include/envoy/network/BUILD | 8 ++++ include/envoy/network/resolver.h | 63 ++++++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+) create mode 100644 include/envoy/network/resolver.h diff --git a/include/envoy/network/BUILD b/include/envoy/network/BUILD index 270a45cf96f6..a492f4f94a68 100644 --- a/include/envoy/network/BUILD +++ b/include/envoy/network/BUILD @@ -65,3 +65,11 @@ envoy_cc_library( name = "listener_interface", hdrs = ["listener.h"], ) + +envoy_cc_library( + name = "resolver_interface", + hdrs = ["resolver.h"], + deps = [ + ":address_interface", + ], +) diff --git a/include/envoy/network/resolver.h b/include/envoy/network/resolver.h new file mode 100644 index 000000000000..30b76bad3aea --- /dev/null +++ b/include/envoy/network/resolver.h @@ -0,0 +1,63 @@ +#pragma once + +#include + +#include +#include + +#include "envoy/common/pure.h" +#include "envoy/network/address.h" + +namespace Envoy { +namespace Network { +namespace Address { + +/** + * Interface for all network address resolvers + */ +class Resolver { +public: + virtual ~Resolver() {} + + /** + * Resolve a custom address string and port to an Address::Instance + * @param address the address to resolve + * @param supplies the port on the address + * @return an appropriate Address::Instance + */ + virtual Network::Address::InstanceConstSharedPtr resolve(const std::string& address, + const uint32_t port) PURE; + + /** + * Resolve a custom address string and named port to an Address::Instance + * @param address the address to resolve + * @param named_port the named port to resolve + * @return an appropriate Address::Instance + */ + virtual Network::Address::InstanceConstSharedPtr resolve(const std::string& address, + const std::string& named_port) PURE; +}; + +typedef std::unique_ptr ResolverPtr; + +/** + * A factory for individual network address resolvers + */ +class ResolverFactory { +public: + virtual ~ResolverFactory() {} + + virtual ResolverPtr create() const PURE; + + /** + * @return std::string the identifying name for a particular implementation of + * a resolver produced by the factory. + */ + virtual std::string name() PURE; +}; + +typedef std::shared_ptr ResolverFactorySharedPtr; + +} // namespace Address +} // namespace Network +} // namespace Envoy From bc87db0aaaed8d223fe1f3f1cc6c8afdd380487b Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Tue, 10 Oct 2017 11:16:48 -0400 Subject: [PATCH 03/16] Add Address::Resolver for IP resolution Does exactly what it says; resolves string IP addresses into Address::Instance objects Signed-off-by: Alex Konradi --- source/common/config/well_known_names.h | 11 ++++++ source/common/network/BUILD | 14 +++++++ source/common/network/resolver_impl.cc | 48 +++++++++++++++++++++++ test/common/network/BUILD | 10 +++++ test/common/network/resolver_impl_test.cc | 38 ++++++++++++++++++ 5 files changed, 121 insertions(+) create mode 100644 source/common/network/resolver_impl.cc create mode 100644 test/common/network/resolver_impl_test.cc diff --git a/source/common/config/well_known_names.h b/source/common/config/well_known_names.h index db097f3bee27..e6752050ab70 100644 --- a/source/common/config/well_known_names.h +++ b/source/common/config/well_known_names.h @@ -77,6 +77,17 @@ class NetworkFilterNameValues { typedef ConstSingleton NetworkFilterNames; +/** + * Well-known address resolver names. + */ +class AddressResolverNameValues { +public: + // Basic IP resolver + const std::string IP = "envoy.ip"; +}; + +typedef ConstSingleton AddressResolverNames; + /** * Well-known http filter names. */ diff --git a/source/common/network/BUILD b/source/common/network/BUILD index 8bd0c66c363a..482dbcbc696f 100644 --- a/source/common/network/BUILD +++ b/source/common/network/BUILD @@ -132,6 +132,20 @@ envoy_cc_library( ], ) +envoy_cc_library( + name = "resolver_lib", + srcs = ["resolver_impl.cc"], + external_deps = ["envoy_base"], + deps = [ + ":utility_lib", + "//include/envoy/network:address_interface", + "//include/envoy/network:resolver_interface", + "//include/envoy/registry", + "//source/common/config:well_known_names", + "//source/common/network:address_lib", + ], +) + envoy_cc_library( name = "utility_lib", srcs = ["utility.cc"], diff --git a/source/common/network/resolver_impl.cc b/source/common/network/resolver_impl.cc new file mode 100644 index 000000000000..fec4fd898704 --- /dev/null +++ b/source/common/network/resolver_impl.cc @@ -0,0 +1,48 @@ +#include "envoy/common/exception.h" +#include "envoy/network/address.h" +#include "envoy/network/resolver.h" +#include "envoy/registry/registry.h" + +#include "common/config/well_known_names.h" +#include "common/network/address_impl.h" +#include "common/network/utility.h" + +namespace Envoy { +namespace Network { +namespace Address { + +/** + * Implementation of a resolver for IP addresses + */ +class IpResolver : public Resolver { + +public: + Network::Address::InstanceConstSharedPtr resolve(const std::string& address, + const uint32_t port) override { + return Network::Utility::parseInternetAddress(address, port); + } + + Network::Address::InstanceConstSharedPtr resolve(const std::string&, + const std::string&) override { + throw EnvoyException("named ports are not supported by this resolver"); + } +}; + +/** + * Implementation of an IP address resolver factory + */ +class IpResolverFactory : public ResolverFactory { +public: + ResolverPtr create() const override { return ResolverPtr{new IpResolver()}; } + + std::string name() override { return Config::AddressResolverNames::get().IP; } +}; + +/** + * Static registration for the IP resolver. @see RegisterFactory. + */ +static Registry::RegisterFactory ip_registered_; + +} // namespace Address +} // namespace Network +} // namespace Envoy diff --git a/test/common/network/BUILD b/test/common/network/BUILD index 746a555447f7..c8b1a8d431fd 100644 --- a/test/common/network/BUILD +++ b/test/common/network/BUILD @@ -138,6 +138,16 @@ envoy_cc_test( ], ) +envoy_cc_test( + name = "resolver_test", + srcs = ["resolver_impl_test.cc"], + deps = [ + "//source/common/network:address_lib", + "//source/common/network:resolver_lib", + "//test/test_common:environment_lib", + ], +) + envoy_cc_test( name = "utility_test", srcs = ["utility_test.cc"], diff --git a/test/common/network/resolver_impl_test.cc b/test/common/network/resolver_impl_test.cc new file mode 100644 index 000000000000..641c4027db44 --- /dev/null +++ b/test/common/network/resolver_impl_test.cc @@ -0,0 +1,38 @@ +#include +#include +#include + +#include "envoy/common/exception.h" +#include "envoy/network/resolver.h" +#include "envoy/registry/registry.h" + +#include "common/common/thread.h" +#include "common/network/address_impl.h" + +#include "test/test_common/environment.h" +#include "test/test_common/utility.h" + +#include "gtest/gtest.h" + +namespace Envoy { +namespace Network { +namespace Address { +class IpResolverTest : public testing::Test { +public: + ResolverFactory* factory_{Registry::FactoryRegistry::getFactory("envoy.ip")}; +}; + +TEST_F(IpResolverTest, Basic) { + auto address = factory_->create()->resolve("1.2.3.4", 443); + EXPECT_EQ(address->ip()->addressAsString(), "1.2.3.4"); + EXPECT_EQ(address->ip()->port(), 443); +} + +TEST_F(IpResolverTest, DisallowsNamedPort) { + auto resolver = factory_->create(); + EXPECT_THROW(resolver->resolve("1.2.3.4", "http"), EnvoyException); +} + +} // namespace Address +} // namespace Network +} // namespace Envoy From 3ed3f63347a15b4d544b3e9ec7c631655f92d73e Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Tue, 10 Oct 2017 17:28:00 -0400 Subject: [PATCH 04/16] Make RegisterFactory remove factory on delete This will make it possible to use RegisterFactory in tests without leaking state between tests. Signed-off-by: Alex Konradi --- include/envoy/registry/registry.h | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/include/envoy/registry/registry.h b/include/envoy/registry/registry.h index 42f96dd44703..88b8180d276e 100644 --- a/include/envoy/registry/registry.h +++ b/include/envoy/registry/registry.h @@ -46,6 +46,16 @@ template class FactoryRegistry { return it->second; } + /** + * Remove a factory by name. + */ + static void unregisterFactory(Base& factory) { + auto result = factories().erase(factory.name()); + if (result == 0) { + throw EnvoyException(fmt::format("No registration for name: '{}'", factory.name())); + } + } + private: /** * Gets the current map of factory implementations. @@ -73,10 +83,17 @@ template class RegisterFactory { /** * Contructor that registers an instance of the factory with the FactoryRegistry. */ - RegisterFactory() { - static T* instance = new T; - FactoryRegistry::registerFactory(*instance); - } + RegisterFactory() { FactoryRegistry::registerFactory(instance_); } + + /** + * Destructor that removes an instance of the factory from the FactoryRegistry + */ + ~RegisterFactory() { FactoryRegistry::unregisterFactory(instance_); } + + T& testGetFactory() { return instance_; } + +private: + T instance_; }; } // namespace Registry From 0a7f98717f2d16140855b35b4bd250b926a9962b Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Tue, 10 Oct 2017 11:25:12 -0400 Subject: [PATCH 05/16] Move Network::Utility proto address parsing Now implemented by the Network::Address::ResolverFactory registry and appropriate interfaces. Signed-off-by: Alex Konradi --- source/common/network/BUILD | 1 + source/common/network/resolver_impl.cc | 42 +++++++++ source/common/network/resolver_impl.h | 30 +++++++ source/common/network/utility.cc | 19 ---- source/common/network/utility.h | 15 ---- source/common/upstream/BUILD | 3 + .../common/upstream/cluster_manager_impl.cc | 3 +- source/common/upstream/eds.cc | 3 +- source/common/upstream/upstream_impl.cc | 7 +- source/server/BUILD | 1 + source/server/config/stats/BUILD | 1 + source/server/config/stats/statsd.cc | 3 +- source/server/configuration_impl.cc | 2 +- source/server/configuration_impl.h | 1 + test/common/network/BUILD | 1 + test/common/network/resolver_impl_test.cc | 88 +++++++++++++++++++ test/common/network/utility_test.cc | 16 ---- test/mocks/network/mocks.h | 22 +++++ 18 files changed, 201 insertions(+), 57 deletions(-) create mode 100644 source/common/network/resolver_impl.h diff --git a/source/common/network/BUILD b/source/common/network/BUILD index 482dbcbc696f..58c83c81cc0b 100644 --- a/source/common/network/BUILD +++ b/source/common/network/BUILD @@ -135,6 +135,7 @@ envoy_cc_library( envoy_cc_library( name = "resolver_lib", srcs = ["resolver_impl.cc"], + hdrs = ["resolver_impl.h"], external_deps = ["envoy_base"], deps = [ ":utility_lib", diff --git a/source/common/network/resolver_impl.cc b/source/common/network/resolver_impl.cc index fec4fd898704..d5b90a8b38be 100644 --- a/source/common/network/resolver_impl.cc +++ b/source/common/network/resolver_impl.cc @@ -1,3 +1,5 @@ +#include "common/network/resolver_impl.h" + #include "envoy/common/exception.h" #include "envoy/network/address.h" #include "envoy/network/resolver.h" @@ -43,6 +45,46 @@ class IpResolverFactory : public ResolverFactory { */ static Registry::RegisterFactory ip_registered_; +InstanceConstSharedPtr resolveProtoAddress(const envoy::api::v2::Address& address) { + switch (address.address_case()) { + case envoy::api::v2::Address::kSocketAddress: + return resolveProtoSocketAddress(address.socket_address()); + case envoy::api::v2::Address::kPipe: + return InstanceConstSharedPtr{new PipeInstance(address.pipe().path())}; + default: + throw EnvoyException("Address must be a socket or pipe: " + address.DebugString()); + } +} + +InstanceConstSharedPtr +resolveProtoSocketAddress(const envoy::api::v2::SocketAddress& socket_address) { + const ResolverFactory* resolver_factory = nullptr; + const std::string& resolver_name = socket_address.resolver_name(); + if (resolver_name.empty()) { + resolver_factory = Registry::FactoryRegistry::getFactory( + Config::AddressResolverNames::get().IP); + } else { + resolver_factory = Registry::FactoryRegistry::getFactory(resolver_name); + } + if (resolver_factory == nullptr) { + throw EnvoyException(fmt::format("Unknown address resolver: {}", resolver_name)); + } + ResolverPtr resolver(resolver_factory->create()); + switch (socket_address.port_specifier_case()) { + case envoy::api::v2::SocketAddress::kNamedPort: + return resolver->resolve(socket_address.address(), socket_address.named_port()); + + case envoy::api::v2::SocketAddress::kPortValue: + // default to port 0 if no port value is specified + case envoy::api::v2::SocketAddress::PORT_SPECIFIER_NOT_SET: + return resolver->resolve(socket_address.address(), socket_address.port_value()); + + default: + throw EnvoyException( + fmt::format("Unknown port specifier type {}", socket_address.port_specifier_case())); + } +} + } // namespace Address } // namespace Network } // namespace Envoy diff --git a/source/common/network/resolver_impl.h b/source/common/network/resolver_impl.h new file mode 100644 index 000000000000..da5a2f83729f --- /dev/null +++ b/source/common/network/resolver_impl.h @@ -0,0 +1,30 @@ +#pragma once + +#include "envoy/network/address.h" +#include "envoy/network/connection.h" +#include "envoy/network/resolver.h" + +#include "common/network/address_impl.h" + +#include "api/address.pb.h" + +namespace Envoy { +namespace Network { +namespace Address { +/** + * Create an Instance from a envoy::api::v2::Address. + * @param address message. + * @return pointer to the Instance. + */ +Address::InstanceConstSharedPtr resolveProtoAddress(const envoy::api::v2::Address& address); + +/** + * Create an Instance from a envoy::api::v2::SocketAddress. + * @param socket address message. + * @return pointer to the Instance. + */ +Address::InstanceConstSharedPtr +resolveProtoSocketAddress(const envoy::api::v2::SocketAddress& address); +} // namespace Address +} // namespace Network +} // namespace Envoy diff --git a/source/common/network/utility.cc b/source/common/network/utility.cc index 091c7f5fccce..64172cd80d42 100644 --- a/source/common/network/utility.cc +++ b/source/common/network/utility.cc @@ -146,25 +146,6 @@ Address::InstanceConstSharedPtr Utility::copyInternetAddressAndPort(const Addres return std::make_shared(ip.addressAsString(), ip.port()); } -Address::InstanceConstSharedPtr Utility::fromProtoAddress(const envoy::api::v2::Address& address) { - switch (address.address_case()) { - case envoy::api::v2::Address::kSocketAddress: - return Utility::fromProtoSocketAddress(address.socket_address()); - case envoy::api::v2::Address::kPipe: - return Address::InstanceConstSharedPtr{new Address::PipeInstance(address.pipe().path())}; - default: - throw EnvoyException("Address must be a socket or pipe: " + address.DebugString()); - } -} - -Address::InstanceConstSharedPtr -Utility::fromProtoSocketAddress(const envoy::api::v2::SocketAddress& socket_address) { - // TODO(htuch): Support custom resolvers #1477. - ASSERT(socket_address.resolver_name().empty()); - ASSERT(socket_address.named_port().empty()); - return parseInternetAddress(socket_address.address(), socket_address.port_value()); -} - void Utility::throwWithMalformedIp(const std::string& ip_address) { throw EnvoyException(fmt::format("malformed IP address: {}", ip_address)); } diff --git a/source/common/network/utility.h b/source/common/network/utility.h index 9b0feac5bbc5..fc7ea76d8eb8 100644 --- a/source/common/network/utility.h +++ b/source/common/network/utility.h @@ -85,21 +85,6 @@ class Utility { */ static Address::InstanceConstSharedPtr parseInternetAddressAndPort(const std::string& ip_address); - /** - * Create an Instance from a envoy::api::v2::Address. - * @param address message. - * @return pointer to the Instance. - */ - static Address::InstanceConstSharedPtr fromProtoAddress(const envoy::api::v2::Address& address); - - /** - * Create an Instance from a envoy::api::v2::SocketAddress. - * @param socket address message. - * @return pointer to the Instance. - */ - static Address::InstanceConstSharedPtr - fromProtoSocketAddress(const envoy::api::v2::SocketAddress& address); - /** * Get the local address of the first interface address that is of type * version and is not a loopback address. If no matches are found, return the diff --git a/source/common/upstream/BUILD b/source/common/upstream/BUILD index b8c2445bead3..be69120578ca 100644 --- a/source/common/upstream/BUILD +++ b/source/common/upstream/BUILD @@ -70,6 +70,7 @@ envoy_cc_library( "//source/common/http:async_client_lib", "//source/common/http/http1:conn_pool_lib", "//source/common/http/http2:conn_pool_lib", + "//source/common/network:resolver_lib", "//source/common/network:utility_lib", "//source/common/protobuf:utility_lib", "//source/common/router:shadow_writer_lib", @@ -218,6 +219,7 @@ envoy_cc_library( "//source/common/config:utility_lib", "//source/common/config:well_known_names", "//source/common/network:address_lib", + "//source/common/network:resolver_lib", "//source/common/network:utility_lib", ], ) @@ -265,6 +267,7 @@ envoy_cc_library( "//source/common/config:tls_context_json_lib", "//source/common/http:utility_lib", "//source/common/network:address_lib", + "//source/common/network:resolver_lib", "//source/common/network:utility_lib", "//source/common/protobuf", "//source/common/protobuf:utility_lib", diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index 83514e468338..a02abf41870b 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -19,6 +19,7 @@ #include "common/http/http1/conn_pool.h" #include "common/http/http2/conn_pool.h" #include "common/json/config_schemas.h" +#include "common/network/resolver_impl.h" #include "common/network/utility.h" #include "common/protobuf/utility.h" #include "common/router/shadow_writer_impl.h" @@ -196,7 +197,7 @@ ClusterManagerImpl::ClusterManagerImpl(const envoy::api::v2::Bootstrap& bootstra } if (bootstrap.cluster_manager().upstream_bind_config().has_source_address()) { - source_address_ = Network::Utility::fromProtoSocketAddress( + source_address_ = Network::Address::resolveProtoSocketAddress( bootstrap.cluster_manager().upstream_bind_config().source_address()); } diff --git a/source/common/upstream/eds.cc b/source/common/upstream/eds.cc index 6072215b1fe6..6808465366bf 100644 --- a/source/common/upstream/eds.cc +++ b/source/common/upstream/eds.cc @@ -7,6 +7,7 @@ #include "common/config/utility.h" #include "common/config/well_known_names.h" #include "common/network/address_impl.h" +#include "common/network/resolver_impl.h" #include "common/network/utility.h" #include "common/upstream/sds_subscription.h" @@ -60,7 +61,7 @@ void EdsClusterImpl::onConfigUpdate(const ResourceVector& resources) { for (const auto& locality_lb_endpoint : cluster_load_assignment.endpoints()) { for (const auto& lb_endpoint : locality_lb_endpoint.lb_endpoints()) { new_hosts.emplace_back(new HostImpl( - info_, "", Network::Utility::fromProtoAddress(lb_endpoint.endpoint().address()), + info_, "", Network::Address::resolveProtoAddress(lb_endpoint.endpoint().address()), lb_endpoint.metadata(), lb_endpoint.load_balancing_weight().value(), locality_lb_endpoint.locality())); } diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 3bd569b3b8b0..3ae6476f63cd 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -20,6 +20,7 @@ #include "common/config/tls_context_json.h" #include "common/http/utility.h" #include "common/network/address_impl.h" +#include "common/network/resolver_impl.h" #include "common/network/utility.h" #include "common/protobuf/protobuf.h" #include "common/protobuf/utility.h" @@ -41,7 +42,7 @@ getSourceAddress(const envoy::api::v2::Cluster& cluster, const Network::Address::InstanceConstSharedPtr source_address) { // The source address from cluster config takes precedence. if (cluster.upstream_bind_config().has_source_address()) { - return Network::Utility::fromProtoSocketAddress( + return Network::Address::resolveProtoSocketAddress( cluster.upstream_bind_config().source_address()); } // If there's no source address in the cluster config, use any default from the bootstrap proto. @@ -150,7 +151,7 @@ ClusterSharedPtr ClusterImplBase::create(const envoy::api::v2::Cluster& cluster, std::vector resolvers; resolvers.reserve(resolver_addrs.size()); for (const auto& resolver_addr : resolver_addrs) { - resolvers.push_back(Network::Utility::fromProtoAddress(resolver_addr)); + resolvers.push_back(Network::Address::resolveProtoAddress(resolver_addr)); } selected_dns_resolver = dispatcher.createDnsResolver(resolvers); } @@ -342,7 +343,7 @@ StaticClusterImpl::StaticClusterImpl(const envoy::api::v2::Cluster& cluster, HostVectorSharedPtr new_hosts(new std::vector()); for (const auto& host : cluster.hosts()) { new_hosts->emplace_back( - HostSharedPtr{new HostImpl(info_, "", Network::Utility::fromProtoAddress(host), + HostSharedPtr{new HostImpl(info_, "", Network::Address::resolveProtoAddress(host), envoy::api::v2::Metadata::default_instance(), 1, envoy::api::v2::Locality().default_instance())}); } diff --git a/source/server/BUILD b/source/server/BUILD index d998c2d01df9..e33387e6a8f3 100644 --- a/source/server/BUILD +++ b/source/server/BUILD @@ -40,6 +40,7 @@ envoy_cc_library( "//source/common/common:utility_lib", "//source/common/config:lds_json_lib", "//source/common/config:utility_lib", + "//source/common/network:resolver_lib", "//source/common/network:utility_lib", "//source/common/protobuf:utility_lib", "//source/common/ratelimit:ratelimit_lib", diff --git a/source/server/config/stats/BUILD b/source/server/config/stats/BUILD index 7c9b95170460..3048f8a08621 100644 --- a/source/server/config/stats/BUILD +++ b/source/server/config/stats/BUILD @@ -19,6 +19,7 @@ envoy_cc_library( "//include/envoy/registry", "//source/common/config:well_known_names", "//source/common/network:address_lib", + "//source/common/network:resolver_lib", "//source/common/stats:statsd_lib", "//source/server:configuration_lib", ], diff --git a/source/server/config/stats/statsd.cc b/source/server/config/stats/statsd.cc index 14615c15d1f4..5fa3b5a52ca8 100644 --- a/source/server/config/stats/statsd.cc +++ b/source/server/config/stats/statsd.cc @@ -5,6 +5,7 @@ #include "envoy/registry/registry.h" #include "common/config/well_known_names.h" +#include "common/network/resolver_impl.h" #include "common/stats/statsd.h" #include "api/bootstrap.pb.h" @@ -20,7 +21,7 @@ Stats::SinkPtr StatsdSinkFactory::createStatsSink(const Protobuf::Message& confi switch (statsd_sink.statsd_specifier_case()) { case envoy::api::v2::StatsdSink::kAddress: { Network::Address::InstanceConstSharedPtr address = - Network::Utility::fromProtoAddress(statsd_sink.address()); + Network::Address::resolveProtoAddress(statsd_sink.address()); ENVOY_LOG(info, "statsd UDP ip address: {}", address->asString()); return Stats::SinkPtr( new Stats::Statsd::UdpStatsdSink(server.threadLocal(), std::move(address))); diff --git a/source/server/configuration_impl.cc b/source/server/configuration_impl.cc index cfcb8da944fe..bba9b9006a92 100644 --- a/source/server/configuration_impl.cc +++ b/source/server/configuration_impl.cc @@ -123,7 +123,7 @@ InitialImpl::InitialImpl(const envoy::api::v2::Bootstrap& bootstrap) { admin_.access_log_path_ = admin.access_log_path(); admin_.profile_path_ = admin.profile_path().empty() ? "/var/log/envoy/envoy.prof" : admin.profile_path(); - admin_.address_ = Network::Utility::fromProtoAddress(admin.address()); + admin_.address_ = Network::Address::resolveProtoAddress(admin.address()); if (!bootstrap.flags_path().empty()) { flags_path_.value(bootstrap.flags_path()); diff --git a/source/server/configuration_impl.h b/source/server/configuration_impl.h index ee06faf824d5..6d4ea01cda0c 100644 --- a/source/server/configuration_impl.h +++ b/source/server/configuration_impl.h @@ -18,6 +18,7 @@ #include "common/common/logger.h" #include "common/json/json_loader.h" +#include "common/network/resolver_impl.h" #include "common/network/utility.h" #include "server/lds_api.h" diff --git a/test/common/network/BUILD b/test/common/network/BUILD index c8b1a8d431fd..ba4a8d65bd56 100644 --- a/test/common/network/BUILD +++ b/test/common/network/BUILD @@ -144,6 +144,7 @@ envoy_cc_test( deps = [ "//source/common/network:address_lib", "//source/common/network:resolver_lib", + "//test/mocks/network:network_mocks", "//test/test_common:environment_lib", ], ) diff --git a/test/common/network/resolver_impl_test.cc b/test/common/network/resolver_impl_test.cc index 641c4027db44..187d1cfaadd1 100644 --- a/test/common/network/resolver_impl_test.cc +++ b/test/common/network/resolver_impl_test.cc @@ -8,7 +8,9 @@ #include "common/common/thread.h" #include "common/network/address_impl.h" +#include "common/network/resolver_impl.h" +#include "test/mocks/network/mocks.h" #include "test/test_common/environment.h" #include "test/test_common/utility.h" @@ -33,6 +35,92 @@ TEST_F(IpResolverTest, DisallowsNamedPort) { EXPECT_THROW(resolver->resolve("1.2.3.4", "http"), EnvoyException); } +TEST(ResolverTest, FromProtoAddress) { + envoy::api::v2::Address ipv4_address; + ipv4_address.mutable_socket_address()->set_address("1.2.3.4"); + ipv4_address.mutable_socket_address()->set_port_value(5); + EXPECT_EQ("1.2.3.4:5", resolveProtoAddress(ipv4_address)->asString()); + + envoy::api::v2::Address ipv6_address; + ipv4_address.mutable_socket_address()->set_address("1::1"); + ipv4_address.mutable_socket_address()->set_port_value(2); + EXPECT_EQ("[1::1]:2", resolveProtoAddress(ipv4_address)->asString()); + + envoy::api::v2::Address pipe_address; + pipe_address.mutable_pipe()->set_path("/foo/bar"); + EXPECT_EQ("/foo/bar", resolveProtoAddress(pipe_address)->asString()); +} + +class TestResolver : public Resolver { +public: + TestResolver(const std::map name_mappings) + : name_mappings_(name_mappings) {} + InstanceConstSharedPtr resolve(const std::string& logical, uint32_t port) { + std::string physical = getPhysicalName(logical); + return InstanceConstSharedPtr{new MockResolvedAddress(fmt::format("{}:{}", logical, port), + fmt::format("{}:{}", physical, port))}; + } + + InstanceConstSharedPtr resolve(const std::string& logical, const std::string& named_port) { + std::string physical = getPhysicalName(logical); + return InstanceConstSharedPtr{new MockResolvedAddress( + fmt::format("{}:{}", logical, named_port), fmt::format("{}:{}", physical, named_port))}; + } + +private: + std::string getPhysicalName(const std::string& logical) { + auto it = name_mappings_.find(logical); + if (it == name_mappings_.end()) { + throw EnvoyException("no such mapping exists"); + } + return it->second; + } + + std::map name_mappings_; +}; + +class TestResolverFactory : public ResolverFactory { +public: + std::string name() override { return "envoy.test.resolver"; } + + ResolverPtr create() const override { return ResolverPtr{new TestResolver(name_mappings_)}; } + + void addMapping(const std::string& logical, const std::string& physical) { + name_mappings_[logical] = physical; + } + +private: + std::map name_mappings_; +}; + +TEST(ResolverTest, NonStandardResolver) { + Registry::RegisterFactory register_resolver; + auto& test_factory = register_resolver.testGetFactory(); + test_factory.addMapping("foo", "1.2.3.4"); + test_factory.addMapping("bar", "4.3.2.1"); + + { + envoy::api::v2::Address address; + auto socket = address.mutable_socket_address(); + socket->set_address("foo"); + socket->set_port_value(5); + socket->set_resolver_name("envoy.test.resolver"); + auto instance = resolveProtoAddress(address); + EXPECT_EQ("1.2.3.4:5", instance->asString()); + EXPECT_EQ("foo:5", instance->logicalName()); + } + { + envoy::api::v2::Address address; + auto socket = address.mutable_socket_address(); + socket->set_address("bar"); + socket->set_named_port("http"); + socket->set_resolver_name("envoy.test.resolver"); + auto instance = resolveProtoAddress(address); + EXPECT_EQ("4.3.2.1:http", instance->asString()); + EXPECT_EQ("bar:http", instance->logicalName()); + } +} + } // namespace Address } // namespace Network } // namespace Envoy diff --git a/test/common/network/utility_test.cc b/test/common/network/utility_test.cc index 956f82211311..7a1abd9923df 100644 --- a/test/common/network/utility_test.cc +++ b/test/common/network/utility_test.cc @@ -119,22 +119,6 @@ TEST(NetworkUtility, ParseInternetAddressAndPort) { EXPECT_EQ("[::1]:0", Utility::parseInternetAddressAndPort("[::1]:0")->asString()); } -TEST(NetworkUtility, FromProtoAddress) { - envoy::api::v2::Address ipv4_address; - ipv4_address.mutable_socket_address()->set_address("1.2.3.4"); - ipv4_address.mutable_socket_address()->set_port_value(5); - EXPECT_EQ("1.2.3.4:5", Utility::fromProtoAddress(ipv4_address)->asString()); - - envoy::api::v2::Address ipv6_address; - ipv4_address.mutable_socket_address()->set_address("1::1"); - ipv4_address.mutable_socket_address()->set_port_value(2); - EXPECT_EQ("[1::1]:2", Utility::fromProtoAddress(ipv4_address)->asString()); - - envoy::api::v2::Address pipe_address; - pipe_address.mutable_pipe()->set_path("/foo/bar"); - EXPECT_EQ("/foo/bar", Utility::fromProtoAddress(pipe_address)->asString()); -} - class NetworkUtilityGetLocalAddress : public testing::TestWithParam {}; INSTANTIATE_TEST_CASE_P(IpVersions, NetworkUtilityGetLocalAddress, diff --git a/test/mocks/network/mocks.h b/test/mocks/network/mocks.h index 73e370a36056..9024a1550583 100644 --- a/test/mocks/network/mocks.h +++ b/test/mocks/network/mocks.h @@ -249,5 +249,27 @@ class MockConnectionHandler : public ConnectionHandler { MOCK_METHOD0(stopListeners, void()); }; +class MockResolvedAddress : public Address::Instance { +public: + MockResolvedAddress(const std::string& logical, const std::string& physical) + : logical_(logical), physical_(physical) {} + + bool operator==(const Address::Instance& other) const override { + return asString() == other.asString(); + } + + MOCK_CONST_METHOD1(bind, int(int)); + MOCK_CONST_METHOD1(connect, int(int)); + MOCK_CONST_METHOD0(ip, Address::Ip*()); + MOCK_CONST_METHOD1(socket, int(Address::SocketType)); + MOCK_CONST_METHOD0(type, Address::Type()); + + const std::string& asString() const override { return physical_; } + const std::string& logicalName() const override { return logical_; } + + const std::string logical_; + const std::string physical_; +}; + } // namespace Network } // namespace Envoy From c0cac0b3e14c4629a79a50a080cf4d2ee197f32a Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Wed, 11 Oct 2017 14:16:23 -0400 Subject: [PATCH 06/16] Make Address::Resolver take a proto to resolve Signed-off-by: Alex Konradi --- include/envoy/network/resolver.h | 21 ++++--------- source/common/network/BUILD | 1 + source/common/network/resolver_impl.cc | 34 +++++++++----------- test/common/network/BUILD | 1 + test/common/network/resolver_impl_test.cc | 38 +++++++++++++++++------ 5 files changed, 50 insertions(+), 45 deletions(-) diff --git a/include/envoy/network/resolver.h b/include/envoy/network/resolver.h index 30b76bad3aea..912e6098b9ad 100644 --- a/include/envoy/network/resolver.h +++ b/include/envoy/network/resolver.h @@ -8,6 +8,8 @@ #include "envoy/common/pure.h" #include "envoy/network/address.h" +#include "api/address.pb.h" + namespace Envoy { namespace Network { namespace Address { @@ -20,22 +22,11 @@ class Resolver { virtual ~Resolver() {} /** - * Resolve a custom address string and port to an Address::Instance - * @param address the address to resolve - * @param supplies the port on the address - * @return an appropriate Address::Instance - */ - virtual Network::Address::InstanceConstSharedPtr resolve(const std::string& address, - const uint32_t port) PURE; - - /** - * Resolve a custom address string and named port to an Address::Instance - * @param address the address to resolve - * @param named_port the named port to resolve - * @return an appropriate Address::Instance + * Resolve a custom address string and port to an Address::Instance. + * @param socket_address supplies the socket address to resolve. + * @return InstanceConstSharedPtr appropriate Address::Instance. */ - virtual Network::Address::InstanceConstSharedPtr resolve(const std::string& address, - const std::string& named_port) PURE; + virtual InstanceConstSharedPtr resolve(const envoy::api::v2::SocketAddress& socket_address) PURE; }; typedef std::unique_ptr ResolverPtr; diff --git a/source/common/network/BUILD b/source/common/network/BUILD index 58c83c81cc0b..5cdb0d2eeffe 100644 --- a/source/common/network/BUILD +++ b/source/common/network/BUILD @@ -144,6 +144,7 @@ envoy_cc_library( "//include/envoy/registry", "//source/common/config:well_known_names", "//source/common/network:address_lib", + "//source/common/protobuf", ], ) diff --git a/source/common/network/resolver_impl.cc b/source/common/network/resolver_impl.cc index d5b90a8b38be..2abb8f064a0d 100644 --- a/source/common/network/resolver_impl.cc +++ b/source/common/network/resolver_impl.cc @@ -9,6 +9,8 @@ #include "common/network/address_impl.h" #include "common/network/utility.h" +#include "api/address.pb.h" + namespace Envoy { namespace Network { namespace Address { @@ -19,14 +21,18 @@ namespace Address { class IpResolver : public Resolver { public: - Network::Address::InstanceConstSharedPtr resolve(const std::string& address, - const uint32_t port) override { - return Network::Utility::parseInternetAddress(address, port); - } + InstanceConstSharedPtr resolve(const envoy::api::v2::SocketAddress& socket_address) { + switch (socket_address.port_specifier_case()) { + case envoy::api::v2::SocketAddress::kPortValue: + // default to port 0 if no port value is specified + case envoy::api::v2::SocketAddress::PORT_SPECIFIER_NOT_SET: + return Network::Utility::parseInternetAddress(socket_address.address(), + socket_address.port_value()); - Network::Address::InstanceConstSharedPtr resolve(const std::string&, - const std::string&) override { - throw EnvoyException("named ports are not supported by this resolver"); + default: + throw EnvoyException(fmt::format("IP resolver can't handle port specifier type {}", + socket_address.port_specifier_case())); + } } }; @@ -70,19 +76,7 @@ resolveProtoSocketAddress(const envoy::api::v2::SocketAddress& socket_address) { throw EnvoyException(fmt::format("Unknown address resolver: {}", resolver_name)); } ResolverPtr resolver(resolver_factory->create()); - switch (socket_address.port_specifier_case()) { - case envoy::api::v2::SocketAddress::kNamedPort: - return resolver->resolve(socket_address.address(), socket_address.named_port()); - - case envoy::api::v2::SocketAddress::kPortValue: - // default to port 0 if no port value is specified - case envoy::api::v2::SocketAddress::PORT_SPECIFIER_NOT_SET: - return resolver->resolve(socket_address.address(), socket_address.port_value()); - - default: - throw EnvoyException( - fmt::format("Unknown port specifier type {}", socket_address.port_specifier_case())); - } + return resolver->resolve(socket_address); } } // namespace Address diff --git a/test/common/network/BUILD b/test/common/network/BUILD index ba4a8d65bd56..20cf438930fe 100644 --- a/test/common/network/BUILD +++ b/test/common/network/BUILD @@ -144,6 +144,7 @@ envoy_cc_test( deps = [ "//source/common/network:address_lib", "//source/common/network:resolver_lib", + "//source/common/protobuf", "//test/mocks/network:network_mocks", "//test/test_common:environment_lib", ], diff --git a/test/common/network/resolver_impl_test.cc b/test/common/network/resolver_impl_test.cc index 187d1cfaadd1..9de0c87b1e0d 100644 --- a/test/common/network/resolver_impl_test.cc +++ b/test/common/network/resolver_impl_test.cc @@ -14,6 +14,7 @@ #include "test/test_common/environment.h" #include "test/test_common/utility.h" +#include "api/address.pb.h" #include "gtest/gtest.h" namespace Envoy { @@ -25,14 +26,20 @@ class IpResolverTest : public testing::Test { }; TEST_F(IpResolverTest, Basic) { - auto address = factory_->create()->resolve("1.2.3.4", 443); + envoy::api::v2::SocketAddress socket_address; + socket_address.set_address("1.2.3.4"); + socket_address.set_port_value(443); + auto address = factory_->create()->resolve(socket_address); EXPECT_EQ(address->ip()->addressAsString(), "1.2.3.4"); EXPECT_EQ(address->ip()->port(), 443); } TEST_F(IpResolverTest, DisallowsNamedPort) { auto resolver = factory_->create(); - EXPECT_THROW(resolver->resolve("1.2.3.4", "http"), EnvoyException); + envoy::api::v2::SocketAddress socket_address; + socket_address.set_address("1.2.3.4"); + socket_address.set_named_port("http"); + EXPECT_THROW(resolver->resolve(socket_address), EnvoyException); } TEST(ResolverTest, FromProtoAddress) { @@ -55,18 +62,14 @@ class TestResolver : public Resolver { public: TestResolver(const std::map name_mappings) : name_mappings_(name_mappings) {} - InstanceConstSharedPtr resolve(const std::string& logical, uint32_t port) { - std::string physical = getPhysicalName(logical); + InstanceConstSharedPtr resolve(const envoy::api::v2::SocketAddress& socket_address) { + const std::string logical = socket_address.address(); + const std::string physical = getPhysicalName(logical); + const std::string port = getPort(socket_address); return InstanceConstSharedPtr{new MockResolvedAddress(fmt::format("{}:{}", logical, port), fmt::format("{}:{}", physical, port))}; } - InstanceConstSharedPtr resolve(const std::string& logical, const std::string& named_port) { - std::string physical = getPhysicalName(logical); - return InstanceConstSharedPtr{new MockResolvedAddress( - fmt::format("{}:{}", logical, named_port), fmt::format("{}:{}", physical, named_port))}; - } - private: std::string getPhysicalName(const std::string& logical) { auto it = name_mappings_.find(logical); @@ -76,6 +79,21 @@ class TestResolver : public Resolver { return it->second; } + std::string getPort(const envoy::api::v2::SocketAddress& socket_address) { + switch (socket_address.port_specifier_case()) { + case envoy::api::v2::SocketAddress::kNamedPort: + return socket_address.named_port(); + case envoy::api::v2::SocketAddress::kPortValue: + // default to port 0 if no port value is specified + case envoy::api::v2::SocketAddress::PORT_SPECIFIER_NOT_SET: + return fmt::format("{}", socket_address.port_value()); + + default: + throw EnvoyException( + fmt::format("Unknown port specifier type {}", socket_address.port_specifier_case())); + } + } + std::map name_mappings_; }; From 9451f074da6d4d4a61ff5feb6e9ac3d7688a56a7 Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Wed, 11 Oct 2017 14:26:44 -0400 Subject: [PATCH 07/16] Fix documentation in comments. Signed-off-by: Alex Konradi --- include/envoy/network/address.h | 2 +- include/envoy/network/resolver.h | 4 ++-- include/envoy/registry/registry.h | 2 +- source/common/network/address_impl.h | 2 +- source/common/network/resolver_impl.cc | 6 +++--- source/common/network/resolver_impl.h | 4 ++-- 6 files changed, 10 insertions(+), 10 deletions(-) diff --git a/include/envoy/network/address.h b/include/envoy/network/address.h index 87c8439d136f..4479d9f9fae8 100644 --- a/include/envoy/network/address.h +++ b/include/envoy/network/address.h @@ -116,7 +116,7 @@ class Instance { * logical/unresolved name. * * This string has a source-dependent format and should preserve the original - * name for Address::Instances resolved by a Network::AddressResolver. + * name for Address::Instances resolved by a Network::Address::Resolver. */ virtual const std::string& logicalName() const PURE; diff --git a/include/envoy/network/resolver.h b/include/envoy/network/resolver.h index 912e6098b9ad..30434480137d 100644 --- a/include/envoy/network/resolver.h +++ b/include/envoy/network/resolver.h @@ -15,7 +15,7 @@ namespace Network { namespace Address { /** - * Interface for all network address resolvers + * Interface for all network address resolvers. */ class Resolver { public: @@ -32,7 +32,7 @@ class Resolver { typedef std::unique_ptr ResolverPtr; /** - * A factory for individual network address resolvers + * A factory for individual network address resolvers. */ class ResolverFactory { public: diff --git a/include/envoy/registry/registry.h b/include/envoy/registry/registry.h index 88b8180d276e..05e659bd8c57 100644 --- a/include/envoy/registry/registry.h +++ b/include/envoy/registry/registry.h @@ -86,7 +86,7 @@ template class RegisterFactory { RegisterFactory() { FactoryRegistry::registerFactory(instance_); } /** - * Destructor that removes an instance of the factory from the FactoryRegistry + * Destructor that removes an instance of the factory from the FactoryRegistry. */ ~RegisterFactory() { FactoryRegistry::unregisterFactory(instance_); } diff --git a/source/common/network/address_impl.h b/source/common/network/address_impl.h index 517e207b8eff..5a2306adc851 100644 --- a/source/common/network/address_impl.h +++ b/source/common/network/address_impl.h @@ -48,7 +48,7 @@ class InstanceBase : public Instance { // Network::Address::Instance bool operator==(const Instance& rhs) const override { return asString() == rhs.asString(); } const std::string& asString() const override { return friendly_name_; } - // Default logical name is the human-readable name + // Default logical name is the human-readable name. const std::string& logicalName() const override { return asString(); } Type type() const override { return type_; } diff --git a/source/common/network/resolver_impl.cc b/source/common/network/resolver_impl.cc index 2abb8f064a0d..2363abf548e7 100644 --- a/source/common/network/resolver_impl.cc +++ b/source/common/network/resolver_impl.cc @@ -16,7 +16,7 @@ namespace Network { namespace Address { /** - * Implementation of a resolver for IP addresses + * Implementation of a resolver for IP addresses. */ class IpResolver : public Resolver { @@ -24,7 +24,7 @@ class IpResolver : public Resolver { InstanceConstSharedPtr resolve(const envoy::api::v2::SocketAddress& socket_address) { switch (socket_address.port_specifier_case()) { case envoy::api::v2::SocketAddress::kPortValue: - // default to port 0 if no port value is specified + // Default to port 0 if no port value is specified. case envoy::api::v2::SocketAddress::PORT_SPECIFIER_NOT_SET: return Network::Utility::parseInternetAddress(socket_address.address(), socket_address.port_value()); @@ -37,7 +37,7 @@ class IpResolver : public Resolver { }; /** - * Implementation of an IP address resolver factory + * Implementation of an IP address resolver factory. */ class IpResolverFactory : public ResolverFactory { public: diff --git a/source/common/network/resolver_impl.h b/source/common/network/resolver_impl.h index da5a2f83729f..30af33cb5a1c 100644 --- a/source/common/network/resolver_impl.h +++ b/source/common/network/resolver_impl.h @@ -13,14 +13,14 @@ namespace Network { namespace Address { /** * Create an Instance from a envoy::api::v2::Address. - * @param address message. + * @param address supplies the address proto to resolve. * @return pointer to the Instance. */ Address::InstanceConstSharedPtr resolveProtoAddress(const envoy::api::v2::Address& address); /** * Create an Instance from a envoy::api::v2::SocketAddress. - * @param socket address message. + * @param address supplies the socket address proto to resolve. * @return pointer to the Instance. */ Address::InstanceConstSharedPtr From a633aafe08a010ee464d80c70f3736c3e8a00c4c Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Wed, 11 Oct 2017 14:31:07 -0400 Subject: [PATCH 08/16] Change ResolverFactory::create() to createResolver Signed-off-by: Alex Konradi --- include/envoy/network/resolver.h | 2 +- source/common/network/resolver_impl.cc | 4 ++-- test/common/network/resolver_impl_test.cc | 8 +++++--- 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/include/envoy/network/resolver.h b/include/envoy/network/resolver.h index 30434480137d..6f8f8d47cb78 100644 --- a/include/envoy/network/resolver.h +++ b/include/envoy/network/resolver.h @@ -38,7 +38,7 @@ class ResolverFactory { public: virtual ~ResolverFactory() {} - virtual ResolverPtr create() const PURE; + virtual ResolverPtr createResolver() const PURE; /** * @return std::string the identifying name for a particular implementation of diff --git a/source/common/network/resolver_impl.cc b/source/common/network/resolver_impl.cc index 2363abf548e7..95b3702308a0 100644 --- a/source/common/network/resolver_impl.cc +++ b/source/common/network/resolver_impl.cc @@ -41,7 +41,7 @@ class IpResolver : public Resolver { */ class IpResolverFactory : public ResolverFactory { public: - ResolverPtr create() const override { return ResolverPtr{new IpResolver()}; } + ResolverPtr createResolver() const override { return ResolverPtr{new IpResolver()}; } std::string name() override { return Config::AddressResolverNames::get().IP; } }; @@ -75,7 +75,7 @@ resolveProtoSocketAddress(const envoy::api::v2::SocketAddress& socket_address) { if (resolver_factory == nullptr) { throw EnvoyException(fmt::format("Unknown address resolver: {}", resolver_name)); } - ResolverPtr resolver(resolver_factory->create()); + ResolverPtr resolver(resolver_factory->createResolver()); return resolver->resolve(socket_address); } diff --git a/test/common/network/resolver_impl_test.cc b/test/common/network/resolver_impl_test.cc index 9de0c87b1e0d..523355fdaa66 100644 --- a/test/common/network/resolver_impl_test.cc +++ b/test/common/network/resolver_impl_test.cc @@ -29,13 +29,13 @@ TEST_F(IpResolverTest, Basic) { envoy::api::v2::SocketAddress socket_address; socket_address.set_address("1.2.3.4"); socket_address.set_port_value(443); - auto address = factory_->create()->resolve(socket_address); + auto address = factory_->createResolver()->resolve(socket_address); EXPECT_EQ(address->ip()->addressAsString(), "1.2.3.4"); EXPECT_EQ(address->ip()->port(), 443); } TEST_F(IpResolverTest, DisallowsNamedPort) { - auto resolver = factory_->create(); + auto resolver = factory_->createResolver(); envoy::api::v2::SocketAddress socket_address; socket_address.set_address("1.2.3.4"); socket_address.set_named_port("http"); @@ -101,7 +101,9 @@ class TestResolverFactory : public ResolverFactory { public: std::string name() override { return "envoy.test.resolver"; } - ResolverPtr create() const override { return ResolverPtr{new TestResolver(name_mappings_)}; } + ResolverPtr createResolver() const override { + return ResolverPtr{new TestResolver(name_mappings_)}; + } void addMapping(const std::string& logical, const std::string& physical) { name_mappings_[logical] = physical; From 134034cb71d907c82947c2af5a7d93eb3248bd20 Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Wed, 11 Oct 2017 15:22:19 -0400 Subject: [PATCH 09/16] Add tests to cover address resolution error cases Signed-off-by: Alex Konradi --- test/common/network/resolver_impl_test.cc | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/test/common/network/resolver_impl_test.cc b/test/common/network/resolver_impl_test.cc index 523355fdaa66..94642368a8b1 100644 --- a/test/common/network/resolver_impl_test.cc +++ b/test/common/network/resolver_impl_test.cc @@ -141,6 +141,20 @@ TEST(ResolverTest, NonStandardResolver) { } } +TEST(ResolverTest, UninitializedAddress) { + envoy::api::v2::Address address; + EXPECT_THROW(resolveProtoAddress(address), EnvoyException); +} + +TEST(ResolverTest, NoSuchResolver) { + envoy::api::v2::Address address; + auto socket = address.mutable_socket_address(); + socket->set_address("foo"); + socket->set_port_value(5); + socket->set_resolver_name("envoy.test.resolver"); + EXPECT_THROW(resolveProtoAddress(address), EnvoyException); +} + } // namespace Address } // namespace Network } // namespace Envoy From de0ee8e5ba8ef23723bcb219304215c7bc89f273 Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Wed, 11 Oct 2017 15:36:03 -0400 Subject: [PATCH 10/16] Check exception message for IP with named port Signed-off-by: Alex Konradi --- test/common/network/resolver_impl_test.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/common/network/resolver_impl_test.cc b/test/common/network/resolver_impl_test.cc index 94642368a8b1..bf101a6dcbd8 100644 --- a/test/common/network/resolver_impl_test.cc +++ b/test/common/network/resolver_impl_test.cc @@ -39,7 +39,9 @@ TEST_F(IpResolverTest, DisallowsNamedPort) { envoy::api::v2::SocketAddress socket_address; socket_address.set_address("1.2.3.4"); socket_address.set_named_port("http"); - EXPECT_THROW(resolver->resolve(socket_address), EnvoyException); + EXPECT_THROW_WITH_MESSAGE(resolver->resolve(socket_address), EnvoyException, + fmt::format("IP resolver can't handle port specifier type {}", + envoy::api::v2::SocketAddress::kNamedPort)); } TEST(ResolverTest, FromProtoAddress) { From aa73c50198791d535251da084a3ebafdeae03f38 Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Thu, 12 Oct 2017 10:06:51 -0400 Subject: [PATCH 11/16] Make resolver name method const Signed-off-by: Alex Konradi --- include/envoy/network/resolver.h | 2 +- source/common/network/resolver_impl.cc | 2 +- test/common/network/resolver_impl_test.cc | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/include/envoy/network/resolver.h b/include/envoy/network/resolver.h index 6f8f8d47cb78..e014c11eebea 100644 --- a/include/envoy/network/resolver.h +++ b/include/envoy/network/resolver.h @@ -44,7 +44,7 @@ class ResolverFactory { * @return std::string the identifying name for a particular implementation of * a resolver produced by the factory. */ - virtual std::string name() PURE; + virtual std::string name() const PURE; }; typedef std::shared_ptr ResolverFactorySharedPtr; diff --git a/source/common/network/resolver_impl.cc b/source/common/network/resolver_impl.cc index 95b3702308a0..a12f8669dd1e 100644 --- a/source/common/network/resolver_impl.cc +++ b/source/common/network/resolver_impl.cc @@ -43,7 +43,7 @@ class IpResolverFactory : public ResolverFactory { public: ResolverPtr createResolver() const override { return ResolverPtr{new IpResolver()}; } - std::string name() override { return Config::AddressResolverNames::get().IP; } + std::string name() const override { return Config::AddressResolverNames::get().IP; } }; /** diff --git a/test/common/network/resolver_impl_test.cc b/test/common/network/resolver_impl_test.cc index bf101a6dcbd8..1f0c9c93b3be 100644 --- a/test/common/network/resolver_impl_test.cc +++ b/test/common/network/resolver_impl_test.cc @@ -101,7 +101,7 @@ class TestResolver : public Resolver { class TestResolverFactory : public ResolverFactory { public: - std::string name() override { return "envoy.test.resolver"; } + std::string name() const override { return "envoy.test.resolver"; } ResolverPtr createResolver() const override { return ResolverPtr{new TestResolver(name_mappings_)}; From 0236b12929972db2a2e2bbb445501a40422db197 Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Thu, 12 Oct 2017 10:07:28 -0400 Subject: [PATCH 12/16] Remove ResolverFactory from address resolution It might be helpful to have separate Resolvers and ResolverFactories if addresses were begin resolved asynchronously. Until then, though, there is no need to have both. Signed-off-by: Alex Konradi --- include/envoy/network/resolver.h | 16 +--------- source/common/network/resolver_impl.cc | 21 ++++-------- test/common/network/resolver_impl_test.cc | 39 ++++++++--------------- 3 files changed, 20 insertions(+), 56 deletions(-) diff --git a/include/envoy/network/resolver.h b/include/envoy/network/resolver.h index e014c11eebea..b4c93bc145dc 100644 --- a/include/envoy/network/resolver.h +++ b/include/envoy/network/resolver.h @@ -27,28 +27,14 @@ class Resolver { * @return InstanceConstSharedPtr appropriate Address::Instance. */ virtual InstanceConstSharedPtr resolve(const envoy::api::v2::SocketAddress& socket_address) PURE; -}; - -typedef std::unique_ptr ResolverPtr; - -/** - * A factory for individual network address resolvers. - */ -class ResolverFactory { -public: - virtual ~ResolverFactory() {} - - virtual ResolverPtr createResolver() const PURE; /** * @return std::string the identifying name for a particular implementation of - * a resolver produced by the factory. + * a resolver. */ virtual std::string name() const PURE; }; -typedef std::shared_ptr ResolverFactorySharedPtr; - } // namespace Address } // namespace Network } // namespace Envoy diff --git a/source/common/network/resolver_impl.cc b/source/common/network/resolver_impl.cc index a12f8669dd1e..88a809939995 100644 --- a/source/common/network/resolver_impl.cc +++ b/source/common/network/resolver_impl.cc @@ -34,14 +34,6 @@ class IpResolver : public Resolver { socket_address.port_specifier_case())); } } -}; - -/** - * Implementation of an IP address resolver factory. - */ -class IpResolverFactory : public ResolverFactory { -public: - ResolverPtr createResolver() const override { return ResolverPtr{new IpResolver()}; } std::string name() const override { return Config::AddressResolverNames::get().IP; } }; @@ -49,7 +41,7 @@ class IpResolverFactory : public ResolverFactory { /** * Static registration for the IP resolver. @see RegisterFactory. */ -static Registry::RegisterFactory ip_registered_; +static Registry::RegisterFactory ip_registered_; InstanceConstSharedPtr resolveProtoAddress(const envoy::api::v2::Address& address) { switch (address.address_case()) { @@ -64,18 +56,17 @@ InstanceConstSharedPtr resolveProtoAddress(const envoy::api::v2::Address& addres InstanceConstSharedPtr resolveProtoSocketAddress(const envoy::api::v2::SocketAddress& socket_address) { - const ResolverFactory* resolver_factory = nullptr; + Resolver* resolver = nullptr; const std::string& resolver_name = socket_address.resolver_name(); if (resolver_name.empty()) { - resolver_factory = Registry::FactoryRegistry::getFactory( - Config::AddressResolverNames::get().IP); + resolver = + Registry::FactoryRegistry::getFactory(Config::AddressResolverNames::get().IP); } else { - resolver_factory = Registry::FactoryRegistry::getFactory(resolver_name); + resolver = Registry::FactoryRegistry::getFactory(resolver_name); } - if (resolver_factory == nullptr) { + if (resolver == nullptr) { throw EnvoyException(fmt::format("Unknown address resolver: {}", resolver_name)); } - ResolverPtr resolver(resolver_factory->createResolver()); return resolver->resolve(socket_address); } diff --git a/test/common/network/resolver_impl_test.cc b/test/common/network/resolver_impl_test.cc index 1f0c9c93b3be..18d60ce5eedb 100644 --- a/test/common/network/resolver_impl_test.cc +++ b/test/common/network/resolver_impl_test.cc @@ -22,24 +22,23 @@ namespace Network { namespace Address { class IpResolverTest : public testing::Test { public: - ResolverFactory* factory_{Registry::FactoryRegistry::getFactory("envoy.ip")}; + Resolver* resolver_{Registry::FactoryRegistry::getFactory("envoy.ip")}; }; TEST_F(IpResolverTest, Basic) { envoy::api::v2::SocketAddress socket_address; socket_address.set_address("1.2.3.4"); socket_address.set_port_value(443); - auto address = factory_->createResolver()->resolve(socket_address); + auto address = resolver_->resolve(socket_address); EXPECT_EQ(address->ip()->addressAsString(), "1.2.3.4"); EXPECT_EQ(address->ip()->port(), 443); } TEST_F(IpResolverTest, DisallowsNamedPort) { - auto resolver = factory_->createResolver(); envoy::api::v2::SocketAddress socket_address; socket_address.set_address("1.2.3.4"); socket_address.set_named_port("http"); - EXPECT_THROW_WITH_MESSAGE(resolver->resolve(socket_address), EnvoyException, + EXPECT_THROW_WITH_MESSAGE(resolver_->resolve(socket_address), EnvoyException, fmt::format("IP resolver can't handle port specifier type {}", envoy::api::v2::SocketAddress::kNamedPort)); } @@ -62,8 +61,6 @@ TEST(ResolverTest, FromProtoAddress) { class TestResolver : public Resolver { public: - TestResolver(const std::map name_mappings) - : name_mappings_(name_mappings) {} InstanceConstSharedPtr resolve(const envoy::api::v2::SocketAddress& socket_address) { const std::string logical = socket_address.address(); const std::string physical = getPhysicalName(logical); @@ -72,6 +69,12 @@ class TestResolver : public Resolver { fmt::format("{}:{}", physical, port))}; } + void addMapping(const std::string& logical, const std::string& physical) { + name_mappings_[logical] = physical; + } + + std::string name() const override { return "envoy.test.resolver"; } + private: std::string getPhysicalName(const std::string& logical) { auto it = name_mappings_.find(logical); @@ -99,27 +102,11 @@ class TestResolver : public Resolver { std::map name_mappings_; }; -class TestResolverFactory : public ResolverFactory { -public: - std::string name() const override { return "envoy.test.resolver"; } - - ResolverPtr createResolver() const override { - return ResolverPtr{new TestResolver(name_mappings_)}; - } - - void addMapping(const std::string& logical, const std::string& physical) { - name_mappings_[logical] = physical; - } - -private: - std::map name_mappings_; -}; - TEST(ResolverTest, NonStandardResolver) { - Registry::RegisterFactory register_resolver; - auto& test_factory = register_resolver.testGetFactory(); - test_factory.addMapping("foo", "1.2.3.4"); - test_factory.addMapping("bar", "4.3.2.1"); + Registry::RegisterFactory register_resolver; + auto& test_resolver = register_resolver.testGetFactory(); + test_resolver.addMapping("foo", "1.2.3.4"); + test_resolver.addMapping("bar", "4.3.2.1"); { envoy::api::v2::Address address; From 915c7d3b432ae9a4abbc4f69da5bf19694feb59a Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Thu, 12 Oct 2017 10:09:51 -0400 Subject: [PATCH 13/16] Use EXPECT_THROW_WITH_MESSAGE for resolver tests Signed-off-by: Alex Konradi --- test/common/network/resolver_impl_test.cc | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/test/common/network/resolver_impl_test.cc b/test/common/network/resolver_impl_test.cc index 18d60ce5eedb..a5f938324882 100644 --- a/test/common/network/resolver_impl_test.cc +++ b/test/common/network/resolver_impl_test.cc @@ -132,7 +132,8 @@ TEST(ResolverTest, NonStandardResolver) { TEST(ResolverTest, UninitializedAddress) { envoy::api::v2::Address address; - EXPECT_THROW(resolveProtoAddress(address), EnvoyException); + EXPECT_THROW_WITH_MESSAGE(resolveProtoAddress(address), EnvoyException, + "Address must be a socket or pipe: "); } TEST(ResolverTest, NoSuchResolver) { @@ -141,7 +142,8 @@ TEST(ResolverTest, NoSuchResolver) { socket->set_address("foo"); socket->set_port_value(5); socket->set_resolver_name("envoy.test.resolver"); - EXPECT_THROW(resolveProtoAddress(address), EnvoyException); + EXPECT_THROW_WITH_MESSAGE(resolveProtoAddress(address), EnvoyException, + "Unknown address resolver: envoy.test.resolver"); } } // namespace Address From 594721dd27e31e80930e056fdf57ebd6f6810b21 Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Thu, 12 Oct 2017 12:05:31 -0400 Subject: [PATCH 14/16] Add missing 'override's to Address::Resolver impls Fixes broken CI builds Signed-off-by: Alex Konradi --- source/common/network/resolver_impl.cc | 2 +- test/common/network/resolver_impl_test.cc | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/source/common/network/resolver_impl.cc b/source/common/network/resolver_impl.cc index 88a809939995..64bec7b5b213 100644 --- a/source/common/network/resolver_impl.cc +++ b/source/common/network/resolver_impl.cc @@ -21,7 +21,7 @@ namespace Address { class IpResolver : public Resolver { public: - InstanceConstSharedPtr resolve(const envoy::api::v2::SocketAddress& socket_address) { + InstanceConstSharedPtr resolve(const envoy::api::v2::SocketAddress& socket_address) override { switch (socket_address.port_specifier_case()) { case envoy::api::v2::SocketAddress::kPortValue: // Default to port 0 if no port value is specified. diff --git a/test/common/network/resolver_impl_test.cc b/test/common/network/resolver_impl_test.cc index a5f938324882..5deb05badedf 100644 --- a/test/common/network/resolver_impl_test.cc +++ b/test/common/network/resolver_impl_test.cc @@ -61,7 +61,7 @@ TEST(ResolverTest, FromProtoAddress) { class TestResolver : public Resolver { public: - InstanceConstSharedPtr resolve(const envoy::api::v2::SocketAddress& socket_address) { + InstanceConstSharedPtr resolve(const envoy::api::v2::SocketAddress& socket_address) override { const std::string logical = socket_address.address(); const std::string physical = getPhysicalName(logical); const std::string port = getPort(socket_address); From e6e8cc9c007162a23c6488137337f8263c3a90db Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Fri, 13 Oct 2017 09:43:36 -0400 Subject: [PATCH 15/16] Add TODO for registry injection once that's done Signed-off-by: Alex Konradi --- test/common/network/resolver_impl_test.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/test/common/network/resolver_impl_test.cc b/test/common/network/resolver_impl_test.cc index 5deb05badedf..cb400de539c9 100644 --- a/test/common/network/resolver_impl_test.cc +++ b/test/common/network/resolver_impl_test.cc @@ -103,6 +103,7 @@ class TestResolver : public Resolver { }; TEST(ResolverTest, NonStandardResolver) { + // TODO(akonradi) Use singleton override for this test once #1808 is resolved. Registry::RegisterFactory register_resolver; auto& test_resolver = register_resolver.testGetFactory(); test_resolver.addMapping("foo", "1.2.3.4"); From 16d8e5d9b43fdd7a545fca114594e535dba4786b Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Fri, 13 Oct 2017 10:28:55 -0400 Subject: [PATCH 16/16] Make test TODO more detailed Signed-off-by: Alex Konradi --- test/common/network/resolver_impl_test.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/common/network/resolver_impl_test.cc b/test/common/network/resolver_impl_test.cc index cb400de539c9..4be15c4bd435 100644 --- a/test/common/network/resolver_impl_test.cc +++ b/test/common/network/resolver_impl_test.cc @@ -103,7 +103,8 @@ class TestResolver : public Resolver { }; TEST(ResolverTest, NonStandardResolver) { - // TODO(akonradi) Use singleton override for this test once #1808 is resolved. + // TODO(akonradi) Use singleton override instead of adding and removing + // resolvers for this test once issue #1808 is resolved. Registry::RegisterFactory register_resolver; auto& test_resolver = register_resolver.testGetFactory(); test_resolver.addMapping("foo", "1.2.3.4");