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

[JR Aquino]

On Sep 16, 2011, at 2:11 AM, Martin Kosek wrote:

> 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

This typo is now corrected

> 
> 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

The output is now corrected.

> # 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

I have now also corrected the --list / -e / --entry to support/display shorthand for the managed entries instead of the full DN.

# ipa-managed-entries --list
Available Managed Entry Definitions:
UPG Definition
NGP Definition
#

# ipa-managed-entries --entry="UPG Definition" status
Plugin Enabled
#

-------------- 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: 24879 bytes
Desc: freeipa-jraquino-0025-Create-Tool-for-Enabling-Disabling-Managed-Entries.patch
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20110916/c0a36b88/attachment.obj>