-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Log exception on keystore build for custom certificate #6394
Conversation
@blueorangutan package |
@nvazquez a Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 3419 |
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.
code LGTM
@blueorangutan test |
@rohityadavcloud a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
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.
LGTM
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.
it is not appropriate to add exception stack traces to warn level logging. If the stack traces are needed add a debug statement with the stack trace but don´t add it to warn level.
@@ -109,15 +109,15 @@ public byte[] getKeystoreBits(String name, String aliasForCertificateInStore, St | |||
try { | |||
return CertificateHelper.buildAndSaveKeystore(certs, storePassword); | |||
} catch (KeyStoreException e) { | |||
s_logger.warn("Unable to build keystore for " + name + " due to KeyStoreException"); | |||
s_logger.warn("Unable to build keystore for " + name + " due to KeyStoreException", 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.
Doing like this,
s_logger.warn("Unable to build keystore for " + name + " due to KeyStoreException", e); | |
s_logger.warn(String.format("Unable to build keystore for %s due to %s", name, e.getClass().getSimpleName())); | |
if (s_logger.isDebugEnabled()) { | |
s_logger.debug(String.format("Unable to build keystore for %s due to %s", name, e.getClass().getSimpleName()), e); | |
} |
the catch clauses can be unified to one
} catch (KeyStoreException | CertificateException | InvalidKeySpecException | IOException 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.
Thanks, done
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.
@DaanHoogland
I have a question, is the check if (s_logger.isDebugEnabled()) {
necessary ?
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.
not if the string is generated separately anyway. I think in this case (as @nvazquez implemented it) we could leave it out, @weizhouapache
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.
OK. thanks @DaanHoogland
@blueorangutan package |
@nvazquez a Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 3435 |
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.
clgtm
Trillian test result (tid-4201)
|
Description
If CloudStack fails to build and save keystore using the keystone certificate then it silently fails with just the following line:
This PR adds the exception to the logger
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?