utility: Fix address comparisons in isLocalConnection(), add tests. by jrajahalme · Pull Request #5373 · envoyproxy/envoy

jrajahalme · 2018-12-20T21:04:07Z

Add test case for LocalConnection test where local and remote
addresses have the same IP address. Fix the test code to actually use
the intended local address. Fix the utility implementation to compare
the actual addresses rather than the pointers to them.

Address comparisons in Utility::isLocalConnection() should be performed on
the IP addresses, ignoring any port numbers. Skip the temporary
Instance in the comparison loop. Add test cases with port numbers.

Fixes: #4682
Signed-off-by: Jarno Rajahalme jarno@covalent.io

Add test case for LocalConnection test where local and remote addresses have the same IP address. Fix the test code to actually use the intended local address. Fix the utility implementation to compare the actual addresses rather than the pointers to them. Fixes: envoyproxy#4682 Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

Address comparisons in Utility::isLocalConnection() should be performed on the IP addresses, ignoring any port numbers. Skip the temporary Instance in the comparison loop. Add test cases with port numbers. Fixes: envoyproxy#4682 Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

dnoe · 2018-12-20T21:11:04Z

This is a lot to read in a conditional statement. Is there a way you can add the needed comparison into an equality operator on the appropriate object to make this statement look a lot clearer? Then you can hide the complexity of this and v4 vs. v6 in the right place.

Envoy does not have a generic address type for an IP address without a port number. Typically an IP address type with a zero port number is used to approximate it, but real connection addresses always have non-zero port numbers, so given the current type system this is the only way to implement the comparison here. We could move this to an utility function (e.g., isSameIPAddress(addr1, addr2)) to make it easier to read, though.

Ah, that's right. I think it would be best to use a utility function, then.

Add a new utility function to compare if two address instances are the same IP address, ignoring the port numbers. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

dnoe · 2018-12-20T21:38:19Z

+    return false;
+  }
+
+  if (address1.ip()->version() == Address::IpVersion::v4) {


This might look less verbose as a switch statement.

dnoe · 2018-12-20T21:40:11Z


+bool Utility::isSameIPAddress(const Address::Instance& address1,
+                              const Address::Instance& address2) {
+  if (address1.type() != Address::Type::Ip ||


maybe this should be an assert?

dnoe · 2018-12-20T21:41:50Z

+        }
+      } else {
+        const auto* addr = reinterpret_cast<const struct sockaddr_in6*>(ifa->ifa_addr);
+        absl::uint128 v6addr = remote_address->ip()->ipv6()->address();


nit: const absl::uint128

might be good to call this remote_v6addr for maximum clarity, too.

dnoe · 2018-12-20T21:45:58Z

+          return true;
+        }
+      } else {
+        const auto* addr = reinterpret_cast<const struct sockaddr_in6*>(ifa->ifa_addr);


local_addr?

Used if_addr instead.

dnoe · 2018-12-20T21:46:10Z

-      if (remote_address == local_address) {
-        return true;
+      if (af_look_up == AF_INET) {
+        const auto* addr = reinterpret_cast<const struct sockaddr_in*>(ifa->ifa_addr);


local_addr?

Restore the order of comparisons to pass tests that count the number of calls to localAddress(). Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

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

PiotrSikora · 2018-12-21T00:47:58Z

-  if (remote_address->type() == Envoy::Network::Address::Type::Pipe ||
-      remote_address == socket.localAddress() || isLoopbackAddress(*remote_address)) {
+  if (remote_address->type() != Envoy::Network::Address::Type::Ip ||
+      isSameIPAddress(*remote_address, *socket.localAddress()) ||


Sorry, but could you explain why doesn't this work with ==?

Also, can't we fix the operator== instead of adding an utility function, since presumably this is broken in other places as well?

remote_address (as well as socket.localAddress are shared pointers (InstanceConstSharedPtr). Equality operator of pointers compares the pointers themselves, rather than the objects they point to.

Secondly, even if the addresses were compared, this comparison must ignore the port numbers that are embedded in every Envoy Ip address instance.

operator== is correct for comparing the whole IP address + port combination. It also works in many places where it is known that the port numbers of both addresses are the same (e.g., zero).

You may want to review the two commits separately, as they address the above issues one at a time.

Ah, I completely missed the port part, thanks!

alyssawilk · 2018-12-21T13:23:00Z

Dan: can you assign a senior reviewer when you're done on your pass?

PiotrSikora · 2018-12-27T03:02:58Z

-
-      if (remote_address == local_address) {
-        return true;
+      if (af_look_up == AF_INET) {


Could you move this comparison into isSameIPAddress(const Address::Instance& address1, const struct sockaddr *address2)?

isSameIPAddress() already checks the IP version, but here, within a loop, we don't want to create temporary IP Address Instances so we are open-coding the comparison.

However, upon a second look I removed the assert in isSameIPAddress() so that it is safe to call on any type of an Address.

Note that I'm suggesting extracting this to isSameIPAddress() with different arguments, so you'd invoke it using isSameIPAddress(*remote_address, ifa->ifa_addr) without creating temporary Address::Instance object (which I agree would be an overkill).

Basically, extracting this code for readability, since compiler is going to inline it here anyway.

I'm fine with you leaving it here, if feel strongly about it, though.

Friendly ping on what you prefer to do here (leave it as-is, in which case it's ready to be merged, or extracting it to isSameIPAddress(const Address::Instance&, const struct sockaddr *)), to make sure that we're not stuck waiting on each other.

PiotrSikora · 2018-12-27T03:14:29Z

+        }
+      } else {
+        const auto* if_addr = reinterpret_cast<const struct sockaddr_in6*>(ifa->ifa_addr);
+        const absl::uint128 remote_v6addr = remote_address->ip()->ipv6()->address();


Please make sure to not overread the memory:

static_assert(sizeof(absl::uint128) == sizeof(if_addr->sin6_addr.s6_addr), "sizeof(absl::uint128) != sizeof(if_addr->sin6_addr.s6_addr)");

PiotrSikora · 2018-12-27T03:14:41Z


  testing::NiceMock<Network::MockConnectionSocket> socket;

-  EXPECT_CALL(socket, remoteAddress()).WillRepeatedly(testing::ReturnRef(local_addr));


PiotrSikora · 2018-12-27T03:15:58Z

+  local_addr.reset(new Network::Address::Ipv6Instance("fabc::42", 80));
+  remote_addr.reset(new Network::Address::Ipv6Instance(
+      Utility::getLocalAddress(Address::IpVersion::v6)->ip()->addressAsString(), 23413));
+  EXPECT_TRUE(Utility::isLocalConnection(socket));


Could you add a test comparing IPv4 and IPv6 addresses? It looks that we're missing it.

PiotrSikora · 2018-12-27T03:17:00Z

-  if (remote_address->type() == Envoy::Network::Address::Type::Pipe ||
-      remote_address == socket.localAddress() || isLoopbackAddress(*remote_address)) {
+  if (remote_address->type() != Envoy::Network::Address::Type::Ip ||
+      isSameIPAddress(*remote_address, *socket.localAddress()) ||


Ah, I completely missed the port part, thanks!

Make isSameIPAddress() safe to call on any type of Address Instances. Add an assert for IPv6 address size. Add a case with mixed IPv4/IPv6 addresses to the LocalConnection Test. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

jrajahalme · 2019-01-02T18:50:58Z

@PiotrSikora Addressed your concerns, can you have another look?

stale · 2019-01-12T03:38:28Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

stale · 2019-01-19T03:50:25Z

This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

PiotrSikora · 2019-01-19T05:08:52Z

Not stale, pending response from @jrajahalme.

jrajahalme added 2 commits December 20, 2018 13:08

dnoe reviewed Dec 20, 2018

View reviewed changes

jrajahalme force-pushed the local-connection-utility branch from 3f41d3d to d8337e7 Compare December 20, 2018 21:11

utility: Add isSameIPAddress()

49716d8

Add a new utility function to compare if two address instances are the same IP address, ignoring the port numbers. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

dnoe reviewed Dec 20, 2018

View reviewed changes

jrajahalme added 2 commits December 20, 2018 14:24

utility: Restore comparison order to pass tests.

dd3f76f

Restore the order of comparisons to pass tests that count the number of calls to localAddress(). Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

utility: Address comments.

45bb0e7

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

PiotrSikora suggested changes Dec 21, 2018

View reviewed changes

alyssawilk assigned dnoe Dec 21, 2018

PiotrSikora suggested changes Dec 27, 2018

View reviewed changes

utility: Add IPv4/v6 test for isLocalConnection()

53627ae

Make isSameIPAddress() safe to call on any type of Address Instances. Add an assert for IPv6 address size. Add a case with mixed IPv4/IPv6 addresses to the LocalConnection Test. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

stale Bot added the stale stalebot believes this issue/PR has not been touched recently label Jan 12, 2019

stale Bot closed this Jan 19, 2019


		testing::NiceMock<Network::MockConnectionSocket> socket;

		EXPECT_CALL(socket, remoteAddress()).WillRepeatedly(testing::ReturnRef(local_addr));

Conversation

jrajahalme commented Dec 20, 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alyssawilk commented Dec 21, 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!

Choose a reason for hiding this comment

Uh oh!

jrajahalme commented Jan 2, 2019

Uh oh!

stale Bot commented Jan 12, 2019

Uh oh!

stale Bot commented Jan 19, 2019

Uh oh!

PiotrSikora commented Jan 19, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!