[kudu-CR] Enable GSSAPI for servers and ExternalMiniCluster

["Dan Burkert (Code Review)" <ge...@cloudera.org>]

Dan Burkert has posted comments on this change.

Change subject: Enable GSSAPI for servers and ExternalMiniCluster
......................................................................


Patch Set 11:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/4765/11/src/kudu/integration-tests/external_mini_cluster.cc
File src/kudu/integration-tests/external_mini_cluster.cc:

Line 145:                           "could not create client principal");
Are the status' returned from these methods not detailed enough?  Maybe we should fix it there, instead of adding this to the call sites.


http://gerrit.cloudera.org:8080/#/c/4765/11/src/kudu/integration-tests/external_mini_cluster.h
File src/kudu/integration-tests/external_mini_cluster.h:

Line 123:   bool enable_kerberos;
I think we need to have a discussion about what different configuration options will look like for Kudu before we commit some of these things.  For instance, it might be better to name this 'enable_security', so that when we add alternate authorization mechanisms, we can use those.  Or perhaps 'require_authentication' could be somewhere between the two (not specific to kerberos, but more fine grained than just security).

We don't need to change this yet, but we should figure out the design sooner rather than later.


http://gerrit.cloudera.org:8080/#/c/4765/11/src/kudu/rpc/connection.cc
File src/kudu/rpc/connection.cc:

Line 55: DEFINE_bool(server_require_kerberos, false,
Why is 'server' in the flag name? Flags are only used server side.

Echoing what I said earlier, but we should spend some time thinking through this.  I have a feeling 'require_authentication' will be a better flag.

Also, I'm not sure if we can pull this off since it would technically be a backwards incompatible change, but we should strongly consider defaulting this to true.


http://gerrit.cloudera.org:8080/#/c/4765/11/src/kudu/util/subprocess.h
File src/kudu/util/subprocess.h:

Line 71:   void SetEnvVars(std::map<std::string, std::string> env);
This is good, but it looks like we aren't using it anywhere?


-- 
To view, visit http://gerrit.cloudera.org:8080/4765
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I595469e9cc8b2b2f57e9726014eeeb8112595801
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes