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:18 UTC
[impala] branch master updated (3a4dce2 -> 3bd5397)
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 3a4dce2 IMPALA-8436: Do not create materialized_view in Kudu/HBase
new 02a7982 Remove "Could not transfer" exclusion in mvn-quiet.sh
new 3bd5397 IMPALA-8583: Add metrics for Basic auth
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/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 ++++++++++++++++------
bin/mvn-quiet.sh | 3 +-
common/thrift/metrics.json | 20 ++++++++++++++
.../apache/impala/customcluster/LdapHS2Test.java | 26 ++++++++++++++++++
9 files changed, 112 insertions(+), 24 deletions(-)
[impala] 02/02: IMPALA-8583: Add metrics for Basic auth
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 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");
[impala] 01/02: Remove "Could not transfer" exclusion in
mvn-quiet.sh
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 02a79822eb1714293ba72c18b52181f33663843a
Author: Fredy Wijaya <fw...@cloudera.com>
AuthorDate: Thu Jun 13 19:10:08 2019 -0700
Remove "Could not transfer" exclusion in mvn-quiet.sh
"Could not transfer" warning messages are noisy. However, excluding
"Could not transfer" words can lead to actual error messages that
contain "Could not transfer" to not be shown in the stdout, which can
make debugging difficult. This patch updates mvn-quiet.sh to show
"Could not transfer" messages.
Testing:
- Ran FE build
Change-Id: Ide3367fd98abbbe11eec1fa86fbad8b32eeecb8d
Reviewed-on: http://gerrit.cloudera.org:8080/13647
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
bin/mvn-quiet.sh | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/bin/mvn-quiet.sh b/bin/mvn-quiet.sh
index 709ad28..beeaeb9 100755
--- a/bin/mvn-quiet.sh
+++ b/bin/mvn-quiet.sh
@@ -32,8 +32,7 @@ Directory $(pwd)
EOF
if ! mvn $IMPALA_MAVEN_OPTIONS "$@" | \
- tee -a "$LOG_FILE" | grep -E -e WARNING -e ERROR -e SUCCESS -e FAILURE -e Test | \
- grep -v "Could not transfer"; then
+ tee -a "$LOG_FILE" | grep -E -e WARNING -e ERROR -e SUCCESS -e FAILURE -e Test; then
echo "mvn $IMPALA_MAVEN_OPTIONS $@ exited with code $?"
exit 1
fi