exceptionfactory commented on a change in pull request #5322:
URL: https://github.com/apache/nifi/pull/5322#discussion_r694422664
##########
File path:
nifi-nar-bundles/nifi-standard-services/nifi-hbase_2-client-service-bundle/nifi-hbase_2-client-service/src/main/java/org/apache/nifi/hbase/HBase_2_ClientService.java
##########
@@ -413,8 +448,20 @@ public void shutdown() {
if (connection != null) {
try {
connection.close();
- } catch (final IOException ioe) {
- getLogger().warn("Failed to close connection to HBase due to
{}", new Object[]{ioe});
+ } catch (final Exception e) {
+ getLogger().warn("Failed to close connection to HBase due to
{}", new Object[]{e});
+ }
+ }
+
+ final KerberosUser kerberosUser = kerberosUserReference.get();
+ if (kerberosUser != null) {
+ try {
+ kerberosUser.logout();
+ } catch (final Exception e) {
+ getLogger().warn("Error logging out KerberosUser: {}",
e.getMessage(), e);
Review comment:
Is it necessary to include the exception message since it will also be
included in the stack trace?
```suggestion
getLogger().warn("KeberosUser Logout Failed", e);
```
##########
File path:
nifi-nar-bundles/nifi-standard-services/nifi-hbase_2-client-service-bundle/nifi-hbase_2-client-service/src/main/java/org/apache/nifi/hbase/HBase_2_ClientService.java
##########
@@ -413,8 +448,20 @@ public void shutdown() {
if (connection != null) {
try {
connection.close();
- } catch (final IOException ioe) {
- getLogger().warn("Failed to close connection to HBase due to
{}", new Object[]{ioe});
+ } catch (final Exception e) {
+ getLogger().warn("Failed to close connection to HBase due to
{}", new Object[]{e});
Review comment:
Recommend rewording the message and passing the exception argument so
that the stack trace will be included.
```suggestion
getLogger().warn("HBase connection close failed", e);
```
##########
File path:
nifi-nar-bundles/nifi-standard-services/nifi-hbase_1_1_2-client-service-bundle/nifi-hbase_1_1_2-client-service/src/main/java/org/apache/nifi/hbase/HBase_1_1_2_ClientService.java
##########
@@ -359,43 +392,45 @@ protected Connection createConnection(final
ConfigurationContext context) throws
}
if (SecurityUtil.isSecurityEnabled(hbaseConfig)) {
- String principal =
context.getProperty(kerberosProperties.getKerberosPrincipal()).evaluateAttributeExpressions().getValue();
- String keyTab =
context.getProperty(kerberosProperties.getKerberosKeytab()).evaluateAttributeExpressions().getValue();
- String password =
context.getProperty(kerberosProperties.getKerberosPassword()).getValue();
-
- // If the Kerberos Credentials Service is specified, we need to
use its configuration, not the explicit properties for principal/keytab.
- // The customValidate method ensures that only one can be set, so
we know that the principal & keytab above are null.
- final KerberosCredentialsService credentialsService =
context.getProperty(KERBEROS_CREDENTIALS_SERVICE).asControllerService(KerberosCredentialsService.class);
- if (credentialsService != null) {
- principal = credentialsService.getPrincipal();
- keyTab = credentialsService.getKeytab();
- }
+ getLogger().info("HBase Security Enabled, creating KerberosUser");
Review comment:
Is this necessary as an info log? Perhaps changing this to a debug log
is sufficient given the info log message on successful login.
##########
File path:
nifi-nar-bundles/nifi-hbase-bundle/nifi-hbase-processors/src/main/java/org/apache/nifi/hbase/AbstractPutHBase.java
##########
@@ -221,9 +222,11 @@ public void onTrigger(final ProcessContext context, final
ProcessSession session
try {
clientService.put(entry.getKey(), entry.getValue());
successes.addAll(entry.getValue());
- } catch (Exception e) {
+ } catch (final KerberosLoginException kle) {
+ getLogger().error("Failed to connect to HBase due to {}.
Rolling back session, and penalizing flow files.", new Object[]{kle}, kle);
Review comment:
The wrapping `Object[]` can be removed:
```suggestion
getLogger().error("Failed to connect to HBase due to {}:
Rolling back session, and penalizing flow files", kle, kle);
```
##########
File path:
nifi-nar-bundles/nifi-standard-services/nifi-hbase_1_1_2-client-service-bundle/nifi-hbase_1_1_2-client-service/src/main/java/org/apache/nifi/hbase/HBase_1_1_2_ClientService.java
##########
@@ -359,43 +392,45 @@ protected Connection createConnection(final
ConfigurationContext context) throws
}
if (SecurityUtil.isSecurityEnabled(hbaseConfig)) {
- String principal =
context.getProperty(kerberosProperties.getKerberosPrincipal()).evaluateAttributeExpressions().getValue();
- String keyTab =
context.getProperty(kerberosProperties.getKerberosKeytab()).evaluateAttributeExpressions().getValue();
- String password =
context.getProperty(kerberosProperties.getKerberosPassword()).getValue();
-
- // If the Kerberos Credentials Service is specified, we need to
use its configuration, not the explicit properties for principal/keytab.
- // The customValidate method ensures that only one can be set, so
we know that the principal & keytab above are null.
- final KerberosCredentialsService credentialsService =
context.getProperty(KERBEROS_CREDENTIALS_SERVICE).asControllerService(KerberosCredentialsService.class);
- if (credentialsService != null) {
- principal = credentialsService.getPrincipal();
- keyTab = credentialsService.getKeytab();
- }
+ getLogger().info("HBase Security Enabled, creating KerberosUser");
+ final KerberosUser kerberosUser = createKerberosUser(context);
+ ugi = SecurityUtil.getUgiForKerberosUser(hbaseConfig,
kerberosUser);
+ kerberosUserReference.set(kerberosUser);
+ getLogger().info("Successfully logged in as principal {}",
kerberosUser.getPrincipal());
+ return getUgi().doAs((PrivilegedExceptionAction<Connection>)() ->
ConnectionFactory.createConnection(hbaseConfig));
+ } else {
+ getLogger().info("Simple Authentication");
Review comment:
This seems like it should be a debug log.
##########
File path:
nifi-nar-bundles/nifi-standard-services/nifi-hbase_2-client-service-bundle/nifi-hbase_2-client-service/src/main/java/org/apache/nifi/hbase/HBase_2_ClientService.java
##########
@@ -359,43 +392,45 @@ protected Connection createConnection(final
ConfigurationContext context) throws
}
if (SecurityUtil.isSecurityEnabled(hbaseConfig)) {
- String principal =
context.getProperty(kerberosProperties.getKerberosPrincipal()).evaluateAttributeExpressions().getValue();
- String keyTab =
context.getProperty(kerberosProperties.getKerberosKeytab()).evaluateAttributeExpressions().getValue();
- String password =
context.getProperty(kerberosProperties.getKerberosPassword()).getValue();
-
- // If the Kerberos Credentials Service is specified, we need to
use its configuration, not the explicit properties for principal/keytab.
- // The customValidate method ensures that only one can be set, so
we know that the principal & keytab above are null.
- final KerberosCredentialsService credentialsService =
context.getProperty(KERBEROS_CREDENTIALS_SERVICE).asControllerService(KerberosCredentialsService.class);
- if (credentialsService != null) {
- principal = credentialsService.getPrincipal();
- keyTab = credentialsService.getKeytab();
- }
+ getLogger().info("HBase Security Enabled, creating KerberosUser");
Review comment:
See note on info and debug logging.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
