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/06/14 23:07:20 UTC
[impala] 02/02: IMPALA-8583: Add metrics for Basic auth
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 3bd53972cec3b8a863075cb2344115a950be848a
Author: Thomas Tauber-Marshall <tm...@cloudera.com>
AuthorDate: Thu Jun 13 11:37:59 2019 -0700
IMPALA-8583: Add metrics for Basic auth
The patch adds two metrics:
impala.thrift-server.hiveserver2-http-frontend.total-basic-auth-success
impala.thrift-server.hiveserver2-http-frontend.total-basic-auth-failure
which track the number of successful and failed attempts at using
Basic auth to authentication to the hiveserver2 http server.
A followup patch will add similar metrics for SASL attempts on the
beeswax and hiveserver2 binary servers.
Testing:
- Modified existing HTTP auth test to check that the produced metrics
are correct.
Change-Id: I6d1b1bcc093e4b00802bc108ff4d8d4e2cdfd88f
Reviewed-on: http://gerrit.cloudera.org:8080/13640
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
be/src/rpc/auth-provider.h | 3 ++
be/src/rpc/authentication.cc | 11 ++++----
be/src/rpc/thrift-server.cc | 13 +++++----
be/src/rpc/thrift-server.h | 3 ++
be/src/transport/THttpServer.cpp | 25 +++++++++++++++--
be/src/transport/THttpServer.h | 32 ++++++++++++++++------
common/thrift/metrics.json | 20 ++++++++++++++
.../apache/impala/customcluster/LdapHS2Test.java | 26 ++++++++++++++++++
8 files changed, 111 insertions(+), 22 deletions(-)
diff --git a/be/src/rpc/auth-provider.h b/be/src/rpc/auth-provider.h
index d575aef..c7a6fd5 100644
--- a/be/src/rpc/auth-provider.h
+++ b/be/src/rpc/auth-provider.h
@@ -46,6 +46,7 @@ class AuthProvider {
/// around another transport type, eg. a TSaslServerTransport.
virtual Status GetServerTransportFactory(
ThriftServer::TransportType underlying_transport_type,
+ const std::string& server_name, MetricGroup* metrics,
boost::shared_ptr<apache::thrift::transport::TTransportFactory>* factory)
WARN_UNUSED_RESULT = 0;
@@ -100,6 +101,7 @@ class SecureAuthProvider : public AuthProvider {
/// This is only applicable to Thrift connections and not KRPC connections.
virtual Status GetServerTransportFactory(
ThriftServer::TransportType underlying_transport_type,
+ const std::string& server_name, MetricGroup* metrics,
boost::shared_ptr<apache::thrift::transport::TTransportFactory>* factory);
/// IF sasl was used, the username will be available from the handshake, and we set it
@@ -176,6 +178,7 @@ class NoAuthProvider : public AuthProvider {
virtual Status GetServerTransportFactory(
ThriftServer::TransportType underlying_transport_type,
+ const std::string& server_name, MetricGroup* metrics,
boost::shared_ptr<apache::thrift::transport::TTransportFactory>* factory);
virtual Status WrapClientTransport(const std::string& hostname,
diff --git a/be/src/rpc/authentication.cc b/be/src/rpc/authentication.cc
index addedd9..cac10ba 100644
--- a/be/src/rpc/authentication.cc
+++ b/be/src/rpc/authentication.cc
@@ -864,8 +864,8 @@ Status SecureAuthProvider::Start() {
}
Status SecureAuthProvider::GetServerTransportFactory(
- ThriftServer::TransportType underlying_transport_type,
- boost::shared_ptr<TTransportFactory>* factory) {
+ ThriftServer::TransportType underlying_transport_type, const std::string& server_name,
+ MetricGroup* metrics, boost::shared_ptr<TTransportFactory>* factory) {
DCHECK(!principal_.empty() || has_ldap_);
if (underlying_transport_type == ThriftServer::HTTP) {
@@ -878,7 +878,8 @@ Status SecureAuthProvider::GetServerTransportFactory(
factory->reset(new ThriftServer::BufferedTransportFactory(
ThriftServer::BufferedTransportFactory::DEFAULT_BUFFER_SIZE_BYTES,
- new THttpServerTransportFactory(/* requireBasicAuth */ true)));
+ new THttpServerTransportFactory(
+ server_name, metrics, /* requireBasicAuth */ true)));
return Status::OK();
}
@@ -972,8 +973,8 @@ void SecureAuthProvider::SetupConnectionContext(
}
Status NoAuthProvider::GetServerTransportFactory(
- ThriftServer::TransportType underlying_transport_type,
- boost::shared_ptr<TTransportFactory>* factory) {
+ ThriftServer::TransportType underlying_transport_type, const std::string& server_name,
+ MetricGroup* metrics, boost::shared_ptr<TTransportFactory>* factory) {
// No Sasl - yawn. Here, have a regular old buffered transport.
switch (underlying_transport_type) {
case ThriftServer::BINARY:
diff --git a/be/src/rpc/thrift-server.cc b/be/src/rpc/thrift-server.cc
index 84c17cc..a859c9a 100644
--- a/be/src/rpc/thrift-server.cc
+++ b/be/src/rpc/thrift-server.cc
@@ -338,6 +338,7 @@ ThriftServer::ThriftServer(const string& name,
max_concurrent_connections_(max_concurrent_connections),
queue_timeout_ms_(queue_timeout_ms),
name_(name),
+ metrics_name_(Substitute("impala.thrift-server.$0", name_)),
server_(NULL),
processor_(processor),
connection_handler_(NULL),
@@ -350,10 +351,10 @@ ThriftServer::ThriftServer(const string& name,
if (metrics != NULL) {
metrics_enabled_ = true;
stringstream count_ss;
- count_ss << "impala.thrift-server." << name << ".connections-in-use";
+ count_ss << metrics_name_ << ".connections-in-use";
num_current_connections_metric_ = metrics->AddGauge(count_ss.str(), 0);
stringstream max_ss;
- max_ss << "impala.thrift-server." << name << ".total-connections";
+ max_ss << metrics_name_ << ".total-connections";
total_connections_metric_ = metrics->AddCounter(max_ss.str(), 0);
metrics_ = metrics;
} else {
@@ -491,15 +492,15 @@ Status ThriftServer::Start() {
boost::shared_ptr<TServerSocket> server_socket;
boost::shared_ptr<TTransportFactory> transport_factory;
RETURN_IF_ERROR(CreateSocket(&server_socket));
- RETURN_IF_ERROR(
- auth_provider_->GetServerTransportFactory(transport_type_, &transport_factory));
+ RETURN_IF_ERROR(auth_provider_->GetServerTransportFactory(
+ transport_type_, metrics_name_, metrics_, &transport_factory));
server_.reset(new TAcceptQueueServer(processor_, server_socket, transport_factory,
protocol_factory, thread_factory, name_, max_concurrent_connections_,
queue_timeout_ms_));
if (metrics_ != NULL) {
- (static_cast<TAcceptQueueServer*>(server_.get()))->InitMetrics(metrics_,
- Substitute("impala.thrift-server.$0", name_));
+ (static_cast<TAcceptQueueServer*>(server_.get()))
+ ->InitMetrics(metrics_, metrics_name_);
}
boost::shared_ptr<ThriftServer::ThriftServerEventProcessor> event_processor(
new ThriftServer::ThriftServerEventProcessor(this));
diff --git a/be/src/rpc/thrift-server.h b/be/src/rpc/thrift-server.h
index b716593..f208a10 100644
--- a/be/src/rpc/thrift-server.h
+++ b/be/src/rpc/thrift-server.h
@@ -221,6 +221,9 @@ class ThriftServer {
/// User-specified identifier that shows up in logs
const std::string name_;
+ /// Identifier used to prefix all metric names that are produced by this server.
+ const std::string metrics_name_;
+
/// Thread that runs ThriftServerEventProcessor::Supervise() in a separate loop
std::unique_ptr<Thread> server_thread_;
diff --git a/be/src/transport/THttpServer.cpp b/be/src/transport/THttpServer.cpp
index 6e77b6c..29b60a9 100644
--- a/be/src/transport/THttpServer.cpp
+++ b/be/src/transport/THttpServer.cpp
@@ -22,6 +22,7 @@
#include <iostream>
#include <gutil/strings/strip.h>
+#include <gutil/strings/substitute.h>
#include <gutil/strings/util.h>
#include "transport/THttpServer.h"
@@ -30,16 +31,34 @@
#include <Shlwapi.h>
#endif
+#include "util/metrics.h"
+
namespace apache {
namespace thrift {
namespace transport {
using namespace std;
+using strings::Substitute;
-THttpServer::THttpServer(boost::shared_ptr<TTransport> transport, bool requireBasicAuth)
- : THttpTransport(transport), requireBasicAuth_(requireBasicAuth) {
+THttpServerTransportFactory::THttpServerTransportFactory(
+ const std::string server_name, impala::MetricGroup* metrics, bool requireBasicAuth)
+ : requireBasicAuth_(requireBasicAuth) {
+ if (requireBasicAuth_) {
+ total_basic_auth_success_ =
+ metrics->AddCounter(Substitute("$0.total-basic-auth-success", server_name), 0);
+ total_basic_auth_failure_ =
+ metrics->AddCounter(Substitute("$0.total-basic-auth-failure", server_name), 0);
+ }
}
+THttpServer::THttpServer(boost::shared_ptr<TTransport> transport, bool requireBasicAuth,
+ impala::IntCounter* total_basic_auth_success,
+ impala::IntCounter* total_basic_auth_failure)
+ : THttpTransport(transport),
+ requireBasicAuth_(requireBasicAuth),
+ total_basic_auth_success_(total_basic_auth_success),
+ total_basic_auth_failure_(total_basic_auth_failure) {}
+
THttpServer::~THttpServer() {
}
@@ -130,10 +149,12 @@ void THttpServer::headersDone() {
if (TryStripPrefixString(authValue_, "Basic ", &base64)) {
if (authFn_(base64.c_str())) {
authorized = true;
+ total_basic_auth_success_->Increment(1);
}
}
}
if (!authorized) {
+ total_basic_auth_failure_->Increment(1);
returnUnauthorized();
throw TTransportException("HTTP Basic auth failed.");
}
diff --git a/be/src/transport/THttpServer.h b/be/src/transport/THttpServer.h
index 85413c5..bd3366e 100644
--- a/be/src/transport/THttpServer.h
+++ b/be/src/transport/THttpServer.h
@@ -21,6 +21,7 @@
#define IMPALA_TRANSPORT_THTTPSERVER_H
#include "transport/THttpTransport.h"
+#include "util/metrics-fwd.h"
namespace apache {
namespace thrift {
@@ -36,7 +37,9 @@ public:
// returns true if authentication is successful.
typedef std::function<bool(const char*)> BasicAuthFn;
- THttpServer(boost::shared_ptr<TTransport> transport, bool requireBasicAuth = false);
+ THttpServer(boost::shared_ptr<TTransport> transport, bool requireBasicAuth,
+ impala::IntCounter* total_basic_auth_success,
+ impala::IntCounter* total_basic_auth_failure);
virtual ~THttpServer();
@@ -65,6 +68,10 @@ protected:
// The value from the 'Authorization' header.
std::string authValue_ = "";
+
+ // Metrics
+ impala::IntCounter* total_basic_auth_success_;
+ impala::IntCounter* total_basic_auth_failure_;
};
/**
@@ -72,21 +79,28 @@ protected:
*/
class THttpServerTransportFactory : public TTransportFactory {
public:
+ THttpServerTransportFactory() : requireBasicAuth_(false) {}
- explicit THttpServerTransportFactory(bool requireBasicAuth = false)
- : requireBasicAuth_(requireBasicAuth) {}
+ THttpServerTransportFactory(
+ const std::string server_name, impala::MetricGroup* metrics, bool requireBasicAuth);
- virtual ~THttpServerTransportFactory() {}
+ virtual ~THttpServerTransportFactory() {}
- /**
- * Wraps the transport into a buffered one.
- */
- virtual boost::shared_ptr<TTransport> getTransport(boost::shared_ptr<TTransport> trans) {
- return boost::shared_ptr<TTransport>(new THttpServer(trans, requireBasicAuth_));
+ /**
+ * Wraps the transport into a buffered one.
+ */
+ virtual boost::shared_ptr<TTransport> getTransport(boost::shared_ptr<TTransport> trans) {
+ return boost::shared_ptr<TTransport>(new THttpServer(
+ trans, requireBasicAuth_, total_basic_auth_success_, total_basic_auth_failure_));
}
private:
bool requireBasicAuth_ = false;
+
+ // If 'requireBasicAuth_' is true, metrics for the number of successful and failed Basic
+ // auth ettempts for every transport produced by this factory.
+ impala::IntCounter* total_basic_auth_success_;
+ impala::IntCounter* total_basic_auth_failure_;
};
}
}
diff --git a/common/thrift/metrics.json b/common/thrift/metrics.json
index c75175a..c688fc1 100644
--- a/common/thrift/metrics.json
+++ b/common/thrift/metrics.json
@@ -942,6 +942,26 @@
"key": "impala.thrift-server.hiveserver2-http-frontend.timedout-cnxn-requests"
},
{
+ "description": "The number of HiveServer2 HTTP API connection requests to this Impala Daemon that were successfully authenticated with Basic (username/password) auth.",
+ "contexts": [
+ "IMPALAD"
+ ],
+ "label": "HiveServer2 HTTP API Connection Basic Auth Success",
+ "units": "NONE",
+ "kind": "COUNTER",
+ "key": "impala.thrift-server.hiveserver2-http-frontend.total-basic-auth-success"
+ },
+ {
+ "description": "The number of HiveServer2 HTTP API connection requests to this Impala Daemon that failed to authenticate with Basic auth due to an invalid username or password.",
+ "contexts": [
+ "IMPALAD"
+ ],
+ "label": "HiveServer2 HTTP API Connection Basic Auth Failure",
+ "units": "NONE",
+ "kind": "COUNTER",
+ "key": "impala.thrift-server.hiveserver2-http-frontend.total-basic-auth-failure"
+ },
+ {
"description": "The amount of memory freed by the last memory tracker garbage collection.",
"contexts": [
"IMPALAD"
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 cbc7ab5..acf2835 100644
--- a/fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java
+++ b/fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java
@@ -32,6 +32,7 @@ import org.apache.directory.server.annotations.CreateTransport;
import org.apache.directory.server.core.annotations.ApplyLdifFiles;
import org.apache.directory.server.core.integ.CreateLdapServerRule;
import org.apache.hive.service.rpc.thrift.*;
+import org.apache.impala.util.Metrics;
import org.apache.thrift.transport.THttpClient;
import org.apache.thrift.protocol.TBinaryProtocol;
import org.junit.Before;
@@ -51,6 +52,8 @@ public class LdapHS2Test {
@ClassRule
public static CreateLdapServerRule serverRule = new CreateLdapServerRule();
+ Metrics metrics = new Metrics();
+
@Before
public void setUp() throws Exception {
String uri =
@@ -92,11 +95,22 @@ public class LdapHS2Test {
return execResp.getOperationHandle();
}
+ private void verifyMetrics(long expectedBasicAuthSuccess, long expectedBasicAuthFailure)
+ throws Exception {
+ long actualBasicAuthSuccess = (long) metrics.getMetric(
+ "impala.thrift-server.hiveserver2-http-frontend.total-basic-auth-success");
+ assertEquals(expectedBasicAuthSuccess, actualBasicAuthSuccess);
+ long actualBasicAuthFailure = (long) metrics.getMetric(
+ "impala.thrift-server.hiveserver2-http-frontend.total-basic-auth-failure");
+ assertEquals(expectedBasicAuthFailure, actualBasicAuthFailure);
+ }
+
/**
* Tests LDAP authentication to the HTTP hiveserver2 endpoint.
*/
@Test
public void testHiveserver2() throws Exception {
+ verifyMetrics(0, 0);
THttpClient transport = new THttpClient("http://localhost:28000");
Map<String, String> headers = new HashMap<String, String>();
// Authenticate as 'Test1Ldap' with password '12345'
@@ -108,9 +122,13 @@ public class LdapHS2Test {
// Open a session which will get username 'Test1Ldap'.
TOpenSessionReq openReq = new TOpenSessionReq();
TOpenSessionResp openResp = client.OpenSession(openReq);
+ // One successful authentication.
+ verifyMetrics(1, 0);
// Running a query should succeed.
TOperationHandle operationHandle = execAndFetch(
client, openResp.getSessionHandle(), "select logged_in_user()", "Test1Ldap");
+ // Two more successful authentications - for the Exec() and the Fetch().
+ verifyMetrics(3, 0);
// Authenticate as 'Test2Ldap' with password 'abcde'
headers.put("Authorization", "Basic VGVzdDJMZGFwOmFiY2Rl");
@@ -123,25 +141,31 @@ public class LdapHS2Test {
fail("Expected error: " + expectedError);
} catch (Exception e) {
assertTrue(e.getMessage().contains(expectedError));
+ // The connection for the Exec() will be successful.
+ verifyMetrics(4, 0);
}
// Try to cancel the first query, which should fail.
TCancelOperationReq cancelReq = new TCancelOperationReq(operationHandle);
TCancelOperationResp cancelResp = client.CancelOperation(cancelReq);
+ verifyMetrics(5, 0);
assertEquals(cancelResp.getStatus().getStatusCode(), TStatusCode.ERROR_STATUS);
assertEquals(cancelResp.getStatus().getErrorMessage(), expectedError);
// Open another session which will get username 'Test2Ldap'.
TOpenSessionReq openReq2 = new TOpenSessionReq();
TOpenSessionResp openResp2 = client.OpenSession(openReq);
+ verifyMetrics(6, 0);
// Running a query with the new session should succeed.
execAndFetch(
client, openResp2.getSessionHandle(), "select logged_in_user()", "Test2Ldap");
+ verifyMetrics(8, 0);
// Attempt to authenticate with some bad headers:
// - invalid username/password combination
// - invalid base64 encoded value
// - Invalid mechanism
+ int numFailures = 0;
for (String authStr : new String[] {"Basic VGVzdDJMZGFwOjEyMzQ1",
"Basic invalid-base64", "Negotiate VGVzdDFMZGFwOjEyMzQ1"}) {
// Attempt to authenticate with an invalid password.
@@ -150,6 +174,8 @@ public class LdapHS2Test {
try {
TOpenSessionReq openReq3 = new TOpenSessionReq();
TOpenSessionResp openResp3 = client.OpenSession(openReq);
+ ++numFailures;
+ verifyMetrics(8, numFailures);
fail("Exception exception.");
} catch (Exception e) {
assertEquals(e.getMessage(), "HTTP Response code: 401");