-
Notifications
You must be signed in to change notification settings - Fork 2.9k
NIFI-14782 Extended Kafka3ConnectionService OAuth authentication with… #10567
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
… SASL Extensions support
...dle/nifi-kafka-shared/src/main/java/org/apache/nifi/kafka/shared/util/SaslExtensionUtil.java
Show resolved
Hide resolved
...afka-service-shared/src/main/java/org/apache/nifi/kafka/service/Kafka3ConnectionService.java
Show resolved
Hide resolved
...afka-service-shared/src/main/java/org/apache/nifi/kafka/service/Kafka3ConnectionService.java
Outdated
Show resolved
Hide resolved
|
@dan-s1 Thanks for your suggestions. Updated the PR. |
mark-bathori
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @turcsanyip for this improvement. The changes look good and work as expected.
+1
...ed/src/main/java/org/apache/nifi/kafka/service/security/OAuthBearerLoginCallbackHandler.java
Show resolved
Hide resolved
pvillard31
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is extremely unlikely but the dynamic properties are configured with support for expression language at environment level. Users should definitely use parameters instead but they could technically reference environment variable through expression language. Do we want to do something about it? Right now we would not evaluate the dynamic property with expression language for the SASL extensions.
| if (subject.startsWith(PARTITIONS_PROPERTY_PREFIX)) { | ||
| builder.valid(true); | ||
| } else if (subject.startsWith(SASL_EXTENSION_PROPERTY_PREFIX)) { | ||
| builder.valid(true); | ||
| } else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably makes sense to combine these
| if (subject.startsWith(PARTITIONS_PROPERTY_PREFIX)) { | |
| builder.valid(true); | |
| } else if (subject.startsWith(SASL_EXTENSION_PROPERTY_PREFIX)) { | |
| builder.valid(true); | |
| } else { | |
| if (subject.startsWith(PARTITIONS_PROPERTY_PREFIX) || subject.startsWith(SASL_EXTENSION_PROPERTY_PREFIX)) { | |
| builder.valid(true); | |
| } else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are unrelated concepts, so I did not want to combine them, even with minimal code duplication. Also, the PARTITIONS_PROPERTY_PREFIX is a leftover from the old Kafka processors and it is not supported at the moment. It will be re-added in #10538. I would defer to that PR to finalize it.
Good catch @pvillard31, I did not notice it. Checked |
|
Are you suggesting to have a customValidate to make the component invalid if EL is used for SASL extension properties? Or completely remove EL support for all dynamic properties? I'd be fine with the first option, I'm not sure that would be OK to go with the second option. |
|
None of them but setting the EL flag on the given dynamic property conditionally, similar to the description. |
|
Oh yeah, of course, even better |
pvillard31
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes @turcsanyip
… SASL Extensions support Signed-off-by: Pierre Villard <[email protected]> This closes apache#10567.
… SASL Extensions support
Summary
NIFI-14782
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
./mvnw clean install -P contrib-checkLicensing
LICENSEandNOTICEfilesDocumentation