KUDU-2541: Fill out basic sentry client API This commit contains more plumbing for the upcoming Sentry integration (KUDU-428). The Sentry client class is filled out with enough methods to ensure the Thrift RPC is working correctly, as well as error conversion from Sentry and Thrift's format to Kudu's Status.
Change-Id: I1004a11506ed109d1f23389ca73d95a34c32c194 Reviewed-on: http://gerrit.cloudera.org:8080/11524 Reviewed-by: Adar Dembo <[email protected]> Reviewed-by: Hao Hao <[email protected]> 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/14f3e6f6 Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/14f3e6f6 Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/14f3e6f6 Branch: refs/heads/master Commit: 14f3e6f60b9fbfc7c7b41c6b5ce4ba8612088869 Parents: fad56a6 Author: Dan Burkert <[email protected]> Authored: Fri Sep 21 10:43:05 2018 -0700 Committer: Dan Burkert <[email protected]> Committed: Thu Sep 27 00:34:24 2018 +0000 ---------------------------------------------------------------------- src/kudu/sentry/CMakeLists.txt | 1 + src/kudu/sentry/mini_sentry.cc | 53 ++++++++++-- src/kudu/sentry/mini_sentry.h | 5 +- src/kudu/sentry/sentry_client-test.cc | 57 +++++++++++++ src/kudu/sentry/sentry_client.cc | 131 +++++++++++++++++++++++++++++ src/kudu/sentry/sentry_client.h | 63 ++++++++++++++ 6 files changed, 300 insertions(+), 10 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/kudu/blob/14f3e6f6/src/kudu/sentry/CMakeLists.txt ---------------------------------------------------------------------- diff --git a/src/kudu/sentry/CMakeLists.txt b/src/kudu/sentry/CMakeLists.txt index 8e96c97..910117c 100644 --- a/src/kudu/sentry/CMakeLists.txt +++ b/src/kudu/sentry/CMakeLists.txt @@ -35,6 +35,7 @@ set(SENTRY_SRCS sentry_client.cc) set(SENTRY_DEPS kudu_common + kudu_thrift kudu_util sentry_thrift) http://git-wip-us.apache.org/repos/asf/kudu/blob/14f3e6f6/src/kudu/sentry/mini_sentry.cc ---------------------------------------------------------------------- diff --git a/src/kudu/sentry/mini_sentry.cc b/src/kudu/sentry/mini_sentry.cc index 865fd2d..ff54ad8 100644 --- a/src/kudu/sentry/mini_sentry.cc +++ b/src/kudu/sentry/mini_sentry.cc @@ -74,7 +74,7 @@ Status MiniSentry::Start() { auto tmp_dir = GetTestDataDirectory(); - RETURN_NOT_OK(CreateSentrySite(tmp_dir)); + RETURN_NOT_OK(CreateSentryConfigs(tmp_dir)); map<string, string> env_vars { { "JAVA_HOME", java_home }, @@ -126,7 +126,7 @@ Status MiniSentry::Resume() { return Status::OK(); } -Status MiniSentry::CreateSentrySite(const string& tmp_dir) const { +Status MiniSentry::CreateSentryConfigs(const string& tmp_dir) const { // - sentry.store.jdbc.url // - sentry.store.jdbc.password @@ -136,6 +136,15 @@ Status MiniSentry::CreateSentrySite(const string& tmp_dir) const { // - datanucleus.schema.autoCreateAll // - sentry.verify.schema.version // Allow Sentry to startup and run without first running the schemaTool. + // + // - sentry.store.group.mapping + // sentry.store.group.mapping.resource + // Production Sentry instances use Hadoop's UGI infrastructure to map users + // to groups, but that's difficult to mock for tests, so we configure a + // simpler static user/group mapping based on a generated INI file. + // + // - sentry.service.admin.group + // Set up admin groups which have unrestricted privileges in Sentry. static const string kFileTemplate = R"( <configuration> @@ -160,17 +169,45 @@ Status MiniSentry::CreateSentrySite(const string& tmp_dir) const { </property> <property> - <name>sentry.verify.schema.version</name> + <name>sentry.verify.schema.version</name> <value>false</value> </property> + + <property> + <name>sentry.store.group.mapping</name> + <value>org.apache.sentry.provider.file.LocalGroupMappingService</value> + </property> + + <property> + <name>sentry.store.group.mapping.resource</name> + <value>$1</value> + </property> + + <property> + <name>sentry.service.admin.group</name> + <value>admin</value> + </property> </configuration> )"; - string file_contents = Substitute(kFileTemplate, tmp_dir); - - return WriteStringToFile(Env::Default(), - file_contents, - JoinPathSegments(tmp_dir, "sentry-site.xml")); + string users_ini_path = JoinPathSegments(tmp_dir, "users.ini"); + string file_contents = Substitute(kFileTemplate, tmp_dir, users_ini_path); + RETURN_NOT_OK(WriteStringToFile(Env::Default(), + file_contents, + JoinPathSegments(tmp_dir, "sentry-site.xml"))); + + // Simple file format containing mapping of user to groups in INI syntax, see + // the LocalGroupMappingService class for more information. + static const string kUsers = R"( +[users] +test-admin=admin +test-user=user +kudu=admin +joe-interloper="" +)"; + + RETURN_NOT_OK(WriteStringToFile(Env::Default(), kUsers, users_ini_path)); + return Status::OK(); } } // namespace sentry http://git-wip-us.apache.org/repos/asf/kudu/blob/14f3e6f6/src/kudu/sentry/mini_sentry.h ---------------------------------------------------------------------- diff --git a/src/kudu/sentry/mini_sentry.h b/src/kudu/sentry/mini_sentry.h index fe3ccca..bcf46aa 100644 --- a/src/kudu/sentry/mini_sentry.h +++ b/src/kudu/sentry/mini_sentry.h @@ -61,8 +61,9 @@ class MiniSentry { private: - // Creates a sentry-site.xml for the mini Sentry. - Status CreateSentrySite(const std::string& tmp_dir) const WARN_UNUSED_RESULT; + // Creates a sentry-site.xml for the mini Sentry, and other supporting + // configuration files. + Status CreateSentryConfigs(const std::string& tmp_dir) const WARN_UNUSED_RESULT; // Waits for the metastore process to bind to a port. Status WaitForSentryPorts() WARN_UNUSED_RESULT; http://git-wip-us.apache.org/repos/asf/kudu/blob/14f3e6f6/src/kudu/sentry/sentry_client-test.cc ---------------------------------------------------------------------- diff --git a/src/kudu/sentry/sentry_client-test.cc b/src/kudu/sentry/sentry_client-test.cc index 8f3c560..7e3815b 100644 --- a/src/kudu/sentry/sentry_client-test.cc +++ b/src/kudu/sentry/sentry_client-test.cc @@ -15,9 +15,16 @@ // specific language governing permissions and limitations // under the License. +#include "kudu/sentry/sentry_client.h" + +#include <string> + #include <gtest/gtest.h> #include "kudu/sentry/mini_sentry.h" +#include "kudu/sentry/sentry_policy_service_types.h" +#include "kudu/thrift/client.h" +#include "kudu/util/status.h" #include "kudu/util/test_macros.h" #include "kudu/util/test_util.h" @@ -38,5 +45,55 @@ TEST_F(SentryClientTest, TestMiniSentryLifecycle) { ASSERT_OK(mini_sentry.Pause()); ASSERT_OK(mini_sentry.Resume()); } + +// Basic functionality test of the Sentry client. The goal is not an exhaustive +// test of Sentry's role handling, but instead verification that the client can +// communicate with the Sentry service, and errors are converted to Status +// instances. +TEST_F(SentryClientTest, TestCreateDropRole) { + MiniSentry mini_sentry; + ASSERT_OK(mini_sentry.Start()); + + SentryClient client(mini_sentry.address(), thrift::ClientOptions()); + ASSERT_OK(client.Start()); + + { // Create a role + ::sentry::TCreateSentryRoleRequest req; + req.requestorUserName = "test-admin"; + req.roleName = "viewer"; + ASSERT_OK(client.CreateRole(req)); + + // Attempt to create the role again. + Status s = client.CreateRole(req); + ASSERT_TRUE(s.IsAlreadyPresent()) << s.ToString(); + } + + { // Attempt to create a role as a non-admin user. + ::sentry::TCreateSentryRoleRequest req; + req.requestorUserName = "joe-interloper"; + req.roleName = "fuzz"; + Status s = client.CreateRole(req); + ASSERT_TRUE(s.IsNotAuthorized()) << s.ToString(); + } + + { // Attempt to drop the role as a non-admin user. + ::sentry::TDropSentryRoleRequest req; + req.requestorUserName = "joe-interloper"; + req.roleName = "viewer"; + Status s = client.DropRole(req); + ASSERT_TRUE(s.IsNotAuthorized()) << s.ToString(); + } + + { // Drop the role + ::sentry::TDropSentryRoleRequest req; + req.requestorUserName = "test-admin"; + req.roleName = "viewer"; + ASSERT_OK(client.DropRole(req)); + + // Attempt to drop the role again. + Status s = client.DropRole(req); + ASSERT_TRUE(s.IsNotFound()) << s.ToString(); + } +} } // namespace sentry } // namespace kudu http://git-wip-us.apache.org/repos/asf/kudu/blob/14f3e6f6/src/kudu/sentry/sentry_client.cc ---------------------------------------------------------------------- diff --git a/src/kudu/sentry/sentry_client.cc b/src/kudu/sentry/sentry_client.cc index 53d7928..530bd2e 100644 --- a/src/kudu/sentry/sentry_client.cc +++ b/src/kudu/sentry/sentry_client.cc @@ -17,7 +17,138 @@ #include "kudu/sentry/sentry_client.h" +#include <exception> +#include <memory> +#include <ostream> +#include <string> + +#include <glog/logging.h> +#include <thrift/Thrift.h> +#include <thrift/protocol/TMultiplexedProtocol.h> +#include <thrift/protocol/TProtocol.h> +#include <thrift/transport/TTransport.h> +#include <thrift/transport/TTransportException.h> + +#include "kudu/gutil/strings/substitute.h" +#include "kudu/sentry/sentry_common_service_constants.h" +#include "kudu/sentry/sentry_common_service_types.h" +#include "kudu/sentry/sentry_policy_service_types.h" +#include "kudu/thrift/client.h" +#include "kudu/thrift/sasl_client_transport.h" +#include "kudu/util/status.h" +#include "kudu/util/stopwatch.h" + +using apache::thrift::TException; +using apache::thrift::protocol::TMultiplexedProtocol; +using apache::thrift::transport::TTransportException; +using kudu::thrift::CreateClientProtocol; +using kudu::thrift::SaslException; +using std::make_shared; +using strings::Substitute; + namespace kudu { namespace sentry { + +const uint16_t SentryClient::kDefaultSentryPort = 8038; + +const int kSlowExecutionWarningThresholdMs = 1000; + +namespace { +Status ConvertStatus(const ::sentry::TSentryResponseStatus& status) { + const auto& constants = ::sentry::g_sentry_common_service_constants; + // A switch isn't possible because these values aren't generated as real constants. + if (status.value == constants.TSENTRY_STATUS_OK) { + return Status::OK(); + } + if (status.value == constants.TSENTRY_STATUS_ALREADY_EXISTS) { + return Status::AlreadyPresent(status.message, status.stack); + } + if (status.value == constants.TSENTRY_STATUS_NO_SUCH_OBJECT) { + return Status::NotFound(status.message, status.stack); + } + if (status.value == constants.TSENTRY_STATUS_RUNTIME_ERROR) { + return Status::RuntimeError(status.message, status.stack); + } + if (status.value == constants.TSENTRY_STATUS_INVALID_INPUT) { + return Status::InvalidArgument(status.message, status.stack); + } + if (status.value == constants.TSENTRY_STATUS_ACCESS_DENIED) { + return Status::NotAuthorized(status.message, status.stack); + } + if (status.value == constants.TSENTRY_STATUS_THRIFT_VERSION_MISMATCH) { + return Status::NotSupported(status.message, status.stack); + } + LOG(WARNING) << "Unknown error code in Sentry status: " << status; + return Status::RuntimeError( + Substitute("unknown error code: $0: $1", status.value, status.message), status.stack); +} +} // namespace + +// Wraps calls to the Sentry Thrift service, catching any exceptions and +// converting the response status to a Kudu Status. If there is no +// response_status then {} can be substituted. +#define SENTRY_RET_NOT_OK(call, response_status, msg) \ + try { \ + (call); \ + RETURN_NOT_OK_PREPEND(ConvertStatus(response_status), (msg)); \ + } catch (const SaslException& e) { \ + return e.status().CloneAndPrepend(msg); \ + } catch (const TTransportException& e) { \ + switch (e.getType()) { \ + case TTransportException::TIMED_OUT: return Status::TimedOut((msg), e.what()); \ + case TTransportException::BAD_ARGS: return Status::InvalidArgument((msg), e.what()); \ + case TTransportException::CORRUPTED_DATA: return Status::Corruption((msg), e.what()); \ + default: return Status::NetworkError((msg), e.what()); \ + } \ + } catch (const TException& e) { \ + return Status::IOError((msg), e.what()); \ + } catch (const std::exception& e) { \ + return Status::RuntimeError((msg), e.what()); \ + } + +SentryClient::SentryClient(const HostPort& address, const thrift::ClientOptions& options) + : client_(::sentry::SentryPolicyServiceClient( + make_shared<TMultiplexedProtocol>(CreateClientProtocol(address, options), + "SentryPolicyService"))) { +} + +SentryClient::~SentryClient() { + WARN_NOT_OK(Stop(), "failed to shutdown Sentry client"); +} + +Status SentryClient::Start() { + SCOPED_LOG_SLOW_EXECUTION(WARNING, kSlowExecutionWarningThresholdMs, "starting Sentry client"); + SENTRY_RET_NOT_OK(client_.getOutputProtocol()->getTransport()->open(), + {}, "failed to open Sentry connection"); + return Status::OK(); +} + +Status SentryClient::Stop() { + SCOPED_LOG_SLOW_EXECUTION(WARNING, kSlowExecutionWarningThresholdMs, "stopping Sentry client"); + SENTRY_RET_NOT_OK(client_.getInputProtocol()->getTransport()->close(), + {}, "failed to close Sentry connection"); + return Status::OK(); +} + +bool SentryClient::IsConnected() { + return client_.getInputProtocol()->getTransport()->isOpen(); +} + +Status SentryClient::CreateRole(const ::sentry::TCreateSentryRoleRequest& request) { + SCOPED_LOG_SLOW_EXECUTION(WARNING, kSlowExecutionWarningThresholdMs, "create Sentry role"); + ::sentry::TCreateSentryRoleResponse response; + SENTRY_RET_NOT_OK(client_.create_sentry_role(response, request), + response.status, "failed to create Sentry role"); + return Status::OK(); +} + +Status SentryClient::DropRole(const ::sentry::TDropSentryRoleRequest& request) { + SCOPED_LOG_SLOW_EXECUTION(WARNING, kSlowExecutionWarningThresholdMs, "drop Sentry role"); + ::sentry::TDropSentryRoleResponse response; + SENTRY_RET_NOT_OK(client_.drop_sentry_role(response, request), + response.status, "failed to drop Sentry role"); + return Status::OK(); +} + } // namespace sentry } // namespace kudu http://git-wip-us.apache.org/repos/asf/kudu/blob/14f3e6f6/src/kudu/sentry/sentry_client.h ---------------------------------------------------------------------- diff --git a/src/kudu/sentry/sentry_client.h b/src/kudu/sentry/sentry_client.h index 09d7916..e5d9226 100644 --- a/src/kudu/sentry/sentry_client.h +++ b/src/kudu/sentry/sentry_client.h @@ -17,11 +17,74 @@ #pragma once +#include <cstdint> + +#include "kudu/gutil/port.h" +#include "kudu/sentry/SentryPolicyService.h" +#include "kudu/util/status.h" + +namespace sentry { +class TCreateSentryRoleRequest; +class TDropSentryRoleRequest; +} + namespace kudu { + +class HostPort; + +namespace thrift { +struct ClientOptions; +} // namespace thrift + namespace sentry { +// A client for a Sentry service. +// +// All operations are synchronous, and may block. +// +// SentryClient is not thread safe. +// +// SentryClient wraps a single TCP connection to a Sentry service instance, and +// does not attempt to handle or retry on failure. It's expected that a +// higher-level component will wrap SentryClient to provide retry, pooling, and +// HA deployment features if necessary. +// +// Note: see HmsClient for why TSocketPool is not used for transparently +// handling connections to Sentry HA instances. class SentryClient { public: + + static const uint16_t kDefaultSentryPort; + + // Create a SentryClient connection to the provided Sentry service Thrift RPC address. + SentryClient(const HostPort& address, const thrift::ClientOptions& options); + ~SentryClient(); + + // Starts the Sentry service client. + // + // This method will open a synchronous TCP connection to the Sentry service. + // If the Sentry service can not be reached within the connection timeout + // interval, an error is returned. + // + // Must be called before any subsequent operations using the client. + Status Start() WARN_UNUSED_RESULT; + + // Stops the Sentry service client. + // + // This is optional; if not called the destructor will stop the client. + Status Stop() WARN_UNUSED_RESULT; + + // Returns 'true' if the client is connected to the remote server. + bool IsConnected() WARN_UNUSED_RESULT; + + // Creates a new role in Sentry. + Status CreateRole(const ::sentry::TCreateSentryRoleRequest& request) WARN_UNUSED_RESULT; + + // Drops a role in Sentry. + Status DropRole(const ::sentry::TDropSentryRoleRequest& request) WARN_UNUSED_RESULT; + + private: + ::sentry::SentryPolicyServiceClient client_; }; } // namespace sentry
