[Freeipa-devel] [PATCH] 25 Create Tool for Enabling Disabling Managed Entry
JR Aquino
JR.Aquino at citrix.com
Tue Sep 20 16:16:59 UTC 2011
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
>
-------------- 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: 24982 bytes
Desc: freeipa-jraquino-0025-Create-Tool-for-Enabling-Disabling-Managed-Entries.patch
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20110920/bec979fc/attachment.obj>
More information about the Freeipa-devel
mailing list