Todd Lipcon has posted comments on this change. Change subject: KUDU-1875: Refuse unauthenticated connections from publicly routable IP addrs ......................................................................
Patch Set 22: (14 comments) http://gerrit.cloudera.org:8080/#/c/6514/22//COMMIT_MSG Commit Message: PS22, Line 13: adavance typo: advanced nit: change to a full sentence please PS22, Line 17: However network "However, if" http://gerrit.cloudera.org:8080/#/c/6514/22/src/kudu/rpc/server_negotiation.cc File src/kudu/rpc/server_negotiation.cc: Line 82: kudu::g_trusted_subnets->push_back(network); I don't think the validator is supposed to have side effects -- it's not clear how many times it will get called, etc PS22, Line 85: can only be interpreted as "must be specified in" http://gerrit.cloudera.org:8080/#/c/6514/22/src/kudu/rpc/server_negotiation.h File src/kudu/rpc/server_negotiation.h: Line 42: extern vector<Network>* g_trusted_subnets; hm, where is this used externally? we try to avoid globals with such wide scope http://gerrit.cloudera.org:8080/#/c/6514/22/src/kudu/util/net/net_util-test.cc File src/kudu/util/net/net_util-test.cc: PS22, Line 95: TestPrivateAddresses rename to TestWithinNetwork (this isn't specific to public/private) http://gerrit.cloudera.org:8080/#/c/6514/22/src/kudu/util/net/net_util.cc File src/kudu/util/net/net_util.cc: PS22, Line 186: string Network::ToString() const { : return Substitute("$0:$1", addr_, mask_); : } this ToString isn't really human-readable. I think it would be better to either implement one that returns something like CIDR or host/netmask, or just remove it Line 252: Status GetLocalNetwork(std::vector<Network>* net) { nit: rename param to 'nets' since it is a list, not a single one PS22, Line 256: auto cleanup = MakeScopedCleanup([&]() { : freeifaddrs(ifap); : }); this should probably be moved below the error check, otherwise we may freeifaddrs() an uninitialized 'ifap'. Alternatively, initialize 'ifap' to nullptr, and add an 'if (ifap) freeifaddrs(ifap);' here Line 265: should probably 'net->clear()' here, since the docs say it gets them, not appends them. http://gerrit.cloudera.org:8080/#/c/6514/22/src/kudu/util/net/net_util.h File src/kudu/util/net/net_util.h: Line 98: Network(uint32_t addr, uint32_t mask); add doc here regarding endianness, etc. PS22, Line 107: ParseString rename to ParseCIDR so it's clear this is a CIDR style string and not a netmask-style PS22, Line 111: mask_ I think conventionally people refer to this as 'netmask' instead of 'mask'. Can you change here and the above parameters/getters/etc? PS22, Line 131: GetLocalNetwork since this gives back a list, change to GetLocalNetworks -- To view, visit http://gerrit.cloudera.org:8080/6514 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6c3fbb5491785874c5701d6c9d866949cfac905e Gerrit-PatchSet: 22 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Harsh J <ha...@harshj.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
