listener: Use v4-mapped addressing consistently. by jrajahalme · Pull Request #2572 · envoyproxy/envoy

jrajahalme · 2018-02-09T21:42:12Z

Currently the local address of a new connection is mapped to an IPv4
address instance if the listening socket is not in V6ONLY mode and the
local address of the socket is an IPv4-mapped IPv6 address. This leads
to connections having an (IPv4-mapped) IPv6 instance for the remote
address and an IPv4 instance for the local address, which is
confusing. Fix this by allowing the remote address to be v4-mapped if the
local address is an IPv4 address. Or conversely, set the 'v6only'
parameter to 'true' if the local address is an IPv6 address instance.

Fixes: 3b3c009 ("address_impl: Return an Ipv4Instance for v4-mapped v6 addresses. (#2309)")
Signed-off-by: Jarno Rajahalme jarno@covalent.io
Risk Level: Low

mattklein123

Can there be some test that would have failed before this change?

Fail if remote and local addresses of a new server connection are of different address instance types. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

Currently the local address of a new connection is mapped to an IPv4 address instance if the listening socket is not in V6ONLY mode and the local address of the socket is an IPv4-mapped IPv6 address. This leads to connections having an (IPv4-mapped) IPv6 instance for the remote address and an IPv4 instance for the local address, which is confusing. Fix this by allowing the remote address to be v4-mapped if the local address is an IPv4 address. Or conversely, set the 'v6only' parameter to 'true' if the local address is an IPv6 address instance. Fixes: 3b3c009 ("address_impl: Return an Ipv4Instance for v4-mapped v6 addresses. (envoyproxy#2309)") Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

jrajahalme · 2018-02-10T02:18:23Z

Added a test case that fails without the fix.

htuch · 2018-02-11T12:41:35Z

@zuercher can you take a first pass on this? Thanks.

zuercher · 2018-02-12T17:58:39Z

source/common/network/utility.cc

  // Initialized on first call in a thread-safe manner.
-  static Address::InstanceConstSharedPtr any(new Address::Ipv6Instance(static_cast<uint32_t>(0)));
-  return any;
+  static sockaddr_in6 v6any = v6any_();


I think you can just inline this call.

Can you show how would I inline a static initializer for sockaddr_in6, which is different on different OSes and when C++ does not support designated initializers?

Or did you mean adding the inline keyword to v6any_(), making this a call to a static inline function?

zuercher · 2018-02-12T18:14:30Z

test/test_common/network_utility.h

 */
-Address::InstanceConstSharedPtr getAnyAddress(const Address::IpVersion version);
+Address::InstanceConstSharedPtr getAnyAddress(const Address::IpVersion version,
+                                              bool v4_compat = false);


Document v4_compat, please.

zuercher · 2018-02-12T18:15:11Z

test/common/network/listener_impl_test.cc

+TEST_P(ListenerImplTest, WildcardListenerIpv4Compat) {
+  Stats::IsolatedStoreImpl stats_store;
+  Event::DispatcherImpl dispatcher;
+  Network::TcpListenSocket socket(Network::Test::getAnyAddress(version_, true), true);


I'd like a comment here (or at the top of the test) briefly explaining the circumstances this is testing.

It's confusing to me since this is the only place that leads to invoking Network::Utility::getAnyIpv6Address(false), so is this code testing a case that cannot occur outside of tests?

This test mocks a case where a listener would be configured with an IPv6 any address and have it's socket_address configured with ipv4_compat = true. There are a couple of tests that do that, but in those the socket address comes from the listener configuraion, so they do not invoke Network::Utility::getAnyIpv6Address() at all. The effect is the same, though, when the listener is configured with the IPv6 ANY address ([::]).

zuercher · 2018-02-12T18:26:03Z

source/common/network/utility.h

   *         IPv6 address are to be accepted.
   */
-  static Address::InstanceConstSharedPtr getIpv6AnyAddress();
+  static Address::InstanceConstSharedPtr getIpv6AnyAddress(bool v6only = true);


This is only ever called with v6only = false in a test (that I can see). Perhaps the v6only = false behavior should be handled in Network::Test::getAnyAddress?

I'll do that.

Revert the changes to source/common/network/utility.[h|cc], as those were only needed for testing. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

zuercher

Looks good. Just a couple nits and a question. Thanks for working on this.

zuercher · 2018-02-13T20:43:31Z

test/common/network/listener_impl_test.cc


+// Test for the correct behavior when a listener is configured with an ANY address that allows
+// receiving IPv4 connections on an IPv6 socket. In this case the address instances of both
+// local and remove addresses of the connection should be IPv4 instances, as the connection really


nit: remote not remove

zuercher · 2018-02-13T20:44:53Z

test/test_common/network_utility.cc

+// There is no portable way to initialize sockaddr_in6 with a static initializer, do it with a
+// helper function instead
+static sockaddr_in6& v6any_() {
+  static sockaddr_in6 v6any = {};


What do you think about dropping the static modifier for this variable? (Since you rely on the function only being called once to initialize the static variable in getAnyAddress.)

Done, had to change the function to return the struct to not return a reference to the stack.

zuercher · 2018-02-13T21:00:57Z

test/test_common/network_utility.cc

  }
+  if (v4_compat) {
+    static sockaddr_in6 v6any = v6any_();
+    static Address::InstanceConstSharedPtr any(new Address::Ipv6Instance(v6any, false));


I would just call v6any_() on this line instead of initializing an extra static var. TIOLI.

Done, thanks!

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

jrajahalme

Thanks for the review!

jrajahalme · 2018-02-13T23:13:48Z

test/test_common/network_utility.cc

+// There is no portable way to initialize sockaddr_in6 with a static initializer, do it with a
+// helper function instead
+static sockaddr_in6& v6any_() {
+  static sockaddr_in6 v6any = {};


Done, had to change the function to return the struct to not return a reference to the stack.

jrajahalme · 2018-02-13T23:14:16Z

test/test_common/network_utility.cc

  }
+  if (v4_compat) {
+    static sockaddr_in6 v6any = v6any_();
+    static Address::InstanceConstSharedPtr any(new Address::Ipv6Instance(v6any, false));


Done, thanks!

jrajahalme · 2018-02-13T23:14:31Z

test/common/network/listener_impl_test.cc


+// Test for the correct behavior when a listener is configured with an ANY address that allows
+// receiving IPv4 connections on an IPv6 socket. In this case the address instances of both
+// local and remove addresses of the connection should be IPv4 instances, as the connection really


zuercher

This looks good to me.

@dnoe can you assign a senior maintainer since Matt is out?

zuercher · 2018-02-14T18:12:30Z

Sorry, I read the wrong list of names. @htuch I think this is ready for you to look at.

htuch

LGTM, just some small cleanup asks.

htuch · 2018-02-15T03:29:37Z

source/common/network/listener_impl.cc

+  // Get the local address from the new socket if the listener is listening on IP ANY
+  // (e.g., 0.0.0.0 for IPv4) (local_address_ is nullptr in this case).
+  const Address::InstanceConstSharedPtr& local_address =
+      !listener->local_address_ ? listener->getLocalAddress(fd) : listener->local_address_;


Tiny nit: prefer to negate the conditional here, easier to parse.

htuch · 2018-02-15T03:40:23Z

test/test_common/network_utility.cc

-Address::InstanceConstSharedPtr getAnyAddress(const Address::IpVersion version) {
+// There is no portable way to initialize sockaddr_in6 with a static initializer, do it with a
+// helper function instead
+static sockaddr_in6 v6any_() {


Envoy convention is to put this in the anonymous namespace rather than mark it static. Might be clearer to rename it to sockaddrIn6Any() to match Envoy convention.

htuch · 2018-02-15T03:43:00Z

test/test_common/network_utility.cc

    return Network::Utility::getIpv4AnyAddress();
  }
+  if (v4_compat) {
+    static Address::InstanceConstSharedPtr any(new Address::Ipv6Instance(v6any_(), false));


Can you add some internal comments here explaining how this (maybe using ASCII address notation) differs from Network::Utility::getIpv6AnyAddress() below?

Added this:

// This will return an IPv6 ANY address ("[::]:0") like the getIpv6AnyAddress() below, but // with the internal 'v6only' member set to false. This will allow a socket created from this // address to accept IPv4 connections. IPv4 connections received on IPv6 sockets will have // Ipv4-mapped IPv6 addresses, which we will then internally interpret as IPv4 addresses so // that, for example, access logging will show IPv4 address format for IPv4 connections even // if they were received on an IPv6 socket.

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

jrajahalme

Thanks for the review!

jrajahalme · 2018-02-15T18:13:21Z

source/common/network/listener_impl.cc

+  // Get the local address from the new socket if the listener is listening on IP ANY
+  // (e.g., 0.0.0.0 for IPv4) (local_address_ is nullptr in this case).
+  const Address::InstanceConstSharedPtr& local_address =
+      !listener->local_address_ ? listener->getLocalAddress(fd) : listener->local_address_;


jrajahalme · 2018-02-15T18:22:27Z

test/test_common/network_utility.cc

-Address::InstanceConstSharedPtr getAnyAddress(const Address::IpVersion version) {
+// There is no portable way to initialize sockaddr_in6 with a static initializer, do it with a
+// helper function instead
+static sockaddr_in6 v6any_() {


jrajahalme · 2018-02-15T18:32:45Z

test/test_common/network_utility.cc

    return Network::Utility::getIpv4AnyAddress();
  }
+  if (v4_compat) {
+    static Address::InstanceConstSharedPtr any(new Address::Ipv6Instance(v6any_(), false));


Added this:

// This will return an IPv6 ANY address ("[::]:0") like the getIpv6AnyAddress() below, but // with the internal 'v6only' member set to false. This will allow a socket created from this // address to accept IPv4 connections. IPv4 connections received on IPv6 sockets will have // Ipv4-mapped IPv6 addresses, which we will then internally interpret as IPv4 addresses so // that, for example, access logging will show IPv4 address format for IPv4 connections even // if they were received on an IPv6 socket.

htuch

LGTM, just need to fix_format.

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

jrajahalme · 2018-02-15T19:41:01Z

Format fixed, sorry for the churn.

jrajahalme · 2018-02-15T19:42:15Z

No idea why it shows I've dismissed your (stale) review. I just pushed the fix for the line you commented on.

dnoe · 2018-02-15T19:54:24Z

Yeah, that's normal GitHub :) it shows dismissed stale review, because you updated a line that had a review comment so it needs to be reviewed again ¯\_(ツ)_/¯

…2572)" This reverts commit 508c7ba.

* bazel: update rules_xcodeproj to 0.8.0 * Add Hello World Objective-C example apps to the generated project * The internal unit test labels are no longer required * There's now an easier way to add device and simulator targets * We can remove `archived_bundles_allowed` which was deprecated Release info: https://github.com/buildbuddy-io/rules_xcodeproj/releases/tag/0.8.0 Signed-off-by: JP Simard <jp@jpsim.com>

mattklein123 reviewed Feb 9, 2018

View reviewed changes

jrajahalme added 2 commits February 9, 2018 17:44

listener: Test for v4-mapped addressing consistency.

0816aa3

Fail if remote and local addresses of a new server connection are of different address instance types. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

jrajahalme force-pushed the fix-v4-compat-remote branch from 17b940d to 59b1077 Compare February 10, 2018 02:16

htuch assigned htuch and zuercher Feb 11, 2018

zuercher reviewed Feb 12, 2018

View reviewed changes

Review update.

3d8e281

Revert the changes to source/common/network/utility.[h|cc], as those were only needed for testing. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

zuercher reviewed Feb 13, 2018

View reviewed changes

Review fixes.

c23d305

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

jrajahalme force-pushed the fix-v4-compat-remote branch from 6a59c2a to c23d305 Compare February 13, 2018 23:13

jrajahalme commented Feb 13, 2018

View reviewed changes

zuercher previously approved these changes Feb 14, 2018

View reviewed changes

htuch suggested changes Feb 15, 2018

View reviewed changes

Review fixes.

659a3aa

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

jrajahalme dismissed zuercher’s stale review via 659a3aa February 15, 2018 18:36

jrajahalme commented Feb 15, 2018

View reviewed changes

htuch previously approved these changes Feb 15, 2018

View reviewed changes

Fix format.

0acec79

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

jrajahalme dismissed htuch’s stale review via 0acec79 February 15, 2018 19:37

htuch approved these changes Feb 15, 2018

View reviewed changes

htuch merged commit 508c7ba into envoyproxy:master Feb 15, 2018

lita pushed a commit to lita/envoy that referenced this pull request Feb 15, 2018

Revert "listener: Use v4-mapped addressing consistently. (envoyproxy#…

a1ebb80

…2572)" This reverts commit 508c7ba.

Conversation

jrajahalme commented Feb 9, 2018

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

jrajahalme commented Feb 10, 2018

Uh oh!

htuch commented Feb 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zuercher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrajahalme left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zuercher left a comment

Choose a reason for hiding this comment

Uh oh!

zuercher commented Feb 14, 2018

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrajahalme left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

dnoe commented Feb 15, 2018 •

edited

Loading