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 2021/05/26 19:33:09 UTC
[kudu] branch master updated (a0bda09 -> bf7a953)
This is an automated email from the ASF dual-hosted git repository.
alexey pushed a change to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git.
from a0bda09 [test] minor clean up on TestUnsafeChangeConfigLeaderWithPendingConfig
new eda678d [master] KUDU-2181 Fix duplicate master address and remove unsafe flag tag
new bf7a953 [txn_manager] allow a super user to call txn-related RPCs
The 2 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
Summary of changes:
src/kudu/integration-tests/security-itest.cc | 121 +++++++++++++++++++++++----
src/kudu/master/sys_catalog.cc | 5 +-
src/kudu/master/txn_manager_service.cc | 2 +-
src/kudu/tools/tool_action_master.cc | 12 ++-
src/kudu/util/net/net_util.cc | 18 ++--
src/kudu/util/net/net_util.h | 4 +
6 files changed, 133 insertions(+), 29 deletions(-)
[kudu] 01/02: [master] KUDU-2181 Fix duplicate master address and
remove unsafe flag tag
Posted by al...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
alexey pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git
commit eda678de47856910c3b2aaef6e0e9e11473eb5ff
Author: Bankim Bhavsar <ba...@apache.org>
AuthorDate: Mon May 24 17:34:14 2021 -0700
[master] KUDU-2181 Fix duplicate master address and remove unsafe flag tag
While testing add master on a physical cluster, observed that
supplying only hostnames without port resulted in duplicates
being supplied to bring up new master which in turn leads
to failure in creating distributed Raft config on startup.
For e.g. kudu master add hp1,hp2 h2
Reason being hp2 is compared as hp2:7051 whereas in the vector
of strings "master_addresses", it contains hp1,hp2.
This changes adds a new function ParseAddresses() in HostPort class
that's a variant of existing ParseStrings() function and takes
a vector of strings instead. This new function is used
in the duplicate detection logic.
This change also removes the unsafe tag from
--master_addr_add_new_master as the feature is ready but keeps
it hidden as it's only meant to be used by the add master orchestration
tool.
This one is tricky to test locally and write a test because
we need to specify ports for starting masters/tservers.
Verified the fix on physical cluster. Along with CM integration
we can add a systest later.
Change-Id: Icf29730e3a6b225adb24ff161cac2ad777b46b81
Reviewed-on: http://gerrit.cloudera.org:8080/17500
Tested-by: Bankim Bhavsar <ba...@cloudera.com>
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
src/kudu/master/sys_catalog.cc | 5 ++---
src/kudu/tools/tool_action_master.cc | 12 ++++++++++--
src/kudu/util/net/net_util.cc | 18 +++++++++++-------
src/kudu/util/net/net_util.h | 4 ++++
4 files changed, 27 insertions(+), 12 deletions(-)
diff --git a/src/kudu/master/sys_catalog.cc b/src/kudu/master/sys_catalog.cc
index 63aec50..f6946e4 100644
--- a/src/kudu/master/sys_catalog.cc
+++ b/src/kudu/master/sys_catalog.cc
@@ -88,11 +88,10 @@ DEFINE_double(sys_catalog_fail_during_write, 0.0,
"Fraction of the time when system table writes will fail");
TAG_FLAG(sys_catalog_fail_during_write, hidden);
-// Following flags related to dynamic multi-master are hidden till the feature is ready to
-// be advertised.
+// This flag is meant to be used by the tool orchestrating addition of a Kudu master and not
+// for external documentation and hence it's hidden.
DEFINE_string(master_address_add_new_master, "",
"Address of master to add as a NON_VOTER on creating a distributed master config.");
-TAG_FLAG(master_address_add_new_master, unsafe);
TAG_FLAG(master_address_add_new_master, hidden);
DECLARE_bool(master_support_change_config);
diff --git a/src/kudu/tools/tool_action_master.cc b/src/kudu/tools/tool_action_master.cc
index 9cf73d0..710903c 100644
--- a/src/kudu/tools/tool_action_master.cc
+++ b/src/kudu/tools/tool_action_master.cc
@@ -357,6 +357,12 @@ Status AddMaster(const RunnerContext& context) {
// Parse input arguments.
vector<string> master_addresses;
RETURN_NOT_OK(ParseMasterAddresses(context, &master_addresses));
+ // "master_addresses" after being successfully parsed above don't contain duplicates.
+ // So "master_hps" won't contain any duplicates as well.
+ vector<HostPort> master_hps;
+ RETURN_NOT_OK(HostPort::ParseAddresses(master_addresses, Master::kDefaultPort, &master_hps));
+ DCHECK_EQ(master_addresses.size(), master_hps.size());
+
const string& new_master_address = FindOrDie(context.required_args, kMasterAddressArg);
HostPort hp;
RETURN_NOT_OK(hp.ParseString(new_master_address, Master::kDefaultPort));
@@ -394,9 +400,11 @@ Status AddMaster(const RunnerContext& context) {
// Bring up the new master that includes master addresses of the cluster and itself.
// It's possible this is a retry in which case the new master is already part of
// master_addresses.
- if (std::find(master_addresses.begin(), master_addresses.end(), hp.ToString()) ==
- master_addresses.end()) {
+ // Using HostPort for comparing instead of strings as in case of latter, the default port
+ // may not be specified.
+ if (std::find(master_hps.begin(), master_hps.end(), hp) == master_hps.end()) {
master_addresses.emplace_back(hp.ToString());
+ master_hps.push_back(hp);
}
unique_ptr<Subprocess> new_master;
diff --git a/src/kudu/util/net/net_util.cc b/src/kudu/util/net/net_util.cc
index 87df153..edfa8e6 100644
--- a/src/kudu/util/net/net_util.cc
+++ b/src/kudu/util/net/net_util.cc
@@ -34,7 +34,7 @@
#include <utility>
#include <vector>
-#include <boost/functional/hash/hash.hpp>
+#include <boost/container_hash/extensions.hpp>
#include <gflags/gflags.h>
#include <glog/logging.h>
@@ -226,14 +226,18 @@ Status HostPort::ResolveAddresses(vector<Sockaddr>* addresses) const {
Status HostPort::ParseStrings(const string& comma_sep_addrs,
uint16_t default_port,
vector<HostPort>* res) {
- res->clear();
+ return ParseAddresses(strings::Split(comma_sep_addrs, ",", strings::SkipEmpty()),
+ default_port, res);
+}
- vector<string> addr_strings = strings::Split(comma_sep_addrs, ",", strings::SkipEmpty());
- res->reserve(addr_strings.size());
- for (const string& addr_string : addr_strings) {
+Status HostPort::ParseAddresses(const vector<string>& addrs, uint16_t default_port,
+ vector<HostPort>* res) {
+ res->clear();
+ res->reserve(addrs.size());
+ for (const string& addr : addrs) {
HostPort host_port;
- RETURN_NOT_OK(host_port.ParseString(addr_string, default_port));
- res->emplace_back(host_port);
+ RETURN_NOT_OK(host_port.ParseString(addr, default_port));
+ res->emplace_back(std::move(host_port));
}
return Status::OK();
}
diff --git a/src/kudu/util/net/net_util.h b/src/kudu/util/net/net_util.h
index 731d0ec..d29ca97 100644
--- a/src/kudu/util/net/net_util.h
+++ b/src/kudu/util/net/net_util.h
@@ -82,6 +82,10 @@ class HostPort {
static Status ParseStrings(
const std::string& comma_sep_addrs, uint16_t default_port, std::vector<HostPort>* res);
+ // Similar to above but uses a vector of strings 'addrs' as input parameter.
+ static Status ParseAddresses(const std::vector<std::string>& addrs, uint16_t default_port,
+ std::vector<HostPort>* res);
+
// Similar to above but allow the addresses to have scheme and path,
// which are ignored.
static Status ParseStringsWithScheme(
[kudu] 02/02: [txn_manager] allow a super user to call txn-related
RPCs
Posted by al...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
alexey pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git
commit bf7a9535a264c7579bd783a7136bf3becb1a37a9
Author: Alexey Serbin <al...@apache.org>
AuthorDate: Tue May 25 00:05:36 2021 -0700
[txn_manager] allow a super user to call txn-related RPCs
Since we are planning to use transactional operations in kudu CLI tools
(e.g., in 'kudu perf loadgen'), it makes sense to allow txn-related RPCs
to be invoked by a super user as well since some kudu CLI tools are often
run under such credentials. This patch changes the RPC-level coarse authz
settings for all methods in the TxnManagerService accordingly.
I also had a version of this patch where a service user was able to call
txn API as well, but it turned out that TabletServerService API isn't
accessible to service users, e.g. TabletServerService::Write() is
accessible only to regular and super users. With that, I don't quite see
the necessity of allowing a service user to call txn API.
Change-Id: I21e4b29634fd01f0ced80b54373b1ce156e274ad
Reviewed-on: http://gerrit.cloudera.org:8080/17510
Tested-by: Alexey Serbin <as...@cloudera.com>
Reviewed-by: Andrew Wong <aw...@cloudera.com>
---
src/kudu/integration-tests/security-itest.cc | 121 +++++++++++++++++++++++----
src/kudu/master/txn_manager_service.cc | 2 +-
2 files changed, 106 insertions(+), 17 deletions(-)
diff --git a/src/kudu/integration-tests/security-itest.cc b/src/kudu/integration-tests/security-itest.cc
index 0f5fad1..a4486dd 100644
--- a/src/kudu/integration-tests/security-itest.cc
+++ b/src/kudu/integration-tests/security-itest.cc
@@ -303,6 +303,99 @@ TEST_F(SecurityITest, SmokeTestAsAuthorizedUser) {
s.ToString());
}
+// Make sure that it's possible to call TxnManagerService's txn-related RPCs
+// using credentials of a regular and a super user.
+TEST_F(SecurityITest, TxnSmokeWithDifferentUserTypes) {
+ constexpr const auto kTxnKeepaliveIntervalMs = 500;
+ const auto usernames = {
+ "test-admin",
+ "test-user",
+ };
+
+ cluster_opts_.extra_master_flags.emplace_back(
+ "--txn_manager_enabled=true");
+ cluster_opts_.extra_tserver_flags.emplace_back(
+ "--enable_txn_system_client_init=true");
+ cluster_opts_.extra_tserver_flags.emplace_back(
+ Substitute("--txn_keepalive_interval_ms=$0", kTxnKeepaliveIntervalMs));
+ ASSERT_OK(StartCluster());
+
+ for (const auto& username : usernames) {
+ SCOPED_TRACE(Substitute("Running with credentials of user $0", username));
+ ASSERT_OK(cluster_->kdc()->Kinit(username));
+
+ shared_ptr<KuduClient> client;
+ ASSERT_OK(cluster_->CreateClient(nullptr, &client));
+ NO_FATALS(SmokeTestCluster(client, /* transactional */ true));
+
+ // Now, check for the rest of TxnManagerService RPCs.
+ unique_ptr<KuduTableCreator> table_creator(client->NewTableCreator());
+ ASSERT_OK(table_creator->table_name(kTableName)
+ .set_range_partition_columns({ "key" })
+ .schema(&kTestKuduSchema)
+ .num_replicas(3)
+ .Create());
+
+ auto smoke_starter = [&](shared_ptr<KuduTable>* table,
+ shared_ptr<KuduTransaction>* txn) {
+ RETURN_NOT_OK(client->OpenTable(kTableName, table));
+ RETURN_NOT_OK(client->NewTransaction(txn));
+ shared_ptr<KuduSession> session;
+ RETURN_NOT_OK((*txn)->CreateSession(&session));
+ RETURN_NOT_OK(session->SetFlushMode(KuduSession::MANUAL_FLUSH));
+ {
+ unique_ptr<KuduInsert> ins((*table)->NewInsert());
+ RETURN_NOT_OK(ins->mutable_row()->SetInt32(0, 0));
+ RETURN_NOT_OK(ins->mutable_row()->SetInt32(1, 1));
+ RETURN_NOT_OK(session->Apply(ins.release()));
+ }
+ return session->Flush();
+ };
+
+ // Check TxnManagerService::AbortTransaction() RPC as well.
+ {
+ shared_ptr<KuduTable> table;
+ shared_ptr<KuduTransaction> txn;
+ ASSERT_OK(smoke_starter(&table, &txn));
+ ASSERT_OK(txn->Rollback());
+
+ // Read the inserted row back.
+ ASSERT_EQ(0, CountTableRows(table.get()));
+ }
+
+ // Check TxnManagerService::KeepTransactionAlive() -- wait for for over than
+ // keepalive interval for a transaction and make sure it's still possible to
+ // commit a transaction using its handle. Calling StartCommit() and then
+ // calling IsCommitComplete() checks calling of the
+ // TxnManagerService::GetTransactionState() RPC.
+ {
+ shared_ptr<KuduTable> table;
+ shared_ptr<KuduTransaction> txn;
+ ASSERT_OK(smoke_starter(&table, &txn));
+
+ // Wait for a few keepalive intervals: the transaction would be aborted if
+ // calls to TxnManagerService::KeepTransactionAlive() RPC do not come
+ // through, and then KuduTransaction::StartCommit() below would fail.
+ SleepFor(MonoDelta::FromMilliseconds(3 * kTxnKeepaliveIntervalMs));
+
+ ASSERT_OK(txn->StartCommit());
+ ASSERT_EVENTUALLY([&]{
+ bool is_complete = false;
+ Status completion_status;
+ ASSERT_OK(txn->IsCommitComplete(&is_complete, &completion_status));
+ ASSERT_TRUE(is_complete);
+ ASSERT_OK(completion_status);
+ });
+
+ // Read the inserted row back.
+ ASSERT_EQ(1, CountTableRows(table.get()));
+ }
+
+ // Finally, drop the table.
+ ASSERT_OK(client->DeleteTable(kTableName));
+ }
+}
+
// Test trying to access the cluster with no Kerberos credentials at all.
TEST_F(SecurityITest, TestNoKerberosCredentials) {
ASSERT_OK(StartCluster());
@@ -478,9 +571,9 @@ TEST_F(SecurityITest, TestNonDefaultPrincipal) {
ASSERT_OK(StartCluster());
// A client with the default SASL proto shouldn't be able to connect to
- // a cluster using custom Kerberos principal for Kudu service user.
+ // a cluster using custom Kerberos principal for Kudu service user. Verify
+ // that for both the regular and the super-user.
for (const auto& username : {"test-user", "test-admin"}) {
- // Verify that for both the regular and the super-user.
ASSERT_OK(cluster_->kdc()->Kinit(username));
// Instantiate a KuduClientBuilder outside of this cluster's context, so
@@ -494,21 +587,17 @@ TEST_F(SecurityITest, TestNonDefaultPrincipal) {
const auto s = builder.Build(&client);
ASSERT_TRUE(s.IsNotAuthorized());
ASSERT_STR_CONTAINS(s.ToString(), "not found in Kerberos database");
- }
-
- // Create a client with the matching SASL proto name and verify it's able to
- // connect to the cluster and perform basic actions.
- {
- // StartCluster() does 'kinit' as test-admin super-user. Anyways, let's
- // switch to a regular user credentials to perform user-specific tasks.
- ASSERT_OK(cluster_->kdc()->Kinit("test-user"));
- // Here we don't use out-of-this-cluster KuduClientBuilder instance,
- // so the client is created with matching SASL proto name.
- shared_ptr<KuduClient> client;
- ASSERT_OK(cluster_->CreateClient(nullptr, &client));
- SmokeTestCluster(client);
- SmokeTestCluster(client, /*transactional*/ true);
+ // Create a client with the matching SASL proto name and verify it's able to
+ // connect to the cluster and perform basic actions.
+ {
+ // Here we don't use out-of-this-cluster KuduClientBuilder instance,
+ // so the client is created with matching SASL proto name.
+ shared_ptr<KuduClient> client;
+ ASSERT_OK(cluster_->CreateClient(nullptr, &client));
+ SmokeTestCluster(client);
+ SmokeTestCluster(client, /*transactional*/ true);
+ }
}
}
diff --git a/src/kudu/master/txn_manager_service.cc b/src/kudu/master/txn_manager_service.cc
index 01d4a2c..f804ec3 100644
--- a/src/kudu/master/txn_manager_service.cc
+++ b/src/kudu/master/txn_manager_service.cc
@@ -168,7 +168,7 @@ bool TxnManagerServiceImpl::AuthorizeClient(
const google::protobuf::Message* /* req */,
google::protobuf::Message* /* resp */,
RpcContext* ctx) {
- return server_->Authorize(ctx, ServerBase::USER);
+ return server_->Authorize(ctx, ServerBase::SUPER_USER | ServerBase::USER);
}
} // namespace transactions