This is an automated email from the ASF dual-hosted git repository.
mgreber pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git
The following commit(s) were added to refs/heads/master by this push:
new d2d0e0a92 KUDU-3608 REST API Follow-up
d2d0e0a92 is described below
commit d2d0e0a92975c847cf5db87d1b99c042d4abdf84
Author: gabriellalotz <lotzgabrie...@gmail.com>
AuthorDate: Mon Mar 31 14:51:56 2025 +0000
KUDU-3608 REST API Follow-up
Follow-up on the REST API for Metadata Management commit
(https://gerrit.cloudera.org/c/21823/) by adding additional error
handling and refactoring. Improved error responses for table operations.
Refactored path handler functions for better readability and
consistency, including handling of authentication principal extraction.
Adjusted test cases to align with the changes and ensure correctness.
Change-Id: I3cdb14d5a932eb8d1ba209ca803731749117024e
Reviewed-on: http://gerrit.cloudera.org:8080/22744
Reviewed-by: Alexey Serbin <ale...@apache.org>
Reviewed-by: Zoltan Chovan <zcho...@cloudera.com>
Tested-by: Marton Greber <greber...@gmail.com>
---
src/kudu/master/master.cc | 2 +-
src/kudu/master/rest_catalog-test.cc | 182 ++++++++++++++++----------
src/kudu/master/rest_catalog_path_handlers.cc | 105 +++++++++------
src/kudu/master/rest_catalog_path_handlers.h | 15 ++-
src/kudu/master/rest_catalog_test_base.h | 36 +++--
src/kudu/master/spnego_rest_catalog-test.cc | 34 ++---
6 files changed, 220 insertions(+), 154 deletions(-)
diff --git a/src/kudu/master/master.cc b/src/kudu/master/master.cc
index 5472c86d7..c0fa3bf1c 100644
--- a/src/kudu/master/master.cc
+++ b/src/kudu/master/master.cc
@@ -316,7 +316,7 @@ Status Master::Init() {
if (web_server_) {
RETURN_NOT_OK(path_handlers_->Register(web_server_.get()));
if (rest_catalog_path_handlers_) {
- RETURN_NOT_OK(rest_catalog_path_handlers_->Register(web_server_.get()));
+ rest_catalog_path_handlers_->Register(web_server_.get());
}
}
diff --git a/src/kudu/master/rest_catalog-test.cc
b/src/kudu/master/rest_catalog-test.cc
index 2e09a6fd0..6e17ff9c7 100644
--- a/src/kudu/master/rest_catalog-test.cc
+++ b/src/kudu/master/rest_catalog-test.cc
@@ -109,30 +109,59 @@ class RestCatalogTest : public RestCatalogTestBase {
"{\"range_schema\":{\"columns\":[{\"id\":0}]}}";
};
-TEST_F(RestCatalogTest, TestInvalidMethod) {
- EasyCurl c;
- c.set_custom_method("DELETE");
- faststring buf;
- Status s = c.FetchURL(
- Substitute("http://$0/api/v1/tables";,
cluster_->mini_master()->bound_http_addr().ToString()),
- &buf);
- ASSERT_STR_CONTAINS(s.ToString(), "HTTP 405");
- ASSERT_STR_CONTAINS(buf.ToString(), "{\"error\":\"Method not allowed\"}");
+TEST_F(RestCatalogTest, TestInvalidMethods) {
+ const std::vector<std::string> invalid_methods = {"PUT", "DELETE",
"CONNECT"};
+
+ for (const auto& method : invalid_methods) {
+ EasyCurl c;
+ c.set_custom_method(method);
+ faststring buf;
+ Status s;
+ if (method == "PUT") {
+ s = c.PostToURL(Substitute("http://$0/api/v1/tables";,
+
cluster_->mini_master()->bound_http_addr().ToString()),
+ "{\"name\":\"test_table\"}",
+ &buf);
+ } else {
+ s = c.FetchURL(Substitute("http://$0/api/v1/tables";,
+
cluster_->mini_master()->bound_http_addr().ToString()),
+ &buf);
+ }
+ ASSERT_TRUE(!s.ok());
+ ASSERT_STR_CONTAINS(s.ToString(), "HTTP 405") << "Failed for method: " <<
method;
+ ASSERT_STR_CONTAINS(buf.ToString(), "{\"error\":\"Method not allowed\"}")
+ << "Failed for method: " << method;
+ }
}
TEST_F(RestCatalogTest, TestInvalidMethodOnTableEndpoint) {
ASSERT_OK(CreateTestTable());
string table_id;
ASSERT_OK(GetTableId(kTableName, &table_id));
- EasyCurl c;
- c.set_custom_method("CONNECT");
- faststring buf;
- Status s = c.FetchURL(Substitute("http://$0/api/v1/tables/$1";,
-
cluster_->mini_master()->bound_http_addr().ToString(),
- table_id),
- &buf);
- ASSERT_STR_CONTAINS(s.ToString(), "HTTP 405");
- ASSERT_STR_CONTAINS(buf.ToString(), "{\"error\":\"Method not allowed\"}");
+ const std::vector<std::string> invalid_methods = {"POST", "CONNECT"};
+
+ for (const auto& method : invalid_methods) {
+ EasyCurl c;
+ c.set_custom_method(method);
+ faststring buf;
+ Status s;
+ if (method == "POST") {
+ s = c.PostToURL(Substitute("http://$0/api/v1/tables/$1";,
+
cluster_->mini_master()->bound_http_addr().ToString(),
+ table_id),
+ "{\"name\":\"test_table\"}",
+ &buf);
+ } else {
+ s = c.FetchURL(Substitute("http://$0/api/v1/tables/$1";,
+
cluster_->mini_master()->bound_http_addr().ToString(),
+ table_id),
+ &buf);
+ }
+ ASSERT_TRUE(!s.ok());
+ ASSERT_STR_CONTAINS(s.ToString(), "HTTP 405") << "Failed for method: " <<
method;
+ ASSERT_STR_CONTAINS(buf.ToString(), "{\"error\":\"Method not allowed\"}")
+ << "Failed for method: " << method;
+ }
}
TEST_F(RestCatalogTest, TestGetTablesZeroTables) {
@@ -158,6 +187,16 @@ TEST_F(RestCatalogTest, TestGetTablesOneTable) {
Substitute("{\"tables\":[{\"table_id\":\"$0\",\"table_name\":\"test_table\"}]}",
table_id));
}
+TEST_F(RestCatalogTest, TestGetTableNoId) {
+ EasyCurl c;
+ faststring buf;
+ Status s = c.FetchURL(Substitute("http://$0/api/v1/tables/";,
+
cluster_->mini_master()->bound_http_addr().ToString()),
+ &buf);
+ ASSERT_TRUE(!s.ok());
+ ASSERT_STR_CONTAINS(s.ToString(), "HTTP 404");
+}
+
TEST_F(RestCatalogTest, TestGetTableEndpoint) {
ASSERT_OK(CreateTestTable());
string table_id;
@@ -192,6 +231,7 @@ TEST_F(RestCatalogTest, TestGetTableNotFound) {
Status s =
c.FetchURL(Substitute("http://$0/api/v1/tables/05755b4c0c7640cd9f6673c2530a4e78";,
cluster_->mini_master()->bound_http_addr().ToString()),
&buf);
+ ASSERT_TRUE(!s.ok());
ASSERT_STR_CONTAINS(s.ToString(), "HTTP 404");
ASSERT_STR_CONTAINS(buf.ToString(), "{\"error\":\"Table not found\"}");
}
@@ -202,6 +242,7 @@ TEST_F(RestCatalogTest, TestGetTableMalformedId) {
Status s = c.FetchURL(Substitute("http://$0/api/v1/tables/123";,
cluster_->mini_master()->bound_http_addr().ToString()),
&buf);
+ ASSERT_TRUE(!s.ok());
ASSERT_STR_CONTAINS(s.ToString(), "HTTP 400");
ASSERT_STR_CONTAINS(buf.ToString(),
"{\"error\":\"Invalid table ID: must be exactly 32
characters long.\"}");
@@ -211,10 +252,10 @@ TEST_F(RestCatalogTest, TestDeleteTableNonExistent) {
EasyCurl c;
faststring buf;
c.set_custom_method("DELETE");
- c.set_verbose(true);
Status s =
c.FetchURL(Substitute("http://$0/api/v1/tables/05755b4c0c7640cd9f6673c2530a4e78";,
cluster_->mini_master()->bound_http_addr().ToString()),
&buf);
+ ASSERT_TRUE(!s.ok());
ASSERT_STR_CONTAINS(s.ToString(), "HTTP 404");
ASSERT_STR_CONTAINS(buf.ToString(), "{\"error\":\"Table not found\"}");
}
@@ -226,14 +267,13 @@ TEST_F(RestCatalogTest, TestDeleteTableEndpoint) {
EasyCurl c;
faststring buf;
c.set_custom_method("DELETE");
- Status s = c.FetchURL(Substitute("http://$0/api/v1/tables/$1";,
-
cluster_->mini_master()->bound_http_addr().ToString(),
- table_id),
- &buf);
- ASSERT_STR_CONTAINS(s.ToString(), "OK");
+ ASSERT_OK(c.FetchURL(Substitute("http://$0/api/v1/tables/$1";,
+
cluster_->mini_master()->bound_http_addr().ToString(),
+ table_id),
+ &buf));
ASSERT_TRUE(buf.size() == 0);
shared_ptr<KuduTable> table;
- s = client_->OpenTable(kTableName, &table);
+ Status s = client_->OpenTable(kTableName, &table);
ASSERT_STR_CONTAINS(s.ToString(),
"Not found: the table does not exist: table_name:
\"test_table\"");
ASSERT_TRUE(table == nullptr);
@@ -246,6 +286,7 @@ TEST_F(RestCatalogTest, TestDeleteTableMalformedId) {
Status s = c.FetchURL(Substitute("http://$0/api/v1/tables/123";,
cluster_->mini_master()->bound_http_addr().ToString()),
&buf);
+ ASSERT_TRUE(!s.ok());
ASSERT_STR_CONTAINS(s.ToString(), "HTTP 400");
ASSERT_STR_CONTAINS(buf.ToString(),
"{\"error\":\"Invalid table ID: must be exactly 32
characters long.\"}");
@@ -258,6 +299,7 @@ TEST_F(RestCatalogTest, TestPostTableNoData) {
Status s = c.FetchURL(
Substitute("http://$0/api/v1/tables";,
cluster_->mini_master()->bound_http_addr().ToString()),
&buf);
+ ASSERT_TRUE(!s.ok());
ASSERT_STR_CONTAINS(s.ToString(), "HTTP 411");
}
@@ -268,6 +310,7 @@ TEST_F(RestCatalogTest, TestPostTableMalformedData) {
Substitute("http://$0/api/v1/tables";,
cluster_->mini_master()->bound_http_addr().ToString()),
"{\"name\":\"test_table\"}",
&buf);
+ ASSERT_TRUE(!s.ok());
ASSERT_STR_CONTAINS(s.ToString(), "HTTP 400");
ASSERT_STR_CONTAINS(
buf.ToString(),
@@ -278,7 +321,7 @@ TEST_F(RestCatalogTest, TestPostTableEndpoint) {
EasyCurl c;
faststring buf;
c.set_custom_method("POST");
- Status s = c.PostToURL(
+ ASSERT_OK(c.PostToURL(
Substitute("http://$0/api/v1/tables";,
cluster_->mini_master()->bound_http_addr().ToString()),
R"({
"name": "test_table",
@@ -295,8 +338,7 @@ TEST_F(RestCatalogTest, TestPostTableEndpoint) {
},
"num_replicas": 1
})",
- &buf);
- ASSERT_STR_CONTAINS(s.ToString(), "OK");
+ &buf));
string table_id;
ASSERT_OK(GetTableId(kTableName, &table_id));
shared_ptr<KuduTable> table;
@@ -314,8 +356,8 @@ TEST_F(RestCatalogTest, TestPostTableEndpoint) {
partition_schema,
table->owner()));
ASSERT_TRUE(table != nullptr);
- ASSERT_EQ(table->name(), kTableName);
- ASSERT_EQ(table->num_replicas(), 1);
+ ASSERT_EQ(kTableName, table->name());
+ ASSERT_EQ(1, table->num_replicas());
}
TEST_F(RestCatalogTest, TestPutTableMalformedId) {
@@ -326,6 +368,7 @@ TEST_F(RestCatalogTest, TestPutTableMalformedId) {
cluster_->mini_master()->bound_http_addr().ToString()),
"{\"name\":\"test_table\"}",
&buf);
+ ASSERT_TRUE(!s.ok());
ASSERT_STR_CONTAINS(s.ToString(), "HTTP 400");
ASSERT_STR_CONTAINS(buf.ToString(),
"{\"error\":\"Invalid table ID: must be exactly 32
characters long.\"}");
@@ -342,6 +385,7 @@ TEST_F(RestCatalogTest, TestPutTableNoData) {
cluster_->mini_master()->bound_http_addr().ToString(),
table_id),
&buf);
+ ASSERT_TRUE(!s.ok());
ASSERT_STR_CONTAINS(s.ToString(), "HTTP 411");
}
@@ -357,6 +401,7 @@ TEST_F(RestCatalogTest, TestPutTableMalformedData) {
table_id),
"{\"name\":\"test_table\"}",
&buf);
+ ASSERT_TRUE(!s.ok());
ASSERT_STR_CONTAINS(s.ToString(), "HTTP 400");
ASSERT_STR_CONTAINS(
buf.ToString(),
@@ -388,6 +433,7 @@ TEST_F(RestCatalogTest, TestPutTableNonExistent) {
}
)",
&buf);
+ ASSERT_TRUE(!s.ok());
ASSERT_STR_CONTAINS(s.ToString(), "HTTP 404");
ASSERT_STR_CONTAINS(buf.ToString(), "{\"error\":\"Table not found\"}");
}
@@ -399,10 +445,10 @@ TEST_F(RestCatalogTest, TestPutTableEndpointAddColumn) {
EasyCurl c;
faststring buf;
c.set_custom_method("PUT");
- Status s = c.PostToURL(Substitute("http://$0/api/v1/tables/$1";,
-
cluster_->mini_master()->bound_http_addr().ToString(),
- table_id),
- R"({
+ ASSERT_OK(c.PostToURL(Substitute("http://$0/api/v1/tables/$1";,
+
cluster_->mini_master()->bound_http_addr().ToString(),
+ table_id),
+ R"({
"table": {
"table_name": "test_table"
},
@@ -420,10 +466,9 @@ TEST_F(RestCatalogTest, TestPutTableEndpointAddColumn) {
]
}
)",
- &buf);
- ASSERT_STR_CONTAINS(s.ToString(), "OK");
+ &buf));
shared_ptr<KuduTable> table;
- s = client_->OpenTable(kTableName, &table);
+ ASSERT_OK(client_->OpenTable(kTableName, &table));
int first_column_id = FindColumnId(buf.ToString());
ASSERT_NE(first_column_id, -1) << "Column ID not found in the schema";
string partition_schema =
@@ -436,12 +481,11 @@ TEST_F(RestCatalogTest, TestPutTableEndpointAddColumn) {
ConstructTableSchema(first_column_id, true),
partition_schema,
table->owner()));
- ASSERT_TRUE(s.ok());
const KuduSchema& schema = table->schema();
- ASSERT_EQ(schema.num_columns(), 3);
- ASSERT_EQ(schema.Column(0).name(), "key");
- ASSERT_EQ(schema.Column(1).name(), "int_val");
- ASSERT_EQ(schema.Column(2).name(), "new_column");
+ ASSERT_EQ(3, schema.num_columns());
+ ASSERT_EQ("key", schema.Column(0).name());
+ ASSERT_EQ("int_val", schema.Column(1).name());
+ ASSERT_EQ("new_column", schema.Column(2).name());
}
TEST_F(RestCatalogTest, TestPutTableEndpointRenameColumn) {
@@ -451,10 +495,10 @@ TEST_F(RestCatalogTest, TestPutTableEndpointRenameColumn)
{
EasyCurl c;
faststring buf;
c.set_custom_method("PUT");
- Status s = c.PostToURL(Substitute("http://$0/api/v1/tables/$1";,
-
cluster_->mini_master()->bound_http_addr().ToString(),
- table_id),
- R"({
+ ASSERT_OK(c.PostToURL(Substitute("http://$0/api/v1/tables/$1";,
+
cluster_->mini_master()->bound_http_addr().ToString(),
+ table_id),
+ R"({
"table": {
"table_name": "test_table"
},
@@ -469,15 +513,13 @@ TEST_F(RestCatalogTest, TestPutTableEndpointRenameColumn)
{
]
}
)",
- &buf);
- ASSERT_STR_CONTAINS(s.ToString(), "OK");
+ &buf));
shared_ptr<KuduTable> table;
- s = client_->OpenTable(kTableName, &table);
- ASSERT_TRUE(s.ok());
+ ASSERT_OK(client_->OpenTable(kTableName, &table));
const KuduSchema& schema = table->schema();
- ASSERT_EQ(schema.num_columns(), 2);
- ASSERT_EQ(schema.Column(0).name(), "key");
- ASSERT_EQ(schema.Column(1).name(), "new_int_val");
+ ASSERT_EQ(2, schema.num_columns());
+ ASSERT_EQ("key", schema.Column(0).name());
+ ASSERT_EQ("new_int_val", schema.Column(1).name());
}
TEST_F(RestCatalogTest, TestPutTableEndpointDropColumn) {
@@ -487,10 +529,10 @@ TEST_F(RestCatalogTest, TestPutTableEndpointDropColumn) {
EasyCurl c;
faststring buf;
c.set_custom_method("PUT");
- Status s = c.PostToURL(Substitute("http://$0/api/v1/tables/$1";,
-
cluster_->mini_master()->bound_http_addr().ToString(),
- table_id),
- R"({
+ ASSERT_OK(c.PostToURL(Substitute("http://$0/api/v1/tables/$1";,
+
cluster_->mini_master()->bound_http_addr().ToString(),
+ table_id),
+ R"({
"table": {
"table_name": "test_table"
},
@@ -504,14 +546,12 @@ TEST_F(RestCatalogTest, TestPutTableEndpointDropColumn) {
]
}
)",
- &buf);
- ASSERT_STR_CONTAINS(s.ToString(), "OK");
+ &buf));
shared_ptr<KuduTable> table;
- s = client_->OpenTable(kTableName, &table);
- ASSERT_TRUE(s.ok());
+ ASSERT_OK(client_->OpenTable(kTableName, &table));
const KuduSchema& schema = table->schema();
- ASSERT_EQ(schema.num_columns(), 1);
- ASSERT_EQ(schema.Column(0).name(), "key");
+ ASSERT_EQ(1, schema.num_columns());
+ ASSERT_EQ("key", schema.Column(0).name());
}
TEST_F(RestCatalogTest, TestPutTableEndpointChangeOwner) {
@@ -521,22 +561,20 @@ TEST_F(RestCatalogTest, TestPutTableEndpointChangeOwner) {
EasyCurl c;
faststring buf;
c.set_custom_method("PUT");
- Status s = c.PostToURL(Substitute("http://$0/api/v1/tables/$1";,
-
cluster_->mini_master()->bound_http_addr().ToString(),
- table_id),
- R"({
+ ASSERT_OK(c.PostToURL(Substitute("http://$0/api/v1/tables/$1";,
+
cluster_->mini_master()->bound_http_addr().ToString(),
+ table_id),
+ R"({
"table": {
"table_name": "test_table"
},
"new_table_owner": "new_owner"
}
)",
- &buf);
- ASSERT_STR_CONTAINS(s.ToString(), "OK");
+ &buf));
shared_ptr<KuduTable> table;
- s = client_->OpenTable(kTableName, &table);
- ASSERT_TRUE(s.ok());
- ASSERT_EQ(table->owner(), "new_owner");
+ ASSERT_OK(client_->OpenTable(kTableName, &table));
+ ASSERT_EQ("new_owner", table->owner());
}
} // namespace master
diff --git a/src/kudu/master/rest_catalog_path_handlers.cc
b/src/kudu/master/rest_catalog_path_handlers.cc
index 078c98beb..ee5ecd626 100644
--- a/src/kudu/master/rest_catalog_path_handlers.cc
+++ b/src/kudu/master/rest_catalog_path_handlers.cc
@@ -20,10 +20,12 @@
#include <functional>
#include <optional>
#include <string>
+#include <unordered_map>
+#include <utility>
-#include "google/protobuf/util/json_util.h"
#include <gflags/gflags.h>
#include <google/protobuf/stubs/status.h>
+#include <google/protobuf/util/json_util.h>
#include "kudu/common/common.pb.h"
#include "kudu/gutil/ref_counted.h"
@@ -35,6 +37,7 @@
#include "kudu/util/flag_tags.h"
#include "kudu/util/jsonwriter.h"
#include "kudu/util/monotime.h"
+#include "kudu/util/status.h"
#include "kudu/util/web_callback_registry.h"
// We only use macros here to maintain cohesion with the existing
RETURN_NOT_OK-style pattern.
@@ -52,6 +55,9 @@
return retval; \
}
+#define RETURN_JSON_ERROR_FROM_STATUS(jw, status, status_code) \
+ RETURN_JSON_ERROR_VAL(jw, (status).ToString(), status_code,
GetHttpCodeFromStatus(status), void())
+
DEFINE_int32(rest_catalog_default_request_timeout_ms, 30 * 1000, "Default
request timeout in ms");
TAG_FLAG(rest_catalog_default_request_timeout_ms, advanced);
TAG_FLAG(rest_catalog_default_request_timeout_ms, runtime);
@@ -66,30 +72,45 @@ using strings::Substitute;
namespace kudu {
namespace master {
-RestCatalogPathHandlers::~RestCatalogPathHandlers() {}
-
-static bool CheckIsInitializedAndIsLeader(JsonWriter* jw,
+static bool CheckIsInitializedAndIsLeader(JsonWriter& jw, // NOLINT
JsonWriter cannot be const
const
CatalogManager::ScopedLeaderSharedLock& l,
- HttpStatusCode* status_code) {
+ HttpStatusCode& status_code //
NOLINT
+) {
if (!l.catalog_status().ok()) {
- RETURN_JSON_ERROR_VAL(*jw,
- l.catalog_status().ToString(),
- *status_code,
- HttpStatusCode::ServiceUnavailable,
- false);
+ RETURN_JSON_ERROR_VAL(
+ jw, l.catalog_status().ToString(), status_code,
HttpStatusCode::ServiceUnavailable, false);
}
if (!l.leader_status().ok()) {
RETURN_JSON_ERROR_VAL(
- *jw, "Master is not the leader", *status_code,
HttpStatusCode::ServiceUnavailable, false);
+ jw, "Master is not the leader", status_code,
HttpStatusCode::InternalServerError, false);
}
return true;
}
+static HttpStatusCode GetHttpCodeFromStatus(const Status& status) {
+ DCHECK(!status.ok());
+ if (status.IsInvalidArgument() || status.IsAlreadyPresent()) {
+ return HttpStatusCode::BadRequest;
+ }
+ if (status.IsNotFound()) {
+ return HttpStatusCode::NotFound;
+ }
+ if (status.IsIllegalState() || status.IsServiceUnavailable()) {
+ return HttpStatusCode::ServiceUnavailable;
+ }
+ return HttpStatusCode::InternalServerError;
+}
+
void RestCatalogPathHandlers::HandleApiTableEndpoint(const
Webserver::WebRequest& req,
Webserver::PrerenderedWebResponse* resp) {
- string table_id = req.path_params.at("table_id");
ostringstream* output = &resp->output;
JsonWriter jw(output, JsonWriter::COMPACT);
+ string table_id;
+ auto table_id_it = req.path_params.find("table_id");
+ if (table_id_it == req.path_params.end()) {
+ RETURN_JSON_ERROR(jw, "Table ID not provided", resp->status_code,
HttpStatusCode::BadRequest);
+ }
+ table_id = table_id_it->second;
if (table_id.length() != 32) {
RETURN_JSON_ERROR(jw,
@@ -98,14 +119,14 @@ void RestCatalogPathHandlers::HandleApiTableEndpoint(const
Webserver::WebRequest
HttpStatusCode::BadRequest);
}
CatalogManager::ScopedLeaderSharedLock l(master_->catalog_manager());
- if (!CheckIsInitializedAndIsLeader(&jw, l, &resp->status_code)) {
+ if (!CheckIsInitializedAndIsLeader(jw, l, resp->status_code)) {
return;
}
scoped_refptr<TableInfo> table;
Status status = master_->catalog_manager()->GetTableInfo(table_id, &table);
if (!status.ok()) {
- RETURN_JSON_ERROR(jw, status.ToString(), resp->status_code,
HttpStatusCode::ServiceUnavailable);
+ RETURN_JSON_ERROR_FROM_STATUS(jw, status, resp->status_code);
}
if (!table) {
@@ -129,7 +150,7 @@ void RestCatalogPathHandlers::HandleApiTablesEndpoint(const
Webserver::WebReques
ostringstream* output = &resp->output;
JsonWriter jw(output, JsonWriter::COMPACT);
CatalogManager::ScopedLeaderSharedLock l(master_->catalog_manager());
- if (!CheckIsInitializedAndIsLeader(&jw, l, &resp->status_code)) {
+ if (!CheckIsInitializedAndIsLeader(jw, l, resp->status_code)) {
return;
}
@@ -143,17 +164,17 @@ void
RestCatalogPathHandlers::HandleApiTablesEndpoint(const Webserver::WebReques
}
}
-void RestCatalogPathHandlers::HandleGetTables(std::ostringstream* output,
+void RestCatalogPathHandlers::HandleGetTables(ostringstream* output,
const Webserver::WebRequest& req,
HttpStatusCode* status_code) {
ListTablesRequestPB request;
ListTablesResponsePB response;
- std::optional<std::string> user = req.authn_principal.empty() ? "default" :
req.authn_principal;
+ optional<string> user = req.authn_principal.empty() ? "default" :
req.authn_principal;
Status status = master_->catalog_manager()->ListTables(&request, &response,
user);
JsonWriter jw(output, JsonWriter::COMPACT);
if (!status.ok()) {
- RETURN_JSON_ERROR(jw, status.ToString(), *status_code,
HttpStatusCode::ServiceUnavailable);
+ RETURN_JSON_ERROR_FROM_STATUS(jw, status, *status_code);
}
jw.StartObject();
jw.String("tables");
@@ -172,7 +193,7 @@ void
RestCatalogPathHandlers::HandleGetTables(std::ostringstream* output,
*status_code = HttpStatusCode::Ok;
}
-void RestCatalogPathHandlers::HandlePostTables(std::ostringstream* output,
+void RestCatalogPathHandlers::HandlePostTables(ostringstream* output,
const Webserver::WebRequest&
req,
HttpStatusCode* status_code) {
CreateTableRequestPB request;
@@ -190,11 +211,11 @@ void
RestCatalogPathHandlers::HandlePostTables(std::ostringstream* output,
*status_code,
HttpStatusCode::BadRequest);
}
- std::optional<std::string> user = req.authn_principal.empty() ? "default" :
req.authn_principal;
+ optional<string> user = req.authn_principal.empty() ? "default" :
req.authn_principal;
Status status = master_->catalog_manager()->CreateTableWithUser(&request,
&response, user);
if (!status.ok()) {
- RETURN_JSON_ERROR(jw, status.ToString(), *status_code,
HttpStatusCode::ServiceUnavailable);
+ RETURN_JSON_ERROR_FROM_STATUS(jw, status, *status_code);
}
IsCreateTableDoneRequestPB check_req;
@@ -207,18 +228,18 @@ void
RestCatalogPathHandlers::HandlePostTables(std::ostringstream* output,
status = master_->catalog_manager()->IsCreateTableDone(&check_req,
&check_resp, user);
if (!status.ok()) {
- RETURN_JSON_ERROR(jw, status.ToString(), *status_code,
HttpStatusCode::ServiceUnavailable);
+ RETURN_JSON_ERROR_FROM_STATUS(jw, status, *status_code);
}
if (check_resp.has_error()) {
RETURN_JSON_ERROR(jw,
check_resp.error().ShortDebugString(),
*status_code,
- HttpStatusCode::ServiceUnavailable);
+ HttpStatusCode::InternalServerError);
}
if (check_resp.done()) {
- PrintTableObject(output, response.table_id());
+ PrintTableObject(output, response.table_id(), status_code);
*status_code = HttpStatusCode::Created;
return;
}
@@ -227,18 +248,18 @@ void
RestCatalogPathHandlers::HandlePostTables(std::ostringstream* output,
RETURN_JSON_ERROR(jw,
"Create table timed out while waiting for operation
completion",
*status_code,
- HttpStatusCode::ServiceUnavailable);
+ HttpStatusCode::InternalServerError);
}
-void RestCatalogPathHandlers::HandleGetTable(std::ostringstream* output,
+void RestCatalogPathHandlers::HandleGetTable(ostringstream* output,
const Webserver::WebRequest& req,
HttpStatusCode* status_code) {
string table_id = req.path_params.at("table_id");
- PrintTableObject(output, table_id);
+ PrintTableObject(output, table_id, status_code);
*status_code = HttpStatusCode::Ok;
}
-void RestCatalogPathHandlers::HandlePutTable(std::ostringstream* output,
+void RestCatalogPathHandlers::HandlePutTable(ostringstream* output,
const Webserver::WebRequest& req,
HttpStatusCode* status_code) {
string table_id = req.path_params.at("table_id");
@@ -259,11 +280,11 @@ void
RestCatalogPathHandlers::HandlePutTable(std::ostringstream* output,
HttpStatusCode::BadRequest);
}
- std::optional<std::string> user = req.authn_principal.empty() ? "default" :
req.authn_principal;
+ optional<string> user = req.authn_principal.empty() ? "default" :
req.authn_principal;
Status status = master_->catalog_manager()->AlterTableWithUser(request,
&response, user);
if (!status.ok()) {
- RETURN_JSON_ERROR(jw, status.ToString(), *status_code,
HttpStatusCode::ServiceUnavailable);
+ RETURN_JSON_ERROR_FROM_STATUS(jw, status, *status_code);
}
IsAlterTableDoneRequestPB check_req;
@@ -276,18 +297,18 @@ void
RestCatalogPathHandlers::HandlePutTable(std::ostringstream* output,
status = master_->catalog_manager()->IsAlterTableDone(&check_req,
&check_resp, user);
if (!status.ok()) {
- RETURN_JSON_ERROR(jw, status.ToString(), *status_code,
HttpStatusCode::ServiceUnavailable);
+ RETURN_JSON_ERROR_FROM_STATUS(jw, status, *status_code);
}
if (check_resp.has_error()) {
RETURN_JSON_ERROR(jw,
check_resp.error().ShortDebugString(),
*status_code,
- HttpStatusCode::ServiceUnavailable);
+ HttpStatusCode::InternalServerError);
}
if (check_resp.done()) {
- PrintTableObject(output, table_id);
+ PrintTableObject(output, table_id, status_code);
*status_code = HttpStatusCode::Ok;
return;
}
@@ -296,10 +317,10 @@ void
RestCatalogPathHandlers::HandlePutTable(std::ostringstream* output,
RETURN_JSON_ERROR(jw,
"Alter table timed out while waiting for operation
completion",
*status_code,
- HttpStatusCode::ServiceUnavailable);
+ HttpStatusCode::InternalServerError);
}
-void RestCatalogPathHandlers::HandleDeleteTable(std::ostringstream* output,
+void RestCatalogPathHandlers::HandleDeleteTable(ostringstream* output,
const Webserver::WebRequest&
req,
HttpStatusCode* status_code) {
string table_id = req.path_params.at("table_id");
@@ -307,20 +328,25 @@ void
RestCatalogPathHandlers::HandleDeleteTable(std::ostringstream* output,
DeleteTableResponsePB response;
request.mutable_table()->set_table_id(table_id);
JsonWriter jw(output, JsonWriter::COMPACT);
- std::optional<std::string> user = req.authn_principal.empty() ? "default" :
req.authn_principal;
+ optional<string> user = req.authn_principal.empty() ? "default" :
req.authn_principal;
Status status = master_->catalog_manager()->DeleteTableWithUser(request,
&response, user);
if (status.ok()) {
*status_code = HttpStatusCode::NoContent;
} else {
- RETURN_JSON_ERROR(jw, status.ToString(), *status_code,
HttpStatusCode::ServiceUnavailable);
+ RETURN_JSON_ERROR_FROM_STATUS(jw, status, *status_code);
}
}
-void RestCatalogPathHandlers::PrintTableObject(std::ostringstream* output,
const string& table_id) {
+void RestCatalogPathHandlers::PrintTableObject(ostringstream* output,
+ const string& table_id,
+ HttpStatusCode* status_code) {
scoped_refptr<TableInfo> table;
Status status = master_->catalog_manager()->GetTableInfo(table_id, &table);
JsonWriter jw(output, JsonWriter::COMPACT);
+ if (!status.ok()) {
+ RETURN_JSON_ERROR_FROM_STATUS(jw, status, *status_code);
+ }
jw.StartObject();
{
@@ -343,7 +369,7 @@ void
RestCatalogPathHandlers::PrintTableObject(std::ostringstream* output, const
jw.EndObject();
}
-Status RestCatalogPathHandlers::Register(Webserver* server) {
+void RestCatalogPathHandlers::Register(Webserver* server) {
server->RegisterPrerenderedPathHandler(
"/api/v1/tables/<table_id>",
"",
@@ -360,7 +386,6 @@ Status RestCatalogPathHandlers::Register(Webserver* server)
{
},
StyleMode::JSON,
false);
- return Status::OK();
}
} // namespace master
diff --git a/src/kudu/master/rest_catalog_path_handlers.h
b/src/kudu/master/rest_catalog_path_handlers.h
index 079c000d2..a4f0d97ce 100644
--- a/src/kudu/master/rest_catalog_path_handlers.h
+++ b/src/kudu/master/rest_catalog_path_handlers.h
@@ -20,9 +20,10 @@
#include <iosfwd>
#include <string>
+#include <glog/logging.h>
+
#include "kudu/gutil/macros.h"
#include "kudu/server/webserver.h"
-#include "kudu/util/status.h"
#include "kudu/util/web_callback_registry.h"
namespace kudu {
@@ -31,13 +32,13 @@ namespace master {
class Master;
-class RestCatalogPathHandlers {
+class RestCatalogPathHandlers final {
public:
- explicit RestCatalogPathHandlers(Master* master) : master_(master) {}
+ explicit RestCatalogPathHandlers(Master* master) :
master_(DCHECK_NOTNULL(master)) {}
- ~RestCatalogPathHandlers();
+ ~RestCatalogPathHandlers() = default;
- Status Register(Webserver* server);
+ void Register(Webserver* server);
private:
void HandleApiTableEndpoint(const Webserver::WebRequest& req,
@@ -63,7 +64,9 @@ class RestCatalogPathHandlers {
HttpStatusCode* status_code);
// Print a JSON object representing a table to 'output'.
- void PrintTableObject(std::ostringstream* output, const std::string&
table_id);
+ void PrintTableObject(std::ostringstream* output,
+ const std::string& table_id,
+ HttpStatusCode* status_code);
Master* master_;
DISALLOW_COPY_AND_ASSIGN(RestCatalogPathHandlers);
diff --git a/src/kudu/master/rest_catalog_test_base.h
b/src/kudu/master/rest_catalog_test_base.h
index 4047c9532..6522f8ac1 100644
--- a/src/kudu/master/rest_catalog_test_base.h
+++ b/src/kudu/master/rest_catalog_test_base.h
@@ -40,50 +40,46 @@
#include "kudu/util/test_macros.h"
#include "kudu/util/test_util.h"
-DECLARE_bool(enable_rest_api);
-
namespace kudu {
namespace master {
class RestCatalogTestBase : public KuduTest {
protected:
- kudu::Status CreateTestTable(const std::string& owner = "") {
- kudu::client::KuduSchema schema;
- kudu::client::KuduSchemaBuilder b;
-
b.AddColumn("key")->Type(kudu::client::KuduColumnSchema::INT32)->NotNull()->PrimaryKey();
-
b.AddColumn("int_val")->Type(kudu::client::KuduColumnSchema::INT32)->NotNull();
+ Status CreateTestTable(const std::string& owner = "") {
+ client::KuduSchema schema;
+ client::KuduSchemaBuilder b;
+
b.AddColumn("key")->Type(client::KuduColumnSchema::INT32)->NotNull()->PrimaryKey();
+ b.AddColumn("int_val")->Type(client::KuduColumnSchema::INT32)->NotNull();
RETURN_NOT_OK(b.Build(&schema));
- std::vector<std::string> columnNames;
- columnNames.emplace_back("key");
+ const std::vector<std::string> column_names{ "key" };
// Set the schema and range partition columns.
- std::unique_ptr<kudu::client::KuduTableCreator>
tableCreator(client_->NewTableCreator());
-
tableCreator->table_name(kTableName).schema(&schema).set_range_partition_columns(columnNames);
+ std::unique_ptr<client::KuduTableCreator>
table_creator(client_->NewTableCreator());
+
table_creator->table_name(kTableName).schema(&schema).set_range_partition_columns(column_names);
// Generate and add the range partition splits for the table.
- int32_t increment = 1000 / 10;
+ constexpr int32_t increment = 1000 / 10;
for (int32_t i = 1; i < 10; i++) {
- kudu::KuduPartialRow* row = schema.NewRow();
+ KuduPartialRow* row = schema.NewRow();
KUDU_CHECK_OK(row->SetInt32(0, i * increment));
- tableCreator->add_range_partition_split(row);
+ table_creator->add_range_partition_split(row);
}
- tableCreator->num_replicas(1);
+ table_creator->num_replicas(1);
if (!owner.empty()) {
- tableCreator->set_owner(owner);
+ table_creator->set_owner(owner);
}
- kudu::Status s = tableCreator->Create();
- return s;
+ return table_creator->Create();
}
Status GetTableId(const std::string& table_name, std::string* table_id) {
DCHECK(table_id);
- kudu::client::sp::shared_ptr<kudu::client::KuduTable> table;
+ client::sp::shared_ptr<client::KuduTable> table;
RETURN_NOT_OK(client_->OpenTable(table_name, &table));
*table_id = table->id();
return Status::OK();
}
- kudu::client::sp::shared_ptr<kudu::client::KuduClient> client_;
+ client::sp::shared_ptr<client::KuduClient> client_;
std::string kTableName = "test_table";
};
diff --git a/src/kudu/master/spnego_rest_catalog-test.cc
b/src/kudu/master/spnego_rest_catalog-test.cc
index e1e0e898b..4ceab0eec 100644
--- a/src/kudu/master/spnego_rest_catalog-test.cc
+++ b/src/kudu/master/spnego_rest_catalog-test.cc
@@ -18,6 +18,7 @@
#include <memory>
#include <string>
#include <type_traits>
+#include <utility>
#include <gflags/gflags_declare.h>
#include <gtest/gtest.h>
@@ -64,15 +65,15 @@ class SpnegoRestCatalogTest : public RestCatalogTestBase {
ASSERT_OK(kdc_->SetKrb5Environment());
string kt_path;
ASSERT_OK(kdc_->CreateServiceKeytabWithName("HTTP/127.0.0.1",
"spnego.dedicated", &kt_path));
- ASSERT_OK(kdc_->CreateUserPrincipal("alice"));
+ ASSERT_OK(kdc_->CreateUserPrincipal(kDefaultPrincipal));
FLAGS_spnego_keytab_file = kt_path;
FLAGS_webserver_require_spnego = true;
FLAGS_enable_rest_api = true;
- auto opts = InternalMiniClusterOptions();
+ InternalMiniClusterOptions opts;
opts.bind_mode = BindMode::LOOPBACK;
- cluster_.reset(new InternalMiniCluster(env_, opts));
+ cluster_.reset(new InternalMiniCluster(env_, std::move(opts)));
ASSERT_OK(cluster_->Start());
ASSERT_OK(KuduClientBuilder()
@@ -83,12 +84,13 @@ class SpnegoRestCatalogTest : public RestCatalogTestBase {
protected:
unique_ptr<MiniKdc> kdc_;
unique_ptr<InternalMiniCluster> cluster_;
+ const string kDefaultPrincipal = "alice";
Status CreateTestTableAsAlice() { return
CreateTestTable("al...@krbtest.com"); }
};
TEST_F(SpnegoRestCatalogTest, TestGetTablesOneTable) {
- ASSERT_OK(kdc_->Kinit("alice"));
+ ASSERT_OK(kdc_->Kinit(kDefaultPrincipal));
ASSERT_OK(CreateTestTableAsAlice());
EasyCurl c;
faststring buf;
@@ -104,7 +106,7 @@ TEST_F(SpnegoRestCatalogTest, TestGetTablesOneTable) {
}
TEST_F(SpnegoRestCatalogTest, TestGetTableWithUser) {
- ASSERT_OK(kdc_->Kinit("alice"));
+ ASSERT_OK(kdc_->Kinit(kDefaultPrincipal));
ASSERT_OK(CreateTestTableAsAlice());
string table_id;
ASSERT_OK(GetTableId(kTableName, &table_id));
@@ -121,12 +123,12 @@ TEST_F(SpnegoRestCatalogTest, TestGetTableWithUser) {
}
TEST_F(SpnegoRestCatalogTest, TestPostTableWithUser) {
- ASSERT_OK(kdc_->Kinit("alice"));
+ ASSERT_OK(kdc_->Kinit(kDefaultPrincipal));
EasyCurl c;
faststring buf;
c.set_custom_method("POST");
c.set_auth(CurlAuthType::SPNEGO);
- Status s = c.PostToURL(
+ ASSERT_OK(c.PostToURL(
Substitute("http://$0/api/v1/tables";,
cluster_->mini_master()->bound_http_addr().ToString()),
R"({
"name": "test_table",
@@ -143,17 +145,16 @@ TEST_F(SpnegoRestCatalogTest, TestPostTableWithUser) {
},
"num_replicas": 1
})",
- &buf);
- ASSERT_STR_CONTAINS(s.ToString(), "OK");
+ &buf));
string table_id;
ASSERT_OK(GetTableId(kTableName, &table_id));
shared_ptr<KuduTable> table;
ASSERT_OK(client_->OpenTable(kTableName, &table));
- ASSERT_STR_CONTAINS(table->owner(), "alice");
+ ASSERT_STR_CONTAINS(table->owner(), kDefaultPrincipal);
}
TEST_F(SpnegoRestCatalogTest, TestDeleteTableWithUser) {
- ASSERT_OK(kdc_->Kinit("alice"));
+ ASSERT_OK(kdc_->Kinit(kDefaultPrincipal));
ASSERT_OK(CreateTestTableAsAlice());
string table_id;
ASSERT_OK(GetTableId(kTableName, &table_id));
@@ -171,7 +172,7 @@ TEST_F(SpnegoRestCatalogTest, TestDeleteTableWithUser) {
}
TEST_F(SpnegoRestCatalogTest, TestAlterTableWithUser) {
- ASSERT_OK(kdc_->Kinit("alice"));
+ ASSERT_OK(kdc_->Kinit(kDefaultPrincipal));
ASSERT_OK(CreateTestTableAsAlice());
string table_id;
ASSERT_OK(GetTableId(kTableName, &table_id));
@@ -203,8 +204,8 @@ TEST_F(SpnegoRestCatalogTest, TestAlterTableWithUser) {
&buf));
shared_ptr<KuduTable> table;
ASSERT_OK(client_->OpenTable(kTableName, &table));
- ASSERT_EQ(table->schema().num_columns(), 3);
- ASSERT_STR_CONTAINS(table->owner(), "alice");
+ ASSERT_EQ(3, table->schema().num_columns());
+ ASSERT_STR_CONTAINS(table->owner(), kDefaultPrincipal);
}
TEST_F(SpnegoRestCatalogTest, TestInvalidHeaders) {
@@ -226,6 +227,9 @@ TEST_F(SpnegoRestCatalogTest, TestInvalidHeaders) {
ASSERT_STR_CONTAINS(s.ToString(), "HTTP 401");
}
+// The following tests are skipped on macOS due to inconsistent behavior of
SPNEGO.
+// macOS heimdal kerberos caches the KDC port number, which can cause
subsequent tests to fail.
+// For more details, refer to KUDU-3533
(https://issues.apache.org/jira/browse/KUDU-3533)
#ifndef __APPLE__
TEST_F(SpnegoRestCatalogTest, TestNoKinitGetTables) {
@@ -255,7 +259,7 @@ TEST_F(SpnegoRestCatalogTest, TestNoKinitGetTable) {
}
TEST_F(SpnegoRestCatalogTest, TestUnauthenticatedBadKeytab) {
- ASSERT_OK(kdc_->Kinit("alice"));
+ ASSERT_OK(kdc_->Kinit(kDefaultPrincipal));
ASSERT_OK(kdc_->RandomizePrincipalKey("HTTP/127.0.0.1"));
EasyCurl c;
faststring buf;
