You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by tm...@apache.org on 2019/10/09 00:27:49 UTC
[impala] branch master updated (c47fca5 -> d1b42c8)
This is an automated email from the ASF dual-hosted git repository.
tmarshall pushed a change to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git.
from c47fca5 IMPALA-8962: FETCH_ROWS_TIMEOUT_MS should apply before rows are available
new 4f8a1b9 IMPALA-9017: Alter table events on dropped table/db should be ignored.
new d1b42c8 IMPALA-8899, IMPALA-8898: Add cookie support to the webui
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:
be/src/catalog/catalogd-main.cc | 2 +-
be/src/rpc/authentication.cc | 21 ++-
be/src/rpc/cookie-util.cc | 48 +++---
be/src/rpc/cookie-util.h | 10 +-
be/src/runtime/exec-env.cc | 2 +-
be/src/statestore/statestored-main.cc | 2 +-
be/src/util/webserver-test.cc | 138 ++++++++++++---
be/src/util/webserver.cc | 192 ++++++++++-----------
be/src/util/webserver.h | 27 ++-
common/thrift/metrics.json | 48 ++++++
.../impala/catalog/CatalogServiceCatalog.java | 34 ++--
.../impala/catalog/events/MetastoreEvents.java | 18 +-
.../events/MetastoreEventsProcessorTest.java | 25 +++
.../apache/impala/customcluster/LdapHS2Test.java | 1 +
14 files changed, 372 insertions(+), 196 deletions(-)
[impala] 02/02: IMPALA-8899,
IMPALA-8898: Add cookie support to the webui
Posted by tm...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
tmarshall pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git
commit d1b42c836c3458a2ef3662c0b0b1fd8fbf8f2baf
Author: Thomas Tauber-Marshall <tm...@cloudera.com>
AuthorDate: Wed Sep 25 15:45:45 2019 -0700
IMPALA-8899, IMPALA-8898: Add cookie support to the webui
This patches takes the machinery for generating and checking cookies
for authentication that was added in IMPALA-8584 and applies to the
webui.
It also fixes an issue where some clients, for example Knox, may
return the cookie value surrounded by "".
It adds metrics for both SPNEGO auth success/failure and cookie auth
success/failure to the webserver.
This patch also fixes IMPALA-8898 by returning an empty cookie with a
Max-Age of 0 on requests where an invalid cookie was provided to
indicate to the client that the cookie should be deleted.
Testing:
- Added a test that uses curl to access the webserver with SPNEGO
enabled while storing and using cookies. This test only runs when
curl is present and has the necessary options enabled, which is
generally not the case in our automated testing runs.
Change-Id: I30788e0539627ee6154ad8183b124947c5da8ef4
Reviewed-on: http://gerrit.cloudera.org:8080/14339
Tested-by: Impala Public Jenkins <im...@cloudera.com>
Reviewed-by: Thomas Tauber-Marshall <tm...@cloudera.com>
---
be/src/catalog/catalogd-main.cc | 2 +-
be/src/rpc/authentication.cc | 21 ++-
be/src/rpc/cookie-util.cc | 48 +++---
be/src/rpc/cookie-util.h | 10 +-
be/src/runtime/exec-env.cc | 2 +-
be/src/statestore/statestored-main.cc | 2 +-
be/src/util/webserver-test.cc | 138 ++++++++++++---
be/src/util/webserver.cc | 192 ++++++++++-----------
be/src/util/webserver.h | 27 ++-
common/thrift/metrics.json | 48 ++++++
.../apache/impala/customcluster/LdapHS2Test.java | 1 +
11 files changed, 325 insertions(+), 166 deletions(-)
diff --git a/be/src/catalog/catalogd-main.cc b/be/src/catalog/catalogd-main.cc
index 67d6f39..8479326 100644
--- a/be/src/catalog/catalogd-main.cc
+++ b/be/src/catalog/catalogd-main.cc
@@ -62,8 +62,8 @@ int CatalogdMain(int argc, char** argv) {
InitCommonRuntime(argc, argv, true);
InitFeSupport();
- scoped_ptr<Webserver> webserver(new Webserver());
scoped_ptr<MetricGroup> metrics(new MetricGroup("catalog"));
+ scoped_ptr<Webserver> webserver(new Webserver(metrics.get()));
if (FLAGS_enable_webserver) {
AddDefaultUrlCallbacks(webserver.get(), metrics.get());
diff --git a/be/src/rpc/authentication.cc b/be/src/rpc/authentication.cc
index 3ba6a31..af2808c 100644
--- a/be/src/rpc/authentication.cc
+++ b/be/src/rpc/authentication.cc
@@ -503,6 +503,23 @@ static int SaslGetPath(void* context, const char** path) {
return SASL_OK;
}
+bool CookieAuth(ThriftServer::ConnectionContext* connection_context,
+ const AuthenticationHash& hash, const std::string& cookie_header) {
+ string username;
+ Status cookie_status = AuthenticateCookie(hash, cookie_header, &username);
+ if (cookie_status.ok()) {
+ connection_context->username = username;
+ return true;
+ }
+
+ LOG(INFO) << "Invalid cookie provided: " << cookie_header
+ << " from: " << TNetworkAddressToString(connection_context->network_address)
+ << ": " << cookie_status.GetDetail();
+ connection_context->return_headers.push_back(
+ Substitute("Set-Cookie: $0", GetDeleteCookie()));
+ return false;
+}
+
bool BasicAuth(ThriftServer::ConnectionContext* connection_context,
const AuthenticationHash& hash, const std::string& base64) {
if (base64.empty()) {
@@ -1062,8 +1079,8 @@ void SecureAuthProvider::SetupConnectionContext(
callbacks.path_fn = std::bind(
HttpPathFn, connection_ptr.get(), std::placeholders::_1, std::placeholders::_2);
callbacks.return_headers_fn = std::bind(ReturnHeaders, connection_ptr.get());
- callbacks.cookie_auth_fn = std::bind(
- AuthenticateCookie, connection_ptr.get(), hash_, std::placeholders::_1);
+ callbacks.cookie_auth_fn =
+ std::bind(CookieAuth, connection_ptr.get(), hash_, std::placeholders::_1);
if (has_ldap_) {
callbacks.basic_auth_fn =
std::bind(BasicAuth, connection_ptr.get(), hash_, std::placeholders::_1);
diff --git a/be/src/rpc/cookie-util.cc b/be/src/rpc/cookie-util.cc
index 20130be..d8a0ca9 100644
--- a/be/src/rpc/cookie-util.cc
+++ b/be/src/rpc/cookie-util.cc
@@ -41,7 +41,7 @@ static const string COOKIE_SEPARATOR = "&";
// Cookies generated and processed by the HTTP server will be of the form:
// COOKIE_NAME=<cookie>
-static const string COOKIE_NAME = "impala.hs2.auth";
+static const string COOKIE_NAME = "impala.auth";
// The maximum lenth for the base64 encoding of a SHA256 hash.
static const int SHA256_BASE64_LEN =
@@ -53,9 +53,8 @@ static const int SHA256_BASE64_LEN =
// find the first one with COOKIE_NAME.
static const int MAX_COOKIES_TO_CHECK = 5;
-bool AuthenticateCookie(ThriftServer::ConnectionContext* connection_context,
- const AuthenticationHash& hash, const string& cookie_header) {
- string error_str = "";
+Status AuthenticateCookie(
+ const AuthenticationHash& hash, const string& cookie_header, string* username) {
// The 'Cookie' header allows sending multiple name/value pairs separated by ';'.
vector<string> cookies = strings::Split(cookie_header, ";");
if (cookies.size() > MAX_COOKIES_TO_CHECK) {
@@ -68,64 +67,55 @@ bool AuthenticateCookie(ThriftServer::ConnectionContext* connection_context,
StripWhiteSpace(&cookie_pair);
string cookie;
if (!TryStripPrefixString(cookie_pair, StrCat(COOKIE_NAME, "="), &cookie)) {
- error_str = Substitute("Did not find expected cookie name: $0", COOKIE_NAME);
continue;
}
+ if (cookie[0] == '"' && cookie[cookie.length() - 1] == '"') {
+ cookie = cookie.substr(1, cookie.length() - 2);
+ }
// Split the cookie into the signature and the cookie value.
vector<string> cookie_split = Split(cookie, delimiter::Limit(COOKIE_SEPARATOR, 1));
if (cookie_split.size() != 2) {
- error_str = "The cookie has an invalid format.";
- goto error;
+ return Status("The cookie has an invalid format.");
}
const string& base64_signature = cookie_split[0];
const string& cookie_value = cookie_split[1];
string signature;
if (!WebSafeBase64Unescape(base64_signature, &signature)) {
- error_str = "Unable to decode base64 signature.";
- goto error;
+ return Status("Unable to decode base64 signature.");
}
if (signature.length() != AuthenticationHash::HashLen()) {
- error_str = "Signature is an incorrect length.";
- goto error;
+ return Status("Signature is an incorrect length.");
}
bool verified = hash.Verify(reinterpret_cast<const uint8_t*>(cookie_value.data()),
cookie_value.length(), reinterpret_cast<const uint8_t*>(signature.data()));
if (!verified) {
- error_str = "The signature is incorrect.";
- goto error;
+ return Status("The signature is incorrect.");
}
// Split the cookie value into username, timestamp, and random number.
vector<string> cookie_value_split = Split(cookie_value, COOKIE_SEPARATOR);
if (cookie_value_split.size() != 3) {
- error_str = "The cookie value has an invalid format.";
- goto error;
+ return Status("The cookie value has an invalid format.");
}
StringParser::ParseResult result;
int64_t create_time = StringParser::StringToInt<int64_t>(
cookie_value_split[1].c_str(), cookie_value_split[1].length(), &result);
if (result != StringParser::PARSE_SUCCESS) {
- error_str = "Could not parse cookie timestamp.";
- goto error;
+ return Status("Could not parse cookie timestamp.");
}
// Check that the timestamp contained in the cookie is recent enough for the cookie
// to still be valid.
if (MonotonicMillis() - create_time <= FLAGS_max_cookie_lifetime_s * 1000) {
// We've successfully authenticated.
- connection_context->username = cookie_value_split[0];
- return true;
+ *username = cookie_value_split[0];
+ return Status::OK();
} else {
- error_str = "Cookie is past its max lifetime.";
- goto error;
+ return Status("Cookie is past its max lifetime.");
}
}
-error:
- LOG(INFO) << "Invalid cookie provided: " << cookie_header
- << " from: " << TNetworkAddressToString(connection_context->network_address)
- << ": " << error_str;
- return false;
+ return Status(Substitute("Did not find expected cookie name: $0", COOKIE_NAME));
}
string GenerateCookie(const string& username, const AuthenticationHash& hash) {
@@ -155,8 +145,12 @@ string GenerateCookie(const string& username, const AuthenticationHash& hash) {
// connections. This is for testing only.
secure_flag = "";
}
- return Substitute("$0=$1$2$3;HttpOnly;MaxAge=$4$5", COOKIE_NAME, base64_signature,
+ return Substitute("$0=$1$2$3;HttpOnly;Max-Age=$4$5", COOKIE_NAME, base64_signature,
COOKIE_SEPARATOR, cookie_value, FLAGS_max_cookie_lifetime_s, secure_flag);
}
+string GetDeleteCookie() {
+ return Substitute("$0=;HttpOnly;Max-Age=0", COOKIE_NAME);
+}
+
} // namespace impala
diff --git a/be/src/rpc/cookie-util.h b/be/src/rpc/cookie-util.h
index 93adb2d..5482087 100644
--- a/be/src/rpc/cookie-util.h
+++ b/be/src/rpc/cookie-util.h
@@ -25,12 +25,16 @@ class AuthenticationHash;
// Takes a single 'key=value' pair from a 'Cookie' header and attempts to verify its
// signature with 'hash'. If verification is successful and the cookie is still valid,
-// sets the corresponding username on 'connection_context' and returns true.
-bool AuthenticateCookie(ThriftServer::ConnectionContext* connection_context,
- const AuthenticationHash& hash, const std::string& cookie_header);
+// sets 'username' to the corresponding username and returns OK.
+Status AuthenticateCookie(const AuthenticationHash& hash,
+ const std::string& cookie_header, std::string* username);
// Generates and returns a cookie containing the username set on 'connection_context' and
// a signature generated with 'hash'.
std::string GenerateCookie(const std::string& username, const AuthenticationHash& hash);
+// Returns a empty cookie. Returned in a 'Set-Cookie' when cookie auth fails to indicate
+// to the client that the cookie should be deleted.
+std::string GetDeleteCookie();
+
} // namespace impala
diff --git a/be/src/runtime/exec-env.cc b/be/src/runtime/exec-env.cc
index 6d8dc82..600ce73 100644
--- a/be/src/runtime/exec-env.cc
+++ b/be/src/runtime/exec-env.cc
@@ -208,7 +208,7 @@ ExecEnv::ExecEnv(int backend_port, int krpc_port,
!FLAGS_ssl_client_ca_certificate.empty())),
htable_factory_(new HBaseTableFactory()),
disk_io_mgr_(new io::DiskIoMgr()),
- webserver_(new Webserver(FLAGS_webserver_interface, webserver_port)),
+ webserver_(new Webserver(FLAGS_webserver_interface, webserver_port, metrics_.get())),
pool_mem_trackers_(new PoolMemTrackerRegistry),
thread_mgr_(new ThreadResourceMgr),
tmp_file_mgr_(new TmpFileMgr),
diff --git a/be/src/statestore/statestored-main.cc b/be/src/statestore/statestored-main.cc
index d563de5..205447c 100644
--- a/be/src/statestore/statestored-main.cc
+++ b/be/src/statestore/statestored-main.cc
@@ -49,8 +49,8 @@ int StatestoredMain(int argc, char** argv) {
FLAGS_webserver_port = 25010;
InitCommonRuntime(argc, argv, false);
- scoped_ptr<Webserver> webserver(new Webserver());
scoped_ptr<MetricGroup> metrics(new MetricGroup("statestore"));
+ scoped_ptr<Webserver> webserver(new Webserver(metrics.get()));
if (FLAGS_enable_webserver) {
AddDefaultUrlCallbacks(webserver.get(), metrics.get());
diff --git a/be/src/util/webserver-test.cc b/be/src/util/webserver-test.cc
index 0ea9a41..fcab593 100644
--- a/be/src/util/webserver-test.cc
+++ b/be/src/util/webserver-test.cc
@@ -18,6 +18,7 @@
#include <string>
#include <boost/asio.hpp>
#include <boost/bind.hpp>
+#include <boost/filesystem.hpp>
#include <boost/lexical_cast.hpp>
#include <gutil/strings/substitute.h>
#include <openssl/ssl.h>
@@ -28,6 +29,8 @@
#include "util/default-path-handlers.h"
#include "util/kudu-status-util.h"
+#include "util/metrics.h"
+#include "util/os-util.h"
#include "util/webserver.h"
#include "kudu/security/test/mini_kdc.h"
@@ -41,10 +44,12 @@ DECLARE_string(webserver_private_key_password_cmd);
DECLARE_string(webserver_x_frame_options);
DECLARE_string(ssl_cipher_list);
DECLARE_string(ssl_minimum_version);
+DECLARE_bool(ldap_passwords_in_clear_ok);
#include "common/names.h"
using boost::asio::ip::tcp;
+namespace filesystem = boost::filesystem;
using namespace impala;
using namespace rapidjson;
using namespace strings;
@@ -102,7 +107,8 @@ Status HttpGet(const string& host, const int32_t& port, const string& url_path,
}
TEST(Webserver, SmokeTest) {
- Webserver webserver("", FLAGS_webserver_port);
+ MetricGroup metrics("webserver-test");
+ Webserver webserver("", FLAGS_webserver_port, &metrics);
ASSERT_OK(webserver.Start());
AddDefaultUrlCallbacks(&webserver);
@@ -117,7 +123,8 @@ void AssertArgsCallback(bool* success, const Webserver::WebRequest& req,
}
TEST(Webserver, ArgsTest) {
- Webserver webserver("", FLAGS_webserver_port);
+ MetricGroup metrics("webserver-test");
+ Webserver webserver("", FLAGS_webserver_port, &metrics);
const string ARGS_TEST_PATH = "/args-test";
bool success = false;
@@ -147,7 +154,8 @@ void JsonCallback(bool always_text, const Webserver::WebRequest& req,
}
TEST(Webserver, JsonTest) {
- Webserver webserver("", FLAGS_webserver_port);
+ MetricGroup metrics("webserver-test");
+ Webserver webserver("", FLAGS_webserver_port, &metrics);
const string JSON_TEST_PATH = "/json-test";
const string RAW_TEXT_PATH = "/text";
@@ -189,7 +197,8 @@ TEST(Webserver, JsonTest) {
}
TEST(Webserver, EscapingTest) {
- Webserver webserver("", FLAGS_webserver_port);
+ MetricGroup metrics("webserver-test");
+ Webserver webserver("", FLAGS_webserver_port, &metrics);
const string JSON_TEST_PATH = "/json-test";
Webserver::UrlCallback callback = bind<void>(JsonCallback, false, _1, _2);
@@ -202,7 +211,8 @@ TEST(Webserver, EscapingTest) {
}
TEST(Webserver, EscapeErrorUriTest) {
- Webserver webserver("", FLAGS_webserver_port);
+ MetricGroup metrics("webserver-test");
+ Webserver webserver("", FLAGS_webserver_port, &metrics);
ASSERT_OK(webserver.Start());
stringstream contents;
ASSERT_OK(HttpGet("localhost", FLAGS_webserver_port,
@@ -218,7 +228,8 @@ TEST(Webserver, SslTest) {
auto key = ScopedFlagSetter<string>::Make(&FLAGS_webserver_private_key_file,
Substitute("$0/be/src/testutil/server-key.pem", getenv("IMPALA_HOME")));
- Webserver webserver("", FLAGS_webserver_port);
+ MetricGroup metrics("webserver-test");
+ Webserver webserver("", FLAGS_webserver_port, &metrics);
ASSERT_OK(webserver.Start());
}
@@ -228,7 +239,8 @@ TEST(Webserver, SslBadCertTest) {
auto key = ScopedFlagSetter<string>::Make(&FLAGS_webserver_private_key_file,
Substitute("$0/be/src/testutil/server-key.pem", getenv("IMPALA_HOME")));
- Webserver webserver("", FLAGS_webserver_port);
+ MetricGroup metrics("webserver-test");
+ Webserver webserver("", FLAGS_webserver_port, &metrics);
ASSERT_FALSE(webserver.Start().ok());
}
@@ -240,7 +252,8 @@ TEST(Webserver, SslWithPrivateKeyPasswordTest) {
auto cmd = ScopedFlagSetter<string>::Make(
&FLAGS_webserver_private_key_password_cmd, "echo password");
- Webserver webserver("", FLAGS_webserver_port);
+ MetricGroup metrics("webserver-test");
+ Webserver webserver("", FLAGS_webserver_port, &metrics);
ASSERT_OK(webserver.Start());
}
@@ -252,7 +265,8 @@ TEST(Webserver, SslBadPrivateKeyPasswordTest) {
auto cmd = ScopedFlagSetter<string>::Make(
&FLAGS_webserver_private_key_password_cmd, "echo wrongpassword");
- Webserver webserver("", FLAGS_webserver_port);
+ MetricGroup metrics("webserver-test");
+ Webserver webserver("", FLAGS_webserver_port, &metrics);
ASSERT_FALSE(webserver.Start().ok());
}
@@ -267,14 +281,16 @@ TEST(Webserver, SslCipherSuite) {
{
auto ciphers = ScopedFlagSetter<string>::Make(
&FLAGS_ssl_cipher_list, "not_a_cipher");
- Webserver webserver("", FLAGS_webserver_port);
+ MetricGroup metrics("webserver-test");
+ Webserver webserver("", FLAGS_webserver_port, &metrics);
ASSERT_FALSE(webserver.Start().ok());
}
{
auto ciphers = ScopedFlagSetter<string>::Make(
&FLAGS_ssl_cipher_list, "AES128-SHA");
- Webserver webserver("", FLAGS_webserver_port);
+ MetricGroup metrics("webserver-test");
+ Webserver webserver("", FLAGS_webserver_port, &metrics);
ASSERT_OK(webserver.Start());
}
}
@@ -290,7 +306,8 @@ TEST(Webserver, SslBadTlsVersion) {
auto ssl_version = ScopedFlagSetter<string>::Make(
&FLAGS_ssl_minimum_version, "not_a_version");
- Webserver webserver("", FLAGS_webserver_port);
+ MetricGroup metrics("webserver-test");
+ Webserver webserver("", FLAGS_webserver_port, &metrics);
ASSERT_FALSE(webserver.Start().ok());
}
@@ -313,14 +330,16 @@ TEST(Webserver, SslGoodTlsVersion) {
auto ssl_version = ScopedFlagSetter<string>::Make(
&FLAGS_ssl_minimum_version, v);
- Webserver webserver("", FLAGS_webserver_port);
+ MetricGroup metrics("webserver-test");
+ Webserver webserver("", FLAGS_webserver_port, &metrics);
ASSERT_OK(webserver.Start());
}
for (auto v : unsupported_versions) {
auto ssl_version = ScopedFlagSetter<string>::Make(&FLAGS_ssl_minimum_version, v);
- Webserver webserver("", FLAGS_webserver_port);
+ MetricGroup metrics("webserver-test");
+ Webserver webserver("", FLAGS_webserver_port, &metrics);
EXPECT_FALSE(webserver.Start().ok()) << "Version: " << v;
}
}
@@ -328,6 +347,22 @@ TEST(Webserver, SslGoodTlsVersion) {
using kudu::MiniKdc;
using kudu::MiniKdcOptions;
+void CheckAuthMetrics(MetricGroup* metrics, int num_negotiate_success,
+ int num_negotiate_failure, int num_cookie_success, int num_cookie_failure) {
+ IntCounter* negotiate_success_metric = metrics->FindMetricForTesting<IntCounter>(
+ "impala.webserver.total-negotiate-auth-success");
+ ASSERT_EQ(negotiate_success_metric->GetValue(), num_negotiate_success);
+ IntCounter* negotiate_failure_metric = metrics->FindMetricForTesting<IntCounter>(
+ "impala.webserver.total-negotiate-auth-failure");
+ ASSERT_EQ(negotiate_failure_metric->GetValue(), num_negotiate_failure);
+ IntCounter* cookie_success_metric = metrics->FindMetricForTesting<IntCounter>(
+ "impala.webserver.total-cookie-auth-success");
+ ASSERT_EQ(cookie_success_metric->GetValue(), num_cookie_success);
+ IntCounter* cookie_failure_metric = metrics->FindMetricForTesting<IntCounter>(
+ "impala.webserver.total-cookie-auth-failure");
+ ASSERT_EQ(cookie_failure_metric->GetValue(), num_cookie_failure);
+}
+
TEST(Webserver, TestWithSpnego) {
MiniKdc kdc(MiniKdcOptions{});
KUDU_ASSERT_OK(kdc.Start());
@@ -340,22 +375,62 @@ TEST(Webserver, TestWithSpnego) {
gflags::FlagSaver saver;
FLAGS_webserver_require_spnego = true;
+ FLAGS_ldap_passwords_in_clear_ok = true;
- Webserver webserver("", FLAGS_webserver_port);
+ MetricGroup metrics("webserver-test");
+ Webserver webserver("", FLAGS_webserver_port, &metrics);
ASSERT_OK(webserver.Start());
// Don't expect HTTP requests to work without Kerberos credentials.
stringstream contents;
ASSERT_FALSE(HttpGet("localhost", FLAGS_webserver_port, "/", &contents).ok());
+ // There should be one failed auth attempt.
+ CheckAuthMetrics(&metrics, 0, 1, 0, 0);
// TODO(todd) IMPALA-8987: import curl into native-toolchain and test this with
// authentication.
- // Test that OPTIONS works with and without having kinit-ed.
- //string options_cmd = Substitute(
- // "curl -X OPTIONS -v --negotiate -u : 'http://127.0.0.1:$0'", FLAGS_webserver_port);
- //system(options_cmd.c_str());
- //KUDU_ASSERT_OK(kdc.Kinit("alice"));
- //system(options_cmd.c_str());
+ string curl_output;
+ if (RunShellProcess("curl --version", &curl_output)
+ && curl_output.find("GSS-API") != string::npos
+ && curl_output.find("SPNEGO") != string::npos) {
+ //if (system("curl --version") == 0) {
+ // Test that OPTIONS works with and without having kinit-ed.
+ string options_cmd =
+ Substitute("curl -X OPTIONS -v --negotiate -u : 'http://127.0.0.1:$0'",
+ FLAGS_webserver_port);
+ system(options_cmd.c_str());
+ KUDU_ASSERT_OK(kdc.Kinit("alice"));
+ system(options_cmd.c_str());
+
+ // Test that GET works with cookies.
+ filesystem::path cookie_dir = filesystem::unique_path();
+ filesystem::create_directories(cookie_dir);
+ filesystem::path cookie_path = cookie_dir / "cookiejar";
+ LOG(INFO) << "Storing cookies in " << cookie_path;
+ string curl_cmd =
+ Substitute("curl -c $0 -b $0 -X GET -v --negotiate -u : 'http://127.0.0.1:$1'",
+ cookie_path.string(), FLAGS_webserver_port);
+ // Run the command twice, the first time we should authenticate with SPNEGO, the
+ // second time with a cookie.
+ system(Substitute("$0 && $0", curl_cmd).c_str());
+ // There should be one more failed auth attempt, when curl first tries to connect
+ // without authentication, then one successful attempt, then a successful cookie auth.
+ CheckAuthMetrics(&metrics, 1, 2, 1, 0);
+
+ webserver.Stop();
+ MetricGroup metrics2("webserver-test");
+ Webserver webserver2("", FLAGS_webserver_port, &metrics2);
+ ASSERT_OK(webserver2.Start());
+ // Run the command again. We should get a failed cookie attempt because the new
+ // webserver uses a different HMAC key.
+ system(curl_cmd.c_str());
+ CheckAuthMetrics(&metrics2, 1, 1, 0, 1);
+
+ filesystem::remove_all(cookie_dir);
+ } else {
+ LOG(INFO) << "Skipping test, curl was not present or did not have the required "
+ << "features: " << curl_output;
+ }
}
TEST(Webserver, StartWithPasswordFileTest) {
@@ -364,7 +439,8 @@ TEST(Webserver, StartWithPasswordFileTest) {
auto password =
ScopedFlagSetter<string>::Make(&FLAGS_webserver_password_file, password_file.str());
- Webserver webserver("", FLAGS_webserver_port);
+ MetricGroup metrics("webserver-test");
+ Webserver webserver("", FLAGS_webserver_port, &metrics);
ASSERT_OK(webserver.Start());
// Don't expect HTTP requests to work without a password
@@ -378,12 +454,14 @@ TEST(Webserver, StartWithMissingPasswordFileTest) {
auto password =
ScopedFlagSetter<string>::Make(&FLAGS_webserver_password_file, password_file.str());
- Webserver webserver("", FLAGS_webserver_port);
+ MetricGroup metrics("webserver-test");
+ Webserver webserver("", FLAGS_webserver_port, &metrics);
ASSERT_FALSE(webserver.Start().ok());
}
TEST(Webserver, DirectoryListingDisabledTest) {
- Webserver webserver("", FLAGS_webserver_port);
+ MetricGroup metrics("webserver-test");
+ Webserver webserver("", FLAGS_webserver_port, &metrics);
ASSERT_OK(webserver.Start());
stringstream contents;
ASSERT_OK(HttpGet("localhost", FLAGS_webserver_port,
@@ -399,7 +477,8 @@ void FrameCallback(const Webserver::WebRequest& req, Document* document) {
TEST(Webserver, NoFrameEmbeddingTest) {
const string FRAME_TEST_PATH = "/frames_test";
- Webserver webserver("", FLAGS_webserver_port);
+ MetricGroup metrics("webserver-test");
+ Webserver webserver("", FLAGS_webserver_port, &metrics);
Webserver::UrlCallback callback = bind<void>(FrameCallback, _1, _2);
webserver.RegisterUrlCallback(FRAME_TEST_PATH, "raw_text.tmpl", callback, true);
ASSERT_OK(webserver.Start());
@@ -414,7 +493,8 @@ TEST(Webserver, FrameAllowEmbeddingTest) {
const string FRAME_TEST_PATH = "/frames_test";
auto x_frame_opt =
ScopedFlagSetter<string>::Make(&FLAGS_webserver_x_frame_options, "ALLOWALL");
- Webserver webserver("", FLAGS_webserver_port);
+ MetricGroup metrics("webserver-test");
+ Webserver webserver("", FLAGS_webserver_port, &metrics);
Webserver::UrlCallback callback = bind<void>(FrameCallback, _1, _2);
webserver.RegisterUrlCallback(FRAME_TEST_PATH, "raw_text.tmpl", callback, true);
ASSERT_OK(webserver.Start());
@@ -435,7 +515,8 @@ void NullCharCallback(const Webserver::WebRequest& req, stringstream* out,
TEST(Webserver, NullCharTest) {
const string NULL_CHAR_TEST_PATH = "/null-char-test";
- Webserver webserver("", FLAGS_webserver_port);
+ MetricGroup metrics("webserver-test");
+ Webserver webserver("", FLAGS_webserver_port, &metrics);
webserver.RegisterUrlCallback(NULL_CHAR_TEST_PATH, NullCharCallback);
ASSERT_OK(webserver.Start());
stringstream contents;
@@ -445,7 +526,8 @@ TEST(Webserver, NullCharTest) {
}
TEST(Webserver, Options) {
- Webserver webserver("", FLAGS_webserver_port);
+ MetricGroup metrics("webserver-test");
+ Webserver webserver("", FLAGS_webserver_port, &metrics);
ASSERT_OK(webserver.Start());
stringstream contents;
ASSERT_OK(HttpGet("localhost", FLAGS_webserver_port, "/", &contents, 200, "OPTIONS"));
diff --git a/be/src/util/webserver.cc b/be/src/util/webserver.cc
index 555760f..9952a30 100644
--- a/be/src/util/webserver.cc
+++ b/be/src/util/webserver.cc
@@ -40,6 +40,7 @@
#include "kudu/util/logging.h"
#include "kudu/util/net/sockaddr.h"
#include "kudu/security/gssapi.h"
+#include "rpc/cookie-util.h"
#include "rpc/thrift-util.h"
#include "runtime/exec-env.h"
#include "service/impala-server.h"
@@ -50,6 +51,7 @@
#include "util/debug-util.h"
#include "util/disk-info.h"
#include "util/mem-info.h"
+#include "util/metrics.h"
#include "util/os-info.h"
#include "util/os-util.h"
#include "util/pretty-printer.h"
@@ -116,6 +118,7 @@ DEFINE_bool(webserver_require_spnego, false,
"using SPNEGO.");
DECLARE_bool(is_coordinator);
+DECLARE_int64(max_cookie_lifetime_s);
DECLARE_string(ssl_minimum_version);
DECLARE_string(ssl_cipher_list);
@@ -174,18 +177,20 @@ string HttpStatusCodeToString(HttpStatusCode code) {
return "";
}
-
-void SendPlainResponse(struct sq_connection* connection,
- const string& response_code_line,
- const string& content,
- const vector<string>& header_lines) {
+void SendResponse(struct sq_connection* connection, const string& response_code_line,
+ const string& context_type, const string& content,
+ const vector<string>& header_lines) {
sq_printf(connection, "HTTP/1.1 %s\r\n", response_code_line.c_str());
for (const auto& h : header_lines) {
sq_printf(connection, "%s\r\n", h.c_str());
}
- sq_printf(connection, "Content-Type: text/plain\r\n");
- sq_printf(connection, "Content-Length: %zd\r\n\r\n", content.size());
- sq_printf(connection, "%s", content.c_str());
+ sq_printf(connection,
+ "X-Frame-Options: %s\r\n"
+ "Content-Type: %s\r\n"
+ "Content-Length: %zd\r\n\r\n",
+ FLAGS_webserver_x_frame_options.c_str(), context_type.c_str(), content.size());
+ // Make sure to use sq_write for printing the body; sq_printf truncates at 8kb
+ sq_write(connection, content.c_str(), content.length());
}
// Return the address of the remote user from the squeasel request info.
@@ -202,15 +207,15 @@ kudu::Sockaddr GetRemoteAddress(const struct sq_request_info* req) {
// 'authz_header' and running it through GSSAPI. If authentication fails or the header
// is invalid, a bad Status will be returned (and the other out-parameters left
// untouched).
-kudu::Status RunSpnegoStep(const char* authz_header, string* resp_header,
- string* authn_user) {
+kudu::Status RunSpnegoStep(
+ const char* authz_header, vector<string>* response_headers, string* authn_user) {
string neg_token;
if (authz_header && !TryStripPrefixString(authz_header, "Negotiate ", &neg_token)) {
return kudu::Status::InvalidArgument("bad Negotiate header");
}
if (!authz_header) {
- *resp_header = "WWW-Authenticate: Negotiate";
+ response_headers->push_back("WWW-Authenticate: Negotiate");
return kudu::Status::Incomplete("authn incomplete");
}
@@ -220,33 +225,37 @@ kudu::Status RunSpnegoStep(const char* authz_header, string* resp_header,
neg_token, &resp_token_b64, &is_complete, authn_user));
if (!resp_token_b64.empty()) {
- *resp_header = Substitute("WWW-Authenticate: Negotiate $0", resp_token_b64);
+ response_headers->push_back(
+ Substitute("WWW-Authenticate: Negotiate $0", resp_token_b64));
}
return is_complete ? kudu::Status::OK() : kudu::Status::Incomplete("authn incomplete");
}
} // anonymous namespace
-// Builds a valid HTTP header given the response code and a content type.
-string BuildHeaderString(HttpStatusCode response, ContentType content_type) {
- static const string RESPONSE_TEMPLATE = "HTTP/1.1 $0\r\n"
- "Content-Type: $1\r\n"
- "Content-Length: %d\r\n"
- "X-Frame-Options: $2\r\n"
- "\r\n";
-
- return Substitute(RESPONSE_TEMPLATE, HttpStatusCodeToString(response),
- Webserver::GetMimeType(content_type), FLAGS_webserver_x_frame_options.c_str());
-}
-
-Webserver::Webserver() : Webserver(FLAGS_webserver_interface, FLAGS_webserver_port) {}
+Webserver::Webserver(MetricGroup* metrics)
+ : Webserver(FLAGS_webserver_interface, FLAGS_webserver_port, metrics) {}
-Webserver::Webserver(const string& interface, const int port)
- : context_(nullptr),
- error_handler_(UrlHandler(bind<void>(&Webserver::ErrorHandler, this, _1, _2),
- "error.tmpl", false)) {
+Webserver::Webserver(const string& interface, const int port, MetricGroup* metrics)
+ : context_(nullptr),
+ error_handler_(UrlHandler(
+ bind<void>(&Webserver::ErrorHandler, this, _1, _2), "error.tmpl", false)),
+ use_cookies_(FLAGS_max_cookie_lifetime_s > 0) {
http_address_ = MakeNetworkAddress(interface.empty() ? "0.0.0.0" : interface, port);
Init();
+
+ if (FLAGS_webserver_require_spnego) {
+ total_negotiate_auth_success_ =
+ metrics->AddCounter("impala.webserver.total-negotiate-auth-success", 0);
+ total_negotiate_auth_failure_ =
+ metrics->AddCounter("impala.webserver.total-negotiate-auth-failure", 0);
+ if (use_cookies_) {
+ total_cookie_auth_success_ =
+ metrics->AddCounter("impala.webserver.total-cookie-auth-success", 0);
+ total_cookie_auth_failure_ =
+ metrics->AddCounter("impala.webserver.total-cookie-auth-failure", 0);
+ }
+ }
}
Webserver::~Webserver() {
@@ -501,10 +510,51 @@ sq_callback_result_t Webserver::BeginRequestCallback(struct sq_connection* conne
return SQ_CONTINUE_HANDLING;
}
+ vector<string> response_headers;
if (FLAGS_webserver_require_spnego){
- sq_callback_result_t spnego_result = HandleSpnego(connection, request_info);
- if (spnego_result != SQ_CONTINUE_HANDLING) {
- return spnego_result;
+ bool authenticated = false;
+ // Try authenticating with a cookie first, if enabled.
+ if (use_cookies_) {
+ const char* cookie_header = sq_get_header(connection, "Cookie");
+ string username;
+ if (cookie_header != nullptr) {
+ Status cookie_status = AuthenticateCookie(hash_, cookie_header, &username);
+ if (cookie_status.ok()) {
+ authenticated = true;
+ request_info->remote_user = strdup(username.c_str());
+ total_cookie_auth_success_->Increment(1);
+ } else {
+ LOG(INFO) << "Invalid cookie provided: " << cookie_header << ": "
+ << cookie_status.GetDetail();
+ response_headers.push_back(Substitute("Set-Cookie: $0", GetDeleteCookie()));
+ total_cookie_auth_failure_->Increment(1);
+ }
+ }
+ }
+
+ if (!authenticated) {
+ sq_callback_result_t spnego_result =
+ HandleSpnego(connection, request_info, &response_headers);
+ if (spnego_result == SQ_CONTINUE_HANDLING) {
+ // Spnego negotiation was successful.
+ if (use_cookies_) {
+ // If cookie auth failed above and we generated a 'delete cookie' header,
+ // remove it.
+ auto eq = [](const string& header) {
+ return header.rfind("Set-Cookie", 0) == 0;
+ };
+ auto it = find_if(response_headers.begin(), response_headers.end(), eq);
+ if (it != response_headers.end()) {
+ response_headers.erase(it);
+ }
+ // Generate a cookie to return.
+ response_headers.push_back(Substitute(
+ "Set-Cookie: $0", GenerateCookie(request_info->remote_user, hash_)));
+ }
+ } else {
+ // Spnego negotiation is incomplete or failed, stop processing the request.
+ return spnego_result;
+ }
}
}
@@ -594,32 +644,21 @@ sq_callback_result_t Webserver::BeginRequestCallback(struct sq_connection* conne
VLOG(3) << "Rendering page " << request_info->uri << " took "
<< PrettyPrinter::Print(sw.ElapsedTime(), TUnit::TIME_NS);
- const string& str = output.str();
-
- const string& headers = BuildHeaderString(response, content_type);
+ SendResponse(connection, HttpStatusCodeToString(response),
+ Webserver::GetMimeType(content_type), output.str(), response_headers);
- // printf with a non-literal format string is a security concern, but BuildHeaderString
- // returns a limited set of strings and all members of that set are safe.
-#pragma clang diagnostic push
-#pragma clang diagnostic ignored "-Wformat-nonliteral"
- sq_printf(connection, headers.c_str(), (int)str.length());
-#pragma clang diagnostic pop
-
- // Make sure to use sq_write for printing the body; sq_printf truncates at 8kb
- sq_write(connection, str.c_str(), str.length());
return SQ_HANDLED_OK;
}
-sq_callback_result_t Webserver::HandleSpnego(
- struct sq_connection* connection,
- struct sq_request_info* request_info) {
+sq_callback_result_t Webserver::HandleSpnego(struct sq_connection* connection,
+ struct sq_request_info* request_info, vector<string>* response_headers) {
const char* authz_header = sq_get_header(connection, "Authorization");
- string resp_header, authn_princ;
- kudu::Status s = RunSpnegoStep(authz_header, &resp_header, &authn_princ);
+ string authn_princ;
+ kudu::Status s = RunSpnegoStep(authz_header, response_headers, &authn_princ);
if (s.IsIncomplete()) {
- SendPlainResponse(connection, "401 Authentication Required",
- "Must authenticate with SPNEGO.",
- { resp_header });
+ SendResponse(connection, "401 Authentication Required", "text/plain",
+ "Must authenticate with SPNEGO.", *response_headers);
+ total_negotiate_auth_failure_->Increment(1);
return SQ_HANDLED_OK;
}
if (s.ok() && authn_princ.empty()) {
@@ -638,57 +677,14 @@ sq_callback_result_t Webserver::HandleSpnego(
const char* http_status = s.IsNotAuthorized() ? "401 Authentication Required" :
"500 Internal Server Error";
- SendPlainResponse(connection, http_status, s.ToString(), {});
+ SendResponse(connection, http_status, "text/plain", s.ToString(), *response_headers);
+ total_negotiate_auth_failure_->Increment(1);
return SQ_HANDLED_OK;
}
-
request_info->remote_user = strdup(authn_princ.c_str());
- // NOTE: According to the SPNEGO RFC (https://tools.ietf.org/html/rfc4559) it
- // is possible that a non-empty token will be returned along with the HTTP 200
- // response:
- //
- // A status code 200 status response can also carry a "WWW-Authenticate"
- // response header containing the final leg of an authentication. In
- // this case, the gssapi-data will be present. Before using the
- // contents of the response, the gssapi-data should be processed by
- // gss_init_security_context to determine the state of the security
- // context. If this function indicates success, the response can be
- // used by the application. Otherwise, an appropriate action, based on
- // the authentication status, should be taken.
- //
- // For example, the authentication could have failed on the final leg if
- // mutual authentication was requested and the server was not able to
- // prove its identity. In this case, the returned results are suspect.
- // It is not always possible to mutually authenticate the server before
- // the HTTP operation. POST methods are in this category.
- //
- // In fact, from inspecting the MIT krb5 source code, it appears that this
- // only happens when the client requests mutual authentication by passing
- // 'GSS_C_MUTUAL_FLAG' when establishing its side of the protocol. In practice,
- // this seems to be widely unimplemented:
- //
- // - curl has some source code to support GSS_C_MUTUAL_FLAG, but in order to
- // enable it, you have to modify a FALSE constant to TRUE and recompile curl.
- // In fact, it was broken for all of 2015 without anyone noticing (see curl
- // commit 73f1096335d468b5be7c3cc99045479c3314f433)
- //
- // - Chrome doesn't support mutual auth at all -- see DelegationTypeToFlag(...)
- // in src/net/http/http_auth_gssapi_posix.cc.
- //
- // In practice, users depend on TLS to authenticate the server, and SPNEGO
- // is used to authenticate the client.
- //
- // Given this, and because actually sending back the token on an OK response
- // would require significant code restructuring (eg buffering the header until
- // after the response handler has run) we just ignore any response token, but
- // log a periodic warning just in case it turns out we're wrong about the above.
- if (!resp_header.empty()) {
- KLOG_EVERY_N_SECS(WARNING, 5) << "ignoring SPNEGO token on HTTP 200 response "
- << "for user " << authn_princ << " at host "
- << GetRemoteAddress(request_info).ToString();
- }
+ total_negotiate_auth_success_->Increment(1);
return SQ_CONTINUE_HANDLING;
}
diff --git a/be/src/util/webserver.h b/be/src/util/webserver.h
index 84dda9e..7d619a4 100644
--- a/be/src/util/webserver.h
+++ b/be/src/util/webserver.h
@@ -26,7 +26,9 @@
#include "common/status.h"
#include "kudu/util/web_callback_registry.h"
#include "thirdparty/squeasel/squeasel.h"
+#include "util/metrics-fwd.h"
#include "util/network-util.h"
+#include "util/openssl-util.h"
namespace impala {
@@ -82,10 +84,10 @@ class Webserver {
/// Using this constructor, the webserver will bind to 'interface', or all available
/// interfaces if not specified.
- Webserver(const std::string& interface, const int port);
+ Webserver(const std::string& interface, const int port, MetricGroup* metrics);
/// Uses FLAGS_webserver_{port, interface}
- Webserver();
+ Webserver(MetricGroup* metrics);
~Webserver();
@@ -184,9 +186,8 @@ class Webserver {
// Handle SPNEGO authentication for this request. Returns SQ_CONTINUE_HANDLING
// if authentication was successful, otherwise responds to the request and
// returns SQ_HANDLED_OK.
- sq_callback_result_t HandleSpnego(
- struct sq_connection* connection,
- struct sq_request_info* request_info);
+ sq_callback_result_t HandleSpnego(struct sq_connection* connection,
+ struct sq_request_info* request_info, std::vector<std::string>* response_headers);
/// Renders URLs through the Mustache templating library.
/// - Default ContentType is HTML.
@@ -233,6 +234,22 @@ class Webserver {
/// Catch-all handler for error messages
UrlHandler error_handler_;
+
+ /// Used to generate and verify signatures for cookies.
+ AuthenticationHash hash_;
+
+ /// If true and SPNEGO is in use, cookies will be used for authentication.
+ bool use_cookies_;
+
+ /// If 'FLAGS_webserver_require_spnego' is true, metrics for the number of successful
+ /// and failed Negotiate auth attempts.
+ IntCounter* total_negotiate_auth_success_ = nullptr;
+ IntCounter* total_negotiate_auth_failure_ = nullptr;
+
+ /// If 'use_cookies_' is true, metrics for the number of successful and failed cookie
+ /// auth attempts.
+ IntCounter* total_cookie_auth_success_ = nullptr;
+ IntCounter* total_cookie_auth_failure_ = nullptr;
};
}
diff --git a/common/thrift/metrics.json b/common/thrift/metrics.json
index be317c9..6f75bcf 100644
--- a/common/thrift/metrics.json
+++ b/common/thrift/metrics.json
@@ -2530,5 +2530,53 @@
"units": "UNIT",
"kind": "GAUGE",
"key": "admission-controller.executor-group.num-queries-executing.$0"
+ },
+ {
+ "description": "The number of HTTP connection requests to this daemon's webserver that were successfully authenticated with Kerberos",
+ "contexts": [
+ "IMPALAD",
+ "CATALOGSERVER",
+ "STATESTORE"
+ ],
+ "label": "Webserver HTTP Connection Kerberos Auth Success",
+ "units": "NONE",
+ "kind": "COUNTER",
+ "key": "impala.webserver.total-negotiate-auth-success"
+ },
+ {
+ "description": "The number of HTTP connection requests to this daemon's webserver that attempted to authenticate with Kerberos but were unsuccessful.",
+ "contexts": [
+ "IMPALAD",
+ "CATALOGSERVER",
+ "STATESTORE"
+ ],
+ "label": "Webserver HTTP Connection Kerberos Auth Failure",
+ "units": "NONE",
+ "kind": "COUNTER",
+ "key": "impala.webserver.total-negotiate-auth-failure"
+ },
+ {
+ "description": "The number of HTTP connection requests to this daemon's webserver that were successfully authenticated using a cookie.",
+ "contexts": [
+ "IMPALAD",
+ "CATALOGSERVER",
+ "STATESTORE"
+ ],
+ "label": "Webserver HTTP Connection Cookie Auth Success",
+ "units": "NONE",
+ "kind": "COUNTER",
+ "key": "impala.webserver.total-cookie-auth-success"
+ },
+ {
+ "description": "The number of HTTP connection requests to this daemon's webserver that provided an invalid cookie.",
+ "contexts": [
+ "IMPALAD",
+ "CATALOGSERVER",
+ "STATESTORE"
+ ],
+ "label": "Webserver HTTP Connection Cookie Auth Failure",
+ "units": "NONE",
+ "kind": "COUNTER",
+ "key": "impala.webserver.total-cookie-auth-failure"
}
]
diff --git a/fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java b/fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java
index f2d1ea0..c362de6 100644
--- a/fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java
+++ b/fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java
@@ -223,6 +223,7 @@ public class LdapHS2Test {
"invalid-format", // invalid cookie format
"x&impala&0&0", // signature value that is invalid base64
"eA==&impala&0&0", // signature decodes to an incorrect length
+ "\"eA==&impala&0&0\"", // signature decodes to an incorrect length
"eHh4eHh4eHh4eHh4eHh4eHh4eHh4eHh4eHh4eHh4eHg=&impala&0&0" // incorrect signature
};
for (String cookieStr : badCookies) {
[impala] 01/02: IMPALA-9017: Alter table events on dropped table/db
should be ignored.
Posted by tm...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
tmarshall pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git
commit 4f8a1b9b088e44821c59710c353a13c94f85d305
Author: Anurag Mantripragada <an...@cloudera.com>
AuthorDate: Mon Oct 7 20:58:49 2019 -0700
IMPALA-9017: Alter table events on dropped table/db should be
ignored.
Alter table events received on a table/db that does not exist in
catalog can be safely ignored. This is because a table not present
in the catalog means either:
1. The table was created in HMS but not processed by the events
processor beacuase it is already in an error state or,
2. It was dropped/renamed by the same catalog.
Prior to this change, alter table events generated by renames would
try to add the new table names to the cache irrespective of weather
old table was in catalog cache. After this change a drop + create
sequence happens only if table before rename exists in catalog.
Testing:
Added tests for rename + drop table/db sequences to verify events
processor does not error out.
Change-Id: I534accd704f76772ebe47865eff0fcdc7a033a48
Reviewed-on: http://gerrit.cloudera.org:8080/14385
Reviewed-by: Vihang Karajgaonkar <vi...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
.../impala/catalog/CatalogServiceCatalog.java | 34 ++++++++++------------
.../impala/catalog/events/MetastoreEvents.java | 18 ++++--------
.../events/MetastoreEventsProcessorTest.java | 25 ++++++++++++++++
3 files changed, 47 insertions(+), 30 deletions(-)
diff --git a/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java b/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
index 6e40dd0..7798c3e 100644
--- a/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
+++ b/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
@@ -1957,30 +1957,28 @@ public class CatalogServiceCatalog extends Catalog {
}
/**
- * Renames the table by atomically removing oldTable and adding the newTable. If the
- * oldTable is not found this operation becomes a add new table if not exists
- * operation.
- *
- * @return a pair of booleans. The first of the pair is set if the oldTableName was
- * found and removed. The second boolean is set if the new table didn't exist before
- * and hence was added.
+ * Renames the table by atomically removing oldTable and adding the newTable.
+ * @return true if oldTable was removed and newTable was added, false if oldTable or
+ * it's db are not in catalog.
*/
- public Pair<Boolean, Boolean> renameOrAddTableIfNotExists(TTableName oldTableName,
- TTableName newTableName)
- throws CatalogException {
- boolean oldTableRemoved = false;
- boolean newTableAdded;
+ public boolean renameTableIfExists(TTableName oldTableName,
+ TTableName newTableName) {
+ boolean tableRenamed = false;
versionLock_.writeLock().lock();
try {
Db db = getDb(oldTableName.db_name);
if (db != null) {
- // remove the oldTable if it exists
- oldTableRemoved =
- removeTable(oldTableName.db_name, oldTableName.table_name) != null;
+ Table existingTable = removeTable(oldTableName.db_name, oldTableName.table_name);
+ // Add the newTable only if oldTable existed.
+ if (existingTable != null) {
+ Table incompleteTable = IncompleteTable.createUninitializedTable(db,
+ newTableName.getTable_name());
+ incompleteTable.setCatalogVersion(incrementAndGetCatalogVersion());
+ db.addTable(incompleteTable);
+ tableRenamed = true;
+ }
}
- // add the new tbl if it doesn't exist
- newTableAdded = addTableIfNotExists(newTableName.db_name, newTableName.table_name);
- return new Pair<>(oldTableRemoved, newTableAdded);
+ return tableRenamed;
} finally {
versionLock_.writeLock().unlock();
}
diff --git a/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java b/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
index bd892a5..9909e9d 100644
--- a/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
+++ b/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
@@ -983,21 +983,15 @@ public class MetastoreEvents {
TTableName newTTableName =
new TTableName(tableAfter_.getDbName(), tableAfter_.getTableName());
- // atomically rename the old table to new table
- Pair<Boolean, Boolean> result = null;
- result = catalog_.renameOrAddTableIfNotExists(oldTTableName, newTTableName);
-
- // old table was not found. This could be because catalogD is stale and didn't
- // have any entry for the oldTable
- if (!result.first) {
+ // Table is renamed only if old table existed in the catalog. Otherwise the event
+ // is skipped.
+ if (!catalog_.renameTableIfExists(oldTTableName, newTTableName)) {
debugLog("Did not remove old table to rename table {} to {} since "
+ "it does not exist anymore", qualify(oldTTableName),
qualify(newTTableName));
- }
- // the new table from the event was not added since it was already present
- if (!result.second) {
- debugLog("Did not add new table name while renaming table {} to {}",
- qualify(oldTTableName), qualify(newTTableName));
+ } else {
+ infoLog("Renamed old table {} to new table {}.", qualify(oldTTableName),
+ qualify(newTTableName));
}
}
diff --git a/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java b/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
index 7056b7d..e25475d 100644
--- a/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
+++ b/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
@@ -914,6 +914,31 @@ public class MetastoreEventsProcessorTest {
.getCounter(MetastoreEventsProcessor.NUMBER_OF_TABLE_INVALIDATES).getCount();
assertEquals("Unexpected number of table invalidates after trivial alter",
numberOfInvalidatesBefore + 4, numberOfInvalidatesAfterTrivialAlter);
+
+ // Simulate rename and drop sequence for table/db.
+ String tblName = "alter_drop_test";
+ createTable(tblName, false);
+ eventsProcessor_.processEvents();
+ alterTableRename(tblName, "new_table_1");
+ dropTableFromImpala(TEST_DB_NAME, tblName);
+ eventsProcessor_.processEvents();
+ // Alter event generated by a rename on a dropped table should be skipped.
+ assertEquals(EventProcessorStatus.ACTIVE, eventsProcessor_.getStatus());
+
+ // Rename from Impala and drop table.
+ createTable(tblName, false);
+ eventsProcessor_.processEvents();
+ alterTableRenameFromImpala(TEST_DB_NAME, tblName, "new_table_2");
+ dropTableFromImpala(TEST_DB_NAME, tblName);
+ eventsProcessor_.processEvents();
+
+ createTable(tblName, false);
+ eventsProcessor_.processEvents();
+ alterTableRename(tblName, "new_tbl_3");
+ dropDatabaseCascadeFromImpala(TEST_DB_NAME);
+ eventsProcessor_.processEvents();
+ // Alter event generated by a rename on a dropped database should be skipped.
+ assertEquals(EventProcessorStatus.ACTIVE, eventsProcessor_.getStatus());
}
/**