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

[Ade Lee]

Made fix 1 below and pushed 315-318 to master.

After much discussion with Endi, figured out how best to refactor 319.
Will resubmit as 320.

Thanks,
Ade
 
On Thu, 2016-06-02 at 14:10 -0500, Endi Sukma Dewata wrote:
> 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.
>