ipv6: use absl:uint128 instead of array of uint_8 for address return. by ccaraman · Pull Request #2431 · envoyproxy/envoy

ccaraman · 2018-01-23T06:21:08Z

Description: Change IPv6 to return absl::uint128 instead of array of uint8_t. Equivalent ntohl and htonl have been implemented in Utility.

Risk Level: Low.

Testing: Existing Ipv6 tests and extended utility unit tests to cover new method.

Signed-off-by: Constance Caramanolis <ccaramanolis@lyft.com>

mattklein123

cool, few comments

mattklein123 · 2018-01-23T17:48:37Z

-  std::copy(std::begin(address_.sin6_addr.s6_addr), std::end(address_.sin6_addr.s6_addr),
-            std::begin(result));
+absl::uint128 Ipv6Instance::Ipv6Helper::address() const {
+  absl::uint128 result{0};


Whether the type is intrinsic or not, you should be able to just memcpy here.

Updated all call paths to use memcpy instead.

mattklein123 · 2018-01-23T17:50:13Z

+
+absl::uint128 Utility::Ip6htonl(const absl::uint128& address) { return flipOrder(address); }
+
+std::array<uint8_t, 16> Utility::getArrayRepresentation(const absl::uint128& address) {


I think this method is not needed. memcpy should work where this is used.

Updated all call paths to use memcpy.

mattklein123 · 2018-01-23T17:51:18Z

  return false;
 }

+absl::uint128 Utility::Ip6ntohl(const absl::uint128& address) { return flipOrder(address); }


I don't think it makes sense that both of these functions do the same thing unless you actually check for local endianness somewhere. I would either do that, or use a single function, assert endianness somehow, and add a TODO.

Doesn't this break if Envoy is running on a big endian system?

This has been updated to have an assert on little endianess and a todo has been added to support big endian.

dnoe · 2018-01-23T20:47:01Z

-    }
+    absl::uint128 ip6 = Utility::Ip6ntohl(address->ip()->ipv6()->address());
+    // The maximum number stored in absl::uint128 has every bit set to 1.
+    absl::uint128 max_int = absl::MakeUint128(std::numeric_limits<uint64_t>::max(),


Can you use absl::Uint128Max()?

When I try using it, I get the following: error: 'Uint128Max' is not a member of 'absl' absl::uint128 max_int = absl::Uint128Max(); ?

Figured it out. There was an update 11 days ago to change kuint128max to Uint128Max(). I locally have the older absl.

dnoe · 2018-01-23T20:50:15Z

+    max_int <<= (128 - length);
+    ip6 &= max_int;
+
+    std::array<uint8_t, 16> ip6_array = Utility::getArrayRepresentation(Utility::Ip6htonl(ip6));


Can it be const std::array

dnoe · 2018-01-23T20:54:40Z

  return false;
 }

+absl::uint128 Utility::Ip6ntohl(const absl::uint128& address) { return flipOrder(address); }


Doesn't this break if Envoy is running on a big endian system?

dnoe · 2018-01-23T21:05:41Z

+  /**
+   * Converts IPv6 absl::uint128 in network byte order to host byte order.
+   * @param address supplies the IPv6 address in network byte order.
+   * @return IPv6 in host byte order.


nit: return type in @return (here and the other functions in this file)

dnoe · 2018-01-23T21:14:18Z

+    EXPECT_EQ(expected_result, Utility::getArrayRepresentation(address.ip()->ipv6()->address()));
+  }
+}
+


It's worth thinking about how this test will work (or won't work) on a big endian machine.

I've added an assert to only work on big endian machines.

Signed-off-by: Constance Caramanolis <ccaramanolis@lyft.com>

mattklein123 · 2018-01-23T23:31:34Z

+}
+
+bool Utility::isLittleEndian() {
+  int num = 1;


absl already has a define for this which I think we implicitly include. Can we just use that instead of reimplementing the logic?

mattklein123 · 2018-01-23T23:32:48Z

-            std::begin(result));
+absl::uint128 Ipv6Instance::Ipv6Helper::address() const {
+  absl::uint128 result{0};
+  memcpy(&result, &address_.sin6_addr.s6_addr, 16);


I would add a static_assert everywhere you do memcpy to make sure sizeof(absl::uint128) is what you expect, and then potentially use sizeof(absl::uint128) instead of 16.

Signed-off-by: Constance Caramanolis <ccaramanolis@lyft.com>

mattklein123 · 2018-01-24T01:29:40Z


+absl::uint128 Utility::Ip6ntohl(const absl::uint128& address) {
+  // TODO(ccaraman): Support Ip6ntohl for big-endian.
+  ASSERT(ABSL_IS_LITTLE_ENDIAN);


I would make both this assert and the other similar ones static_assert. We should just prevent the code from compiling on big endian systems until we implement this. With static_assert, the compilation message will be very clear also, much like #error

Signed-off-by: Constance Caramanolis <ccaramanolis@lyft.com>

alyssawilk

Thank you so much for doing this as its own PR with preexisting tests - it's much easier to trust the code when I don't feel responsible for catching bitmath bugs by eye :-)

alyssawilk · 2018-01-24T15:41:57Z

+                                              std::numeric_limits<uint64_t>::max());
+    EXPECT_EQ(max_int, address.ip()->ipv6()->address());
+    EXPECT_EQ(max_int, Utility::Ip6ntohl(address.ip()->ipv6()->address()));
+  }


Optional but I'm a big fan of RNG code for tests like this. What would you think of setting low and high bytes based on TestRandomGenerator and making sure if you ntohl and htonl you end up with the original?

Yes, that sounds good. I'll update the tests.

alyssawilk · 2018-01-24T15:45:48Z

@@ -200,21 +185,17 @@ InstanceConstSharedPtr CidrRange::truncateIpAddressAndLength(InstanceConstShared
    sockaddr_in6 sa6;


Should we update the comment "but we don't have a uint128_t available."?

Yes, I'll remove that.

alyssawilk · 2018-01-24T15:48:51Z

+    absl::uint128 max_int = absl::Uint128Max();
+    // Shifting the value to the left set all bits between 128-length and 128 to zero.
+    max_int <<= (128 - length);
+    ip6 &= max_int;


s/max_int/mask/?

alyssawilk · 2018-01-24T15:55:53Z

 }

+absl::uint128 Utility::Ip6ntohl(const absl::uint128& address) {
+  // TODO(ccaraman): Support Ip6ntohl for big-endian.


This one is up to Matt and you but I wouldn't even TODO big-endian support unless we have a big-endian machine to test on just because I'd be concerned we'd screw something up somewhere else without being able to test.

I was thinking we would be testing this on Google import but looks like that's specifically the Little Endian flavor of PPC (ppc64le). Some kind of compile time assert is probably sufficient to avoid surprises if someone manages to get Envoy build on a BE machine.

No real preference on keeping the TODO or removing it. Will leave it up to you all. I basically figured that with the static_assert if someone cares they can come along and remove the static assert, add appropriate code, and test it.

I'm going to leave the TODO. I think it helps future contributors know that this is something that Envoy wants to support at a future date.

alyssawilk · 2018-01-24T16:12:09Z

+  for (int i = 0; i < 16; i++) {
+    result <<= 8;
+    result |= (data & 0x000000000000000000000000000000FF);
+    data >>= 8;


optional : internally I think we roughly return uint128(htonl(input.low), htonl(input.high)) which obviously only works if htonl is flipping bits. If this is only going to be called for little endian switching might save some operations but I'm also fine leaving as is since if we add big endian support we'd need rename it something awful like flipOrderForLittleEndian to make it clear :-P

I'm going to leave it as is.

Signed-off-by: Constance Caramanolis <ccaramanolis@lyft.com>

… return. (#2431)" This reverts commit 4264986.

This patch resolves the TODO from envoyproxy#2431. Tests pass on a big-endian machine (IBM Z). Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>

This patch resolves the TODO from #2431. Tests pass on a big-endian machine (IBM Z). Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>

Signed-off-by: Douglas Reid <douglas-reid@users.noreply.github.com>

This patch resolves the TODO from envoyproxy/envoy#2431. Tests pass on a big-endian machine (IBM Z). Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>

ccaraman added 4 commits January 22, 2018 21:41

Migrate IPv6 to use absl::uint128

c8f7392

Signed-off-by: Constance Caramanolis <ccaramanolis@lyft.com>

Merge branch 'master' into absl_uint128

be8b824

Signed-off-by: Constance Caramanolis <ccaramanolis@lyft.com>

Clarify wording.

20b128b

Signed-off-by: Constance Caramanolis <ccaramanolis@lyft.com>

Move things around.

6017aae

Signed-off-by: Constance Caramanolis <ccaramanolis@lyft.com>

htuch assigned dnoe and alyssawilk Jan 23, 2018

mattklein123 reviewed Jan 23, 2018

View reviewed changes

dnoe reviewed Jan 23, 2018

View reviewed changes

ccaraman added 2 commits January 23, 2018 13:57

Fix byte order swapping to check for endianess.

9395a2b

Signed-off-by: Constance Caramanolis <ccaramanolis@lyft.com>

Update sha for Absl dependencies and add more comments.

a29d2bf

Signed-off-by: Constance Caramanolis <ccaramanolis@lyft.com>

mattklein123 reviewed Jan 23, 2018

View reviewed changes

ccaraman added 2 commits January 23, 2018 16:58

Update asserts.

1b90196

Signed-off-by: Constance Caramanolis <ccaramanolis@lyft.com>

fix format

39678a2

Signed-off-by: Constance Caramanolis <ccaramanolis@lyft.com>

mattklein123 reviewed Jan 24, 2018

View reviewed changes

Use static assert for endianess check.

7c8d0bc

Signed-off-by: Constance Caramanolis <ccaramanolis@lyft.com>

alyssawilk reviewed Jan 24, 2018

View reviewed changes

Use random number generator test.

1ff7611

Signed-off-by: Constance Caramanolis <ccaramanolis@lyft.com>

mattklein123 approved these changes Jan 26, 2018

View reviewed changes

dnoe approved these changes Jan 26, 2018

View reviewed changes

mattklein123 merged commit 4264986 into master Jan 26, 2018

mattklein123 deleted the absl_uint128 branch January 26, 2018 16:21

mattklein123 added a commit that referenced this pull request Jan 26, 2018

Revert "ipv6: use absl:uint128 instead of array of uint_8 for address…

983bb66

… return. (#2431)" This reverts commit 4264986.

iii-i added a commit to iii-i/envoy that referenced this pull request Mar 4, 2020

common: add big-endian support to Ip6ntohl and Ip6htonl

13dc082

This patch resolves the TODO from envoyproxy#2431. Tests pass on a big-endian machine (IBM Z). Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>

iii-i mentioned this pull request Mar 4, 2020

common: add big-endian support to Ip6ntohl and Ip6htonl #10250

Merged

mattklein123 pushed a commit that referenced this pull request Mar 4, 2020

common: add big-endian support to Ip6ntohl and Ip6htonl (#10250)

a2272df

This patch resolves the TODO from #2431. Tests pass on a big-endian machine (IBM Z). Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>

Shikugawa pushed a commit to Shikugawa/envoy that referenced this pull request Mar 28, 2020

feat(stats): add istio_build metric to prom extension (envoyproxy#2431)

d5701c0

Signed-off-by: Douglas Reid <douglas-reid@users.noreply.github.com>


		absl::uint128 Utility::Ip6htonl(const absl::uint128& address) { return flipOrder(address); }

		std::array<uint8_t, 16> Utility::getArrayRepresentation(const absl::uint128& address) {

		@@ -200,21 +185,17 @@ InstanceConstSharedPtr CidrRange::truncateIpAddressAndLength(InstanceConstShared
		sockaddr_in6 sa6;

Conversation

ccaraman commented Jan 23, 2018

Uh oh!

mattklein123 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!

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 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants