[Freeipa-devel] Re: [PATCHES] Make plugins use baseldap classes.
Pavel Zuna
pzuna at redhat.com
Fri Oct 2 16:02:17 UTC 2009
Pavel Zuna wrote:
> Rob Crittenden wrote:
>> Pavel Zuna wrote:
>>> Pavel Zůna wrote:
>>>> Rob Crittenden wrote:
>>>>> Rob Crittenden wrote:
>>>>>> Pavel Zuna wrote:
>>>>>>> Rob Crittenden wrote:
>>>>>>>> Pavel Zůna wrote:
>>>>>>>>> This is a series of patches that depends on patches:
>>>>>>>>> - Improve attribute printing in the CLI.
>>>>>>>>> - Improve ipalib.plugins.baseldap classes.
>>>>>>>>>
>>>>>>>>> All plugins are converted to extend baseldap classes. This
>>>>>>>>> makes things more consistent, fixes some general bugs (with
>>>>>>>>> return values for example) and it also makes plugins easier to
>>>>>>>>> maintain (as it removes a lot of duplicate code).
>>>>>>>>>
>>>>>>>>> Because baseldap classes have features that enable us to define
>>>>>>>>> relationships between plugins, I thought it might be best to
>>>>>>>>> submit all of the conversions at once and have all the
>>>>>>>>> relationships fully defined.
>>>>>>>>>
>>>>>>>>> Affected plugins:
>>>>>>>>> config
>>>>>>>>> user
>>>>>>>>> host
>>>>>>>>> service
>>>>>>>>> group
>>>>>>>>> hostgroup
>>>>>>>>> netgroup
>>>>>>>>> rolegroup
>>>>>>>>> taskgroup
>>>>>>>>> pwpolicy
>>>>>>>>>
>>>>>>>>> There's also a patch that fixes all unit tests.
>>>>>>>>>
>>>>>>>>> Jenny, I included you to Cc, because the patch introduces a lot
>>>>>>>>> of changes to the UI (and you're probably not going to like me
>>>>>>>>> for this).
>>>>>>>>>
>>>>>>>>> Each command extending the LDAP* base classes now has a --raw
>>>>>>>>> option. Without it, data from LDAP is formated and translated
>>>>>>>>> to human readable. For example:
>>>>>>>>>
>>>>>>>>> # ipa user-show admin --all
>>>>>>>>> ----------
>>>>>>>>> user-show:
>>>>>>>>> ----------
>>>>>>>>> User: admin
>>>>>>>>> user id: admin
>>>>>>>>> full name: Administrator
>>>>>>>>> last name: Administrator
>>>>>>>>> home directory: /home/admin
>>>>>>>>> login shell: /bin/bash
>>>>>>>>> uid number: 999
>>>>>>>>> gid number: 1001
>>>>>>>>> gecos: Administrator
>>>>>>>>> kerberos principal: admin at PZUNA
>>>>>>>>> last password change: 20090904122852Z
>>>>>>>>> password expiration: 20091203122852Z
>>>>>>>>> member of groups: admins
>>>>>>>>>
>>>>>>>>> # ipa user-show admin --all --raw
>>>>>>>>> ----------
>>>>>>>>> user-show:
>>>>>>>>> ----------
>>>>>>>>> dn: uid=admin,cn=users,cn=accounts,dc=pzuna
>>>>>>>>> uid: admin
>>>>>>>>> cn: Administrator
>>>>>>>>> sn: Administrator
>>>>>>>>> homedirectory: /home/admin
>>>>>>>>> loginshell: /bin/bash
>>>>>>>>> uidnumber: 999
>>>>>>>>> gidnumber: 1001
>>>>>>>>> gecos: Administrator
>>>>>>>>> krbprincipalname: admin at PZUNA
>>>>>>>>> krblastpwdchange: 20090904122852Z
>>>>>>>>> krbpasswordexpiration: 20091203122852Z
>>>>>>>>> memberof: cn=admins,cn=groups,cn=accounts,dc=pzuna
>>>>>>>>> objectclass: top
>>>>>>>>> objectclass: person
>>>>>>>>> objectclass: posixaccount
>>>>>>>>> objectclass: krbprincipalaux
>>>>>>>>> objectclass: inetuser
>>>>>>>>>
>>>>>>>>> Advantages: more user friendly, allows for easy localization,
>>>>>>>>> translation of DNs to primary keys (immediately usable as input
>>>>>>>>> to other plugin commands)
>>>>>>>>>
>>>>>>>>> I recommend, that you use the --raw flag for testing.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I know it's a lot of changes, so I setup a git repo at:
>>>>>>>>> git://fedorapeople.org/~pzuna/freeipa.git
>>>>>>>>>
>>>>>>>>> It should be up-to-date with master and all my patches are
>>>>>>>>> applied there. I hope it makes reviewing them at least a bit
>>>>>>>>> easier.
>>>>>>>>>
>>>>>>>>> Pavel
>>>>>>>>
>>>>>>>> Why are you using a pre_callback() to define default values
>>>>>>>> instead of using default_from? It seems clearer to use that.
>>>>>>> You're probably referring to the user plugin, where gecos and
>>>>>>> krbprincipalname defaults are set inside pre_callback. It's a
>>>>>>> residue from some time ago when we didn't use autofill=True with
>>>>>>> default_from and it didn't have any effect on optional
>>>>>>> parameters. It's a small change, but I included an updated
>>>>>>> version of the patch with this email.
>>>>>>
>>>>>> Ok. I gather you've moved a lot of logic into the pre_callback
>>>>>> plugin to avoid overriding execute? One other goal we wanted was
>>>>>> to allow plugins to extend other plugins and this may be good step
>>>>>> on the way there. So for example, a user wants to be able to set
>>>>>> some extra objectclass, they could do it with a similar
>>>>>> pre_callback extension to the user plugin (once we do the plumbing
>>>>>> for it, that is).
>>>> Right. The goal is to have a common execute in the baseclass, that
>>>> does most of the dirty work and let the user/plugin author add the
>>>> specifics of his plugin in the pre/post callbacks. All the data
>>>> generated by the base (before calling python-ldap) is available for
>>>> modification in the pre-callbacks and all data coming out of
>>>> python-ldap is made available in the post-callback.
>>>>
>>>> And yes, the plugins could be almost endlessly extended this way.
>>>> For example, someone could do this:
>>>>
>>>> from ipalib.plugins.user import user_add
>>>>
>>>> class user_add_extended(user_add):
>>>> def pre_callback(self, ldap, dn, entry_attrs, *keys, **options):
>>>> # let the original user_add plugin do its job
>>>> super(user_add_extended, self).pre_callback(
>>>> ldap, dn, entry_attrs, *keys, **options)
>>>> # add an extra object class
>>>> entry_attrs['objectclass'].append('new_object_class')
>>>> return dn
>>>>
>>>> api.register(user_add_extended)
>>>>
>>>>>>>> This also duplicates some values in the attribute_names()
>>>>>>>> dictionary. I wonder if this can be either created from the
>>>>>>>> parameters or define a global set somewhere that covers all
>>>>>>>> plugins.
>>>>>>> I know, but I couldn't find a better solution. I thought we could
>>>>>>> add something like a 'real_name' kwarg to params, but this has 2
>>>>>>> main disadvantages:
>>>>>>> 1) it only makes sense with parameters that refer to an LDAP
>>>>>>> attribute
>>>>>>> 2) it doesn't work for attributes with no param counterparts
>>>>>>>
>>>>>>> The global set is a good idea as long as we consider only our own
>>>>>>> plugins. 3rd party plugins would have to modify this set to add
>>>>>>> their own attributes. Also, attributes might have a different
>>>>>>> meaning for different objects. They usually don't, but take 'cn'
>>>>>>> for example. We use it as an ID, name, full name (for users), etc.
>>>>>>
>>>>>> Ok, that's a good point. I'm wondering if its overkill to write a
>>>>>> registration system for this, something like:
>>>>>>
>>>>>> def set_label(attr, label, context='')
>>>>>>
>>>>>> def get_label(attr, context='')
>>>>>>
>>>>>> In this we'd store a dict that would be keyed on attr+context.
>>>>>> Some examples might be:
>>>>>>
>>>>>> set_label('uid', 'Login')
>>>>>> set_label('gn', 'First Name')
>>>>>> set_label('cn', 'Full Name', context='user')
>>>>>> set_label('cn', 'Group Name', context='group')
>>>>>>
>>>>>> The lookup would first look for a context-specific entry and fall
>>>>>> back to a non-context specific, something like:
>>>>>>
>>>>>> if attr+context in labeldict: return labeldict[attr+context]
>>>>>> elif attr in labeldict: return labeldict[attr]
>>>>>> else return "woop"
>>>>>>
>>>>>> Like I said, might be overkill ;-)
>>>>>>
>>>>>> But still, the alarms go off when we're putting the same things in
>>>>>> multiple places.
>>>>>>
>>>> Yeah, the registration system sounds like overkill all right. :D
>>>>
>>>> Seriously, I was thinking about several ways on how to resolve this
>>>> issue. Ideally, all information about attributes should be retrieved
>>>> from the schema. Unfortunately, it cannot be done. There's just very
>>>> little information in there.
>>>>
>>>> Another idea was to have an LDAPAttribute base class, that we would
>>>> extend for each attribute. We could then have a ldapattribute.py
>>>> module in ipalib/plugins with all the attributes we use defined
>>>> there. If 3rd parties require a new attribute, they just create
>>>> their own LDAPAttribute subclass in their plugins. I think that this
>>>> would be the best and cleanest approach. LDAPObjects would have a
>>>> set of LDAPAttribute references and do all attribute manipulation
>>>> using them. Also, attribute parameters could be automatically
>>>> generated, so they wouldn't mix with control parameters in
>>>> takes_params (like --all, --raw, --posix for groups, etc.). This is
>>>> probably the way we'll be going in the future, but there's still a
>>>> few problems I have to resolve before implementing it.
>>>>
>>>> By the way, my goal is to move all this LDAP infrastructure from the
>>>> plugin layer into the core library itself once it's mature (and
>>>> documented) enough. Probably not going to happen in v2, but we'll
>>>> see how it goes.
>>>>
>>>>>>>> In the user plugin 'ou' is in the default attributes. We don't
>>>>>>>> use this attribute.
>>>>>>> Since we don't use it, it's not going to get displayed anyway.
>>>>>>>
>>>>>>> All the values in attribute_names, default_attribute and
>>>>>>> attribute_order are subject to change. I set them to initial
>>>>>>> values I thought are acceptable, but I don't think I'm the right
>>>>>>> person to decide what's going to be there. Ideally, someone with
>>>>>>> more UI/text/user friendliness experience should review it
>>>>>>> (there's not programming knowledge required to change the values).
>>>>>>
>>>>>> Ok, this particular attribute brings up DIT issues which is why I
>>>>>> flagged it.
>>>>>>
>>>>>>>> I think that in general baseldap needs to be documented to
>>>>>>>> explain how things work. There is no explanation for
>>>>>>>> object_class_config, for example, that it defines the attribute
>>>>>>>> in cn=ipaConfig that contains the default list of objectclasses
>>>>>>>> for the object.
>>>>>>> Yeah, there's no documentation at this point, but I'm working on
>>>>>>> it as we "speak".
>>>>>>>
>>>>>>>> rob
>>>>>>>>
>>>>>>>
>>>>>>> One more thing, I reviewed your latest patches and they make
>>>>>>> change to host and service plugins. Your patches should be pushed
>>>>>>> first, because they are more complex and also more important, so
>>>>>>> I posted updated versions to the corresponding email threads on
>>>>>>> freeipa-devel.
>>>>>>>
>>>>>>> Pavel
>>>>>>
>>>>>> Ok, I'm not too worried about that, its always a game of catch-up :-)
>>>>>>
>>>>>
>>>>> I've found another problem with the attribute map. I like your idea
>>>>> of breaking out memberof by different type but as it is now doesn't
>>>>> work, it only prints group membership. You'd have to parse the DN
>>>>> to distinguish between groups, taskgroups, etc.
>>>>
>>>> And that's exactly what I'm doing in
>>>> LDAPObject.convert_attribute_members. I mean the DN parsing. It just
>>>> doesn't work for users at the moment, because I forgot to add the
>>>> other types of groups in users attribute_members['memberof'].
>>>>
>>>> in ipalib/plugins/user.py:
>>>>
>>>> class user(LDAPObject):
>>>> # ...
>>>> attribute_members = {
>>>> # add other groups here
>>>> # example 'memberof': ['group', 'taskgroup']
>>>> 'memberof': ['group']
>>>> }
>>>> # ...
>>>>
>>>> try:
>>>>
>>>> ipa user-add test --first=first --last=last
>>>> ipa taskgroup-add tasktest --desc=desc
>>>> ipa taskgroup-add-member tasktest --users=test
>>>> ipa user-show test --all
>>>>
>>>> Should work.
>>>>
>>>> Note: Just noticed it won't work for netgroups, because I forgot to
>>>> override LDAPObject.get_primary_key_from_dn in the netgroup class.
>>>> Normally get_primary_key_from_dn just returns the value of RDN, but
>>>> for netgroups we have to do a search to retrieve the primary key.
>>>>
>>>> class netgroup(LDAPObject):
>>>> # ...
>>>> def get_primary_key_from_dn(self, dn):
>>>> (dn, entry_attrs) = self.backend.get_entry(
>>>> dn, [self.primary_key.name]
>>>> )
>>>> return entry_attrs.get(self.primary_key.name, '')
>>>> # ...
>>>>
>>>>> I think for the short-term we can make a note to do this and just
>>>>> print all memberships so we can get this committed. I'm still not a
>>>>> fan of the attribute_names within each plugin though, I need more
>>>>> convincing.
>>>>>
>>>>> rob
>>>>>
>>>>
>>>> By the way, I'm going on vacation next week. So, I think we should
>>>> wait anyway before committing this.
>>>>
>>>> Pavel
>>>>
>>> I'm sending an updated version of all the patches. They should apply
>>> on the current master. I think they should now address most of your
>>> concerns and I also fixed some bugs I found on my own.
>>>
>>> Pavel
>>
>> Some are good to go, others need some more work:
>>
>> 0001-config: ack
>>
>> 0002-group: ack
>>
>> 0003-hostgroups: nack, hostgroups-show doesn't show members by default.
> Fixed in attached patch.
>
>> 0004-netgroup: nack, fails a ton of unit tests
> Fixed in patch I sent previously today.
>
>> 0005-rolgroup: nack, rolegroup-find doesn't work (try serviceadmin).
>> rolegroup-show doesn't include members by default.
>>
>> 0006-taskgroup: nack, taskgroup-find doesn't work (try ipa addhosts)
>> taskgroup-show doesn't include members by default.
> Both "member thing" fixed, but *-find seems to work for me. Can you post
> the exact commands that fail?
>
>> 0007-tests: ack though we should do some non-raw tests as well IMHO.
>> In test-hostgroup looks like there is some leftover debug output that
>> should be removed before committing.
> Removed the leftover debug output in attached patch.
>
>> 0008-pwpolicy: nack, have another patch floating around that covers
>> most of this. I'll try to incorporate some of your ideas into my patch
>> once committed.
> Ok.
>
>> 0009 missing?
> Not missing, just wrong numbering. There was an unrelated patch that got
> in between the plugin patches.
>
>> 0010-service: ack, but we should try to avoid shadowing built-in names
>> (filter)
> Sometimes it might be justified, but in this case we really should avoid
> it, because it's actually in the base class and what if someone wants to
> use python's filter function inside the callback? (They would have to
> rename the param or do some hacking around.) It doesn't hurt anything
> for now, but this will have to be patched at several places - I'll post
> a patch on Monday.
>
>> 0011-user: ack but changes like this seem to make it less readable:
>>
>> - textui.print_dashed('Unlocked user "%s".' % uid)
>> + textui.print_dashed('Unlocked user "%s".' % keys[-1])
>>
>> I realize that *keys is more generic than specifying one argument but
>> there is no documentation saying was keys means.
> I'm really slow at documenting things, but it's in the works.
>
>> Some general observations that don't necessarily need to be addressed
>> here but do need to be done:
>>
>> - adding/removing member failures should return non-zero return code
>> (so you can test $? on cmd-line)
>> - add/remove-member should probably prompt for things if not provided
>> on command-line (and allow empty values). Right now if you do an
>> add-member it prompts for group to modify and then submits an empty
>> request
> Ok, I'll try to address these issues as soon as possible.
>
>> Let me know if it is safe to push these patches individually.
> The user and *group plugins should be pushed at once. Everything else
> should be safe individually.
>
>> rob
>>
>
> Pavel
Awwww. Forgot to attach the patches...
Pavel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Make-the-hostgroup-plugin-use-baseldap-classes.patch
Type: application/mbox
Size: 8715 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20091002/70728456/attachment.mbox>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Make-the-rolegroup-plugin-use-baseldap-classes.patch
Type: application/mbox
Size: 4295 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20091002/70728456/attachment-0001.mbox>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-Fix-unit-tests-for-plugins-using-baseldap-classes.patch
Type: application/mbox
Size: 47063 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20091002/70728456/attachment-0002.mbox>
More information about the Freeipa-devel
mailing list