You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kudu.apache.org by al...@apache.org on 2017/03/16 03:02:39 UTC
kudu git commit: [mini-kdc] a couple of fixes
Repository: kudu
Updated Branches:
refs/heads/master b42e9e519 -> 587173d26
[mini-kdc] a couple of fixes
Updated MiniKdc code to fix the following minor bugs:
* hiding non-OK return code from kinit in MiniKdc::Kinit()
* not waiting for krb5kdc process to start listening on ports
in case of restart sequence: i.e. calling Start(), then Stop(),
and then Start() again.
Added unit test to verify how MiniKdc::CreateUserPrincipal(),
MiniKdc::Kinit() and MiniKdc::Klist() work when krb5kdc is shut down
and then started again.
Also, added WARN_UNUSED_RESULT attribute for kudu::Subprocess methods
returning Status.
Change-Id: Ibe8cf39a2dcca29627f1e40104ea2e44d7f69d5c
Reviewed-on: http://gerrit.cloudera.org:8080/6404
Reviewed-by: Dan Burkert <da...@apache.org>
Tested-by: Kudu Jenkins
Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/587173d2
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/587173d2
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/587173d2
Branch: refs/heads/master
Commit: 587173d26a6cb4a5c374724b23fdb064b8396c9d
Parents: b42e9e5
Author: Alexey Serbin <as...@cloudera.com>
Authored: Wed Mar 15 01:29:49 2017 -0700
Committer: Alexey Serbin <as...@cloudera.com>
Committed: Thu Mar 16 03:01:38 2017 +0000
----------------------------------------------------------------------
.../minidump_generation-itest.cc | 8 ++---
src/kudu/security/test/mini_kdc-test.cc | 37 ++++++++++++++++++++
src/kudu/security/test/mini_kdc.cc | 27 +++++++++-----
src/kudu/util/subprocess.h | 17 ++++-----
4 files changed, 69 insertions(+), 20 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/kudu/blob/587173d2/src/kudu/integration-tests/minidump_generation-itest.cc
----------------------------------------------------------------------
diff --git a/src/kudu/integration-tests/minidump_generation-itest.cc b/src/kudu/integration-tests/minidump_generation-itest.cc
index 2abec12..46c8f23 100644
--- a/src/kudu/integration-tests/minidump_generation-itest.cc
+++ b/src/kudu/integration-tests/minidump_generation-itest.cc
@@ -55,13 +55,13 @@ TEST_F(MinidumpGenerationITest, TestCreateMinidumpOnCrash) {
// Test kudu-tserver.
ExternalTabletServer* ts = cluster_->tablet_server(0);
string dir = Substitute("$0/$1/$2", ts->log_dir(), "minidumps", "kudu-tserver");
- ts->process()->Kill(SIGABRT);
+ ASSERT_OK(ts->process()->Kill(SIGABRT));
NO_FATALS(WaitForMinidumps(1, dir));
// Test kudu-master.
ExternalMaster* master = cluster_->master();
dir = Substitute("$0/$1/$2", master->log_dir(), "minidumps", "kudu-master");
- master->process()->Kill(SIGABRT);
+ ASSERT_OK(master->process()->Kill(SIGABRT));
NO_FATALS(WaitForMinidumps(1, dir));
}
@@ -72,7 +72,7 @@ TEST_F(MinidumpGenerationITest, TestCreateMinidumpOnSIGUSR1) {
// Enable minidumps and ensure SIGUSR1 generates them.
ExternalTabletServer* ts = cluster_->tablet_server(0);
string dir = Substitute("$0/$1/$2", ts->log_dir(), "minidumps", "kudu-tserver");
- ts->process()->Kill(SIGUSR1);
+ ASSERT_OK(ts->process()->Kill(SIGUSR1));
NO_FATALS(WaitForMinidumps(1, dir));
NO_FATALS(cluster_->AssertNoCrashes());
cluster_->Shutdown();
@@ -82,7 +82,7 @@ TEST_F(MinidumpGenerationITest, TestCreateMinidumpOnSIGUSR1) {
ASSERT_OK(env_->DeleteRecursively(dir));
ASSERT_OK(env_->CreateDir(dir));
ASSERT_OK(cluster_->Restart());
- ts->process()->Kill(SIGUSR1);
+ ASSERT_OK(ts->process()->Kill(SIGUSR1));
NO_FATALS(cluster_->AssertNoCrashes());
NO_FATALS(WaitForMinidumps(0, dir)); // There should be no dumps.
}
http://git-wip-us.apache.org/repos/asf/kudu/blob/587173d2/src/kudu/security/test/mini_kdc-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/security/test/mini_kdc-test.cc b/src/kudu/security/test/mini_kdc-test.cc
index cf898ab..01ed84d 100644
--- a/src/kudu/security/test/mini_kdc-test.cc
+++ b/src/kudu/security/test/mini_kdc-test.cc
@@ -109,4 +109,41 @@ TEST_F(MiniKdcTest, TestStopDrop) {
MiniKdc kdc(options);
}
+TEST_F(MiniKdcTest, TestOperationsWhenKdcNotRunning) {
+ MiniKdcOptions options;
+ MiniKdc kdc(options);
+ ASSERT_OK(kdc.Start());
+ ASSERT_OK(kdc.Stop());
+
+ // MiniKdc::CreateUserPrincipal() works directly with the local files,
+ // so it should work fine even if KDC is shut down.
+ ASSERT_OK(kdc.CreateUserPrincipal("alice"));
+
+ {
+ // Without running KDC it should not be possible to obtain and cache an
+ // initial ticket-granting ticket for principal.
+ const Status s = kdc.Kinit("alice");
+ ASSERT_TRUE(s.IsRuntimeError()) << s.ToString();
+ ASSERT_STR_CONTAINS(s.ToString(), "process exited with non-zero status");
+ }
+ {
+ // Without running KDC klist should fail.
+ string klist;
+ const Status s = kdc.Klist(&klist);
+ ASSERT_TRUE(s.IsRuntimeError()) << s.ToString();
+ ASSERT_STR_CONTAINS(s.ToString(), "process exited with non-zero status");
+ }
+
+ ASSERT_OK(kdc.Start());
+
+ // Once KDC has started, 'kinit' and 'klist' should work with no issues.
+ ASSERT_OK(kdc.Kinit("alice"));
+ {
+ // Check that alice is kinit'd.
+ string klist;
+ ASSERT_OK(kdc.Klist(&klist));
+ ASSERT_STR_CONTAINS(klist, "alice@KRBTEST.COM");
+ }
+}
+
} // namespace kudu
http://git-wip-us.apache.org/repos/asf/kudu/blob/587173d2/src/kudu/security/test/mini_kdc.cc
----------------------------------------------------------------------
diff --git a/src/kudu/security/test/mini_kdc.cc b/src/kudu/security/test/mini_kdc.cc
index dc70f52..fac0767 100644
--- a/src/kudu/security/test/mini_kdc.cc
+++ b/src/kudu/security/test/mini_kdc.cc
@@ -145,6 +145,7 @@ Status MiniKdc::Start() {
VLOG(1) << "Starting Kerberos KDC: " << options_.ToString();
if (!Env::Default()->FileExists(options_.data_root)) {
+ VLOG(1) << "Creating KDC database and configuration files";
RETURN_NOT_OK(Env::Default()->CreateDir(options_.data_root));
RETURN_NOT_OK(CreateKdcConf());
@@ -174,10 +175,13 @@ Status MiniKdc::Start() {
RETURN_NOT_OK(kdc_process_->Start());
- // If we asked for an ephemeral port, grab the actual ports and
- // rewrite the configuration so that clients can connect.
- if (options_.port == 0) {
- RETURN_NOT_OK(WaitForKdcPorts());
+ const bool need_config_update = (options_.port == 0);
+ // Wait for KDC to start listening on its ports and commencing operation.
+ RETURN_NOT_OK(WaitForKdcPorts());
+
+ if (need_config_update) {
+ // If we asked for an ephemeral port, grab the actual ports and
+ // rewrite the configuration so that clients can connect.
RETURN_NOT_OK(CreateKrb5Conf());
RETURN_NOT_OK(CreateKdcConf());
}
@@ -267,7 +271,7 @@ Status MiniKdc::WaitForKdcPorts() {
string lsof;
RETURN_NOT_OK(GetBinaryPath("lsof", {"/sbin", "/usr/sbin"}, &lsof));
- vector<string> cmd = {
+ const vector<string> cmd = {
lsof, "-wbnP", "-Ffn",
"-p", std::to_string(kdc_process_->pid()),
"-a", "-i", "4UDP"};
@@ -304,8 +308,15 @@ Status MiniKdc::WaitForKdcPorts() {
}
CHECK(port > 0 && port < std::numeric_limits<uint16_t>::max())
<< "parsed invalid port: " << port;
- options_.port = port;
- VLOG(1) << "Determined bound KDC port: " << options_.port;
+ VLOG(1) << "Determined bound KDC port: " << port;
+ if (options_.port == 0) {
+ options_.port = port;
+ } else {
+ // Sanity check: if KDC's port is already established, it's supposed to be
+ // written into the configuration files, so the process must bind to the
+ // already established port.
+ CHECK(options_.port == port);
+ }
return Status::OK();
}
@@ -339,7 +350,7 @@ Status MiniKdc::Kinit(const string& username) {
SCOPED_LOG_SLOW_EXECUTION(WARNING, 100, Substitute("kinit for $0", username));
string kinit;
RETURN_NOT_OK(GetBinaryPath("kinit", &kinit));
- Subprocess::Call(MakeArgv({ kinit, username }), username);
+ RETURN_NOT_OK(Subprocess::Call(MakeArgv({ kinit, username }), username));
return Status::OK();
}
http://git-wip-us.apache.org/repos/asf/kudu/blob/587173d2/src/kudu/util/subprocess.h
----------------------------------------------------------------------
diff --git a/src/kudu/util/subprocess.h b/src/kudu/util/subprocess.h
index 73c0497..45dbfe8 100644
--- a/src/kudu/util/subprocess.h
+++ b/src/kudu/util/subprocess.h
@@ -76,7 +76,7 @@ class Subprocess {
// note that if the executable path was incorrect such that
// exec() fails, this will still return Status::OK. You must
// use Wait() to check for failure.
- Status Start();
+ Status Start() WARN_UNUSED_RESULT;
// Wait for the subprocess to exit. The return value is the same as
// that of the waitpid() syscall. Only call after starting.
@@ -84,7 +84,7 @@ class Subprocess {
// NOTE: unlike the standard wait(2) call, this may be called multiple
// times. If the process has exited, it will repeatedly return the same
// exit code.
- Status Wait(int* wait_status = nullptr);
+ Status Wait(int* wait_status = nullptr) WARN_UNUSED_RESULT;
// Like the above, but does not block. This returns Status::TimedOut
// immediately if the child has not exited. Otherwise returns Status::OK
@@ -93,23 +93,24 @@ class Subprocess {
// NOTE: unlike the standard wait(2) call, this may be called multiple
// times. If the process has exited, it will repeatedly return the same
// exit code.
- Status WaitNoBlock(int* wait_status = nullptr);
+ Status WaitNoBlock(int* wait_status = nullptr) WARN_UNUSED_RESULT;
// Send a signal to the subprocess.
// Note that this does not reap the process -- you must still Wait()
// in order to reap it. Only call after starting.
- Status Kill(int signal);
+ Status Kill(int signal) WARN_UNUSED_RESULT;
// Retrieve exit status of the process awaited by Wait() and/or WaitNoBlock()
// methods. Must be called only after calling Wait()/WaitNoBlock().
- Status GetExitStatus(int* exit_status, std::string* info_str = nullptr) const;
+ Status GetExitStatus(int* exit_status, std::string* info_str = nullptr) const
+ WARN_UNUSED_RESULT;
// Helper method that creates a Subprocess, issues a Start() then a Wait().
// Expects a blank-separated list of arguments, with the first being the
// full path to the executable.
// The returned Status will only be OK if all steps were successful and
// the return code was 0.
- static Status Call(const std::string& arg_str);
+ static Status Call(const std::string& arg_str) WARN_UNUSED_RESULT;
// Same as above, but accepts a vector that includes the path to the
// executable as argv[0] and the arguments to the program in argv[1..n].
@@ -122,7 +123,7 @@ class Subprocess {
static Status Call(const std::vector<std::string>& argv,
const std::string& stdin_in = "",
std::string* stdout_out = nullptr,
- std::string* stderr_out = nullptr);
+ std::string* stderr_out = nullptr) WARN_UNUSED_RESULT;
// Return the pipe fd to the child's standard stream.
// Stream should not be disabled or shared.
@@ -149,7 +150,7 @@ class Subprocess {
enum StreamMode {SHARED, DISABLED, PIPED};
enum WaitMode {BLOCKING, NON_BLOCKING};
- Status DoWait(int* wait_status, WaitMode mode);
+ Status DoWait(int* wait_status, WaitMode mode) WARN_UNUSED_RESULT;
void SetFdShared(int stdfd, bool share);
int CheckAndOffer(int stdfd) const;
int ReleaseChildFd(int stdfd);