----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72558/#review220919 -----------------------------------------------------------
Fix it, then Ship it! notification/src/main/java/org/apache/atlas/kafka/KafkaNotification.java Lines 436 (patched) <https://reviews.apache.org/r/72558/#comment309633> Consider following renamings for readability: tempJAASConfigPrefix => ticketBasedConfigPrefix tempConfiguration => ticketBasedConfig notification/src/main/java/org/apache/atlas/kafka/KafkaNotification.java Lines 442 (patched) <https://reviews.apache.org/r/72558/#comment309634> - consider logging this message at info level - consider adding more details in log message for help troubleshooting, like: LOG.info("UserGroupInformation.isLoginTicketBased is true, but no JAAS configuration found for client {}. Will use JAAS configuration of client {}", JAAS_TICKET_BASED_CLIENT_NAME, JAAS_DEFAULT_CLIENT_NAME); notification/src/main/java/org/apache/atlas/kafka/KafkaNotification.java Line 486 (original), 494 (patched) <https://reviews.apache.org/r/72558/#comment309635> Please add @VisibleForTesting annotation to methods isLoginKeytabBased() and isLoginTicketBased(). - Madhan Neethiraj On May 30, 2020, 11:30 a.m., Jayendra Parab wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72558/ > ----------------------------------------------------------- > > (Updated May 30, 2020, 11:30 a.m.) > > > Review request for atlas, Madhan Neethiraj, Nixon Rodrigues, and Sarath > Subramanian. > > > Bugs: ATLAS-3779 > https://issues.apache.org/jira/browse/ATLAS-3779 > > > Repository: atlas > > > Description > ------- > > As part of earlier fix for ATLAS-3779, the code to set JAAS configuration for > authentication with Kafka was moved to KafkaNotification class. > In some scenarios, even though on Atlas Client side, the > UserGroupInformation.isLoginTicketBased() is true and > UserGroupInformation.isLoginKeytabBased() is false, the > atlas.jaas.ticketBased-KafkaClient.* configuration properties are not set in > atlas-application.properties file. Because of this, communication with Kafka > is affected. > > Since in the earlier InMemoryJAASConfiguration code, in such scenarios, if > JAAS config for atlas.jaas.ticketBased-KafkaClient.* configuration wasn't > found then it would try to fallback on the JAAS configuration specified by > atlas.jaas.KafkaClient.* properties. This wasn't handled in the earlier fix > for ATLAS-3779. > This review request provides solution to this problem. > NOTE: To add unit testcases for above scenario, changed the access specifier > of isLoginKeytabBased() and isLoginTicketBased() methods and made then > non-static. > > > Diffs > ----- > > notification/src/main/java/org/apache/atlas/kafka/KafkaNotification.java > 278b3a742 > > notification/src/test/java/org/apache/atlas/kafka/KafkaNotificationMockTest.java > e345c8bba > > > Diff: https://reviews.apache.org/r/72558/diff/1/ > > > Testing > ------- > > Tested in scenario where Atlas client uses ticket based login, but > atlas.jaas.ticketBased-KafkaClient.* properties are not specified in > atlas-application.properties. > > > Thanks, > > Jayendra Parab > >
