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 <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 07:06:12 +0000
Gerrit-HasComments: Yes

Reply via email to