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: (6 comments) http://gerrit.cloudera.org:8080/#/c/12474/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12474/5//COMMIT_MSG@23 PS5, Line 23: unencripted auth token Seems to be a typo. Also, what is unencrypted auth token? Per my knowledge, we never encrypt authn/authz tokens themselves. 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; : } : } : } Is it possible to call kudu::GetLocalNetworks() and then just work with the result vector<Network> to extract non-loopback addresses? While doing so, feel free to add new useful functions into src/kudu/util/net/net_util.{h,cc} http://gerrit.cloudera.org:8080/#/c/12474/5/src/kudu/integration-tests/security-itest.cc@375 PS5, Line 375: encrypted Wait, but what about { BindMode::LOOPBACK, "disabled", "disabled", true, false, false, } ? That's the case when connections are not encrypted, right? http://gerrit.cloudera.org:8080/#/c/12474/5/src/kudu/integration-tests/security-itest.cc@382 PS5, Line 382: { BindMode::LOOPBACK, "required", "required", false, false, true, }, : { BindMode::LOOPBACK, "disabled", "required", false, false, true, }, : { BindMode::LOOPBACK, "disabled", "disabled", false, false, true, }, When it became 3 boolean parameters, it's harder to read this matrix. Maybe, define some boolean constants with sound names for better readability and use them instead of just false/true? With that a configuration entry would look like: { BindMode::LOOPBACK, "required", "required", LOOPBACK_UNENCRYPTED, CLIENT_IP_EXTERNAL, TOKEN_PRESENT, } http://gerrit.cloudera.org:8080/#/c/12474/5/src/kudu/integration-tests/security-itest.cc@406 PS5, Line 406: true Is this typo? http://gerrit.cloudera.org:8080/#/c/12474/5/src/kudu/integration-tests/security-itest.cc@437 PS5, Line 437: if (!assignIPToClient(params.force_external_client_ip)) { : LOG(WARNING) << "Skipping external connection test, because the host does " : "not have an external network interface."; : return; : } Is it possible to make this check before starting cluster (i.e. StartCluster() at line 429)? -- 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 <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Greg Solovyev <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 14 Feb 2019 07:06:12 +0000 Gerrit-HasComments: Yes
