Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/8270 )
Change subject: IMPALA-5053: [SECURITY] Make KRPC work with Kerberos ...................................................................... Patch Set 6: (12 comments) http://gerrit.cloudera.org:8080/#/c/8270/6/be/src/rpc/authentication.cc File be/src/rpc/authentication.cc: http://gerrit.cloudera.org:8080/#/c/8270/6/be/src/rpc/authentication.cc@1033 PS6, Line 1033: RETURN_IF_ERROR(GetInternalKerberosPrincipal(&kerberos_internal_principal)); : RETURN_IF_ERROR(GetExternalKerberosPrincipal(&kerberos_external_principal)); : if (!kerberos_internal_principal.empty()) { : RETURN_IF_ERROR(InitKerberosEnv()); : } > Does it make sense to write it as: Done. Using 'bool use_kerberos' http://gerrit.cloudera.org:8080/#/c/8270/6/be/src/rpc/authentication.cc@1055 PS6, Line 1055: !kerberos_internal_principal.empty() > if (IsKerberosEnabled()) { Done. I added the DCHECK in the if() condition above, so I didn't redo it here. http://gerrit.cloudera.org:8080/#/c/8270/6/be/src/rpc/authentication.cc@1070 PS6, Line 1070: !kerberos_external_principal.empty() > IsKerberosEnabled(). Should we just declare the following above and use it Yes, that's much more readable. Done. http://gerrit.cloudera.org:8080/#/c/8270/6/be/src/rpc/rpc-mgr.h File be/src/rpc/rpc-mgr.h: http://gerrit.cloudera.org:8080/#/c/8270/6/be/src/rpc/rpc-mgr.h@180 PS6, Line 180: /// The following strings preserve the Kudu flags original values to restore in : /// Shutdown() as they will be modified by us. : string flag_save_rpc_authentication; > As mentioned in another place, is this actually necessary if we are shuttin I removed this now. http://gerrit.cloudera.org:8080/#/c/8270/6/be/src/rpc/rpc-mgr.cc File be/src/rpc/rpc-mgr.cc: http://gerrit.cloudera.org:8080/#/c/8270/6/be/src/rpc/rpc-mgr.cc@75 PS6, Line 75: FLAGS_rpc_authentication = "required"; > Meant to say Kudu library code. Ah I forgot to do this earlier. I now asked the Kudu team about this and the feedback was that changing any Kudu flags would affect the client as well. So I think we should leave these flags as is, i.e. as the defaults. I reverted the setting of this flag. http://gerrit.cloudera.org:8080/#/c/8270/6/be/src/rpc/rpc-mgr.cc@128 PS6, Line 128: FLAGS_rpc_authentication = flag_save_rpc_authentication; > Is there a reason why we have to restore this flag if we are shutting down I removed this now. But the reason for restoring it was that our tests start the RpcMgr multiple times, so we don't want the setting of one RpcMgr config to leak into the next. http://gerrit.cloudera.org:8080/#/c/8270/6/be/src/testutil/mini-kdc-wrapper.h File be/src/testutil/mini-kdc-wrapper.h: http://gerrit.cloudera.org:8080/#/c/8270/6/be/src/testutil/mini-kdc-wrapper.h@38 PS6, Line 38: /// If the mode is USE_KUDU_KERBEROS or USE_IMPALA_KERBEROS, the MiniKdc which is a wrapper > nit: long line Done http://gerrit.cloudera.org:8080/#/c/8270/6/be/src/testutil/mini-kdc-wrapper.h@71 PS6, Line 71: std::string ticket_lifetime_; > const Done http://gerrit.cloudera.org:8080/#/c/8270/6/be/src/testutil/mini-kdc-wrapper.h@74 PS6, Line 74: std::string renew_lifetime_; > const Done http://gerrit.cloudera.org:8080/#/c/8270/6/be/src/testutil/mini-kdc-wrapper.h@77 PS6, Line 77: int kdc_port_; > const Done http://gerrit.cloudera.org:8080/#/c/8270/6/be/src/util/auth-util.h File be/src/util/auth-util.h: http://gerrit.cloudera.org:8080/#/c/8270/6/be/src/util/auth-util.h@49 PS6, Line 49: /// 'out_principal' will contain the principal string if the cluster is configured to use : /// kerberos, otherwise it will be an empty string. > As mentioned in auth-util.cc, we may want to call this function only when K Done http://gerrit.cloudera.org:8080/#/c/8270/6/be/src/util/auth-util.cc File be/src/util/auth-util.cc: http://gerrit.cloudera.org:8080/#/c/8270/6/be/src/util/auth-util.cc@75 PS6, Line 75: FLAGS_principal.empty() > !IsKerberosEnabled(). Yes, that's right. I've changed it to a DCHECK -- To view, visit http://gerrit.cloudera.org:8080/8270 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8cec5cca5fdb4b1d46bab19e86cb1a8a3ad718fd Gerrit-Change-Number: 8270 Gerrit-PatchSet: 6 Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-Comment-Date: Wed, 29 Nov 2017 17:17:08 +0000 Gerrit-HasComments: Yes
