[Freeipa-devel] [PATCH] 25 Create Tool for Enabling Disabling Managed Entry

[Martin Kosek]

On Thu, 2011-09-15 at 17:25 +0000, JR Aquino wrote:
> On Sep 15, 2011, at 1:47 AM, Martin Kosek wrote:
> 
> > On Thu, 2011-09-15 at 00:47 +0000, JR Aquino wrote:
> >> On Jul 22, 2011, at 7:05 AM, Martin Kosek wrote:
> > 
> >>> 5) I was thinking if there is a better solution to enabling/disabling of
> >>> the plugin. Likes setting something like "managedEntryEnabled" attribute
> >>> to on/off as we do with compat plugin. Current concept with disabling
> >>> the definition by damaging the originFilter and then restoring it from
> >>> an LDIF seems a bit awkward to me.
> >> 
> >> This has been completely changed:
> >> Instead of looking to ldif files, an ldap look up is now performed to dynamically list the available managed entries.
> > 
> > Now we are talking :-) I like the new approach.
> 
> <high five>
> 
> > 
> > I have reviewed your patch, basic functionality looks good. But I still
> > have few (nitpicking) comments:
> > 
> > 1) There are parts from the previous file that are no longer needed
> > since you switched to different approach:
> > 
> > +import os
> > 
> > +    from ipaserver.install.ldapupdate import LDAPUpdate, BadSyntax
> > 
> > +    import StringIO
> > 
> > +    import ldif
> > 
> > +except BadSyntax, e:
> > +    print "There is a syntax error in this update file:"
> > +    print "  %s" % e
> > +    sys.exit(1)
> 
> Removed
> 
> > 
> > 2) I saw few whitespace errors on following lines of the patch: 419, 433
> > and 453
> 
> Fixed whitespace errors
> 
> > 
> > 3) Output of the --list method is confusing:
> > 
> > # ipa-managed-entries --list
> > Directory Manager password: 
> > 
> > Available Managed Entry Plugins:
> > cn=upg definition,cn=definitions,cn=managed
> > entries,cn=etc,dc=idm,dc=lab,dc=bos,dc=redhat,dc=com
> > cn=ngp definition,cn=definitions,cn=managed
> > entries,cn=etc,dc=idm,dc=lab,dc=bos,dc=redhat,dc=com
> > 
> > You must specify a managed entry definition     <<<
> > # echo $?
> > 1        <<<
> > 
> > a) I shouldn't be asked to specify a managed entry definition for --list
> 
> Fixed
> 
> > b) The listing was successful, so we shouldn't return error code
> 
> Corrected error code
> 
> > 
> > 4) Return code for disabling an already disabled entry should be 2
> > (according to man pages):
> > 
> > # ipa-managed-entries -e 'cn=upg definition,cn=definitions,cn=managed entries,cn=etc,dc=idm,dc=lab,dc=bos,dc=redhat,dc=com' disable
> > Directory Manager password: 
> > 
> > Plugin already disabled
> > # echo $?
> > 0
> 
> Fixed error code
> 
> > 
> > 5) Strange is, that enabling a disabled plugin gives me return code 2:
> > # ipa-managed-entries -e 'cn=upg definition,cn=definitions,cn=managed entries,cn=etc,dc=idm,dc=lab,dc=bos,dc=redhat,dc=com' enable
> > Directory Manager password: 
> > 
> > Enabling Plugin
> > # echo $?
> > 2
> > 
> > Return codes for these actions should fit the man pages.
> 
> Fixed error code
> 
> > 
> > 6) I would improve working with LDAP filters, current solution is error
> > prone. Try disabling&enabling NGP Defition, we end up with this
> > originFilter:
> > 
> > originfilter: (&(objectclass=ipahostgroup))
> > 
> > I think the cleanest solution would be to use ldap.make_filter and
> > ldap.combine_filters functions to play with these filter. You can
> > inspire yourself in this example I wrote for DNS plugin:
> > 
> > rev_zone_filter = ldap.make_filter(search_kw, rules=ldap.MATCH_NONE, exact=False,
> >                    trailing_wildcard=False)
> > filter = ldap.combine_filters((rev_zone_filter, filter), rules=ldap.MATCH_ALL)
> 
> Rob and you addressed this in the mailing list.
> For the record, I do agree that we are lacking a method for reading and modifying existing ldap filters.
> We will continue with the simple string method here for this iteration.
> 
> > 
> > 7) Entering Directory Manager every time may be a bit tedious. Could we
> > use current Kerberos credentials and fall-back to asking Directory
> > Manager password if it doesn't work? Its already done this way in
> > ipa-replica-manage for example.
> > 
> > We could fix this, however, as an enhancement in another patch.
> 
> Fixed. We now will use gssapi if available, and prompt for password if there is no ticket.
> 
> > 
> > 8) Man page - please use the new united FreeIPA man page header. Instead
> > of 
> > 
> > +.TH "ipa-managed-entries" "1" "Sept 15 2011" "freeipa" ""
> > 
> > use:
> > 
> > +.TH "ipa-managed-entries" "1" "Sept 15 2011" "FreeIPA" "FreeIPA Manual
> > Pages"
> 
> Fixed
> 
> > 
> > 
> > 9) Man page - comma is missing for --list option:
> > 
> > +\fB\-l\-\-list\fR
> > 
> 
> Fixed
> 
> > 
> > 10) install/po/Makefile.in should be updated to: there is still
> > reference to ipa-host-net-manage and ipa-managed-entries reference is
> > missing
> 
> Fixed
> 

Great, most bugs are fixed. I only saw these 2 minor bugs. If those are
fixed, I think we can ack&push.

1) Man pages: --list option is still not right, formating is wrong
+\fB\-l\fR, -\-list\fR

2) Enable action is missing a notice for the user, like the disable
action has:

# ipa-managed-entries -e 'cn=UPG Definition,cn=Definitions,cn=Managed Entries,cn=etc,dc=idm,dc=lab,dc=bos,dc=redhat,dc=com' disable
Disabling Plugin

# ipa-managed-entries -e 'cn=UPG Definition,cn=Definitions,cn=Managed Entries,cn=etc,dc=idm,dc=lab,dc=bos,dc=redhat,dc=com' enable

Martin