On Sep 20, 2011, at 4:13 AM, Martin Kosek wrote:

> On Fri, 2011-09-16 at 16:37 +0000, JR Aquino wrote:
>> 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
>> #
>> 
> 
> Looks good. I found just one more bug:
> 
> # ipa-managed-entries -e foo status
> Traceback (most recent call last):
>  File "/usr/sbin/ipa-managed-entries", line 236, in <module>
>    sys.exit(main())
>  File "/usr/sbin/ipa-managed-entries", line 152, in main
>    ['originfilter'],
>  File "/usr/lib/python2.7/site-packages/ipaserver/ipaldap.py", line 204, in 
> inner
>    return f(*args, **kargs)
>  File "/usr/lib64/python2.7/site-packages/ldap/ldapobject.py", line 502, in 
> search_s
>    return 
> self.search_ext_s(base,scope,filterstr,attrlist,attrsonly,None,None,timeout=self.timeout)
>  File "/usr/lib/python2.7/site-packages/ipaserver/ipaldap.py", line 204, in 
> inner
>    return f(*args, **kargs)
>  File "/usr/lib64/python2.7/site-packages/ldap/ldapobject.py", line 496, in 
> search_ext_s
>    return self.result(msgid,all=1,timeout=timeout)[1]
>  File "/usr/lib/python2.7/site-packages/ipaserver/ipaldap.py", line 181, in 
> inner
>    objtype, data = f(*args, **kargs)
>  File "/usr/lib64/python2.7/site-packages/ldap/ldapobject.py", line 422, in 
> result
>    res_type,res_data,res_msgid = self.result2(msgid,all,timeout)
>  File "/usr/lib/python2.7/site-packages/ipaserver/ipaldap.py", line 204, in 
> inner
>    return f(*args, **kargs)
>  File "/usr/lib64/python2.7/site-packages/ldap/ldapobject.py", line 426, in 
> result2
>    res_type, res_data, res_msgid, srv_ctrls = self.result3(msgid,all,timeout)
>  File "/usr/lib/python2.7/site-packages/ipaserver/ipaldap.py", line 204, in 
> inner
>    return f(*args, **kargs)
>  File "/usr/lib64/python2.7/site-packages/ldap/ldapobject.py", line 432, in 
> result3
>    ldap_result = self._ldap_call(self._l.result3,msgid,all,timeout)
>  File "/usr/lib/python2.7/site-packages/ipaserver/ipaldap.py", line 204, in 
> inner
>    return f(*args, **kargs)
>  File "/usr/lib64/python2.7/site-packages/ldap/ldapobject.py", line 96, in 
> _ldap_call
>    result = func(*args,**kwargs)
> ldap.NO_SUCH_OBJECT: {'matched': 'cn=definitions,cn=managed 
> entries,cn=etc,dc=idm,dc=lab,dc=bos,dc=redhat,dc=com', 'desc': 'No such 
> object'}

Thanks, It has now been corrected.

> I think we should handle this situation better and report a nice error
> message.
> 
> Martin
> 

Attachment: binmumBjIrrUJ.bin
Description: freeipa-jraquino-0025-Create-Tool-for-Enabling-Disabling-Managed-Entries.patch

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to