NIFI-6576 Add HTTP authentication to confluent schema registry controller #4224
Conversation
| .name("Authentication Type") | ||
| .displayName("Authentication Type") | ||
| .description("'Basic Authentication Username' and 'Basic Authentication Password' are used if Digest Authentication is used" | ||
| + "for authentication.") |
There was a problem hiding this comment.
There is a missing space between "usedfor" based on the String concatenation.
There was a problem hiding this comment.
While people familiar with RFC 2617 will understand that basic authentication and digest authentication both use a username and password as input to an authentication process with the server, the standalone message 'Basic Authentication Username' and 'Basic Authentication Password' are used if Digest Authentication is used for authentication. can reasonably be expected to confuse a normal user. I would recommend changing the username and password property descriptors to remove the "basic" terminology and change the description for this PD to read Both basic authentication and digest authentication will use the 'Authentication Username' and 'Authentication Password' property values. See Confluent Schema Registry documentation for more details. The link to said documentation is https://docs.confluent.io/current/schema-registry/security/index.html#configuring-the-rest-api-for-basic-http-authentication but does not need to be in the property descriptor description, but can be in the processor documentation. I actually don't see any details on that page about configuring digest authentication at all -- do you have a reference for Confluent Schema Registry supporting this authentication mechanism?
| @@ -52,6 +52,8 @@ | |||
| import java.util.concurrent.TimeUnit; | |||
| import java.util.stream.Collectors; | |||
| import java.util.stream.Stream; | |||
| import java.util.regex.Pattern; | |||
| import static org.apache.commons.lang3.StringUtils.trimToEmpty; | |||
There was a problem hiding this comment.
I believe our coding guidelines and checkstyle discourage static imports.
There was a problem hiding this comment.
I don't think static imports have ever been blocked by checkstyle. Is that something we need to verify as behavior?
| + "for authentication.") | ||
| .required(false) | ||
| .allowableValues("DIGEST", "BASIC") | ||
| .build(); |
There was a problem hiding this comment.
We should probably add logic in the #customValidate() method to ensure that if the username and password are present, the auth type property is set and valid as well. Performing the validation earlier improves the user experience and helps with debugging.
|
Just wanted to check up on this PR. We are using confluent and would like to use nifi, but looks like authentication is required. Is this PR ready to be merged. Looks like someone has tested and appears to be working? |
Signed-off-by: Pierre Villard <pierre.villard.fr@gmail.com> This closes apache#4743. This closes apache#4508. This closes apache#4224.
Signed-off-by: Pierre Villard <pierre.villard.fr@gmail.com> This closes apache#4743. This closes apache#4508. This closes apache#4224.
Description of PR
Allow schema registry controller to connect to schema registries requiring Basic/Digest HTTP Auth.
Most of the code was written by Lakshmi Srinivas and posted as a patch under https://issues.apache.org/jira/browse/NIFI-6576, I only fixed two string comparisons.
Fixes NIFI-6576
I'm still having trouble running the tests locally and will keep trying.
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
master)?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
squashor use--forcewhen pushing to allow for clean monitoring of changes.For code changes:
mvn -Pcontrib-check clean installat the rootnififolder?LICENSEfile, including the mainLICENSEfile undernifi-assembly?NOTICEfile, including the mainNOTICEfile found undernifi-assembly?.displayNamein addition to .name (programmatic access) for each of the new properties?For documentation related changes:
Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.