NIFI-10776 add NONE and PKI AuthorizationSchemes for ElasticSearchClientService

[ChrisSamo632]

Summary

NIFI-10776 add NONE and PKI AuthorizationSchemes for ElasticSearchClientService

Fix the ElasticSearchClientService integration-tests for Elasticsearch 6.x/7.x

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

[exceptionfactory]

Thanks for the contribution @ChrisSamo632.

Reviewing the changes, are these new Authorization Schemes necessary? NONE is implied in the absence of credentials, and PKI appears to be handled already through the configuration of an SSL Context Service. It seems better to avoid introducing additional settings if they do not impact runtime behavior.

[ChrisSamo632]

Thanks for the contribution @ChrisSamo632.

Reviewing the changes, are these new Authorization Schemes necessary? NONE is implied in the absence of credentials, and PKI appears to be handled already through the configuration of an SSL Context Service. It seems better to avoid introducing additional settings if they do not impact runtime behavior.

I guess the question is whether people will understand that they can select (for example) BASIC auth but then leave username & password empty in order to get NOONE. Similarly, doing the same but configuring an SSLContextService (with keystore) for PKI auth

The added validation simply adds some more guidance to what's already possible, I get that, but is it obvious enough that alternatives are available if we only have a mandatory Authorisation Scheme that only allows BASIC or API_KEY?

[exceptionfactory]

Thanks for the reply @ChrisSamo632, good points. Of course this would have been more logical if the Scheme property had been there since the beginning, but adding it later does make it a little more challenging.

As it stands, the default BASIC value shows the Username and Password, but since they are optional, the effective behavior is NONE.

Given the complexity of the validation code, I'm not as inclined to add it for informational purposes. Adding NONE might be helpful, as it would effectively hide the Username and Password properties, without the need for custom validation. PKI is tricky because the SSL Context Service may or may not be used for authentication.

Taking a step back, the Authorization Scheme property description does indicate that it is used for authenticating with the HTTP Authorization header, which is not applicable to PKI. I could go either way on NONE. Perhaps just updating the description of the property would be helpful?

[ChrisSamo632]

@exceptionfactory I see the argument either way

I think there are a couple of changes/fixes here with keeping even if the approach is changed (although they could be rolled into other PRs), e.g. fixes for the integration-tests (broken by the previous addition for API_KEY) and a start of unit tests for the controller class

There will possibly be other Auth Schemes to add in future, which could swing the argument - Kerberos and JWT are both supported by Elasticsearch and could be configured for use by NiFi (on my list to raise tickets for these). Do you think this changes the argument (for NONE/PKI) at all?

[exceptionfactory]

I agree it would be helpful to move this pull request forward to get the integration test improvements. I would also note that we should avoid unit testing exact error message strings, instead it is better to assert that a message contains a particular keyword.

The potential for additional schemes is a good point in favor of these additions. Can you take a look at simplifying the custom validation method? I could see the PKI scheme checking for the SSL Context Service being set, but giving it another look, I don't think the other checks are necessary because of the way custom validation hides the other properties.

[ChrisSamo632]

I agree it would be helpful to move this pull request forward to get the integration test improvements. I would also note that we should avoid unit testing exact error message strings, instead it is better to assert that a message contains a particular keyword.

The potential for additional schemes is a good point in favor of these additions. Can you take a look at simplifying the custom validation method? I could see the PKI scheme checking for the SSL Context Service being set, but giving it another look, I don't think the other checks are necessary because of the way custom validation hides the other properties.

@exceptionfactory I've simplified the customValidate logic (largely returning things back to how they are currently on main for BASIC and API_KEY schemes), also addressed your other comments

For the additional AuthorizationSchemes:

• NIFI-10830 - Kerberos
• NIFI-10831 - JWT

[exceptionfactory]

Thanks for making the adjustments @ChrisSamo632, the latest version looks good. +1 merging