New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
NIFI-8312: Support PKCS12 and BCFKS truststores in Atlas reporting task #4893
Conversation
Generate ssl-client.xml on NiFi side in order to be able to configure non-JKS truststores.
The credential store is not related to Atlas REST API SSL connection but would eliminate a warning from Atlas Kafka client. Removed because it caused test failure on Windows due to missing Hadoop native libraries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution @turcsanyip. This looks like a straightforward improvement. See individual comments for a couple minor suggestions on logging, otherwise looks good.
try { | ||
Files.deleteIfExists(path); | ||
} catch (Exception e) { | ||
getLogger().error("Unable to delete " + path, e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommend changing the message to use the placeholder syntax and indicating a little more detail about the file:
getLogger().error("Unable to delete " + path, e); | |
getLogger().error("Unable to delete SSL Client Configuration File {}", path, e); |
try (FileWriter fileWriter = new FileWriter(sslClientXmlFile)) { | ||
configuration.writeXml(fileWriter); | ||
} catch (Exception e) { | ||
getLogger().error("Unable to create SSL config file: " + sslClientXmlFile, e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mention in other comment for parameterized logging:
getLogger().error("Unable to create SSL config file: " + sslClientXmlFile, e); | |
getLogger().error("Unable to create SSL Client Configuration File {}", sslClientXmlFile, e); |
@@ -704,33 +699,47 @@ private void setAtlasSSLConfig(Properties atlasProperties, ConfigurationContext | |||
boolean isAtlasApiSecure = urls.stream().anyMatch(url -> url.toLowerCase().startsWith("https")); | |||
atlasProperties.put(ATLAS_PROPERTY_ENABLE_TLS, String.valueOf(isAtlasApiSecure)); | |||
|
|||
// ssl-client.xml must be deleted, Atlas will not regenerate it otherwise | |||
Path credStorePath = new File(confDir, CRED_STORE_FILENAME).toPath(); | |||
Files.deleteIfExists(credStorePath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you anticipate any problems if the Credentials Store file remains on the file system since it will no longer be deleted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaving the cred store file behind should not be a problem because we do not pass cert.stores.credential.provider.path
property (pointing to the file) to Atlas any more so it will not use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the confirmation.
@exceptionfactory Thanks for the review. I modified the log statements according to your suggestions. Thanks for that. |
- Generate ssl-client.xml on NiFi side in order to be able to configure non-JKS truststores. - Close FileOutputStream in tests to prevent error during clean-up. - Removed generating Hadoop Credential Store. - The credential store is not related to Atlas REST API SSL connection but would eliminate a warning from Atlas Kafka client. Removed because it caused test failure on Windows due to missing Hadoop native libraries. This closes apache#4893 Signed-off-by: David Handermann <exceptionfactory@apache.org>
- Generate ssl-client.xml on NiFi side in order to be able to configure non-JKS truststores. - Close FileOutputStream in tests to prevent error during clean-up. - Removed generating Hadoop Credential Store. - The credential store is not related to Atlas REST API SSL connection but would eliminate a warning from Atlas Kafka client. Removed because it caused test failure on Windows due to missing Hadoop native libraries. This closes apache#4893 Signed-off-by: David Handermann <exceptionfactory@apache.org>
Generate ssl-client.xml on NiFi side in order to be able to configure non-JKS truststores.
https://issues.apache.org/jira/browse/NIFI-8312
Thank you for submitting a contribution to Apache NiFi.
Please provide a short description of the PR here:
Description of PR
Enables X functionality; fixes bug NIFI-YYYY.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
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
main
)?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
squash
or use--force
when pushing to allow for clean monitoring of changes.For code changes:
mvn -Pcontrib-check clean install
at the rootnifi
folder?LICENSE
file, including the mainLICENSE
file undernifi-assembly
?NOTICE
file, including the mainNOTICE
file found undernifi-assembly
?.displayName
in 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 GitHub Actions CI for build issues and submit an update to your PR as soon as possible.