[Pki-devel] [PATCH] 315-319 KRA realm related patches

Endi Sukma Dewata edewata at redhat.com
Thu Jun 2 19:10:26 UTC 2016


On 6/2/2016 8:51 AM, Ade Lee wrote:
> And now with the patches ..
>
> On Thu, 2016-06-02 at 09:50 -0400, Ade Lee wrote:
>> Patch descriptions (in reverse order).
>>
>> The final patch will need some discussion.  Please review,
>>
>> Ade

Some comments:

1. In SrchKey and SrchKeyForRecovery the check probably should have been 
like this (no closing paren):

   if (filter.contains("(realm=")) {

2. If a realm is specified, I suppose it won't match any of the VLV 
indexes. Do the queries still work correctly without a matching VLV?

3. Could you document how to run the upgrade command in this page?
http://pki.fedoraproject.org/wiki/Database_Upgrade_for_PKI_10.3.x

Note that there are still some manual upgrade procedures that also need 
to be executed as documented in that page.

4. In PKISubsystem.open_database() can we use bind_dn and bind_password 
parameter names instead of user and password (it's a DN not a user ID)? 
Also in DBUpgrade should we use -D, -w, and -Z for consistency with DS 
tools?

5. The gnu_getopt() is called with "server_id=". I think it should be 
"server-id=". But since it actually contains the DS instance name maybe 
we should use "ds-instance=" instead?

6. In the DBUpgrade.modify_kra_vlv() the os.unlink(ldif_file) probably 
should be moved into the finally clause above it to ensure the temporary 
file is deleted in all cases. Alternatively the code could be written 
like this:

   with NamedTemporaryFile() as ldif_file:
       # do everything with the ldif_file here

7. In DBUpgrade.modify_schema() why not use subsystem.open_database() 
instead of ldapmodify? That will ensure it uses the same mechanism to 
connect to the LDAP server.

8. Also the DBUpgrade.modify_schema() is ignoring all schema update 
failures. I think we can only ignore the case where the new schema is 
already added. In other cases the upgrade should fail since the 
subsequent upgrades may depend on it.

9. I think DBUpgrade.create_realm_index() should only execute if there's 
a KRA subsystem in the instance. The code right now creates the index in 
the first subsystem's database which may not be a KRA.

   subsystem = instance.subsystems[0]

10. The DBUpgrade uses db2index.pl to rebuild the indexes which means 
the LDAP server must run locally. To support remote LDAP server I think 
we need to use reindex task. We have indextasks.ldif and vlvtasks.ldif 
that can be used for this. See also: 
http://pki.fedoraproject.org/wiki/DS_Database_Indexes

11. The DBUpgrade should not ignore reindex failures since subsequent 
upgrades may depend on it.

12. There's a typo in the last patch's commit description.

-- 
Endi S. Dewata




More information about the Pki-devel mailing list