[Freeipa-devel] Re: [PATCHES] Make plugins use baseldap classes.
Pavel Zuna
pzuna at redhat.com
Fri Oct 2 14:31:07 UTC 2009
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
I found a bug in the netgroup plugin, here's an update.
Also, the pwpolicy plugin patch needs update to included your latest changes.
Should be done by monday.
Pavel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Make-the-netgroup-plugin-use-baseldap-classes.patch
Type: application/mbox
Size: 17015 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20091002/e1635692/attachment.mbox>
More information about the Freeipa-devel
mailing list