[kudu-CR] MiniKdc for C++
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++
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++
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++
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++
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++
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++
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++
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++
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++
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++
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++
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++
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++
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++
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++
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++
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++
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++
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++
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++
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++
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++
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++
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++
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++
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++
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++
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++
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++
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++
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++
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
