Todd Lipcon has posted comments on this change. Change subject: KUDU-1875: Refuse unauthenticated connections from publicly routable IP addrs ......................................................................
Patch Set 34: (11 comments) http://gerrit.cloudera.org:8080/#/c/6514/34/src/kudu/rpc/server_negotiation.cc File src/kudu/rpc/server_negotiation.cc: PS34, Line 66: allows nit: "to allow" Line 95: vector<Network>* g_local_networks = new vector<kudu::Network>(); I think it's better to initialize this to nullptr, and then set it to the vector only after parsing/initializing the value. That way if we have a bug where we accidentally use it before it's been properly set, we'd get a crash instead of silent wrong results. Also, please wrap this in an anonymous namespace so that the symbol doesn't get exported to other translation units PS34, Line 209: NegotiatePB request; : RETURN_NOT_OK(RecvNegotiatePB(&request, &recv_buf)); can you add a comment why we need to consume this? not quite following, since 'request' is unused PS34, Line 212: or unauthenticated this error can be more specific to unencrypted, right? PS34, Line 729: unencrypted or unauthenticated this one can be specific to unauthenticated Line 914: std::call_once(once, [] { the initialization of g_local_networks could be moved inside the 'if' statement below on line 925. this would help us provide a workaround in the case that, for some reason, GetLocalNetworks crashes, we can configure explicit networks and avoid calling it at all. PS34, Line 915: GetLocalNetworks does this need WARN_NOT_OK or something? PS34, Line 919: KUDU_C can just be CHECK_OK (the 'KUDU_...' variants are only used in the context of client headers) PS34, Line 926: = can use |= here instead of the || on line 928 http://gerrit.cloudera.org:8080/#/c/6514/34/src/kudu/util/net/net_util.h File src/kudu/util/net/net_util.h: PS34, Line 95: is nit: are PS34, Line 113: vector Network nit: "vector of" -- 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: 34 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Harsh J <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
