Certificate repository free vlv #4735
Conversation
Methods to find certificates using VLV indexes have been deprecated. All their usage inside the CertificateRepository have been replaced with paged search based query.
|
IPA and other CI tests using serial numbers were failing before the update. I have done a non exhaustive set of tests on a locally installed CA and it works. We should fix these tests ASAP |
There was a problem hiding this comment.
I have some comments but the changes do make sense and the KRA installation in IPA basic test completed successfully (there's another failure later but I think it's unrelated). Feel free to update/merge.
In the future maybe we can update the methods that return Enumeration to return RecordPagedList directly so we don't have to retrieve the entire search results and store it in memory.
| String ldapfilter = "(&("+CertRecord.ATTR_CERT_STATUS+"=*"+")("+CertRecord.ATTR_ID+">="+serial_low_bound+"))"; | ||
|
|
||
| CertRecordList recList = findCertRecordsInList(ldapfilter, attrs, serial_upper_bound.toString(10), "serialno", 5 * -1); | ||
| String[] attrs = {CertRecord.ATTR_ID, "objectclass"}; | ||
|
|
||
| int size = recList.getSize(); | ||
| RecordPagedList<CertRecord> certRecords = findPagedCertRecords(ldapfilter, attrs, "serialno"); |
There was a problem hiding this comment.
I'm not super familiar with the original code, but IIUC it's trying to find the last serial number by searching from the top of the range and picking the first one that is within the range. The new code seems to be searching from the bottom of the range and finding the last one that is still within the range. So it will likely be a lot slower if there's a lot of certs within the range. Is it possible to search from the top of the range using simple paged search?
There was a problem hiding this comment.
Yes, it is not efficient. The paged search is only forward but we can apply a reverse sort of the elements so the first will be the last. Actually, testing the reverse sort I noticed that the filter is not working as I was expecting. To get the correct range we have to filter using the serialno. I'll update the code.
There was a problem hiding this comment.
After additional investigation I discovered it was my fault. I tested command line (with ldapsearch) but the code convert the attribute differently so it should be OK. Only the sort order has to be modified.
| @@ -604,49 +602,35 @@ public BigInteger getLastSerialNumberInRange(BigInteger serial_low_bound, BigInt | |||
| logger.debug("CertificateRepository:getLastCertRecordSerialNo: returning " + ret); | |||
| return ret; | |||
| } | |||
There was a problem hiding this comment.
In case the range is empty we might need to reset mCounter as well:
if (modeChange && mEnableRandomSerialNumbers) {
mCounter = BigInteger.ZERO;
}
I'm not sure why the original code doesn't do this.
There was a problem hiding this comment.
Since the case when there are no records is the same to the case where the first element is outside of the range I have collapsed the two case so they are performing the same operations now.
| CertRecordList list = findCertRecordsInList(filter, null, "serialno", 10); | ||
| int size = list.getSize(); | ||
| Enumeration<CertRecord> e = list.getCertRecords(0, size - 1); | ||
| while (e.hasMoreElements()) { | ||
| CertRecord rec = e.nextElement(); |
There was a problem hiding this comment.
The original code seems to be very inefficient (processing all records in the database), more complicated than it needs to be, and doesn't seem to be used anyway. I'd suggest to just remove this code.
There was a problem hiding this comment.
It seems not used but I am keeping for the moment
| */ | ||
| @Deprecated(since = "v11.6.0", forRemoval = true) |
There was a problem hiding this comment.
In the existing code we usually don't include the v prefix in the @Deprecated annotation.
| // from is not integer | ||
| ldapfilter.append("(") | ||
| .append(CertRecord.ATTR_ID) | ||
| .append("<") |
There was a problem hiding this comment.
Officially LDAP does not support a < operator, so (a<b) needs to be written as (!(b>=a)), but I don't know whether DS actually supports <.
| if (followSerial.compareTo(serial_upper_bound) > 0 && | ||
| curSerial.compareTo(serial_upper_bound) < 0) { |
There was a problem hiding this comment.
IIUC the upper bound should be considered within the range, so this probably should be:
if (followSerial.compareTo(serial_upper_bound) > 0 &&
curSerial.compareTo(serial_upper_bound) <= 0) {
| if (serial.compareTo(serial_low_bound) > 0 && | ||
| serial.compareTo(serial_upper_bound) <= 0) { |
There was a problem hiding this comment.
I think the lower bound is inclusive too, so it should be serial.compareTo(serial_low_bound) >= 0.
To be more efficient the last used serial number in a range is identified from the upper range limit and getting the first element in the direction of the lower limit.
When server sort and paged controls were both used only the last was applied ignoring the sort control because applying multiple control get overwritten. The code has been modified to apply an array of controls when both are present.
fmarco76
force-pushed
the
CertificateRepository-FreeVLV
branch
from
8af6fe4 to
ccd5b3f
Compare
|
|
I have updated the code following all the comment. The CI shows IPA errors (https://github.com/dogtagpki/pki/actions/runs/9029961345/job/24813561737?pr=4735) and few others which are unrelated with this PR so I am merging. @edewata Thanks! |
Updated the
CertificateRepositoryoperations to work with paged search. The previous find method have been deprecated and their used inside the class has been replaced.