You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2021/08/24 01:49:05 UTC
[GitHub] [nifi] exceptionfactory commented on a change in pull request #5322: NIFI-8975 Integrate KerberosUserService into HBase processors/services
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org