[Freeipa-devel] [PATCH] 25 Create Tool for Enabling Disabling Managed Entry
Martin Kosek
mkosek at redhat.com
Wed Sep 21 07:26:43 UTC 2011
On Tue, 2011-09-20 at 16:16 +0000, JR Aquino wrote:
> 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
> >
>
ACK. Pushed to master. Merged to ipa-2-1 branch and pushed as well.
Martin
More information about the Freeipa-devel
mailing list