Skip to content
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

Adding useful OCSP debug messages #4503

Merged
merged 1 commit into from
Jul 19, 2023
Merged

Conversation

ladycfu
Copy link
Contributor

@ladycfu ladycfu commented Jul 18, 2023

This patch fixed and added some useful information in the OCSP area. During investigation setup procedure for ticket RHCS-4264, some debug messages can be confusing. In addition, more information should be shared for administrators to understand why things are not working as expected.

for https://issues.redhat.com/browse/RHCS-4264

Copy link
Contributor

@edewata edewata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some minor comments, but feel free to update/merge.

mCRLs.put(caCert, crl);
logger.debug("LDAPStore: updateCRLHash: mCRLs size=", Integer.toString(mCRLs.size()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be written like this:

logger.debug("LDAPStore: updateCRLHash: mCRLs size={}", mCRLs.size());

or this:

logger.debug("LDAPStore: updateCRLHash: mCRLs size=" + mCRLs.size());

https://www.slf4j.org/api/org/slf4j/Logger.html#debug-java.lang.String-java.lang.Object-
https://www.slf4j.org/api/org/slf4j/helpers/MessageFormatter.html

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OH, I know my mistake now. It's '+' not ','. Silly me! Thanks! I'll make correction.

@@ -399,11 +405,11 @@ public SingleResponse processRequest(Request req) throws Exception {
}

if (theCert == null) {
throw new Exception("Missing issuer certificate");
throw new Exception("Issuer certificate not found/served in mCRL");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exception messages are meant for users, not developers, so I'd suggest not to use an internal variable name like mCRL.

}

if (theCRL == null) {
throw new Exception("Missing CRL data");
throw new Exception("Missing CRL data for issuing CA:" + theCert.getSubjectDN().toString());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The .toString() is redundant.

mCRLs.put(caCert, crl);
logger.debug("LDAPStore: updateCRLHash: mCRLs size=", Integer.toString(mCRLs.size()));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw, I originally simply said logger.debug("LDAPStore: updateCRLHash: mCRLs size=", mCRLs.size()); but it printed out nothing. After changing the code like above, I still got nothing in the debug log. It shows the following in the debug log: LDAPStore: updateCRLHash: mCRLs size=
Any idea?

mCRLs.put(caCert, crl);
logger.debug("LDAPStore: updateCRLHash: mCRLs size=", Integer.toString(mCRLs.size()));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OH, I know my mistake now. It's '+' not ','. Silly me! Thanks! I'll make correction.

This patch fixed and added some useful information in the OCSP area.
During investigation setup procedure for ticket RHCS-4264, some debug
messages can be confusing.  In addition, more information should be
shared for administrators to understand why things are not working as
expected.
It is also useful for RHCS-4261

for RHCS-4264 and RHCS-4261
@ladycfu ladycfu merged commit 2b8c802 into dogtagpki:v10.13 Jul 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants