Adar Dembo has posted comments on this change.

Change subject: MiniKdc for C++
......................................................................


Patch Set 1:

(39 comments)

http://gerrit.cloudera.org:8080/#/c/4752/1/CMakeLists.txt
File CMakeLists.txt:

Line 1054: # Google util libraries borrowed from supersonic, tcmalloc, 
Chromium, etc.
This comment needs to move to gutil.


http://gerrit.cloudera.org:8080/#/c/4752/1/cmake_modules/FindKdc.cmake
File cmake_modules/FindKdc.cmake:

Line 25:              # Linux install location.
Need to also handle SLES, which I believe places it in /usr/lib/mit/sbin (but 
please double check).

Also, if you want to be complete, add /usr/kerberos/sbin for el5.

(I gleaned all these locations from the internal cdep repo, specifically from 
the code that runs kinit).


http://gerrit.cloudera.org:8080/#/c/4752/1/src/kudu/security/CMakeLists.txt
File src/kudu/security/CMakeLists.txt:

Line 25:   kudu_common
Is this dependency actually needed?


http://gerrit.cloudera.org:8080/#/c/4752/1/src/kudu/security/mini_kdc-test.cc
File src/kudu/security/mini_kdc-test.cc:

Line 34:   kdc.Stop();
Can we ASSERT that this worked too?


http://gerrit.cloudera.org:8080/#/c/4752/1/src/kudu/security/mini_kdc.cc
File src/kudu/security/mini_kdc.cc:

Line 44:       realm_(options.realm.empty() ? "KRBTEST.COM" : options.realm),
Would prefer if the default option values were set up in a MiniKdcOptions 
constructor, since that's where the comments about the default values are found.


Line 51:   Stop();
Add WARN_NOT_OK() in case this fails?


Line 54: vector<string> MiniKdc::MakeArgv(const vector<string>& in_argv) {
Might also be interesting to push some of this functionality into Subprocess 
(i.e. allow the creation of subprocesses with an arbitrary map of env 
variables).


PS1, Line 62:   vector<string> real_argv = {
            :       "env", krb5_config,
            :       "env", krb5_kdc_profile,
            :       "env", krb5_ccname,
            :   };
Why is env listed multiple times? Shouldn't the final call be:

  env KRB5_CONFIG=foo KRB5_KDC_PROFILE=bar KRB5_CCNAME=baz ...


PS1, Line 74:   if (kdc_process_) {
            :     return Status::IllegalState("Kerberos KDC already started");
            :   }
I think this could be a CHECK() too, seems indicative of a programming error.


Line 83:   RETURN_NOT_OK(env_->CreateDir(data_root_));
This will fail if data_root_ exists already. Is that intentional?


Line 85:   RETURN_NOT_OK(CreateKrb5Conf());
How about deferring this one until after the port determination, so we need 
only do it once (regardless of whether we've asked for an ephemeral port or 
not)?


Line 87:   // Common Kerberos environment variables.
What's this referring to?


Line 122: Status MiniKdc::Stop() {
Would be nice to test Start(), Stop(), then Start() (since the implementation 
here looks like it allows it).


Line 125:   if (proc) {
Don't want to enforce that Stop() can only be called if the process is actually 
started?


Line 135:   vector<string> common_locations = {
See my earlier comment in FindKdc.cmake about additional locations.

May also want to make this vector static.


Line 145:                               string* path) const {
As per our usual call convention, would rather we didn't mutate the OUT 
parameter unless the function succeeds.


Line 152:     if (env_->FileExists(*path)) {
Should we also test that the path can be executed?


PS1, Line 215:   gscoped_ptr<RWFile> file;
             :   RETURN_NOT_OK(env_->NewRWFile(JoinPathSegments(data_root_, 
"krb5.conf"), &file));
             : 
             :   return file->Write(0, file_contents);
Can't use WriteStringToFile here?


PS1, Line 223: The KDC doesn't log the bound port or expose it
             :   // in any other fashion, and re-implementing lsof involves 
parsing a lot of
             :   // files in /proc/.
FWIW, delete_table-test uses lsof, as does the code in net_util, so there's no 
reason to be sad about its use here.


Line 230:   RETURN_NOT_OK(GetBinaryPath("lsof", {"/sbin"}, &lsof));
You don't think we can assume that lsof is on the PATH? Take a look at 
TryRunLsof() and the invocation in delete_table-test for inspiration.


PS1, Line 283:   Subprocess proc("env",
             :       MakeArgv({
             :         kinit,
             :         username
             :   }));
             : 
             :   RETURN_NOT_OK(proc.Start());
             :   RETURN_NOT_OK(proc.WriteToStdIn(username));
             :   RETURN_NOT_OK(proc.Wait(nullptr));
I think this could benefit from a Subprocess::Call() variant that allowed 
callers to provide stdin.


http://gerrit.cloudera.org:8080/#/c/4752/1/src/kudu/security/mini_kdc.h
File src/kudu/security/mini_kdc.h:

Line 24: #include "kudu/util/subprocess.h"
Can be forward declared.


Line 38:   // The default may only be used from a gtest unit test.
Isn't MiniKdc itself only intended for test usage anyway?


Line 46: class MiniKdc {
Nit: if we want to be consistent with cluster class naming, I'd suggest 
ExternalMiniKdc, to make it obvious that this is NOT an in-proc KDC, but rather 
a completely separate process.


Line 49:   virtual ~MiniKdc();
Why virtual?


Line 55:   Status Stop();
No WARN_UNUSED_RESULT here?


Line 58:     CHECK(kdc_process_) << "must start first";
Missing an include for CHECK()?


PS1, Line 66: with
Nit: "to" (I think that's more precise?)


Line 71:   Status Klist() WARN_UNUSED_RESULT;
Shouldn't the output be captured and made available programmatically, via a 
string or some such?


Line 74:   std::vector<std::string> MakeArgv(const std::vector<std::string>& 
in_argv);
Missing an include for vector.

Also, could you doc this one too?


Line 76:   // Attempts to find the path to the specified Kerberos binary, 
storing it in 'path'.
These can be static functions.


Line 89:   // Determine the ports that the KDC bound to.
Should doc that this may sleep if necessary. Maybe even rename it to something 
like WaitForKdcPorts...


PS1, Line 94:   std::string realm_;
            :   std::string data_root_;
            :   uint16_t kdc_port_;
Maybe just store a copy of the Options struct locally? One less place to update 
when a new option is added...


http://gerrit.cloudera.org:8080/#/c/4752/1/src/kudu/util/subprocess.cc
File src/kudu/util/subprocess.cc:

Line 539:   FILE* file = fdopen(to_child_stdin_fd(), "w");
What does the FILE wrapping actually buy you? Can't you just use the write() 
syscall directly on the raw fd?

If you do switch, you'll probably need to loop for EINTR. See RETRY_ON_EINTR in 
env_posix.cc.


Line 545:   if (fclose(file)) {
This also closes the underlying fd. I doubt that's what you want, but if you 
do, you'll need to use ReleaseChildFd() or one of its variants to prevent 
Subprocess callers from trying to reuse the fd after this call.


Line 550:     return Status::IOError("Unable to write to subprocess stdin");
Let's capture the error with ferror(3) (or errno if you use the write syscall) 
and include it.


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

How about a unit test for the new Subprocess functionality?


PS1, Line 122: processes
Nit: process'


PS1, Line 123: StdIn
Nit: Stdin, to be consistent with ReleaseChildStdinFd().


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I63fc53eeaa1e40b217030adc1ca0c132f43a076c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <danburk...@apache.org>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <danburk...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes

Reply via email to