Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22984 )
Change subject: [net] KUDU-1457 [1/n] Add support for IPv6 ...................................................................... Patch Set 6: (29 comments) http://gerrit.cloudera.org:8080/#/c/22984/6/src/kudu/rpc/server_negotiation.cc File src/kudu/rpc/server_negotiation.cc: http://gerrit.cloudera.org:8080/#/c/22984/6/src/kudu/rpc/server_negotiation.cc@104 PS6, Line 104: "fe80::/10,fc00::/7", What about the loopback address ::1/128? Shouldn't we include it into the list of trusted sub-nets? http://gerrit.cloudera.org:8080/#/c/22984/6/src/kudu/rpc/server_negotiation.cc@104 PS6, Line 104: fe80::/10 nit: it would be great to verify that this results in fe80::/64 network starting from fe80::/10 when parsed with the current code in Network::ParseCIDRStrings() -- please add a small sub-scenario for that http://gerrit.cloudera.org:8080/#/c/22984/6/src/kudu/rpc/server_negotiation.cc@104 PS6, Line 104: fc00::/7 Isn't this the _reserved_ range where no IPv6 address might be actually from as of now? IIRC, fd00::/7 is the range for private networks that might be in use right now, so I'd expect to see fd00::/7 in the list as well. http://gerrit.cloudera.org:8080/#/c/22984/6/src/kudu/util/net/diagnostic_socket.cc File src/kudu/util/net/diagnostic_socket.cc: http://gerrit.cloudera.org:8080/#/c/22984/6/src/kudu/util/net/diagnostic_socket.cc@176 PS6, Line 176: // TODO(araina): Remove this once diagnostic socket handling is added for IPv6. It looks good at the surface, but I suspect that in real life there will be too many warnings in the logs when running tservers and masters that listen on IPv6 addresses. If you aren't planning to adapt the DiagnosticSocket's code to work on IPv6 addresses in the nearest future (which should be quite trivial since the sock_diag kernel utility supports IPv6: https://www.man7.org/linux/man-pages/man7/sock_diag.7.html), then it'd prudent to disable instantiation of DiagnosticSocket instances at a higher level in AcceptorPool::RunThread() if the messenger is listening on IPv6 address. That should help to avoid flood of the related error messages in the logs. http://gerrit.cloudera.org:8080/#/c/22984/6/src/kudu/util/net/net_util-test.cc File src/kudu/util/net/net_util-test.cc: http://gerrit.cloudera.org:8080/#/c/22984/6/src/kudu/util/net/net_util-test.cc@188 PS6, Line 188: // Test some invalid addresses. : Status s = DoParseBindAddresses("0.0.0.0:xyz", &ret); : ASSERT_STR_CONTAINS(s.ToString(), "invalid port"); : : s = DoParseBindAddresses("0.0.0.0:100000", &ret); : ASSERT_STR_CONTAINS(s.ToString(), "invalid port"); : : s = DoParseBindAddresses("0.0.0.0:", &ret); : ASSERT_STR_CONTAINS(s.ToString(), "invalid port"); Add similar negative test cases like these for IPv6 wildcard addresses as well? http://gerrit.cloudera.org:8080/#/c/22984/6/src/kudu/util/net/net_util-test.cc@321 PS6, Line 321: IsNetworkError() nit for here and elsewhere in this file: when using Status::IsXxx(), please make sure the status message is output if the assertion ever triggers -- that helps a lot in debugging&troubleshooting. It's something like below: const auto s = network.ParseCIDRString("192.168.0.0/abcd"); ASSERT_TRUE(s.IsNetworkError()) << s.ToString(); http://gerrit.cloudera.org:8080/#/c/22984/6/src/kudu/util/net/net_util-test.cc@326 PS6, Line 326: fd00:1234:5678:9abc::1 Maybe not in this particular scenario, but in more appropriate one please add verification on parsing addresses expressed in upper-letter ASCII literals -- do we error out on those or the result should be the same as for lower-letter representation? http://gerrit.cloudera.org:8080/#/c/22984/6/src/kudu/util/net/net_util.h File src/kudu/util/net/net_util.h: http://gerrit.cloudera.org:8080/#/c/22984/6/src/kudu/util/net/net_util.h@121 PS6, Line 121: common representation What's 'a common representation'? Is this the term used in corresponding RFC documents? As-is, this looks quite vague, and I'd rather vote for keeping the mention of the dotted-decimal notation for IPv4 addresses and adding corresponding part for IPv6 instead of generic blurring. http://gerrit.cloudera.org:8080/#/c/22984/6/src/kudu/util/net/net_util.h@156 PS6, Line 156: class Network { : public: : Network(); : Network(sa_family_t family, uint128_t addr, uint128_t netmask); Wouldn't it be better to templatize this on the type of IP address family? With such an approach, the implementation would become more straightforward and robust from the resource usage and the performance perspectives. http://gerrit.cloudera.org:8080/#/c/22984/6/src/kudu/util/net/net_util.h@164 PS6, Line 164: Returns true if the network is within 127.0.0.0/8 range. Update the comment? http://gerrit.cloudera.org:8080/#/c/22984/1/src/kudu/util/net/net_util.h File src/kudu/util/net/net_util.h: http://gerrit.cloudera.org:8080/#/c/22984/1/src/kudu/util/net/net_util.h@113 PS1, Line 113: // Takes a vector of HostPort objects and returns a comma separated > This is probably some mistake. Changed the order. Thanks! The updated order looks more natural. http://gerrit.cloudera.org:8080/#/c/22984/6/src/kudu/util/net/net_util.cc File src/kudu/util/net/net_util.cc: http://gerrit.cloudera.org:8080/#/c/22984/6/src/kudu/util/net/net_util.cc@173 PS6, Line 173: Instead of introducing this helper function, consider adopting the code in gutil/strings/split.h: the StringPiece class there has almost one-to-one correspondence with std::string_view. Most likely, you'll need just to add an instantiations for the corresponding class templates with std::string_view. And yes -- it's better doing so in a separate changelist, and then base this IPv6 patch on top. You might even consider updating the code in gutil/strings up to the current code from its upstream repo (IIUC, that's the code from the Chromium project), but that would be much more invasive change. http://gerrit.cloudera.org:8080/#/c/22984/6/src/kudu/util/net/net_util.cc@174 PS6, Line 174: std:: nit: add 'using std::string_view;' into the corresponding section in the beginning of this file and you can omit the 'std::' namespace prefix here and elsewhere in this file. http://gerrit.cloudera.org:8080/#/c/22984/6/src/kudu/util/net/net_util.cc@177 PS6, Line 177: size_t nit: use 'auto' instead? http://gerrit.cloudera.org:8080/#/c/22984/6/src/kudu/util/net/net_util.cc@202 PS6, Line 202: Substitute("invalid address $0", addr This isn't going to help to see the invalid input since 'addr' is already empty at this point. Even if outputting the original 'str', then it's better to put it into quotes or something like that to be able to tell how many whitespaces it has. http://gerrit.cloudera.org:8080/#/c/22984/6/src/kudu/util/net/net_util.cc@206 PS6, Line 206: addr.find(']'); If you want to find the rightmost closing bracket, maybe use 'rfind' instead? http://gerrit.cloudera.org:8080/#/c/22984/6/src/kudu/util/net/net_util.cc@215 PS6, Line 215: validate_host Consider renaming this: 'validate' isn't descriptive since this functor not only checks the input for emptiness, but also sets the 'host_' field. Also, it's not very transparent to have assignment of 'host_' and 'port_' fields to be split like this. Maybe, change the lambda to be constexpr one and just check for the emptiness of the input, but do the assignment of the 'host_' field along with 'port_' field outside of this lambda? http://gerrit.cloudera.org:8080/#/c/22984/6/src/kudu/util/net/net_util.cc@216 PS6, Line 216: host == "" Use host.empty() instead? http://gerrit.cloudera.org:8080/#/c/22984/6/src/kudu/util/net/net_util.cc@217 PS6, Line 217: Substitute( nit: there is nothing to substitute in the formatted message -- is something missing (otherwise, remove Substitute)? http://gerrit.cloudera.org:8080/#/c/22984/6/src/kudu/util/net/net_util.cc@227 PS6, Line 227: Substitute("$0", addr) Could we get away by not using Substitute() here at all? http://gerrit.cloudera.org:8080/#/c/22984/6/src/kudu/util/net/net_util.cc@241 PS6, Line 241: std::string This is a bit funny: creating a std::string instance with all the memory allocation and data copying just to supply it into SimpleAtoi() when there is SimpleAtoi() that takes 'const char*' as its first parameter. Please fix this to avoid useless memory allocation and copying. http://gerrit.cloudera.org:8080/#/c/22984/6/src/kudu/util/net/net_util.cc@264 PS6, Line 264: RETURN_NOT_OK_PREPEND(validate_host(addr.substr(1, addr.length() - 2)), Does it make sense to add extra sanity check of addr.front() == '[' in this case? http://gerrit.cloudera.org:8080/#/c/22984/6/src/kudu/util/net/net_util.cc@272 PS6, Line 272: vector<std::string_view> parts = SplitStringView(addr, ']'); : DCHECK(parts.size() == 2); If you are expecting just one ']' symbol in the string at this point, why not to use addr.rfind(']') instead of http://gerrit.cloudera.org:8080/#/c/22984/6/src/kudu/util/net/net_util.cc@275 PS6, Line 275: after opening bracket Does it make sense to check whether the first symbol is indeed the opening bracket? http://gerrit.cloudera.org:8080/#/c/22984/6/src/kudu/util/net/net_util.cc@280 PS6, Line 280: SimpleAtoi This seems like it's time to introduce SimpleAtoi() that accepts std::string_view as a parameter? http://gerrit.cloudera.org:8080/#/c/22984/6/src/kudu/util/net/net_util.cc@432 PS6, Line 432: strcount(host_, ':') Use find()/rfind() instead -- it's enough to find just one instance of ':' symbol, right? http://gerrit.cloudera.org:8080/#/c/22984/6/src/kudu/util/net/sockaddr.h File src/kudu/util/net/sockaddr.h: http://gerrit.cloudera.org:8080/#/c/22984/6/src/kudu/util/net/sockaddr.h@203 PS6, Line 203: sockaddr_storage Please add static_assert() on the size of sockaddr_storage (in the Sockaddr constructor?) to make sure sockaddr_in6 fits in there. http://gerrit.cloudera.org:8080/#/c/22984/1/src/kudu/util/net/sockaddr.h File src/kudu/util/net/sockaddr.h: http://gerrit.cloudera.org:8080/#/c/22984/1/src/kudu/util/net/sockaddr.h@116 PS1, Line 116: uin > Done OK, now in PS6 the set_port() setter has in_port_t as a parameter type, but the getter has uint16_t return type. Why not to use the same type/typedef in both cases? http://gerrit.cloudera.org:8080/#/c/22984/1/src/kudu/util/net/sockaddr.cc File src/kudu/util/net/sockaddr.cc: http://gerrit.cloudera.org:8080/#/c/22984/1/src/kudu/util/net/sockaddr.cc@225 PS1, Line 225: Sockaddr& Sockaddr::operator=(const struct sockaddr_in &addr) { : // Do not count the padding sin Right -- there isn't sin_zero field in sockaddr_in6, but there is another funny field there: sin6_flowinfo As per RFC available at https://www.rfc-editor.org/rfc/rfc3493: > The sin6_flowinfo field is a 32-bit field intended to contain flow- related information. The exact way this field is mapped to or from a packet is not currently specified. Until such time as its use is specified, applications should set this field to zero when constructing a sockaddr_in6, and ignore this field in a sockaddr_in6 structure constructed by the system. Maybe, it's OK to copy its contents over in this assignment operator, but I'd think of doing proper steps when calling set_length() to avoid counting in any garbage in the `sin6_flowinfo` field, especially when computing various hash functions on the IPv6 address represented by sockaddr_in6. -- To view, visit http://gerrit.cloudera.org:8080/22984 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I22c773ffb2ff44b9cd765b546d6724ec5543586f Gerrit-Change-Number: 22984 Gerrit-PatchSet: 6 Gerrit-Owner: Ashwani Raina <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Ashwani Raina <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 17 Jul 2025 01:18:10 +0000 Gerrit-HasComments: Yes
