-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
config: v2 Address resolution using named resolver #1835
Changes from 10 commits
a0c1ef2
b3ea367
bc87db0
3ed3f63
0a7f987
c0cac0b
9451f07
a633aaf
134034c
de0ee8e
aa73c50
0236b12
915c7d3
594721d
e6e8cc9
16d8e5d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
#pragma once | ||
|
||
#include <sys/types.h> | ||
|
||
#include <cstdint> | ||
#include <string> | ||
|
||
#include "envoy/common/pure.h" | ||
#include "envoy/network/address.h" | ||
|
||
#include "api/address.pb.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 socket_address supplies the socket address to resolve. | ||
* @return InstanceConstSharedPtr appropriate Address::Instance. | ||
*/ | ||
virtual InstanceConstSharedPtr resolve(const envoy::api::v2::SocketAddress& socket_address) PURE; | ||
}; | ||
|
||
typedef std::unique_ptr<Resolver> 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. | ||
*/ | ||
virtual std::string name() PURE; | ||
}; | ||
|
||
typedef std::shared_ptr<ResolverFactory> ResolverFactorySharedPtr; | ||
|
||
} // namespace Address | ||
} // namespace Network | ||
} // namespace Envoy |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -46,6 +46,16 @@ template <class Base> 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 T, class Base> class RegisterFactory { | |
/** | ||
* Contructor that registers an instance of the factory with the FactoryRegistry. | ||
*/ | ||
RegisterFactory() { | ||
static T* instance = new T; | ||
FactoryRegistry<Base>::registerFactory(*instance); | ||
} | ||
RegisterFactory() { FactoryRegistry<Base>::registerFactory(instance_); } | ||
|
||
/** | ||
* Destructor that removes an instance of the factory from the FactoryRegistry. | ||
*/ | ||
~RegisterFactory() { FactoryRegistry<Base>::unregisterFactory(instance_); } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This new setup creates a slightly bigger violation of the static ordering fiasco - we now have destructors being called in some unknown ordering (in addition to the constructors). @akonradi mentioned this was done so that tests don't leak state. Maybe it would be preferable to create an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @akonradi can you point at the specific test where the leak was an issue? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. https://github.com/akonradi/envoy/blob/915c7d3b432ae9a4abbc4f69da5bf19694feb59a/test/common/network/resolver_impl_test.cc#L145 is where there could have been one if the resolver from https://github.com/akonradi/envoy/blob/915c7d3b432ae9a4abbc4f69da5bf19694feb59a/test/common/network/resolver_impl_test.cc#L106 was allowed to leak. FWIW it seems like we've avoided this in the rest of the tests because we don't register test factories and stick to testing the included ones. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is where a singleton override for test would be useful. @alyssawilk any thoughts on how far along the work is to provide this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hasn't started (not assigned) but it looked like a pretty simple implementation. If it's as few lines of code as I think it could be folded in here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're talking about #1808? I'd rather make that a separate pull request and either wait to merge it once it lands in master or add a TODO here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, TODO sounds good to me, and update the issue to reference, thanks. |
||
|
||
T& testGetFactory() { return instance_; } | ||
|
||
private: | ||
T instance_; | ||
}; | ||
|
||
} // namespace Registry | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,84 @@ | ||
#include "common/network/resolver_impl.h" | ||
|
||
#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" | ||
|
||
#include "api/address.pb.h" | ||
|
||
namespace Envoy { | ||
namespace Network { | ||
namespace Address { | ||
|
||
/** | ||
* Implementation of a resolver for IP addresses. | ||
*/ | ||
class IpResolver : public Resolver { | ||
|
||
public: | ||
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()); | ||
|
||
default: | ||
throw EnvoyException(fmt::format("IP resolver can't handle port specifier type {}", | ||
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() override { return Config::AddressResolverNames::get().IP; } | ||
}; | ||
|
||
/** | ||
* Static registration for the IP resolver. @see RegisterFactory. | ||
*/ | ||
static Registry::RegisterFactory<IpResolverFactory, ResolverFactory> 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<ResolverFactory>::getFactory( | ||
Config::AddressResolverNames::get().IP); | ||
} else { | ||
resolver_factory = Registry::FactoryRegistry<ResolverFactory>::getFactory(resolver_name); | ||
} | ||
if (resolver_factory == nullptr) { | ||
throw EnvoyException(fmt::format("Unknown address resolver: {}", resolver_name)); | ||
} | ||
ResolverPtr resolver(resolver_factory->createResolver()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the reasoning behind having a resolver factory which then creates a resolver for each resolved address? Why not just have the resolver in the registry do the job? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The reasoning was that this would allow each resolver to keep state, such as for future asynchronous requests, and is more consistent generally with the rest of the codebase. That being said, it incurs some overhead and is probably unnecessary. |
||
return resolver->resolve(socket_address); | ||
} | ||
|
||
} // namespace Address | ||
} // namespace Network | ||
} // namespace Envoy |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 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 address supplies the socket address proto to resolve. | ||
* @return pointer to the Instance. | ||
*/ | ||
Address::InstanceConstSharedPtr | ||
resolveProtoSocketAddress(const envoy::api::v2::SocketAddress& address); | ||
} // namespace Address | ||
} // namespace Network | ||
} // namespace Envoy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole static registration facility is (already) violating the static initialization order fiasco rules in the style guide. We're adding another way that things can go wrong in terms of destructor ordering here, but since its only effect is to unregister I would argue it's safe.