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

[JR Aquino]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jraquino-0025-Create-Tool-for-Enabling-Disabling-Managed-Entries.patch
Type: application/octet-stream
Size: 24651 bytes
Desc: freeipa-jraquino-0025-Create-Tool-for-Enabling-Disabling-Managed-Entries.patch
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20110915/f1841eff/attachment.obj>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: ATT00001.txt
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20110915/f1841eff/attachment.txt>