Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12474 )
Change subject: KUDU-1900: add loopback check and test ...................................................................... Patch Set 5: (3 comments) http://gerrit.cloudera.org:8080/#/c/12474/5/src/kudu/integration-tests/security-itest.cc File src/kudu/integration-tests/security-itest.cc: http://gerrit.cloudera.org:8080/#/c/12474/5/src/kudu/integration-tests/security-itest.cc@335 PS5, Line 335: struct ifaddrs *ifap; : if (getifaddrs(&ifap) > -1) { : SCOPED_CLEANUP({ : freeifaddrs(ifap); : }); : for (struct ifaddrs *ifa = ifap; ifa; ifa = ifa->ifa_next) { : if (ifa->ifa_addr == nullptr || ifa->ifa_netmask == nullptr : || ifa->ifa_addr->sa_family != AF_INET) : continue; : : struct sockaddr_in *addr_in = reinterpret_cast<struct sockaddr_in*>(ifa->ifa_addr); : if ((NetworkByteOrder::FromHost32(addr_in->sin_addr.s_addr) >> 24) != 127) { : char s[INET_ADDRSTRLEN]; : inet_ntop(AF_INET, &(addr_in->sin_addr), s, INET_ADDRSTRLEN); : FLAGS_local_ip_for_outbound_sockets = string(s, arraysize(s)); : // Found and assigned an external IP. : return true; : } : } : } > Yes, but it will be less efficient, because GetLocalNetworks will loop thro I would not be too much concerned with efficiency in this particular case because it's just a test. However, if you feel strong about this, maybe think about making it more generic and moving into net_util.{h,cc}. http://gerrit.cloudera.org:8080/#/c/12474/5/src/kudu/integration-tests/security-itest.cc@375 PS5, Line 375: encrypted > So, that's something I would like to ask you back :) When we set --rpc_encr Effectively, the application of the --rpc_encrypt_loopback_connections=true flag is gated by the -rpc_encryption=disabled setting (see shttps://github.com/apache/kudu/blob/4e2451dbf18db1faf9545ac1f9663d378b9f5efe/src/kudu/rpc/server_negotiation.cc#L509) http://gerrit.cloudera.org:8080/#/c/12474/5/src/kudu/integration-tests/security-itest.cc@406 PS5, Line 406: true > hm... yes, there is something wrong here. Token should not be passed here Something is fishy here: if in case of encrypted loopback connection between the client and the server the authz token is not present (as I can see it from line 403), then it's a bit surprising that the token is present in this case. At least, it would be nice to add some comment explaining why that's the case. And it seems I know why: https://github.com/apache/kudu/blob/4e2451dbf18db1faf9545ac1f9663d378b9f5efe/src/kudu/rpc/negotiation.cc#L279 Probably, that's a bug. Let's just add a comment and move further -- I think it's worth addressing that issue in a separate changelist. -- To view, visit http://gerrit.cloudera.org:8080/12474 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f Gerrit-Change-Number: 12474 Gerrit-PatchSet: 5 Gerrit-Owner: Greg Solovyev <gsolov...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Greg Solovyev <gsolov...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 14 Feb 2019 19:48:22 +0000 Gerrit-HasComments: Yes
