NIFI-11536: implemented AbstractStoreScanner and KeyStoreScanner that…

[emiliosetiadarma]

… reloads SSL context, changed TrustStoreScanner to extend the AbstractStoreScanner, and implemented unit tests

Summary

NIFI-11536

Tracking

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

Issue Tracking

• Apache NiFi Jira issue created

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 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

[exceptionfactory]

Thanks for addressing the issue @emiliosetiadarma! The general implementation approach looks good.

Reviewing the differences between the Scanner implementations, it looks like it comes down to configuration. Recommend making one concrete class named StoreScanner and using constructor arguments to provide the appropriate settings.

[emiliosetiadarma]

Thanks @exceptionfactory for the quick review! Will address the comments

[emiliosetiadarma]

I made the changes while still preserving KeyStoreScanner and TrustStoreScanner as the derived classes for two reasons.

1. So the logger is class-specific
2. So the server would have beans with distinct types. Having all have the same type would cause some existing tests to fail since it uses server.getBean.

Some thoughts: I was wondering if we were to have just one class whether it's a good idea to have both a keystore scanner and truststore scanner in the same class. One advantage this would have is if both keystore and truststore changed, then the SSLContext would be reloaded only once, as opposed to having two scanners that reloaded it twice.

[exceptionfactory]

I made the changes while still preserving KeyStoreScanner and TrustStoreScanner as the derived classes for two reasons.

1. So the logger is class-specific

It is not necessary to define a class-specific logger, although maintaining two separate class implementations might be useful based on the second concern.

2. So the `server` would have beans with distinct types. Having all have the same type would cause some existing tests to fail since it uses `server.getBean`.

That is a good point regarding addBean and getBean. With that in mind, it does seem reasonable to maintain two separate classes.

Some thoughts: I was wondering if we were to have just one class whether it's a good idea to have both a keystore scanner and truststore scanner in the same class. One advantage this would have is if both keystore and truststore changed, then the SSLContext would be reloaded only once, as opposed to having two scanners that reloaded it twice.

Although having one class might be worth considering, in general the keystore and truststore should change independently, so keeping two separate classes sounds good.

Thanks for considering the options.

[exceptionfactory]

Thanks for the updates @emiliosetiadarma, this looks closer to completion. Can you consolidate the two test classes into a single StoreScannerTest now that there is a single implementation class?

[exceptionfactory]

Thanks for working through the feedback @emiliosetiadarma, the latest version works as expected in success and failure conditions. +1 merging