-
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
Merged
Merged
Changes from all commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
a0c1ef2
Add logicalName() to Network::Address interface
akonradi b3ea367
Add Address::Resolver interface
akonradi bc87db0
Add Address::Resolver for IP resolution
akonradi 3ed3f63
Make RegisterFactory remove factory on delete
akonradi 0a7f987
Move Network::Utility proto address parsing
akonradi c0cac0b
Make Address::Resolver take a proto to resolve
akonradi 9451f07
Fix documentation in comments.
akonradi a633aaf
Change ResolverFactory::create() to createResolver
akonradi 134034c
Add tests to cover address resolution error cases
akonradi de0ee8e
Check exception message for IP with named port
akonradi aa73c50
Make resolver name method const
akonradi 0236b12
Remove ResolverFactory from address resolution
akonradi 915c7d3
Use EXPECT_THROW_WITH_MESSAGE for resolver tests
akonradi 594721d
Add missing 'override's to Address::Resolver impls
akonradi e6e8cc9
Add TODO for registry injection once that's done
akonradi 16d8e5d
Make test TODO more detailed
akonradi File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
#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; | ||
|
||
/** | ||
* @return std::string the identifying name for a particular implementation of | ||
* a resolver. | ||
*/ | ||
virtual std::string name() const PURE; | ||
}; | ||
|
||
} // namespace Address | ||
} // namespace Network | ||
} // namespace Envoy |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,75 @@ | ||
#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) override { | ||
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())); | ||
} | ||
} | ||
|
||
std::string name() const override { return Config::AddressResolverNames::get().IP; } | ||
}; | ||
|
||
/** | ||
* Static registration for the IP resolver. @see RegisterFactory. | ||
*/ | ||
static Registry::RegisterFactory<IpResolver, Resolver> 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) { | ||
Resolver* resolver = nullptr; | ||
const std::string& resolver_name = socket_address.resolver_name(); | ||
if (resolver_name.empty()) { | ||
resolver = | ||
Registry::FactoryRegistry<Resolver>::getFactory(Config::AddressResolverNames::get().IP); | ||
} else { | ||
resolver = Registry::FactoryRegistry<Resolver>::getFactory(resolver_name); | ||
} | ||
if (resolver == nullptr) { | ||
throw EnvoyException(fmt::format("Unknown address resolver: {}", resolver_name)); | ||
} | ||
return resolver->resolve(socket_address); | ||
} | ||
|
||
} // namespace Address | ||
} // namespace Network | ||
} // namespace Envoy |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 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
unregister
method on theFactoryRegistry
that manually removes the pointer (and allows the user to delete, maybe by returning as astd::unique_ptr
) rather than doing it by default on destruction of theRegisterFactory
class since we'd only need this in tests? @htuch thoughts?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.
@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 comment
The 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 comment
The 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 comment
The 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 comment
The 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 comment
The 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.