You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Dan Burkert (Code Review)" <ge...@cloudera.org> on 2016/10/19 00:49:41 UTC
[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 <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] MiniKdc for C++
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] MiniKdc for C++
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <gtest/gtest.h>
: #include <string>
:
: #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 <string>
#include <gtest/gtest.h>
#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 <csignal>
: #include <cstdio>
: #include <glog/logging.h>
: #include <limits>
: #include <memory>
: #include <string>
:
: #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 <csignal>
...
#include <string>
#include <glog/logging.h>
#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 <glog/logging.h>
: #include <memory>
: #include <string>
: #include <vector>
:
: #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 <memory>
#include <string>
#include <vector>
#include <glog/logging.h>
#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 <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] MiniKdc for C++
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
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 <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] MiniKdc for C++
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <ad...@cloudera.com>
---
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 <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] MiniKdc for C++
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] MiniKdc for C++
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
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 <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] MiniKdc for C++
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] MiniKdc for C++
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
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 <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] MiniKdc for C++
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
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<string> MiniKdc::MakeArgv(const vector<string>& 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 <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] MiniKdc for C++
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
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 <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] MiniKdc for C++
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] MiniKdc for C++
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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<string> 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 <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] MiniKdc for C++
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
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<string>
> static const?
Done
Line 135: vector<string> 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 <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] MiniKdc for C++
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
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 <gtest/gtest.h>
: #include <string>
:
: #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 <csignal>
: #include <cstdio>
: #include <glog/logging.h>
: #include <limits>
: #include <memory>
: #include <string>
:
: #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 <glog/logging.h>
: #include <memory>
: #include <string>
: #include <vector>
:
: #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 <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] MiniKdc for C++
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] MiniKdc for C++
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] MiniKdc for C++
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
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 <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] MiniKdc for C++
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: MiniKdc for C++
......................................................................
Patch Set 1:
(39 comments)
http://gerrit.cloudera.org:8080/#/c/4752/1/CMakeLists.txt
File CMakeLists.txt:
Line 1054: # Google util libraries borrowed from supersonic, tcmalloc, Chromium, etc.
This comment needs to move to gutil.
http://gerrit.cloudera.org:8080/#/c/4752/1/cmake_modules/FindKdc.cmake
File cmake_modules/FindKdc.cmake:
Line 25: # Linux install location.
Need to also handle SLES, which I believe places it in /usr/lib/mit/sbin (but please double check).
Also, if you want to be complete, add /usr/kerberos/sbin for el5.
(I gleaned all these locations from the internal cdep repo, specifically from the code that runs kinit).
http://gerrit.cloudera.org:8080/#/c/4752/1/src/kudu/security/CMakeLists.txt
File src/kudu/security/CMakeLists.txt:
Line 25: kudu_common
Is this dependency actually needed?
http://gerrit.cloudera.org:8080/#/c/4752/1/src/kudu/security/mini_kdc-test.cc
File src/kudu/security/mini_kdc-test.cc:
Line 34: kdc.Stop();
Can we ASSERT that this worked too?
http://gerrit.cloudera.org:8080/#/c/4752/1/src/kudu/security/mini_kdc.cc
File src/kudu/security/mini_kdc.cc:
Line 44: realm_(options.realm.empty() ? "KRBTEST.COM" : options.realm),
Would prefer if the default option values were set up in a MiniKdcOptions constructor, since that's where the comments about the default values are found.
Line 51: Stop();
Add WARN_NOT_OK() in case this fails?
Line 54: vector<string> MiniKdc::MakeArgv(const vector<string>& in_argv) {
Might also be interesting to push some of this functionality into Subprocess (i.e. allow the creation of subprocesses with an arbitrary map of env variables).
PS1, Line 62: vector<string> real_argv = {
: "env", krb5_config,
: "env", krb5_kdc_profile,
: "env", krb5_ccname,
: };
Why is env listed multiple times? Shouldn't the final call be:
env KRB5_CONFIG=foo KRB5_KDC_PROFILE=bar KRB5_CCNAME=baz ...
PS1, Line 74: if (kdc_process_) {
: return Status::IllegalState("Kerberos KDC already started");
: }
I think this could be a CHECK() too, seems indicative of a programming error.
Line 83: RETURN_NOT_OK(env_->CreateDir(data_root_));
This will fail if data_root_ exists already. Is that intentional?
Line 85: RETURN_NOT_OK(CreateKrb5Conf());
How about deferring this one until after the port determination, so we need only do it once (regardless of whether we've asked for an ephemeral port or not)?
Line 87: // Common Kerberos environment variables.
What's this referring to?
Line 122: Status MiniKdc::Stop() {
Would be nice to test Start(), Stop(), then Start() (since the implementation here looks like it allows it).
Line 125: if (proc) {
Don't want to enforce that Stop() can only be called if the process is actually started?
Line 135: vector<string> common_locations = {
See my earlier comment in FindKdc.cmake about additional locations.
May also want to make this vector static.
Line 145: string* path) const {
As per our usual call convention, would rather we didn't mutate the OUT parameter unless the function succeeds.
Line 152: if (env_->FileExists(*path)) {
Should we also test that the path can be executed?
PS1, Line 215: gscoped_ptr<RWFile> file;
: RETURN_NOT_OK(env_->NewRWFile(JoinPathSegments(data_root_, "krb5.conf"), &file));
:
: return file->Write(0, file_contents);
Can't use WriteStringToFile here?
PS1, Line 223: The KDC doesn't log the bound port or expose it
: // in any other fashion, and re-implementing lsof involves parsing a lot of
: // files in /proc/.
FWIW, delete_table-test uses lsof, as does the code in net_util, so there's no reason to be sad about its use here.
Line 230: RETURN_NOT_OK(GetBinaryPath("lsof", {"/sbin"}, &lsof));
You don't think we can assume that lsof is on the PATH? Take a look at TryRunLsof() and the invocation in delete_table-test for inspiration.
PS1, Line 283: Subprocess proc("env",
: MakeArgv({
: kinit,
: username
: }));
:
: RETURN_NOT_OK(proc.Start());
: RETURN_NOT_OK(proc.WriteToStdIn(username));
: RETURN_NOT_OK(proc.Wait(nullptr));
I think this could benefit from a Subprocess::Call() variant that allowed callers to provide stdin.
http://gerrit.cloudera.org:8080/#/c/4752/1/src/kudu/security/mini_kdc.h
File src/kudu/security/mini_kdc.h:
Line 24: #include "kudu/util/subprocess.h"
Can be forward declared.
Line 38: // The default may only be used from a gtest unit test.
Isn't MiniKdc itself only intended for test usage anyway?
Line 46: class MiniKdc {
Nit: if we want to be consistent with cluster class naming, I'd suggest ExternalMiniKdc, to make it obvious that this is NOT an in-proc KDC, but rather a completely separate process.
Line 49: virtual ~MiniKdc();
Why virtual?
Line 55: Status Stop();
No WARN_UNUSED_RESULT here?
Line 58: CHECK(kdc_process_) << "must start first";
Missing an include for CHECK()?
PS1, Line 66: with
Nit: "to" (I think that's more precise?)
Line 71: Status Klist() WARN_UNUSED_RESULT;
Shouldn't the output be captured and made available programmatically, via a string or some such?
Line 74: std::vector<std::string> MakeArgv(const std::vector<std::string>& in_argv);
Missing an include for vector.
Also, could you doc this one too?
Line 76: // Attempts to find the path to the specified Kerberos binary, storing it in 'path'.
These can be static functions.
Line 89: // Determine the ports that the KDC bound to.
Should doc that this may sleep if necessary. Maybe even rename it to something like WaitForKdcPorts...
PS1, Line 94: std::string realm_;
: std::string data_root_;
: uint16_t kdc_port_;
Maybe just store a copy of the Options struct locally? One less place to update when a new option is added...
http://gerrit.cloudera.org:8080/#/c/4752/1/src/kudu/util/subprocess.cc
File src/kudu/util/subprocess.cc:
Line 539: FILE* file = fdopen(to_child_stdin_fd(), "w");
What does the FILE wrapping actually buy you? Can't you just use the write() syscall directly on the raw fd?
If you do switch, you'll probably need to loop for EINTR. See RETRY_ON_EINTR in env_posix.cc.
Line 545: if (fclose(file)) {
This also closes the underlying fd. I doubt that's what you want, but if you do, you'll need to use ReleaseChildFd() or one of its variants to prevent Subprocess callers from trying to reuse the fd after this call.
Line 550: return Status::IOError("Unable to write to subprocess stdin");
Let's capture the error with ferror(3) (or errno if you use the write syscall) and include it.
http://gerrit.cloudera.org:8080/#/c/4752/1/src/kudu/util/subprocess.h
File src/kudu/util/subprocess.h:
How about a unit test for the new Subprocess functionality?
PS1, Line 122: processes
Nit: process'
PS1, Line 123: StdIn
Nit: Stdin, to be consistent with ReleaseChildStdinFd().
--
To view, visit http://gerrit.cloudera.org:8080/4752
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I63fc53eeaa1e40b217030adc1ca0c132f43a076c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] MiniKdc for C++
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] MiniKdc for C++
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
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 <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] MiniKdc for C++
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
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 <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] MiniKdc for C++
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] MiniKdc for C++
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
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 <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] MiniKdc for C++
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
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<string> real_argv = {
: "env", krb5_config,
: "env", krb5_kdc_profile,
: "env", krb5_ccname,
: };
> Why is env listed multiple times? Shouldn't the final call be:
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 a105555 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<RWFile> 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 for CHECK()?
Done
PS1, Line 66: with
> Nit: "to" (I think that's more precise?)
Done
PS1, Line 70: caceh
> cache
Done
Line 71: Status Klist() WARN_UNUSED_RESULT;
> Shouldn't the output be captured and made available programmatically, via a
Done
Line 74: std::vector<std::string> MakeArgv(const std::vector<std::string>& in_argv);
> Missing an include for vector.
Done
Line 76: // Attempts to find the path to the specified Kerberos binary, storing it in 'path'.
> These can be static functions.
Done
Line 89: // Determine the ports that the KDC bound to.
> Should doc that this may sleep if necessary. Maybe even rename it to someth
Done
PS1, Line 94: std::string realm_;
: std::string data_root_;
: uint16_t kdc_port_;
> Maybe just store a copy of the Options struct locally? One less place to up
Done
http://gerrit.cloudera.org:8080/#/c/4752/1/src/kudu/util/subprocess.cc
File src/kudu/util/subprocess.cc:
Line 539: FILE* file = fdopen(to_child_stdin_fd(), "w");
> What does the FILE wrapping actually buy you? Can't you just use the write(
Done
Line 545: if (fclose(file)) {
> This also closes the underlying fd. I doubt that's what you want, but if yo
Done
Line 550: return Status::IOError("Unable to write to subprocess stdin");
> Let's capture the error with ferror(3) (or errno if you use the write sysca
Done
http://gerrit.cloudera.org:8080/#/c/4752/1/src/kudu/util/subprocess.h
File src/kudu/util/subprocess.h:
> How about a unit test for the new Subprocess functionality?
Done
PS1, Line 122: processes
> Nit: process'
Done
PS1, Line 123: StdIn
> Nit: Stdin, to be consistent with ReleaseChildStdinFd().
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: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] MiniKdc for C++
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] MiniKdc for C++
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 a105555 has been merged already, you can omit nullptr since that's the parameter value by default.
PS1, Line 135: vector<string>
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 a105555 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 <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] MiniKdc for C++
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
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 <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] MiniKdc for C++
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
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<string> 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 <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] MiniKdc for C++
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] MiniKdc for C++
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] MiniKdc for C++
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes