Hao Hao 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 Done PS22, Line 17: However network > "However, if" Done 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 think something like: Done PS22, Line 85: can only be interpreted as > "must be specified in" Done 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 s Done 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) Done 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 ei Done Line 252: Status GetLocalNetwork(std::vector<Network>* net) { > nit: rename param to 'nets' since it is a list, not a single one Done PS22, Line 256: auto cleanup = MakeScopedCleanup([&]() { : freeifaddrs(ifap); : }); > this should probably be moved below the error check, otherwise we may freei Done Line 265: > should probably 'net->clear()' here, since the docs say it gets them, not a Done 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. Done PS22, Line 107: ParseString > rename to ParseCIDR so it's clear this is a CIDR style string and not a net Done PS22, Line 111: mask_ > I think conventionally people refer to this as 'netmask' instead of 'mask'. Done PS22, Line 131: GetLocalNetwork > since this gives back a list, change to GetLocalNetworks Done -- 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
