config: v2 Address resolution using named resolver #1835

akonradi · 2017-10-10T22:25:26Z

Add Address::ResolverFactory and Address::Resolver interfaces for instantiating Address::Instances from strings in protos. The interface is completely synchronous, so all translation must be done inline without any network access, meaning this can't be used to resolve hostnames via DNS. Doing so would require making more things asynchronous; this can be done, but let's not do it until it's necessary.

This is progress towards #1477

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 <[email protected]>

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 <[email protected]>

Does exactly what it says; resolves string IP addresses into Address::Instance objects Signed-off-by: Alex Konradi <[email protected]>

This will make it possible to use RegisterFactory in tests without leaking state between tests. Signed-off-by: Alex Konradi <[email protected]>

Now implemented by the Network::Address::ResolverFactory registry and appropriate interfaces. Signed-off-by: Alex Konradi <[email protected]>

htuch

Great, looks mostly good, comments are basically nits.

htuch · 2017-10-10T22:42:55Z

include/envoy/network/resolver.h

+   * 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


Nit. Envoy convention is to do @return Network::Address::InstanceConstSharedPtr an appropriate .... TBH, it's a bit inconsistent in practice, but see https://github.com/envoyproxy/envoy/blob/master/STYLE.md#deviations-from-google-c-style-guidelines.

htuch · 2017-10-10T22:44:27Z

include/envoy/network/resolver.h

+   * @param supplies the port on the address
+   * @return an appropriate Address::Instance
+   */
+  virtual Network::Address::InstanceConstSharedPtr resolve(const std::string& address,


Why can't we just resolve on a envoy::api::v2::SocketAddress https://github.com/envoyproxy/data-plane-api/blob/master/api/address.proto#L11? This would allow a single resolve method and might make the interface more extensible should we chose to grow the SocketAddress in the future (not that likely, but you never know).

htuch · 2017-10-10T22:51:32Z

include/envoy/network/resolver.h

+public:
+  virtual ~ResolverFactory() {}
+
+  virtual ResolverPtr create() const PURE;


Nit: createResolver for consistency with other factory interfaces.

htuch · 2017-10-10T23:01:28Z

include/envoy/registry/registry.h

+  /**
+   * Destructor that removes an instance of the factory from the FactoryRegistry
+   */
+  ~RegisterFactory() { FactoryRegistry<Base>::unregisterFactory(instance_); }


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.

htuch · 2017-10-10T23:02:46Z

source/common/network/resolver_impl.cc

+namespace Address {
+
+/**
+ * Implementation of a resolver for IP addresses


Very tiny nit. Prefer sentences to end with . (here and everywhere).

htuch · 2017-10-10T23:03:46Z

source/common/network/resolver_impl.cc

+
+  Network::Address::InstanceConstSharedPtr resolve(const std::string&,
+                                                   const std::string&) override {
+    throw EnvoyException("named ports are not supported by this resolver");


Nit: not support by IP resolver") (or something to that effect).

htuch · 2017-10-10T23:13:41Z

test/common/network/resolver_impl_test.cc

+
+TEST_F(IpResolverTest, DisallowsNamedPort) {
+  auto resolver = factory_->create();
+  EXPECT_THROW(resolver->resolve("1.2.3.4", "http"), EnvoyException);


Could be good to validate it's the exception we're after with EXPECT_THROW_WITH_MESSAGE. Do we have full coverage of all the throw cases in the PR?

htuch · 2017-10-10T23:15:37Z

test/common/network/resolver_impl_test.cc

+  TestResolver(const std::map<std::string, std::string> name_mappings)
+      : name_mappings_(name_mappings) {}
+  InstanceConstSharedPtr resolve(const std::string& logical, uint32_t port) {
+    std::string physical = getPhysicalName(logical);


Nit: const

Signed-off-by: Alex Konradi <[email protected]>

htuch · 2017-10-12T03:33:29Z

test/common/network/resolver_impl_test.cc

+
+TEST(ResolverTest, UninitializedAddress) {
+  envoy::api::v2::Address address;
+  EXPECT_THROW(resolveProtoAddress(address), EnvoyException);


Nit: would also prefer EXPECT_THROW_WITH_MESSAGE here.

mattklein123 · 2017-10-12T03:41:03Z

source/common/network/resolver_impl.cc

+  if (resolver_factory == nullptr) {
+    throw EnvoyException(fmt::format("Unknown address resolver: {}", resolver_name));
+  }
+  ResolverPtr resolver(resolver_factory->createResolver());


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?

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.

Signed-off-by: Alex Konradi <[email protected]>

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 <[email protected]>

Signed-off-by: Alex Konradi <[email protected]>

htuch

LGTM modulo nits, TSAN/ASAN look like they are failing, can you take a look?

htuch · 2017-10-12T14:51:39Z

source/common/network/resolver_impl.cc

-    resolver_factory = Registry::FactoryRegistry<ResolverFactory>::getFactory(
-        Config::AddressResolverNames::get().IP);
+    resolver =
+        Registry::FactoryRegistry<Resolver>::getFactory(Config::AddressResolverNames::get().IP);


Is getFactory now a misnomer? It's not returning a factory.. I thought it was fine before for the reasons you stated, but it's OK this way, just needs some changes to nomenclature.

A Resolver is not a literal factory, but it does produce Address::Instances. Alternatively, we can rename FactoryRegistry and its methods to something more generic like NamedRegistry, since that's the only method classes are required to implement.

mrice32 · 2017-10-12T14:56:43Z

include/envoy/registry/registry.h

+  /**
+   * Destructor that removes an instance of the factory from the FactoryRegistry.
+   */
+  ~RegisterFactory() { FactoryRegistry<Base>::unregisterFactory(instance_); }


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 the FactoryRegistry that manually removes the pointer (and allows the user to delete, maybe by returning as a std::unique_ptr) rather than doing it by default on destruction of the RegisterFactory class since we'd only need this in tests? @htuch thoughts?

@akonradi can you point at the specific test where the leak was an issue?

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.

This is where a singleton override for test would be useful. @alyssawilk any thoughts on how far along the work is to provide this?

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

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.

OK, TODO sounds good to me, and update the issue to reference, thanks.

Fixes broken CI builds Signed-off-by: Alex Konradi <[email protected]>

Signed-off-by: Alex Konradi <[email protected]>

htuch · 2017-10-13T14:26:20Z

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.


... and get rid of the need to unregister factories...

Signed-off-by: Alex Konradi <[email protected]>

htuch · 2017-10-13T17:23:16Z

TSAN was stuck, kicked it.

akonradi added 5 commits October 10, 2017 18:07

Add logicalName() to Network::Address interface

a0c1ef2

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 <[email protected]>

Add Address::Resolver for IP resolution

bc87db0

Does exactly what it says; resolves string IP addresses into Address::Instance objects Signed-off-by: Alex Konradi <[email protected]>

Make RegisterFactory remove factory on delete

3ed3f63

This will make it possible to use RegisterFactory in tests without leaking state between tests. Signed-off-by: Alex Konradi <[email protected]>

Move Network::Utility proto address parsing

0a7f987

Now implemented by the Network::Address::ResolverFactory registry and appropriate interfaces. Signed-off-by: Alex Konradi <[email protected]>

htuch reviewed Oct 10, 2017

View reviewed changes

akonradi added 5 commits October 11, 2017 14:16

Make Address::Resolver take a proto to resolve

c0cac0b

Signed-off-by: Alex Konradi <[email protected]>

Fix documentation in comments.

9451f07

Signed-off-by: Alex Konradi <[email protected]>

Change ResolverFactory::create() to createResolver

a633aaf

Signed-off-by: Alex Konradi <[email protected]>

Add tests to cover address resolution error cases

134034c

Signed-off-by: Alex Konradi <[email protected]>

Check exception message for IP with named port

de0ee8e

Signed-off-by: Alex Konradi <[email protected]>

htuch previously approved these changes Oct 12, 2017

View reviewed changes

mattklein123 reviewed Oct 12, 2017

View reviewed changes

akonradi added 3 commits October 12, 2017 10:06

Make resolver name method const

aa73c50

Signed-off-by: Alex Konradi <[email protected]>

Remove ResolverFactory from address resolution

0236b12

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 <[email protected]>

Use EXPECT_THROW_WITH_MESSAGE for resolver tests

915c7d3

Signed-off-by: Alex Konradi <[email protected]>

akonradi dismissed htuch’s stale review via 915c7d3 October 12, 2017 14:10

htuch reviewed Oct 12, 2017

View reviewed changes

mrice32 reviewed Oct 12, 2017

View reviewed changes

akonradi added 2 commits October 13, 2017 09:34

Add missing 'override's to Address::Resolver impls

594721d

Fixes broken CI builds Signed-off-by: Alex Konradi <[email protected]>

Add TODO for registry injection once that's done

e6e8cc9

Signed-off-by: Alex Konradi <[email protected]>

htuch reviewed Oct 13, 2017

View reviewed changes

Make test TODO more detailed

16d8e5d

Signed-off-by: Alex Konradi <[email protected]>

htuch approved these changes Oct 13, 2017

View reviewed changes

htuch merged commit c85a140 into envoyproxy:master Oct 13, 2017

akonradi deleted the v2-address-resolution branch October 16, 2017 19:51

hennna mentioned this pull request Apr 10, 2018

address: use resolveProtoAddress to resolve listener addresses #3039

Merged

kylebevans mentioned this pull request Dec 24, 2020

SRV service discovery support #125

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

config: v2 Address resolution using named resolver #1835

config: v2 Address resolution using named resolver #1835

akonradi commented Oct 10, 2017

htuch left a comment

htuch Oct 10, 2017

htuch Oct 10, 2017

htuch Oct 10, 2017

htuch Oct 10, 2017

htuch Oct 10, 2017

htuch Oct 10, 2017

htuch Oct 10, 2017

htuch Oct 10, 2017

htuch Oct 12, 2017

mattklein123 Oct 12, 2017

akonradi Oct 12, 2017

htuch left a comment

htuch Oct 12, 2017

akonradi Oct 12, 2017

mrice32 Oct 12, 2017 •

edited

Loading

htuch Oct 12, 2017

akonradi Oct 12, 2017

htuch Oct 12, 2017

alyssawilk Oct 12, 2017

akonradi Oct 12, 2017

htuch Oct 12, 2017

htuch Oct 13, 2017

htuch commented Oct 13, 2017

config: v2 Address resolution using named resolver #1835

config: v2 Address resolution using named resolver #1835

Conversation

akonradi commented Oct 10, 2017

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrice32 Oct 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch commented Oct 13, 2017

mrice32 Oct 12, 2017 •

edited

Loading