You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2022/11/14 20:55:34 UTC
[GitHub] [nifi] ChrisSamo632 opened a new pull request, #6662: NIFI-10776 add NONE and PKI AuthorizationSchemes for ElasticSearchClientService
ChrisSamo632 opened a new pull request, #6662:
URL: https://github.com/apache/nifi/pull/6662
# Summary
[NIFI-10776](https://issues.apache.org/jira/browse/NIFI-10776) add NONE and PKI AuthorizationSchemes for ElasticSearchClientService
Fix the ElasticSearchClientService tests for Elasticsearch 6.x/7.x
# Tracking
Please complete the following tracking steps prior to pull request creation.
### Issue Tracking
- [x] [Apache NiFi Jira](https://issues.apache.org/jira/browse/NIFI) issue created
### Pull Request Tracking
- [x] Pull Request title starts with Apache NiFi Jira issue number, such as `NIFI-00000`
- [x] Pull Request commit message starts with Apache NiFi Jira issue number, as such `NIFI-00000`
### Pull Request Formatting
- [x] Pull Request based on current revision of the `main` branch
- [x] 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
- [x] Build completed using `mvn clean install -P contrib-check`
- [ ] JDK 8
- [ ] JDK 11
- [x] JDK 17
### Licensing
- ~[ ] New dependencies are compatible with the [Apache License 2.0](https://apache.org/licenses/LICENSE-2.0) according to the [License Policy](https://www.apache.org/legal/resolved.html)~
- ~[ ] New dependencies are documented in applicable `LICENSE` and `NOTICE` files~
### Documentation
- ~[ ] Documentation formatting appears as expected in rendered files~
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory closed pull request #6662: NIFI-10776 add NONE and PKI AuthorizationSchemes for ElasticSearchClientService
Posted by GitBox <gi...@apache.org>.
exceptionfactory closed pull request #6662: NIFI-10776 add NONE and PKI AuthorizationSchemes for ElasticSearchClientService
URL: https://github.com/apache/nifi/pull/6662
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] ChrisSamo632 commented on pull request #6662: NIFI-10776 add NONE and PKI AuthorizationSchemes for ElasticSearchClientService
Posted by GitBox <gi...@apache.org>.
ChrisSamo632 commented on PR #6662:
URL: https://github.com/apache/nifi/pull/6662#issuecomment-1317638709
> 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](https://issues.apache.org/jira/browse/NIFI-10830)
- [NIFI-10831 - JWT](https://issues.apache.org/jira/browse/NIFI-10831)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on pull request #6662: NIFI-10776 add NONE and PKI AuthorizationSchemes for ElasticSearchClientService
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on PR #6662:
URL: https://github.com/apache/nifi/pull/6662#issuecomment-1315923174
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on pull request #6662: NIFI-10776 add NONE and PKI AuthorizationSchemes for ElasticSearchClientService
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on PR #6662:
URL: https://github.com/apache/nifi/pull/6662#issuecomment-1315875952
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] ChrisSamo632 commented on pull request #6662: NIFI-10776 add NONE and PKI AuthorizationSchemes for ElasticSearchClientService
Posted by GitBox <gi...@apache.org>.
ChrisSamo632 commented on PR #6662:
URL: https://github.com/apache/nifi/pull/6662#issuecomment-1315911380
@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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] ChrisSamo632 commented on pull request #6662: NIFI-10776 add NONE and PKI AuthorizationSchemes for ElasticSearchClientService
Posted by GitBox <gi...@apache.org>.
ChrisSamo632 commented on PR #6662:
URL: https://github.com/apache/nifi/pull/6662#issuecomment-1315859691
> 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org