[kudu-CR] MiniKdc for C++

2016-10-20 Thread Adar Dembo (Code Review)
Adar Dembo has submitted this change and it was merged.

Change subject: MiniKdc for C++
..


MiniKdc for C++

Change-Id: I63fc53eeaa1e40b217030adc1ca0c132f43a076c
Reviewed-on: http://gerrit.cloudera.org:8080/4752
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo 
---
M CMakeLists.txt
A cmake_modules/FindKdc.cmake
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/delete_table-test.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/master_migration-itest.cc
A src/kudu/security/CMakeLists.txt
A src/kudu/security/mini_kdc-test.cc
A src/kudu/security/mini_kdc.cc
A src/kudu/security/mini_kdc.h
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/kudu-ts-cli-test.cc
M src/kudu/util/net/net_util.cc
M src/kudu/util/subprocess-test.cc
M src/kudu/util/subprocess.cc
M src/kudu/util/subprocess.h
17 files changed, 589 insertions(+), 34 deletions(-)

Approvals:
  Adar Dembo: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I63fc53eeaa1e40b217030adc1ca0c132f43a076c
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 


[kudu-CR] MiniKdc for C++

2016-10-20 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change.

Change subject: MiniKdc for C++
..


Patch Set 7: Code-Review+2

(1 comment)

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

Line 277:   options_.port = port;
> yea I'm skeptical of the use here, I think we should be usecase-driven in t
I generally prefer to retain information than to discard it, when presented 
with the choice. I agree it's not particularly useful in this case.

The other argument is that it'd let you make options_ a const member, but 
that's also not very strong.


-- 
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: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes


[kudu-CR] MiniKdc for C++

2016-10-20 Thread Dan Burkert (Code Review)
Dan Burkert has posted comments on this change.

Change subject: MiniKdc for C++
..


Patch Set 1:

(4 comments)

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),
> Not done?
As discussed, the GetTestDataDirectory() needs to get called after the 
MiniKdcOptions ctor.


Line 54: vector MiniKdc::MakeArgv(const vector& in_argv) {
> Yes? No?
Yes, but I'd rather not do it in this patch.


Line 85:   RETURN_NOT_OK(CreateKrb5Conf());
> I see. Two things:
1) CreateKrb5Conf is private, is that good  enough?
2) It's explained below, where it's called the second time.


Line 230:   RETURN_NOT_OK(GetBinaryPath("lsof", {"/sbin"}, &lsof));
> You're right; I meant, perhaps we can just run lsof with PATH=/sbin:$PATH a
The three-arg call to GetBinaryPath doesn't reuse the default vector of common 
locations, so this is exactly the same as calling with PATH=/sbin:$PATH.


-- 
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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes


[kudu-CR] MiniKdc for C++

2016-10-20 Thread Alexey Serbin (Code Review)
Alexey Serbin has posted comments on this change.

Change subject: MiniKdc for C++
..


Patch Set 7:

(1 comment)

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

PS7, Line 293: MakeArgv({ kinit, username }), username
btw, if there are some issues with adding that stdin content parameter (size 
limitations, necessity to add tests, etc.), consider as an option the following 
way to invoke kinit:

/bin/sh -C "echo _password_ | kinit _username_"


-- 
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: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes


[kudu-CR] MiniKdc for C++

2016-10-20 Thread Todd Lipcon (Code Review)
Todd Lipcon has posted comments on this change.

Change subject: MiniKdc for C++
..


Patch Set 6:

(1 comment)

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

Line 277:   options_.port = port;
> In what circumstances would that be useful?
yea I'm skeptical of the use here, I think we should be usecase-driven in terms 
of adding functionality to the minicluster. In fact I dont know if we really 
have any use for specifying port at all (not that we need to change it now). 
Think we should try to get this committed ASAP and then change it as we need 
new stuff in it.


-- 
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: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes


[kudu-CR] MiniKdc for C++

2016-10-20 Thread Dan Burkert (Code Review)
Hello Alexey Serbin, Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/4752

to look at the new patch set (#7).

Change subject: MiniKdc for C++
..

MiniKdc for C++

Change-Id: I63fc53eeaa1e40b217030adc1ca0c132f43a076c
---
M CMakeLists.txt
A cmake_modules/FindKdc.cmake
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/delete_table-test.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/master_migration-itest.cc
A src/kudu/security/CMakeLists.txt
A src/kudu/security/mini_kdc-test.cc
A src/kudu/security/mini_kdc.cc
A src/kudu/security/mini_kdc.h
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/kudu-ts-cli-test.cc
M src/kudu/util/net/net_util.cc
M src/kudu/util/subprocess-test.cc
M src/kudu/util/subprocess.cc
M src/kudu/util/subprocess.h
17 files changed, 589 insertions(+), 34 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/52/4752/7
-- 
To view, visit http://gerrit.cloudera.org:8080/4752
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I63fc53eeaa1e40b217030adc1ca0c132f43a076c
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 


[kudu-CR] MiniKdc for C++

2016-10-20 Thread Dan Burkert (Code Review)
Dan Burkert has posted comments on this change.

Change subject: MiniKdc for C++
..


Patch Set 6:

(8 comments)

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

Line 106:   static const vector kCommonLocations = {
> This isn't in sync with FindKdc.cmake.
FindKdc is only looking for the krb5kdc binary, which is only expected to be in 
sbin/.  This method is also used to look for kinit/klist, which can be in bin/.


PS6, Line 123:   << " realm: " << options_.realm
 :   << " port: " << options_.port
 :   << " data_root: " << options_.data_root;
> Nit: may be cleaner to encapsulate this as ToString() in MiniKdcOptions; th
Done


Line 277:   options_.port = port;
> Hmm, this one we may want to store separately, so we can remember that the 
In what circumstances would that be useful?


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

PS6, Line 116: // Writes enough bytes to stdout and stderr concurrently that if 
Call() were
 : // fully reading them one at a time, the test would deadlock.
> I understand that it was easy to piggy-back on this test, but it was explic
Done


PS6, Line 118: StdIn
> Nit: Stdin
Done


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

Line 531: const string& stdin_in) {
> If we want to support arbitrarily large stdin values, I think we'll need to
I can't forsee needing that; I've documented the 64k limit.


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

PS6, Line 108: subprocesses
> Nit: subprocess'
Done


Line 112:  const std::string& stdin_in = "");
> Nit: would prefer if stdin came first. Why?
Done


-- 
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: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes


[kudu-CR] MiniKdc for C++

2016-10-20 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change.

Change subject: MiniKdc for C++
..


Patch Set 6:

(12 comments)

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

Line 44: MiniKdc::MiniKdc()
> Done
Not done?


Line 54: options_.data_root = JoinPathSegments(GetTestDataDirectory(), 
"krb5kdc");
> Might also be interesting to push some of this functionality into Subproces
Yes? No?


Line 85:   for (const auto& location : search) {
> kdb5_util and krb5kdc require the files to exist.
I see. Two things:

1) Let's make sure the new integration test fails if an intrepid developer 
accidentally combines the two CreateKrb5Conf() calls.
2) Please add a comment here explaining why it's being called twice.


Line 230: 
> We can't assume the /sbin is on the path.  TryRunLsof does essentially the 
You're right; I meant, perhaps we can just run lsof with PATH=/sbin:$PATH as 
TryRunLsof() does, instead of resorting to finding it with GetBinaryPath(). I'm 
suggesting that because many of the paths tested in GetBinaryPath() are for 
finding  Kerberos binaries; lsof will definitely not be there.


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

Line 106:   static const vector kCommonLocations = {
This isn't in sync with FindKdc.cmake.


PS6, Line 123:   << " realm: " << options_.realm
 :   << " port: " << options_.port
 :   << " data_root: " << options_.data_root;
Nit: may be cleaner to encapsulate this as ToString() in MiniKdcOptions; that 
way when we add a new option, we just update ToString() to add it to the 
logging.


Line 277:   options_.port = port;
Hmm, this one we may want to store separately, so we can remember that the user 
originally asked for an ephemeral port.


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

PS6, Line 116: // Writes enough bytes to stdout and stderr concurrently that if 
Call() were
 : // fully reading them one at a time, the test would deadlock.
I understand that it was easy to piggy-back on this test, but it was explicitly 
testing a deadlock that I don't believe is possible with stdin.


PS6, Line 118: StdIn
Nit: Stdin


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

Line 531: const string& stdin_in) {
If we want to support arbitrarily large stdin values, I think we'll need to 
integrate the write of stdin into ReadFdsFully (that is, use a poll loop and 
periodically writes more bytes to stdin and read bytes out of stdout/stderr).

Writing more than 64k can block the writer, and if the subprocess is blocked 
writing to stdout/stderr for the same reason, we'll deadlock the two.

For now I'm OK with some docs explaining the stdin limitation, though I'm also 
OK with doing that integration now (shouldn't be too hard).


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

PS6, Line 108: subprocesses
Nit: subprocess'


Line 112:  const std::string& stdin_in = "");
Nit: would prefer if stdin came first. Why?
1) It hews to the order of the fd numbers for stdin/stdout/stderr in Unix (0, 
1, and 2).
2) OUT parameters should be listed after IN parameters.


-- 
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: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes


[kudu-CR] MiniKdc for C++

2016-10-20 Thread Alexey Serbin (Code Review)
Alexey Serbin has posted comments on this change.

Change subject: MiniKdc for C++
..


Patch Set 6: Code-Review+2

(1 comment)

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

PS3, Line 220: lms]
> I haven't tested either, but since the kdc_ports="" option is set in the kd
Ah, good point -- I missed the fact it's in kdc.conf, not in krb5.conf.


-- 
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: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes


[kudu-CR] MiniKdc for C++

2016-10-20 Thread Dan Burkert (Code Review)
Dan Burkert has posted comments on this change.

Change subject: MiniKdc for C++
..


Patch Set 4:

(4 comments)

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

PS4, Line 18: #include 
: #include 
: 
: #include "kudu/security/mini_kdc.h"
: #include "kudu/util/test_util.h"
> Nit: by the style guide https://google.github.io/styleguide/cppguide.html#N
Done


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

PS3, Line 220: $1 = {
> I don't know much about this, but I would think if client is given the same
I haven't tested either, but since the kdc_ports="" option is set in the 
kdc.conf and not krb5.conf, I think there is a good chance that the client will 
not use it.


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

PS4, Line 18: #include "kudu/security/mini_kdc.h"
: 
: #include 
: #include 
: #include 
: #include 
: #include 
: #include 
: 
: #include "kudu/gutil/gscoped_ptr.h"
: #include "kudu/gutil/strings/numbers.h"
: #include "kudu/gutil/strings/split.h"
: #include "kudu/gutil/strings/strip.h"
: #include "kudu/gutil/strings/substitute.h"
: #include "kudu/util/env.h"
: #include "kudu/util/monotime.h"
: #include "kudu/util/path_util.h"
: #include "kudu/util/subprocess.h"
: #include "kudu/util/test_util.h"
> Nit: by the style guide https://google.github.io/styleguide/cppguide.html#N
Done


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

PS4, Line 20: #include 
: #include 
: #include 
: #include 
: 
: #include "kudu/util/status.h"
> Nit: by the style guide https://google.github.io/styleguide/cppguide.html#N
Done


-- 
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: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes


[kudu-CR] MiniKdc for C++

2016-10-20 Thread Dan Burkert (Code Review)
Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/4752

to look at the new patch set (#5).

Change subject: MiniKdc for C++
..

MiniKdc for C++

Change-Id: I63fc53eeaa1e40b217030adc1ca0c132f43a076c
---
M CMakeLists.txt
A cmake_modules/FindKdc.cmake
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/security/CMakeLists.txt
A src/kudu/security/mini_kdc-test.cc
A src/kudu/security/mini_kdc.cc
A src/kudu/security/mini_kdc.h
M src/kudu/util/subprocess-test.cc
M src/kudu/util/subprocess.cc
M src/kudu/util/subprocess.h
10 files changed, 564 insertions(+), 23 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/52/4752/5
-- 
To view, visit http://gerrit.cloudera.org:8080/4752
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I63fc53eeaa1e40b217030adc1ca0c132f43a076c
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 


[kudu-CR] MiniKdc for C++

2016-10-20 Thread Dan Burkert (Code Review)
Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/4752

to look at the new patch set (#6).

Change subject: MiniKdc for C++
..

MiniKdc for C++

Change-Id: I63fc53eeaa1e40b217030adc1ca0c132f43a076c
---
M CMakeLists.txt
A cmake_modules/FindKdc.cmake
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/security/CMakeLists.txt
A src/kudu/security/mini_kdc-test.cc
A src/kudu/security/mini_kdc.cc
A src/kudu/security/mini_kdc.h
M src/kudu/util/subprocess-test.cc
M src/kudu/util/subprocess.cc
M src/kudu/util/subprocess.h
10 files changed, 565 insertions(+), 23 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/52/4752/6
-- 
To view, visit http://gerrit.cloudera.org:8080/4752
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I63fc53eeaa1e40b217030adc1ca0c132f43a076c
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 


[kudu-CR] MiniKdc for C++

2016-10-19 Thread Alexey Serbin (Code Review)
Alexey Serbin has posted comments on this change.

Change subject: MiniKdc for C++
..


Patch Set 4:

(3 comments)

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

PS4, Line 18: #include 
: #include 
: 
: #include "kudu/security/mini_kdc.h"
: #include "kudu/util/test_util.h"
Nit: by the style guide 
https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes 
recommendation, it should be

#include 

#include 

#include "..."


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

PS4, Line 18: #include "kudu/security/mini_kdc.h"
: 
: #include 
: #include 
: #include 
: #include 
: #include 
: #include 
: 
: #include "kudu/gutil/gscoped_ptr.h"
: #include "kudu/gutil/strings/numbers.h"
: #include "kudu/gutil/strings/split.h"
: #include "kudu/gutil/strings/strip.h"
: #include "kudu/gutil/strings/substitute.h"
: #include "kudu/util/env.h"
: #include "kudu/util/monotime.h"
: #include "kudu/util/path_util.h"
: #include "kudu/util/subprocess.h"
: #include "kudu/util/test_util.h"
Nit: by the style guide 
https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes 
recommendation, it should be

#include "kudu/security/mini_kdc.h"

#include 
...
#include 

#include 

#include "..."
...


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

PS4, Line 20: #include 
: #include 
: #include 
: #include 
: 
: #include "kudu/util/status.h"
Nit: by the style guide 
https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes 
recommendation, it should be

#include 
#include 
#include 

#include 

#include "kudu/util/status.h"


-- 
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: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes


[kudu-CR] MiniKdc for C++

2016-10-19 Thread Alexey Serbin (Code Review)
Alexey Serbin has posted comments on this change.

Change subject: MiniKdc for C++
..


Patch Set 3:

(3 comments)

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

Line 33: ADD_KUDU_TEST(mini_kdc-test)
> I don't think it's viable in the long term to limit tests requiring KDC, no
It seems the parallel conversation was around find_package(Kdc) line.  :)

Yes, I think your reasoning makes sense if thinking about long-term goals.


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

PS3, Line 220: udp_preference_limit = 0
> I think we still want this, otherwise the client will prefer connecting to 
I don't know much about this, but I would think if client is given the same 
config, it would understand that the server does not listen on UDP ports.  
However, I haven't tried that and cannot say for sure.  That said, I think it's 
reasonable to keep this option :)


PS3, Line 287: "-q"
> See my reply on the previous revision.
Yep, thanks.  It seems I'm lost in my comments for different revisions.  Sorry 
for the inconsistency.


-- 
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: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes


[kudu-CR] MiniKdc for C++

2016-10-19 Thread Dan Burkert (Code Review)
Dan Burkert has posted comments on this change.

Change subject: MiniKdc for C++
..


Patch Set 3:

(5 comments)

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

Line 33: ADD_KUDU_TEST(mini_kdc-test)
> If kdc is not found, it does not make sense to build and run those.  So, co
I don't think it's viable in the long term to limit tests requiring KDC, nor do 
I think disabling security tests is desirable.


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

PS3, Line 67: KRB5CCNAME=$0
> Instead, consider using 'ccache_name' parameter for the particular realms o
See revision 4.


PS3, Line 183: kdc_ports = $2
> It seems my comment is left among those for PS1, but I'll just re-iterate i
See revision 4.


PS3, Line 220: udp_preference_limit = 0
> This is not needed if using kdc_ports = "" in krb5.conf
I think we still want this, otherwise the client will prefer connecting to the 
KDC via UDP.


PS3, Line 287: "-q"
> The '-q' option does not work in case of my installation of krb5 using MacP
See my reply on the previous revision.


-- 
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: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes


[kudu-CR] MiniKdc for C++

2016-10-19 Thread Alexey Serbin (Code Review)
Alexey Serbin has posted comments on this change.

Change subject: MiniKdc for C++
..


Patch Set 1:

(1 comment)

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

PS1, Line 908: find_package(Kdc)
> > OK, I see -- but that means the KDC-related tests should be run only if t
All right, that's fine with me as well.  I just thought that since krb5 is not 
required, those tests might fail not just due to some error, but due to 
'misconfiguration'.  If this is the desired behavior, then it's fine.


-- 
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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes


[kudu-CR] MiniKdc for C++

2016-10-19 Thread Alexey Serbin (Code Review)
Alexey Serbin has posted comments on this change.

Change subject: MiniKdc for C++
..


Patch Set 1:

(6 comments)

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

Line 33: 
If kdc is not found, it does not make sense to build and run those.  So, 
consider either making these optional by the presence of the krb-related 
binaries, or requiring kdc to be present.


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

PS1, Line 276: "-q"
> One thing that is a little strange is that everything after the -q has to b
nope, I was just running these commands (without '-q') manually while going 
trough some tutorials and then I saw this change and decided to try it.

OK, my mistake was not quoting the query -- after I did it, that worked for me.

Thank you for the clarification.


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

PS3, Line 67: in_argv) {
Instead, consider using 'ccache_name' parameter for the particular realms or 
'default_ccache_name' for libdefaults


PS3, Line 183: }
It seems my comment is left among those for PS1, but I'll just re-iterate it 
here: it's possible to avoid listening on UDP ports by

kdc_ports = ""

Otherwise, TCP and UDP ports are different in case of binding to ephemeral ones.


PS3, Line 220: 
This is not needed if using kdc_ports = "" in krb5.conf


PS3, Line 287: 
The '-q' option does not work in case of my installation of krb5 using 
MacPorts.  Is it possible to get rid of this '-q'?  Does it work without it?


-- 
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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes


[kudu-CR] MiniKdc for C++

2016-10-19 Thread Dan Burkert (Code Review)
Dan Burkert has posted comments on this change.

Change subject: MiniKdc for C++
..


Patch Set 1:

(4 comments)

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

PS1, Line 908: find_package(Kdc)
> OK, I see -- but that means the KDC-related tests should be run only if thi
> OK, I see -- but that means the KDC-related tests should be run only if this 
> is found, otherwise it doesn't make sense to run those.

I disagree, I think its important that security tests always get run.  
Additionally, it's probably not going to be the case for very long that tests 
which rely on security are going to be kept separate.  Eventually we will want 
to switch all tests to use security by option or by default.


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

PS1, Line 171: kdc_ports = 0
> If you want, it's possible to disable UDP ports at all via setting
Done


PS1, Line 206: udp_preference_limit
> See my previous comment on disabling UDP ports.
Done


PS1, Line 276: "-q"
> BTW, with the '-q' options it doesn't work for me on MacOS X krb5 if adding
One thing that is a little strange is that everything after the -q has to be in 
quotes, like:

kadmin.local -q "add_principal -pw dan dan"

Is the test failing on your machine?


-- 
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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes


[kudu-CR] MiniKdc for C++

2016-10-19 Thread Dan Burkert (Code Review)
Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/4752

to look at the new patch set (#4).

Change subject: MiniKdc for C++
..

MiniKdc for C++

Change-Id: I63fc53eeaa1e40b217030adc1ca0c132f43a076c
---
M CMakeLists.txt
A cmake_modules/FindKdc.cmake
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/security/CMakeLists.txt
A src/kudu/security/mini_kdc-test.cc
A src/kudu/security/mini_kdc.cc
A src/kudu/security/mini_kdc.h
M src/kudu/util/subprocess-test.cc
M src/kudu/util/subprocess.cc
M src/kudu/util/subprocess.h
10 files changed, 562 insertions(+), 23 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/52/4752/4
-- 
To view, visit http://gerrit.cloudera.org:8080/4752
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I63fc53eeaa1e40b217030adc1ca0c132f43a076c
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 


[kudu-CR] MiniKdc for C++

2016-10-19 Thread Alexey Serbin (Code Review)
Alexey Serbin has posted comments on this change.

Change subject: MiniKdc for C++
..


Patch Set 1:

(1 comment)

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

PS1, Line 908: find_package(Kdc)
> I don't think it's appropriate to fail the build if KDC can't be found.  Ad
OK, I see -- but that means the KDC-related tests should be run only if this is 
found, otherwise it doesn't make sense to run those.

If we are interested in krb binaries, then it's fine.  Using pkgconfig would 
make more sense if trying to find non-standard locations of libraries/header 
files.  For binaries, packages have exec_prefix, so it's possible to find 
binaries as well, as I understand.


-- 
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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes


[kudu-CR] MiniKdc for C++

2016-10-19 Thread Alexey Serbin (Code Review)
Alexey Serbin has posted comments on this change.

Change subject: MiniKdc for C++
..


Patch Set 1:

(4 comments)

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

PS1, Line 60: KRB5_CCNAME
> Should it be KRB5CCNAME instead?
Also, what about using configuration parameter 'default_ccache_name' in 
krb5.conf instead?


PS1, Line 171: kdc_ports = 0
If you want, it's possible to disable UDP ports at all via setting

kdc_ports = ""


PS1, Line 206: udp_preference_limit
See my previous comment on disabling UDP ports.


PS1, Line 276: "-q"
BTW, with the '-q' options it doesn't work for me on MacOS X krb5 if adding a 
principal.  It even does not work for me for get_principal.  It seems MacPort 
version of krb5 (1.14.3) and brew's version is different a bit.

I'm curios: what if we drop the '-q' option?  Would it work for Linux and briew 
krb5?


-- 
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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes


[kudu-CR] MiniKdc for C++

2016-10-19 Thread Alexey Serbin (Code Review)
Alexey Serbin has posted comments on this change.

Change subject: MiniKdc for C++
..


Patch Set 1:

(1 comment)

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

Line 127: RETURN_NOT_OK(proc->Kill(SIGKILL));
> Dan had TERM originally and I changed it to KILL, because it was sometimes 
ok, I see.  thanks for the explanation.


-- 
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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes


[kudu-CR] MiniKdc for C++

2016-10-19 Thread Dan Burkert (Code Review)
Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/4752

to look at the new patch set (#3).

Change subject: MiniKdc for C++
..

MiniKdc for C++

Change-Id: I63fc53eeaa1e40b217030adc1ca0c132f43a076c
---
M CMakeLists.txt
A cmake_modules/FindKdc.cmake
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/security/CMakeLists.txt
A src/kudu/security/mini_kdc-test.cc
A src/kudu/security/mini_kdc.cc
A src/kudu/security/mini_kdc.h
M src/kudu/util/subprocess-test.cc
M src/kudu/util/subprocess.cc
M src/kudu/util/subprocess.h
10 files changed, 564 insertions(+), 23 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/52/4752/3
-- 
To view, visit http://gerrit.cloudera.org:8080/4752
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I63fc53eeaa1e40b217030adc1ca0c132f43a076c
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 


[kudu-CR] MiniKdc for C++

2016-10-19 Thread Dan Burkert (Code Review)
Dan Burkert has posted comments on this change.

Change subject: MiniKdc for C++
..


Patch Set 1:

(41 comments)

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

PS1, Line 908: find_package(Kdc)
> Is it required or not?  If yes, consider adding REQUIRED attribute:
I don't think it's appropriate to fail the build if KDC can't be found.  Adar 
felt differently when I brought it up with him.  The effect of not making it 
required is that a warning is printed when it can't be found.

Not sure I fully understand about pkgconfig.  We're just looking for the 
Kerberos binaries, not the headers/libraries.  Can pkgconfig still be used for 
this?


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


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?
Done


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 c
Done


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


PS1, Line 60: KRB5_CCNAME
> Should it be KRB5CCNAME instead?
Done


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


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 erro
Done


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


Line 85:   RETURN_NOT_OK(CreateKrb5Conf());
> How about deferring this one until after the port determination, so we need
kdb5_util and krb5kdc require the files to exist.


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


Line 122: Status MiniKdc::Stop() {
> Would be nice to test Start(), Stop(), then Start() (since the implementati
Done


Line 125:   if (proc) {
> Don't want to enforce that Stop() can only be called if the process is actu
Done


PS1, Line 128: nullptr
> Since a10 has been merged already, you can omit nullptr since that's th
Done


Line 136: "/usr/local/opt/krb5/sbin", // Homebrew
> Please add "/opt/local/sbin" and "/opt/local/bin" to adapt for MacPorts as 
Done


Line 145:   string* path) const {
> As per our usual call convention, would rather we didn't mutate the OUT par
Done


PS1, Line 151: *path
> Consider using local variable instead, and assigning to the output paramete
Done


Line 152: if (env_->FileExists(*path)) {
> Should we also test that the path can be executed?
Env doesn't expose a way to test for that, as far as I can tell.


PS1, Line 169: string
> static const?
Done


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


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 TryR
We can't assume the /sbin is on the path.  TryRunLsof does essentially the same 
thing as this.


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 c
Done


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.
Done


Line 38:   // The default may only be used from a gtest unit test.
> Isn't MiniKdc itself only intended for test usage anyway?
This is following the behavior of MiniCluster.  I don't think we will end up 
using this outside the context of gtest, but it doesn't seem bad to doc it 
anyway.


Line 48:   MiniKdc(Env* env, const MiniKdcOptions& options);
> I don't think taking an env is really necessary, since there's no reason wh
Done


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


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


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

[kudu-CR] MiniKdc for C++

2016-10-19 Thread Dan Burkert (Code Review)
Dan Burkert has posted comments on this change.

Change subject: MiniKdc for C++
..


Patch Set 1:

(3 comments)

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

PS1, Line 135: vector
> static const?
Done


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


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

Line 46: class MiniKdc {
> Nit: if we want to be consistent with cluster class naming, I'd suggest Ext
I don't think it's going to cause much confusion, e.g. it hasn't been a problem 
for Java's mini cluster.


-- 
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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes


[kudu-CR] MiniKdc for C++

2016-10-19 Thread Dan Burkert (Code Review)
Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/4752

to look at the new patch set (#2).

Change subject: MiniKdc for C++
..

MiniKdc for C++

Change-Id: I63fc53eeaa1e40b217030adc1ca0c132f43a076c
---
M CMakeLists.txt
A cmake_modules/FindKdc.cmake
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/security/CMakeLists.txt
A src/kudu/security/mini_kdc-test.cc
A src/kudu/security/mini_kdc.cc
A src/kudu/security/mini_kdc.h
M src/kudu/util/subprocess-test.cc
M src/kudu/util/subprocess.cc
M src/kudu/util/subprocess.h
10 files changed, 564 insertions(+), 23 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/52/4752/2
-- 
To view, visit http://gerrit.cloudera.org:8080/4752
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I63fc53eeaa1e40b217030adc1ca0c132f43a076c
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 


[kudu-CR] MiniKdc for C++

2016-10-19 Thread Todd Lipcon (Code Review)
Todd Lipcon has posted comments on this change.

Change subject: MiniKdc for C++
..


Patch Set 1:

(1 comment)

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

Line 127: RETURN_NOT_OK(proc->Kill(SIGKILL));
> Why SIGKILL, not SIGTERM?  Just for brevity and less code working around th
Dan had TERM originally and I changed it to KILL, because it was sometimes 
dumping stuff to the log during TERM, and we don't really need a graceful 
cleanup anyway.


-- 
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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes


[kudu-CR] MiniKdc for C++

2016-10-19 Thread Alexey Serbin (Code Review)
Alexey Serbin has posted comments on this change.

Change subject: MiniKdc for C++
..


Patch Set 1:

(10 comments)

First pass, will do more soon.

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

PS1, Line 908: find_package(Kdc)
Is it required or not?  If yes, consider adding REQUIRED attribute:

find_package(Kdc REQUIRED)

BTW, does find_package work for Kerberos5/krb5?  I could not find appropriate 
FindXxx.cmake file if using MacPorts on MacOS X.  If interested in finding 
appropriate include/library directories, consider using pkgconfig functionality 
for that, if needed.  As an example, you could check 
src/kudu/twitter-demo/CMakeLists.txt for liboauth.


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 (b
Consider using pkgconfig instead -- I added comment for CMakeLists.txt about 
that.  Using pkgconfig is more flexible and does not require hard-coded 
locations in here.

If pkgconfig is not an option, then please add /opt/local/sbin as well (that's 
where MacPorts installs it by default).


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

PS1, Line 60: KRB5_CCNAME
Should it be KRB5CCNAME instead?


Line 127: RETURN_NOT_OK(proc->Kill(SIGKILL));
Why SIGKILL, not SIGTERM?  Just for brevity and less code working around the 
lingering process or there is something crucial here?  It would be nice to have 
a comment about that, if having that makes any sense to you.


PS1, Line 128: nullptr
Since a10 has been merged already, you can omit nullptr since that's the 
parameter value by default.


PS1, Line 135: vector
static const?


Line 136: "/usr/local/opt/krb5/sbin", // Homebrew
Please add "/opt/local/sbin" and "/opt/local/bin" to adapt for MacPorts as well.


PS1, Line 151: *path
Consider using local variable instead, and assigning to the output parameter 
only before return from the function.


PS1, Line 169: string
static const?


PS1, Line 291: nullptr
Since a10 nullptr can be omitted.


-- 
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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes


[kudu-CR] MiniKdc for C++

2016-10-19 Thread Todd Lipcon (Code Review)
Todd Lipcon has posted comments on this change.

Change subject: MiniKdc for C++
..


Patch Set 1:

(1 comment)

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

Line 48:   MiniKdc(Env* env, const MiniKdcOptions& options);
I don't think taking an env is really necessary, since there's no reason why 
you'd want to write to any env except the local filesystem (since you have to 
run the external binary against it anyway)

For options maybe we can have it default to the default options, since most 
tests probably don't need to tweak them?


-- 
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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes


[kudu-CR] MiniKdc for C++

2016-10-18 Thread Adar Dembo (Code Review)
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 MiniKdc::MakeArgv(const vector& 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 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 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 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 cl

[kudu-CR] MiniKdc for C++

2016-10-18 Thread Dan Burkert (Code Review)
Dan Burkert has posted comments on this change.

Change subject: MiniKdc for C++
..


Patch Set 1:

(1 comment)

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

PS1, Line 70:  caceh
cache


-- 
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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes


[kudu-CR] MiniKdc for C++

2016-10-18 Thread Dan Burkert (Code Review)
Hello Adar Dembo, Todd Lipcon, Alexey Serbin,

I'd like you to do a code review.  Please visit

http://gerrit.cloudera.org:8080/4752

to review the following change.

Change subject: MiniKdc for C++
..

MiniKdc for C++

Change-Id: I63fc53eeaa1e40b217030adc1ca0c132f43a076c
---
M CMakeLists.txt
A cmake_modules/FindKdc.cmake
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/security/CMakeLists.txt
A src/kudu/security/mini_kdc-test.cc
A src/kudu/security/mini_kdc.cc
A src/kudu/security/mini_kdc.h
M src/kudu/util/subprocess.cc
M src/kudu/util/subprocess.h
9 files changed, 549 insertions(+), 16 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/52/4752/1
-- 
To view, visit http://gerrit.cloudera.org:8080/4752
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I63fc53eeaa1e40b217030adc1ca0c132f43a076c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Todd Lipcon