Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22984 )
Change subject: KUDU-1457 [1/n] add IPv6 support to net utils ...................................................................... Patch Set 13: (5 comments) 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: if (storage_.un.sun_path[0] == '\0') { : return UnixAddressType::kAbst > When you say "from outside" what does that mean? Is sockaddr_in6 coming from > outside of Kudu library? If it is within Kudu library, we should not get any > sockaddr_in6 address with non-zero fields (sin6_flowinfo and sin6_scope_id). I haven't put a lot of effort into articulating all the terms properly. I appreciate you are clarifying on these points. In this context, "outside" means "various system calls that put data into sockaddr_storage and/or sockaddr_in6 when invoked". A quick search shows that such a situation is present at least in Socket::GetSocketAddress(), Socket::GetPeerAddress(), and Socket::Accept(). There we cannot control what's in the sin6_flowinfo and sin6_scope_id fields of the sockaddr_in6 structure returned by a system call. > On a side note, I don't mind zeroing out of these fields but I think that > assignment operator method should just copy the address. It should not zero > out or validate any field inside. If you prefer to just copy the address in this assignment operation, that might be fine if also finding all the places where this operator is invoked and sanitizing a couple of the fields of the source sockaddr_in6 structure before jthe assignment. And even if you did so in this patch, how to make sure a newly added call site of this assignment operator will do proper sanitization as well? It can slip under the radar, and then it will be much harder to troubleshoot the outcome in the field. > The only check it should be doing is that correct copy ctor is called for the > incoming IP address family type. In future, if we do introduce features like > QoS, etc. the assignment operator method doesn't need to change anything. It > can simply copy the field values as is from source to target object. If you don't want to touch any fields of the result sockaddr_in6 structures, an alternative approach to sanitizing the fields might be updating the BytewiseCompare() and Sockaddr::HashCode() methods to take into account only the address field of the sockaddr_in6 structure for IPv6 addresses. Also, if you want to rely only on Sockaddr constructors and assignment operators for Sockaddr instances, you might consider removing the existing Sockaddr::operator=(const struct sockaddr_in&) operator and not adding this new one Sockaddr::operator=(const struct sockaddr_in6&), updating the call sites to construct temporary instances of Sockaddr accordingly. I'm not concerned about very remote and mostly hypothetical features like QoS for IPv6 (or even for IPv4), but I want to make sure we don't step on the same issues that have already bitten us, such as KUDU-3352. http://gerrit.cloudera.org:8080/#/c/22984/11/src/kudu/util/net/sockaddr.cc File src/kudu/util/net/sockaddr.cc: http://gerrit.cloudera.org:8080/#/c/22984/11/src/kudu/util/net/sockaddr.cc@243 PS11, Line 243: set_length(sizeof(addr)); : memcpy(&storage_, &addr, len_); : storage_.in6.sin6_flowinfo = 0; // Flow-label & traffic class : storage_.in6.sin6_scope_id = 0; // Scope identifier : DCHECK_EQ(family(), AF_INET6); : > Setting zero in the object rather than input address. I wasn't suggesting to zero out the fields in the input address here (that would violate the semantics of this method, BTW). My point was to store the data in Sockaddr that would allow BytewiseCompare() and Hash() methods to properly tell same IPv6 addresses, even if incoming sockaddr_in6 structures that they were built from had different bits in in6_flowinfo and sin6_scope_id fields. Right: explicitly zeroing out sin6_flowinfo and sin6_scope_id fields in the stored sockaddr_in6 structure is good enough here, and it matches what's done in Sockaddr::ParseFromNumericHostPort(). And just re-iterating on my earlier comment: an alternative to this field-zeroing is updating Sockaddr::BytewiseCompare() and Sockaddr::HashCode() to take into account only the address field for IPv6 AF_INET6 addresses. http://gerrit.cloudera.org:8080/#/c/22984/13/src/kudu/util/net/sockaddr.cc File src/kudu/util/net/sockaddr.cc: http://gerrit.cloudera.org:8080/#/c/22984/13/src/kudu/util/net/sockaddr.cc@109 PS13, Line 109: Sockaddr::Sockaddr(const struct sockaddr_in6& addr) : As of code in PS13, Sockaddr::operator=(const struct sockaddr_in6&) and Sockaddr::ParseFromNumericHostPort(), sin6_scope_id and in6_flowinfo fields are zeroed out in the stored sockaddr_in6 stucture. Should the same be done here as well to make sure Sockaddr::Hash() behaves as expected? The same is applicable for all other places where the code ends up storing sockaddr_in6 fields in the Sockaddr::storage_ field. http://gerrit.cloudera.org:8080/#/c/22984/11/src/kudu/util/net/socket-test.cc File src/kudu/util/net/socket-test.cc: http://gerrit.cloudera.org:8080/#/c/22984/11/src/kudu/util/net/socket-test.cc@103 PS11, Line 103: ASSERT_OK(s_in6.ParseString(kIpV6Addr, kPort)); : ASSERT_OK(s_un.ParseUnixDomainPath(kPath)); : > Yes, I have verified HashCode() and BytewiseCompare() and both them do catch > those mismatch cases. Here are some of the test cases if that is what you are pointing to: It seems there is a confusion. At least for IPv4 addresses, the whole point of this test scenario is to verify that Sockaddr::HashCode() returns the same value for Sockaddr instances created with the same IPv4 address, but with different bits in the sin_zero fields of the source sockaddr_in structures. As you mentioned in earlier comment in this thread, that's guaranteed by properly setting the 'len_' field. If anything were broken there, this test would catch that. The test should start failing when 'len_' is set incorrectly (i.e., increase it by one or extend it to the whole size of the sockaddr_in structure). To exercise the varying of the bits of the sin_zero fields, s_un.ParseUnixDomainPath(kPath) followed by s_un.ParseString(kIpAddr, kPort) is used. Now, I thought that the whole point of this new code added here is to verify that Sockaddr::HashCode() returns the same value for Sockaddr instances created from same IPv6 address, but with different bits in sin6_flowinfo and/or sin6_scope_id fields of the source sockaddr_in6 structures. I see the same trick with ParseUnixDomainPath/ParseString sequence is used for IPv6 addresses. That's why my question about the length of the kPath string. > Note: These tests make use of set_flowinfo() and set_scope_id() public > methods that were explicitly added to test those scenarios. I am not keen on > adding these unit tests to upstream as I don't see much value in having them.| If you are interested in adding such tests, I have one question - would having set methods be ok to do it along with a comment that these methods are for test purpose only? That's exactly why the ParseUnixDomainPath/ParseString sequence is present in the scenario -- using that, there is no need to add anything test-dedicated, like set_flowinfo() and set_scope_id() you mentioned. But to make it work for IPv6 addresses, it's necessary to make sure the length of the kPath string is enough to stamp over the corresponding fields in the sockaddr_in6 structure stored under the umbrella of the Sockaddr::storage_ union. http://gerrit.cloudera.org:8080/#/c/22984/13/src/kudu/util/net/socket-test.cc File src/kudu/util/net/socket-test.cc: http://gerrit.cloudera.org:8080/#/c/22984/13/src/kudu/util/net/socket-test.cc@288 PS13, Line 288: // IPv6. : DoTestServerConnect("::1", AF_INET6); : // IPv4-mapped IPv6 address. : DoTestServerConnect("::ffff:127.0.0.1", AF_INET6); I've seen test nodes where IPv6 stack was explicitly disabled, and no IPv6 addresses were configured even for the 'lo' interface. As of now, IPv6 stack is enabled for the cloud nodes used by dist-test in the gerrit CI pipeline. Would it make sense to enable this sort of tests only after verifying that the test node has IPv6 stack enabled at least at its loopback network interface(s)? If not, we should skip those. The same is applicable for the updated rpc-test.cc. Or, maybe, after this patch we are about to require IPv6 configured to allow the newly added test scenarios like this to pass? -- 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: 13 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: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Comment-Date: Wed, 27 Aug 2025 00:07:12 +0000 Gerrit-HasComments: Yes
