changing internal representation of ip addresses for reals from string to binary #37

tehnerd · 2019-08-02T15:09:51Z

as stated. currently katran is exposed to capslock like issues.
e.g. we could add same v6 address multiple times and that would pass all the tests
(e.g. real fc00::1 fc00:0::1 fc00:0:0::1 etc).
current behavior wont break anything, as in forwarding plane everything
is in binary anyway, but we would consume more resources than needed.
(e.g. real for regular vip could in any format, but quic's real
in exploded)

tested by:

it compiles (c)
unittests
katran_tester

this is taking care only about real's addresses, struct VipKey still has
vip's address as a string. so it would take more work to modify that as well
(as VipKey is a public API. current changes wont break anything external
as API's return values have not changed. only internal's ones)
if i run clang-format on KatranLb.h/cpp it makes some changes
in unrelated lines (most likely either format from 8.0 has some modifications
or it was not run on that files for a while). what would you prefer?
would you accept diff w/ some unrelated changed caused by clang format?

udippant · 2019-08-02T16:19:26Z

Lets not do the clang format. I'll do it separately.
(I'll see if I can find can import the PR to phabricator internally and then comment on this PR)

udippant · 2019-08-05T14:45:29Z

I have put this diff up for internal review

yangchi · 2019-08-07T17:04:39Z

katran/lib/IpHelpers.h

@@ -42,10 +43,13 @@ class IpHelpers {
   * of beaddr structure. this function could throw, if given string is not


This sentence tho..

yangchi · 2019-08-07T17:05:10Z

katran/lib/IpHelpers.cpp

-    const std::string& addr,
-    bool bigendian) {
+struct beaddr IpHelpers::parseAddrToBe(const std::string &addr,
+                                       bool bigendian) {
  auto ipaddr = folly::IPAddress(addr);


Save a copy:
return parseAddrToBe(folly::IPAddress(addr), bigendian);

yangchi · 2019-08-07T17:09:11Z

katran/lib/IpHelpers.h

-      const std::string& addr,
-      bool bigendian = true);
-  static struct beaddr parseAddrToInt(const std::string& addr);
+  static struct beaddr parseAddrToBe(const std::string &addr,


I think all 4 of them can be free functions instead of static functions on the class.

And the two with bigendian params can be in a anonymous namespace so they are impl detail only local to this file.

non static non class functions would break public API - do you guys want to do this? (e.g. if anyone already using em otuside of KatranLb.h)

yangchi · 2019-08-07T17:11:10Z

katran/lib/IpHelpers.h

@@ -33,7 +34,7 @@ struct beaddr {
 };

 class IpHelpers {
- public:
+public:


why remove the space

clang-format -i . looks like fb's one is too old

yangchi · 2019-08-07T17:13:16Z

katran/lib/KatranLb.cpp

  }
  return src_mapping;
 }

+const std::unordered_map<uint32_t, std::string> KatranLb::getNumToRealMap() {
+  std::unordered_map<uint32_t, std::string> reals;
+  for (auto& real: numToReals_) {


const auto&

yangchi · 2019-08-07T17:30:36Z

katran/lib/KatranLb.cpp

@@ -1177,7 +1195,10 @@ bool KatranLb::delHealthcheckerDst(const uint32_t somark) {

 std::unordered_map<uint32_t, std::string> KatranLb::getHealthcheckersDst() {
  // would be empty map in case if enableHc_ is false
-  std::unordered_map<uint32_t, std::string> hcs(hcReals_);
+  std::unordered_map<uint32_t, std::string> hcs;
+  for (auto& hc : hcReals_) {


const auto&

yangchi · 2019-08-07T17:36:45Z

katran/lib/KatranLb.cpp

@@ -566,6 +567,7 @@ bool KatranLb::modifyRealsForVip(
      LOG(ERROR) << "Invalid real's address: " << real.address;
      continue;
    }
+    auto raddr = folly::IPAddress(real.address);


folly::IPAddress raddr(real.address);

GH doesn't allow me comment on line 565 but real should also be declared as const auto&

By the way, isn't it possible that an string can pass validateAddress() as an AddressType::Network but then when we try to create IPAddress here it will fail?

on the last part - no it is not, by default network would fail validation, unless second param set to true (allow network to pass validation). line 566 does not do that

yangchi · 2019-08-07T17:53:22Z

katran/lib/KatranLb.cpp

-    return kError;
-  } else {
-    return real_iter->second.num;
+  if (validateAddress(real) != AddressType::INVALID) {


same question here, does this condition need to be == AddressType::HOST ?

same as above

yangchi · 2019-08-07T17:53:59Z

katran/lib/KatranLb.cpp

-  } else {
-    return real_iter->second.num;
+  if (validateAddress(real) != AddressType::INVALID) {
+    auto raddr = folly::IPAddress(real);


folly::IPAddress raddr(real);

yangchi · 2019-08-07T17:58:20Z

katran/lib/KatranLb.cpp

@@ -910,7 +922,8 @@ bool KatranLb::addInlineDecapDst(const std::string& dst) {
    LOG(ERROR) << "invalid decap destination address: " << dst;
    return false;
  }
-  if (decapDsts_.find(dst) != decapDsts_.end()) {
+  auto daddr = folly::IPAddress(dst);


folly::Address daddr(dst);

as stated. currently katran is exposed to capslock like issues. e.g. we could add same v6 address multiple times and that would pass all the tests (e.g. real fc00::1 fc00:0::1 fc00:0:0::1 etc). current behavior wont break anything, as in forwarding plane everything is in binary anyway, but we would consume more resources than needed. (e.g. real for regular vip could in any format, but quic's real in exploded) tested by: - it compiles (c) - unittests - katran_tester --- 1. this is taking care only about real's addresses, struct VipKey still has vip's address as a string. so it would take more work to modify that as well (as VipKey is a public API. current changes wont break anything external as API's return values have not changed. only internal's ones) 2. if i run clang-format on KatranLb.h/cpp it makes some changes in unrelated lines (most likely either format from 8.0 has some modifications or it was not run on that files for a while). what would you prefer? would you accept diff w/ some unrelated changed caused by clang format?

tehnerd · 2019-08-07T19:24:15Z

addressed comments. force pushed new version

yangchi

Accept. We will import shortly.

udippant · 2019-08-09T22:18:13Z

Merged this with bf7eeca
(addressed few nits and clang format.. so merged it from the fb side :) )

tehnerd · 2019-08-09T23:24:38Z

what about clang-formating? w/ default values (w/o specifying any stule (like Google etc)) it has around 115 changes for KatranLb.cpp (have not checked any other files).
https://gist.github.com/tehnerd/5612ea92b50814e49c4a0d9d51c4cd3a

long term what do you guys want to do with it? does FB's internal clang-formatter see the same number of changes or? (just want to figure out if it was not run on that files for a while or just difference between clang-format from 8.0 and the old one)

udippant · 2019-08-09T23:43:34Z

I applied clang-formatting on lib/KatranLb.cpp but it lists only one suggestion. It appears that FB's internal formatter (version 8.0.0) is not inline with what you're using.

tehnerd · 2019-08-10T00:02:04Z

Udip see gist above. i'm using stock one from "clang+llvm-8.0.0-x86_64-linux-gnu-ubuntu-18.04/bin/clang-format" . now the question is are there any specific option passed to facebook's one (e.g. custom --style options . e.g. google is using something like: -style="{BasedOnStyle: Google, IndentWidth: 2}" ). what FB is using? obviously clang-8.0.0 is not the same as FB's one. i want to figure out what is the difference so i would be able to do the same on my side

udippant · 2019-08-10T00:28:59Z

The formatting I applied was with clang-format -i katran/lib/KatranLb.cpp without any style. It seemed to agree with our arc lint. I'll confirm this and get back to you.

tehnerd · 2019-08-10T05:20:35Z

could you please share the output of "clang-format -dump-config". default from 8.0 looks like this:
https://gist.github.com/tehnerd/c8d0ed92c2191c12eea98307ad4c15c1

udippant · 2019-08-12T16:37:25Z

Here's the dump of the clang-format config https://gist.github.com/udippant/cd968e480f54ee8bd226625eb89d1ece

tehnerd · 2019-08-12T17:04:32Z

strange. diff shows no particular difference...

Summary: After diving in all the build system I found that the first error mentioned in #219 and #220 ``` 1597 | static_assert(formattable_char, "Mixing character types is disallowed."); ``` Was basically happening while compiling folly, after compiling it by itself I noticed this didn't happened, so I found that there was an issue with the fmt dependencies, removing the one that was downloaded by katran The issue mentioned in: #221 Was because katran was configured to use C++14 by default, and some of the libraries of folly require C++17, updated our requirements. Test Plan: TEST Output: ``` Test project /home/ubuntu/ivanmorett/katran/_build/build/katran/lib/tests Start 1: IpHelpersTests.testV4ParsingBe 1/56 Test #1: IpHelpersTests.testV4ParsingBe ....................... Passed 0.01 sec Start 2: IpHelpersTests.testV4ParsingInt 2/56 Test #2: IpHelpersTests.testV4ParsingInt ...................... Passed 0.01 sec Start 3: IpHelpersTests.testV6ParsingBe 3/56 Test #3: IpHelpersTests.testV6ParsingBe ....................... Passed 0.01 sec Start 4: IpHelpersTests.testV6ParsingInt 4/56 Test #4: IpHelpersTests.testV6ParsingInt ...................... Passed 0.01 sec Start 5: IpHelpersTests.testIncorrectAddr 5/56 Test #5: IpHelpersTests.testIncorrectAddr ..................... Passed 0.01 sec Start 6: CHHelpersTest.testMaglevCHSameWeight 6/56 Test #6: CHHelpersTest.testMaglevCHSameWeight ................. Passed 0.01 sec Start 7: CHHelpersTest.testMaglevV2CHSameWeight 7/56 Test #7: CHHelpersTest.testMaglevV2CHSameWeight ............... Passed 0.01 sec Start 8: CHHelpersTest.testMaglevCHDiffWeight 8/56 Test #8: CHHelpersTest.testMaglevCHDiffWeight ................. Passed 0.01 sec Start 9: CHHelpersTest.testMaglevV2CHDiffWeight 9/56 Test #9: CHHelpersTest.testMaglevV2CHDiffWeight ............... Passed 0.01 sec Start 10: CHHelpersTest.testMaglevWeightsSumLargerThanRing 10/56 Test #10: CHHelpersTest.testMaglevWeightsSumLargerThanRing ..... Passed 0.01 sec Start 11: CHHelpersTest.testMaglevWeightsSumBelowRingSize 11/56 Test #11: CHHelpersTest.testMaglevWeightsSumBelowRingSize ...... Passed 0.01 sec Start 12: KatranLbTest.testChangeMac 12/56 Test #12: KatranLbTest.testChangeMac ........................... Passed 0.01 sec Start 13: KatranLbTest.testIfIndex 13/56 Test #13: KatranLbTest.testIfIndex ............................. Passed 0.01 sec Start 14: KatranLbTest.testVipHelpers 14/56 Test #14: KatranLbTest.testVipHelpers .......................... Passed 0.14 sec Start 15: KatranLbTest.testAddingInvalidVip 15/56 Test #15: KatranLbTest.testAddingInvalidVip .................... Passed 0.01 sec Start 16: KatranLbTest.testRealHelpers 16/56 Test #16: KatranLbTest.testRealHelpers ......................... Passed 0.01 sec Start 17: KatranLbTest.testRealFlags 17/56 Test #17: KatranLbTest.testRealFlags ........................... Passed 0.01 sec Start 18: KatranLbTest.testVipStatsHelper 18/56 Test #18: KatranLbTest.testVipStatsHelper ...................... Passed 0.01 sec Start 19: KatranLbTest.testLruStatsHelper 19/56 Test #19: KatranLbTest.testLruStatsHelper ...................... Passed 0.01 sec Start 20: KatranLbTest.testLruMissStatsHelper 20/56 Test #20: KatranLbTest.testLruMissStatsHelper .................. Passed 0.01 sec Start 21: KatranLbTest.testHcHelpers 21/56 Test #21: KatranLbTest.testHcHelpers ........................... Passed 0.01 sec Start 22: KatranLbTest.getVipFlags 22/56 Test #22: KatranLbTest.getVipFlags ............................. Passed 0.01 sec Start 23: KatranLbTest.getAllVips 23/56 Test #23: KatranLbTest.getAllVips .............................. Passed 0.01 sec Start 24: KatranLbTest.testUpdateRealsHelper 24/56 Test #24: KatranLbTest.testUpdateRealsHelper ................... Passed 0.07 sec Start 25: KatranLbTest.testUpdateQuicRealsHelper 25/56 Test #25: KatranLbTest.testUpdateQuicRealsHelper ............... Passed 0.06 sec Start 26: KatranLbTest.testUpdateQuicReal 26/56 Test #26: KatranLbTest.testUpdateQuicReal ...................... Passed 0.01 sec Start 27: KatranLbTest.getRealsForVip 27/56 Test #27: KatranLbTest.getRealsForVip .......................... Passed 0.01 sec Start 28: KatranLbTest.getHealthcheckersDst 28/56 Test #28: KatranLbTest.getHealthcheckersDst .................... Passed 0.01 sec Start 29: KatranLbTest.invalidAddressHandling 29/56 Test #29: KatranLbTest.invalidAddressHandling .................. Passed 0.01 sec Start 30: KatranLbTest.addInvalidSrcRoutingRule 30/56 Test #30: KatranLbTest.addInvalidSrcRoutingRule ................ Passed 0.01 sec Start 31: KatranLbTest.addValidSrcRoutingRuleV4 31/56 Test #31: KatranLbTest.addValidSrcRoutingRuleV4 ................ Passed 0.01 sec Start 32: KatranLbTest.addValidSrcRoutingRuleV6 32/56 Test #32: KatranLbTest.addValidSrcRoutingRuleV6 ................ Passed 0.01 sec Start 33: KatranLbTest.addMaxSrcRules 33/56 Test #33: KatranLbTest.addMaxSrcRules .......................... Passed 0.01 sec Start 34: KatranLbTest.delSrcRules 34/56 Test #34: KatranLbTest.delSrcRules ............................. Passed 0.01 sec Start 35: KatranLbTest.clearSrcRules 35/56 Test #35: KatranLbTest.clearSrcRules ........................... Passed 0.01 sec Start 36: KatranLbTest.addFewInvalidNets 36/56 Test #36: KatranLbTest.addFewInvalidNets ....................... Passed 0.01 sec Start 37: KatranLbTest.addInvalidDecapDst 37/56 Test #37: KatranLbTest.addInvalidDecapDst ...................... Passed 0.01 sec Start 38: KatranLbTest.addInvalidDecapDstNet 38/56 Test #38: KatranLbTest.addInvalidDecapDstNet ................... Passed 0.01 sec Start 39: KatranLbTest.addValidDecapDst 39/56 Test #39: KatranLbTest.addValidDecapDst ........................ Passed 0.01 sec Start 40: KatranLbTest.delValidDecapDst 40/56 Test #40: KatranLbTest.delValidDecapDst ........................ Passed 0.01 sec Start 41: KatranLbTest.delInvalidDecapDst 41/56 Test #41: KatranLbTest.delInvalidDecapDst ...................... Passed 0.01 sec Start 42: KatranLbTest.addMaxDecapDst 42/56 Test #42: KatranLbTest.addMaxDecapDst .......................... Passed 0.01 sec Start 43: VipTestF.testBatchUpdateReals 43/56 Test #43: VipTestF.testBatchUpdateReals ........................ Passed 0.04 sec Start 44: VipTestF.testBatchUpdateRealsWeight 44/56 Test #44: VipTestF.testBatchUpdateRealsWeight .................. Passed 0.05 sec Start 45: VipTestF.testGetRealsAndWeight 45/56 Test #45: VipTestF.testGetRealsAndWeight ....................... Passed 0.01 sec Start 46: VipTestF.testGetReals 46/56 Test #46: VipTestF.testGetReals ................................ Passed 0.02 sec Start 47: VipTest.testAddRemoveReal 47/56 Test #47: VipTest.testAddRemoveReal ............................ Passed 0.01 sec Start 48: EventPipeCallbackTest.SimpleCallbackTest 48/56 Test #48: EventPipeCallbackTest.SimpleCallbackTest ............. Passed 0.01 sec Start 49: EventPipeCallbackTest.LargeWriteTest 49/56 Test #49: EventPipeCallbackTest.LargeWriteTest ................. Passed 0.15 sec Start 50: TestMonitoringServiceCore.SimpleAcceptSubscription 50/56 Test #50: TestMonitoringServiceCore.SimpleAcceptSubscription ... Passed 0.01 sec Start 51: TestMonitoringServiceCore.SimpleErrors 51/56 Test #51: TestMonitoringServiceCore.SimpleErrors ............... Passed 0.01 sec Start 52: TestMonitoringServiceCore.EventIntersection 52/56 Test #52: TestMonitoringServiceCore.EventIntersection .......... Passed 0.01 sec Start 53: TestMonitoringServiceCore.RacingClients 53/56 Test #53: TestMonitoringServiceCore.RacingClients .............. Passed 0.01 sec Start 54: TestMonitoringServiceCore.SubscribeAndCancel 54/56 Test #54: TestMonitoringServiceCore.SubscribeAndCancel ......... Passed 0.01 sec Start 55: PcapWriterTest.SingleWriter 55/56 Test #55: PcapWriterTest.SingleWriter .......................... Passed 0.02 sec Start 56: PcapWriterTest.MultiWriter 56/56 Test #56: PcapWriterTest.MultiWriter ........................... Passed 0.01 sec 100% tests passed, 0 tests failed out of 56 Total Test time (real) = 1.04 sec + cd ../testing/ + ctest -v ./CMakeFiles ./CTestTestfile.cmake ./Makefile ./base64helpers-tests './base64helpers-tests[1]_include.cmake' './base64helpers-tests[1]_tests.cmake' ./cmake_install.cmake ./katran_tester ./libbase64_helpers.a ./libbpftester.a ./libkatran_test_provision.a ./libkatran_test_util.a ./libpcap_parser.a ctest: /usr/local/lib/libcurl.so.4: no version information available (required by ctest) Test project /home/ubuntu/ivanmorett/katran/_build/build/katran/lib/testing Start 1: Base64Tests.testEncode 1/2 Test #1: Base64Tests.testEncode ........... Passed 0.01 sec Start 2: Base64Tests.testDecode 2/2 Test #2: Base64Tests.testDecode ........... Passed 0.01 sec 100% tests passed, 0 tests failed out of 2 Total Test time (real) = 0.01 sec + popd ~/ivanmorett/katran/_build ``` Differential Revision: D55108012 Pulled By: lima1756

facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Aug 2, 2019

yangchi suggested changes Aug 7, 2019

View reviewed changes

tehnerd force-pushed the string_to_bin branch from f6d9ebf to 66d1ffc Compare August 7, 2019 19:24

yangchi approved these changes Aug 8, 2019

View reviewed changes

udippant closed this Aug 9, 2019

tehnerd deleted the string_to_bin branch September 4, 2019 03:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

changing internal representation of ip addresses for reals from string to binary #37

changing internal representation of ip addresses for reals from string to binary #37

tehnerd commented Aug 2, 2019

udippant commented Aug 2, 2019

udippant commented Aug 5, 2019

yangchi Aug 7, 2019

yangchi Aug 7, 2019

yangchi Aug 7, 2019

yangchi Aug 7, 2019

tehnerd Aug 7, 2019 •

edited

Loading

yangchi Aug 7, 2019

tehnerd Aug 7, 2019

yangchi Aug 7, 2019

yangchi Aug 7, 2019

yangchi Aug 7, 2019

yangchi Aug 7, 2019

yangchi Aug 7, 2019

tehnerd Aug 7, 2019

yangchi Aug 7, 2019

tehnerd Aug 7, 2019

yangchi Aug 7, 2019

yangchi Aug 7, 2019

tehnerd commented Aug 7, 2019

yangchi left a comment

udippant commented Aug 9, 2019

tehnerd commented Aug 9, 2019

udippant commented Aug 9, 2019

tehnerd commented Aug 10, 2019

udippant commented Aug 10, 2019

tehnerd commented Aug 10, 2019

udippant commented Aug 12, 2019

tehnerd commented Aug 12, 2019

		@@ -42,10 +43,13 @@ class IpHelpers {
		* of beaddr structure. this function could throw, if given string is not

changing internal representation of ip addresses for reals from string to binary #37

changing internal representation of ip addresses for reals from string to binary #37

Conversation

tehnerd commented Aug 2, 2019

udippant commented Aug 2, 2019

udippant commented Aug 5, 2019

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

tehnerd Aug 7, 2019 • 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

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

tehnerd commented Aug 7, 2019

yangchi left a comment

Choose a reason for hiding this comment

udippant commented Aug 9, 2019

tehnerd commented Aug 9, 2019

udippant commented Aug 9, 2019

tehnerd commented Aug 10, 2019

udippant commented Aug 10, 2019

tehnerd commented Aug 10, 2019

udippant commented Aug 12, 2019

tehnerd commented Aug 12, 2019

tehnerd Aug 7, 2019 •

edited

Loading