NIFI-16260 - Add a Database Password Provider for GCP CloudSQL - #11604
NIFI-16260 - Add a Database Password Provider for GCP CloudSQL#11604pvillard31 wants to merge 3 commits into
Conversation
exceptionfactory
left a comment
exceptionfactory
left a comment
There was a problem hiding this comment.
Thanks for putting together this new feature @pvillard31.
The basic implementation looks good, but I noted a few questions and concerns around the validation and testing.
Although the detailed introspection of the Connection URL can certainly help catch issues, it adds quite a bit of implementation and test code. I'm particularly concerned about the weight of the "legacy" parameters, which seem unnecessary to check. I recommend scoping down some of the verification steps as one way forward.
|
Thanks for the review. I simplified the implementation substantially and removed the database-specific validation from the password provider. The provider now focuses only on obtaining, scoping, caching, refreshing, and returning the Cloud SQL IAM token. JDBC URL, driver, username, and TLS configuration remain the responsibility of the DBCP service and JDBC driver. I also removed the exception stack-trace introspection and reduced the tests to the essential credential lifecycle, refresh, verification, and failure-handling behavior. The focused tests, complete GCP processor module suite, Checkstyle, RAT, and PMD all pass. I also completed final live validation with both PostgreSQL and MySQL. |
exceptionfactory
left a comment
exceptionfactory
left a comment
There was a problem hiding this comment.
Thanks for making the adjustments @pvillard31, this looks close to completion. I highlighted a few remaining recommendations.
| } | ||
|
|
||
| @Override | ||
| public List<ConfigVerificationResult> verify(final ConfigurationContext context, final ComponentLog verificationLogger, |
There was a problem hiding this comment.
This method has a number of try-catch blocks and a large number of returns. It would be helpful to consolidate to a single return. It may be helpful to collapse multiple exception conditions into a single one, especially since the messages can be the same.
|
|
||
| final ProcessException exception = assertThrows(ProcessException.class, () -> provider.getPassword(requestContext())); | ||
|
|
||
| assertTrue(exception.getMessage().contains("Cloud SQL IAM")); |
There was a problem hiding this comment.
It would be helpful to declare Cloud SQL IAM as a static and reuse the reference across multiple methods.
| } | ||
|
|
||
| @Test | ||
| void testAdditionalDetailsResourceDocumentsSupportedPath() throws IOException { |
There was a problem hiding this comment.
I recommend removing this test method since it is purely about documentation
|
Thanks for the review @exceptionfactory - I pushed a commit to address your feedback |
exceptionfactory
left a comment
exceptionfactory
left a comment
There was a problem hiding this comment.
Thanks for the update @pvillard31, this looks better, I noted a few additional recommendations.
| assertFalse(argValue != null && argValue.contains(value)); | ||
| } | ||
| } | ||
| assertThrowableChainDoesNotContain(logMessage.getThrowable(), value, java.util.Collections.newSetFromMap(new IdentityHashMap<>())); |
There was a problem hiding this comment.
The qualified java.util.Collections should be replaced with an import, along with other fully-qualified class references in this test class
| } | ||
|
|
||
| @Test | ||
| public void testAdditionalDetailsDocumentTargetServiceAccountVerificationSemantics() throws IOException { |
There was a problem hiding this comment.
This test should also be removed
| } | ||
|
|
||
| private void rejectIdentityPoolCredentialsOnEnable(final GoogleCredentials credentials) throws InitializationException { | ||
| if (credentials instanceof IdentityPoolCredentials) { |
There was a problem hiding this comment.
It looks like this rejects other kinds of credentials like ExternalAccountCredentials, is that intentional?
| } | ||
|
|
||
| @Override | ||
| public List<ConfigVerificationResult> verify(final ConfigurationContext context, final ComponentLog verificationLogger, |
There was a problem hiding this comment.
With the verificationLogger passed in but not used, it would be helpful to pass it down to some of the nested methods and log the exceptions thrown, instead of just returning null for verification.
2 participants
Summary
NIFI-16260 - Add a Database Password Provider for GCP CloudSQL
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