[Freeipa-devel] Re: [PATCHES] Make plugins use baseldap classes.

[Pavel Zuna]

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