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

Convert CertStatusUpdate from VLV to paged search #4708

Merged
merged 2 commits into from
Mar 28, 2024

Conversation

fmarco76
Copy link
Member

Note: the VLV update was done on ordered elements. With the paged search elements are not ordered to avoid extra work since the order it is not relevant for the update.

Note: the VLV update was done on ordered elements. With the paged search
elements are not ordered to avoid extra work since the order it is not
relevant for the update.
@fmarco76 fmarco76 requested a review from edewata March 26, 2024 14:34
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 comments for possible improvements, but they can be addressed separately later if you want. The changes look good. Feel free to update/merge.

Comment on lines 94 to 98
Date notBefore = certRecord.getNotBefore();
if (notBefore.after(now)) {
logger.debug("CertStatusUpdateTask: Cert " + certID.toHexString() + " not yet valid");
logger.debug("CertStatusUpdateTask: Cert {} not yet valid", certID.toHexString());
continue;
}
Copy link
Contributor

@edewata edewata Mar 26, 2024

Choose a reason for hiding this comment

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

This is an existing code, but do you think we still need this check? IIUC getInvalidCertsByNotBeforeDate() will return invalid certs that should have been valid at the provided time, so maybe it's not necessary to check again.

continue;
}

logger.debug("CertStatusUpdateTask: Cert " + certID.toHexString() + " has become valid");
logger.debug("CertStatusUpdateTask: Cert {} has become valid", certID.toHexString());
list.add(certID);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is also an existing code, but instead of storing all cert IDs into a list (which could be large) we probably can just update the cert immediately with this code:

repository.updateStatus(certID, CertRecord.STATUS_VALID);

Comment on lines 83 to 86
if (!certRecIterator.hasNext()) {
logger.debug("CertStatusUpdateTask: No invalid certs");
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

If we don't use a list of certIDs anymore, we won't need this check since the while loop should be sufficient.

Comment on lines 131 to 135
Date notAfter = certRecord.getNotAfter();
if (notAfter.after(now)) {
logger.debug("CertStatusUpdateTask: Cert " + certID.toHexString() + " not yet expired");
logger.debug("CertStatusUpdateTask: Cert {} not yet expired", certID.toHexString());
continue;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

continue;
}

logger.debug("CertStatusUpdateTask: Cert " + certID.toHexString() + " has become expired");
logger.debug("CertStatusUpdateTask: Cert {} has become expired", certID.toHexString());
list.add(certID);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

Comment on lines 120 to 123
if (!certRecIterator.hasNext()) {
logger.debug("CertStatusUpdateTask: No invalid certs");
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

Comment on lines 169 to 173
Date notAfter = certRecord.getNotAfter();
if (notAfter.after(now)) {
logger.debug("CertStatusUpdateTask: Cert " + certID.toHexString() + " not yet expired");
logger.debug("CertStatusUpdateTask: Cert {} not yet expired", certID.toHexString());
continue;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

continue;
}

logger.debug("CertStatusUpdateTask: Cert " + certID.toHexString() + " has become expired");
logger.debug("CertStatusUpdateTask: Cert {} has become expired", certID.toHexString());
list.add(certID);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above, but we will need to update the issuing points too:

repository.updateStatus(certID, CertRecord.STATUS_REVOKED_EXPIRED);

for (CRLIssuingPoint issuingPoint : engine.getCRLIssuingPoints()) {
    issuingPoint.addExpiredCert(certID.toBigInteger());
}

Comment on lines 168 to 161
if (totalSize <= 0) {
if (!certRecIterator.hasNext()) {
logger.debug("CertStatusUpdateTask: No invalid certs");
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

Copy link

sonarcloud bot commented Mar 28, 2024

Removing useless temporary lists needed to work with VLV
@fmarco76 fmarco76 force-pushed the CertStatusUpdateTaskDropVLV branch from 9d2dafc to 2746200 Compare March 28, 2024 10:42
@fmarco76
Copy link
Member Author

@edewata Thanks! I have updated the CertStatusUpdateTask following all your comments and this has simplified the code.

@fmarco76 fmarco76 merged commit 3942d22 into dogtagpki:master Mar 28, 2024
20 of 27 checks passed
@fmarco76
Copy link
Member Author

@edewata IPA tests are becoming unstable. They fail in different places but if I try locally they works. Maybe we have a synchronization problem! Not sure when this issue was introduced but I started to observe few weeks ago

@fmarco76 fmarco76 deleted the CertStatusUpdateTaskDropVLV branch March 28, 2024 10:52
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