NIFI-12350 Implement Azure Active Directory authentication option whe…#8023
NIFI-12350 Implement Azure Active Directory authentication option whe…#8023vuong2407 wants to merge 3 commits intoapache:mainfrom
Conversation
…n using Apache Kafka in Azure Event Hubs
exceptionfactory
left a comment
There was a problem hiding this comment.
Thanks for the contribution @vuong2407. This feature for Azure AD authentication looks useful in general, and aligns with optional supports for AWS MSK. However, the initial implementation raises several concerns about shared configuration properties. Work is in progress to redesign the Kafka integration strategy using an extensible Controller Service to handle authentication, and that may provide a better foundation for this kind of feature. For this pull request, some further consideration is necessary to determine whether it can fit in the existing structure given the dependencies required.
| </dependency> | ||
| <dependency> | ||
| <groupId>org.apache.kafka</groupId> | ||
| <artifactId>kafka-clients</artifactId> |
There was a problem hiding this comment.
The nifi-kafka-shared library is intended to avoid direct dependencies on Kafka libraries, so the implementation approach needs to be refactored to avoid this dependency on kafka-clients in this shared library.
| @@ -0,0 +1,86 @@ | |||
| package org.apache.nifi.kafka.shared.aad; | |||
There was a problem hiding this comment.
This class and others are missing the Apache license header.
| public static String authority; | ||
| public static String appId; | ||
| public static String appSecret; | ||
| public static String bootstrapServer; |
There was a problem hiding this comment.
Setting static variables is not suitable for multiple instances of this Processor, although the way the Kafka library instantiates the class makes it difficult to configure. Some further design consideration will be necessary.
exceptionfactory
left a comment
There was a problem hiding this comment.
Thanks for your work on this improvement @vuong2407.
On further review of these changes, it would be best to evaluate an alternative that is not specific to the Azure EventHubs Kafka environment. The OAUTHBEARER authentication strategy is generic, as described in KIP-255, although some implementations may have certain particular approaches. The proposed changes would add a direct dependency on msal4j, which would not apply to many Kafka client deployments.
One option is to revisit this capability after completion of new Kafka components, described in NIFI-11259. There is no precise timeline for that implementation, so the other option is to evaluate an alternative approach that avoids vendor-specific dependencies. That may not be achievable without introducing some new Controller Service extension interface, which again points back to the redesign. As it stands, however, introducing these substantive changes to the current components does not appear to be the best way forward in terms of future maintainability.
|
Thanks again for the work on this change @vuong2407. In light of the current conflicts, and the work in progress on refactored Kafka Processors for NIFI-11259 now in review, it would be better to revisit this implementation after the refactored Processors have been reviewed and merged. That should provide a much better baseline for integrating alternative authentication strategies. |
…n using Apache Kafka in Azure Event Hubs
Summary
NIFI-12350
Tracking
Please complete the following tracking steps prior to pull request creation.
Issue Tracking
Pull Request Tracking
NIFI-00000NIFI-00000Pull Request Formatting
mainbranchVerification
Please indicate the verification steps performed prior to pull request creation.
Build
mvn clean install -P contrib-checkLicensing
LICENSEandNOTICEfilesDocumentation