Skip to content

NIFI-11202 Remove redundant Kerberos related properties from Kafka processors#6978

Closed
nandorsoma wants to merge 1 commit intoapache:mainfrom
nandorsoma:NIFI-11202
Closed

NIFI-11202 Remove redundant Kerberos related properties from Kafka processors#6978
nandorsoma wants to merge 1 commit intoapache:mainfrom
nandorsoma:NIFI-11202

Conversation

@nandorsoma
Copy link
Contributor

Summary

NIFI-11202

Tracking

Please complete the following tracking steps prior to pull request creation.

Issue Tracking

Pull Request Tracking

  • Pull Request title starts with Apache NiFi Jira issue number, such as NIFI-00000
  • Pull Request commit message starts with Apache NiFi Jira issue number, as such NIFI-00000

Pull Request Formatting

  • Pull Request based on current revision of the main branch
  • Pull Request refers to a feature branch with one commit containing changes

Verification

Please indicate the verification steps performed prior to pull request creation.

Build

  • Build completed using mvn clean install -P contrib-check
    • JDK 11
    • JDK 17

Licensing

  • New dependencies are compatible with the Apache License 2.0 according to the License Policy
  • New dependencies are documented in applicable LICENSE and NOTICE files

Documentation

  • Documentation formatting appears as expected in rendered files

Copy link
Contributor

@exceptionfactory exceptionfactory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on the clean up @nandorsoma! The changes appear to be in order, but it looks like the KERBEROS_SERVICE_NAME property can also be removed, since it is only used in conjunction with the direct Kerberos Principal and Kerberos Keytab properties.

@nandorsoma
Copy link
Contributor Author

Thanks for working on the clean up @nandorsoma! The changes appear to be in order, but it looks like the KERBEROS_SERVICE_NAME property can also be removed, since it is only used in conjunction with the direct Kerberos Principal and Kerberos Keytab properties.

Good catch @exceptionfactory! However, it makes me wonder, what is the real purpose of this property? Why is it not needed when KerberosKeyTabUserService is used? Afaik, as an alternative, it is possible to specify it in the principal property like that: <serviceName>/<hostname>@KerberosRealmName. Is it equivalent to having serviceName="<serviceName>" principal="<hostname>@KerberosRealmName" in the JAAS config? If so, why did this property even exist in the first place?

@nandorsoma
Copy link
Contributor Author

nandorsoma commented Feb 26, 2023

Ok, after a second round of investigation, you can disregard my previous question. So it seems like the KERBEROS_SERVICE_NAME property is actually used, but the references are weak, so it is really hard to find the connection. The StandardKafkaPropertyProvider will pick up all PropertyDescriptors that's name matches a standard kafka property name. Corresponding code can be found here. After that serviceName will be set based on those properties in the CustomKerberosLogin class which is, again, weakly configured here.

@exceptionfactory
Copy link
Contributor

Thanks for tracking down the KERBEROS_SERVICE_NAME reference @nandorsoma. I noticed the previous reference based on the on the JAAS configuration provider, but the CustomKerberosLogin class definitely has ongoing use. With that reference, it looks like the property needs to remain in place, which means this pull request should be ready to go.

@nandorsoma
Copy link
Contributor Author

Thank you for the review @exceptionfactory! Yes, it is easy to fall into that trap. It would be a bit better if the SASL_GSSAPI_CUSTOM_LOGIN_CLASS wouldn't be referenced as a string. But it is only possible by importing an additional module which is clearly not a goal here. Since this PR is finalized, I've opened its counterpart (#6990) against support/nifi-1.x to add deprecation logging.

@exceptionfactory
Copy link
Contributor

@nandorsoma Based on the discussion around removing the Kafka 2_0 components in PR #6990, this PR should be scoped down to the 2_6 components. After that, following up with one more PR to remove the 2_0 components from the main branch be helpful.

@nandorsoma
Copy link
Contributor Author

@exceptionfactory, I've reversed the order and opened #7010 to remove the Kafka 2_0 components because otherwise, I cannot remove the deprecated properties from KafkaClientComponent, because those components depend on them. Once #7010 is merged, I'll rebase this PR.

@exceptionfactory
Copy link
Contributor

Thanks for putting together the related pull requests @nandorsoma. Now that #7010 is merged, this PR is ready to be rebased.

@nandorsoma
Copy link
Contributor Author

Thanks for your review, @exceptionfactory! Please see my latest commit, which addresses your comments!

@nandorsoma
Copy link
Contributor Author

I think we will need a recheck because there seem to be intermittent and two timeout errors...

Copy link
Contributor

@exceptionfactory exceptionfactory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making the adjustments @nandorsoma, the latest version looks good and aligns with the deprecation logging added in #6990 for the Kafka 2_6 components. +1 merging

r-vandenbos pushed a commit to r-vandenbos/nifi that referenced this pull request Apr 11, 2023
This closes apache#6978

Signed-off-by: David Handermann <exceptionfactory@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants