NIFI-16367 - Add Azure Entra Database Password Provider - #11693
pvillard31 wants to merge 2 commits into
Conversation
exceptionfactory
left a comment
exceptionfactory
left a comment
There was a problem hiding this comment.
Thanks for putting this together @pvillard31, the implementation looks good in general, just noted a couple minor recommendations.
| | Setting | Value | | ||
| |---|---| | ||
| | Driver Class Name | `org.postgresql.Driver` | | ||
| | JDBC URL | `jdbc:postgresql://<SERVER>.postgres.database.azure.com:5432/<DATABASE>?sslmode=require` | |
There was a problem hiding this comment.
Recommend using verify-full in the example to provide an example of the stronger option as the default.
| | JDBC URL | `jdbc:postgresql://<SERVER>.postgres.database.azure.com:5432/<DATABASE>?sslmode=require` | | |
| | JDBC URL | `jdbc:postgresql://<SERVER>.postgres.database.azure.com:5432/<DATABASE>?sslmode=verify-full` | |
| final AzureCredentialsService configuredCredentialsService = azureCredentialsService; | ||
| if (configuredCredentialsService == null) { | ||
| throw new ProcessException(FAILED_PASSWORD_MESSAGE); | ||
| } | ||
|
|
||
| final TokenCredential credential; | ||
| try { | ||
| credential = configuredCredentialsService.getCredentials(); | ||
| } catch (final RuntimeException e) { | ||
| throw new ProcessException(FAILED_PASSWORD_MESSAGE); | ||
| } | ||
|
|
||
| if (credential == null) { | ||
| throw new ProcessException(FAILED_PASSWORD_MESSAGE); | ||
| } | ||
|
|
||
| final AccessToken accessToken; | ||
| try { | ||
| accessToken = credential.getToken(createTokenRequestContext()).block(); | ||
| } catch (final RuntimeException e) { | ||
| throw new ProcessException(FAILED_PASSWORD_MESSAGE); | ||
| } | ||
|
|
||
| if (!isValidAccessToken(accessToken)) { | ||
| throw new ProcessException(FAILED_PASSWORD_MESSAGE); | ||
| } |
There was a problem hiding this comment.
All of these failures result in the same message, at minimum, it would be helpful to distinguish between credentials retrieval issues and Access Token issues.
|
|
||
| final AccessToken accessToken; | ||
| try { | ||
| accessToken = credential.getToken(createTokenRequestContext()).block(); |
There was a problem hiding this comment.
It looks like this block() call can hang, is that possible, or should a timeout be added?
| return new TokenRequestContext().addScopes(OSS_RDBMS_SCOPE); | ||
| } | ||
|
|
||
| private boolean isValidAccessToken(final AccessToken accessToken) { |
There was a problem hiding this comment.
Is this method necessary? It seems very unlikely that that the Access Token would come back blank. On the other hand, a Controller Service implementation could do something wrong. I recommend keeping the null and blank checks, but skipping the expiration check, since that would be handled on the remote side.
|
Thanks @exceptionfactory - pushed a commit to address your comments |
2 participants
Summary
NIFI-16367 - Add Azure Entra Database Password Provider
Screenshots of the live testing I did can be provided if needed to help with the review.
Tracking
Please complete the following tracking steps prior to pull request creation.
Issue Tracking
Pull Request Tracking
NIFI-00000NIFI-00000VerifiedstatusPull Request Formatting
mainbranchVerification
Please indicate the verification steps performed prior to pull request creation.
Build
./mvnw clean install -P contrib-checkLicensing
LICENSEandNOTICEfilesDocumentation