NIFI-9036 - Add AWS MSK IAM support to Kafka processors

[nrnalamalpu]

Thank you for submitting a contribution to Apache NiFi.

Please provide a short description of the PR here:

Description of PR

Enhance Kafka processors for AWS_MSK_IAM SASL mechanism

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

• Is there a JIRA ticket associated with this PR? Is it referenced
  in the commit message?

• Does your PR title start with NIFI-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.

• Has your PR been rebased against the latest commit within the target branch (typically main)?

• Is your initial contribution a single, squashed commit? Additional commits in response to PR reviewer feedback should be made on this branch and pushed to allow change tracking. Do not squash or use --force when pushing to allow for clean monitoring of changes.

For code changes:

• Have you ensured that the full suite of tests is executed via mvn -Pcontrib-check clean install at the root nifi folder?
• Have you written or updated unit tests to verify your changes?
• Have you verified that the full build is successful on JDK 8?
• Have you verified that the full build is successful on JDK 11?
• If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
• If applicable, have you updated the LICENSE file, including the main LICENSE file under nifi-assembly?
• If applicable, have you updated the NOTICE file, including the main NOTICE file found under nifi-assembly?
• If adding new Properties, have you added .displayName in addition to .name (programmatic access) for each of the new properties?

For documentation related changes:

• Have you ensured that format looks appropriate for the output in which it is rendered?

Note:

Please ensure that once the PR is submitted, you check GitHub Actions CI for build issues and submit an update to your PR as soon as possible.

[mattyb149]

Is there a Jira case associated with this? If not please create one and use the NIFI-XXXX case number in the title of your commit(s), thanks in advance!

[nrnalamalpu]

Hi @mattyb149, can you please look into this PR?

[nrnalamalpu]

@pvillard31 or @mattyb149 can anyone take a look at this PR and see if we can merge this to main?

[pvillard31]

Just looking at the code, it LGTM. I don't have time to give this a try right now though. Hopefully some time next week if no one else does before.

[nrnalamalpu]

@pvillard31 or @mattyb149 can we please take a look at this PR and close this out? All checks have been passed for this branch.

[nrnalamalpu]

@mattyb149 or @pvillard31, Hope you are doing great. Can anyone review and approve this PR ? its been in this state for quite sometime.

[GaudamVokal]

Are we planning to merge this pr. Its been in pending for a long time.

[pvillard31]

There is a conflict with the main branch? Can you fix the conflict and update your pull request?
Once fixed, I'll spend some time reviewing it. I now have some cycles to do it.

[babumahesh-koo]

@nrnalamalpu @pvillard31

We are looking for Nifi to AWS IAM integration, this PR is in pending state from long time.

Any prioritization would be helpful.

[pvillard31]

@babumahesh-koo - as I said, this PR currently has a conflict with the main branch. When it's fixed, I can spend some cycles reviewing/testing it.

I'd also add that you, @babumahesh-koo @GaudamVokal , can help in the process by testing this PR by yourself on your side and by reporting your findings / evidences here. Only a committer can merge this code but anyone in the community can help test the code and make sure this fits real life use cases / needs.

[nrnalamalpu]

@pvillard31, I'm looking into the conflicts. will resolve in a bit and let you guys know. Thanks

[nrnalamalpu]

The merge conflicts are taken care and the build is in progress. This can be reviewed and merged as soon as the build is completed.

[nrnalamalpu]

Can anyone look at this PR and approve?

[pvillard31]

I will, later today

[pvillard31]

Hi @nrnalamalpu - thanks for this contribution, looking forward to merging this improvement. However, I'm not sure how this has been tested in the real life as the newly added properties are not exposed in the Kafka processors. I left a few comments that should be addressed before this gets merged. Please also consider adding unit tests (or integration tests). At this point, I don't see how this code could be used because of the missing properties. I'm also suggesting to add another property so that a user can specify the STS regional endpoint.

[thenatog]

@nrnalamalpu Hi, I notice that AwsKmsSensitivePropertyProvider.java is being deleted in this PR - is there any reason this is being removed or is it a mistake? We can't remove this class, and it seems unrelated to the changes you're making. Thanks!

[nrnalamalpu]

Hi @thenatog, It is removed by mistake. i'm not sure how this file got removed. I'll add the class back to the PR.

[nrnalamalpu]

Hi @pvillard31,

I could not work on this PR till now due to other priorities. I addressed all the review comments and added the AwsKmsSensitivePropertyProvider.java back which is deleted by mistake.

Please see the screenshots for the testing we did in our AWS environment.

Could you please review the latest changes and merge the PR if everything looks fine?

Thank you!

[nrnalamalpu]

@pvillard31, Could you please look at the changes and see if we can get this merged to main branch? Thanks

[nrnalamalpu]

Can anyone please look into this PR and see if we can close this out?

thanks

[github-actions]

We're marking this PR as stale due to lack of updates in the past few months. If after another couple of weeks the stale label has not been removed this PR will be closed. This stale marker and eventual auto close does not indicate a judgement of the PR just lack of reviewer bandwidth and helps us keep the PR queue more manageable. If you would like this PR re-opened you can do so and a committer can remove the stale tag. Or you can open a new PR. Try to help review other PRs to increase PR review bandwidth which in turn helps yours.

[nandorsoma]

Hey @nrnalamalpu!
I've reviewed this PR. It wasn't easy because the git history is a bit messy, which makes it hard to apply to the current main. What I see is that the new PropertyDescriptors are not registered in the processors. Without that, this feature won't work. Did you leave it out from one of the commits by mistake?
I see that this PR has been open for a long time. Do you have the intention to wrap it up? If so, I'll help you push it over the finish line.

[nrnalamalpu]

Hi @nandorsoma, Glad you reviewed the pull request at least. I was waiting for at least one of the committers to review and approve the PR. I would like to work on this and wrap up. Please let me know what should I do to wrap this up.

[nandorsoma]

Hey @nrnalamalpu!
As I mentioned in the previous comment, the PropertyDescriptors are not registered in any of the processors so this feature won't work currently. The first step would be to fix that. After that:

• Rebase the branch on top of the current main.
• Since the git history is a bit messy, it would be hard to merge it. Please squash all of your commits and force-push one single commit, which can be our new baseline. If it's easier for you, you can open a new PR and do a cross-reference (commenting the link of the PR is enough) on both PR.
• Test it with MSK, and only update this PR when it works as expected.

[nandorsoma]

@nrnalamalpu any update on this?

[exceptionfactory]

@nandorsoma and @nrnalamalpu NIFI-10819 implemented in pull request #6690 introduced significant refactoring of duplicate code into a new nifi-kafka-shared module. The current approach will need to be refactored to follow that approach. If either of you are interested in creating a new pull request, that would be helpful, or I may be able to evaluate an implementation based on the core features of this PR.

[nandorsoma]

Hey @exceptionfactory! Thank you for the heads up! I'm interested in opening a new pr, but I'd like to wait for @nrnalamalpu's answer.

[nrnalamalpu]

Hi @nandorsoma and @exceptionfactory, we can have a new PR. Unfortunately, I tied up with other Priority items and could not spend time on this.

[nrnalamalpu]

@nandorsoma, not sure which file you are referring for the property descriptors. The PropertyDescriptors are added in KafkaProcessorUtils.java file which will be rolled up to consumeKafka/publishKafka processors.

[nandorsoma]

@nrnalamalpu, how do you mean they will be rolled up? For example, AWS_PROFILE_NAME is referenced only in one place in the whole codebase when you get the value of the property in setIamJaasConfig(). If you check other PropertyDescriptors, such as PASSWORD, you can see that it is added to the descriptor list of all processors that use it.