[Freeipa-devel] [PATCHES] Add new set of base classes for IPA plugins using LDAP.

[Rob Crittenden]

Pavel Zůna wrote:
> Rob Crittenden wrote:
>> Pavel Zuna wrote:
>>> Dear freeipa-devel,
>>> I know we agreed that we're going to post one patch per email, but it 
>>> this case I think it's justified. Patches 0001 through 0005 are 
>>> prerequisites for the new base classes. Before describing the 
>>> individual patches, let me explain a little bit of what I had in mind 
>>> when implementing this addition.
>>>
>>> For the last few months, I've been mostly working the new LDAP 
>>> backend and IPA plugins that use it (i.e. almost all IPA plugins). I 
>>> tried to make the new backend more powerful and flexible. 
>>> Unfortunately, to take full advantage of the new potential, old 
>>> plugins had to be rewritten. As there were several inconsistencies 
>>> across them (output, naming and sometimes even behavior) and about 
>>> half of them were still using old features from IPAv1, I thought that 
>>> some re-factoring couldn't hurt after all.
>>>
>>> After porting a few plugins to the new backend, I realized that most 
>>> of them were doing pretty much the same thing - manipulating LDAP 
>>> entries. The only difference was that some plugins were generating 
>>> attributes in specific ways based on user input. So, I figured that 
>>> most of the generic work could be done in one place and that's when I 
>>> started working on these new base classes.
>>>
>>> Let's take a quick look at them:
>>>
>>> LDAPObject
>>>  - extends frontend.Object
>>>  - represents a type of LDAP entry
>>>    (or sub-entry if a parent_key=True parameter is present)
>>>
>>> The following methods are to be used with LDAPObject, they are named 
>>> after the CRUD methods they extend with an 'LDAP' prefix. They take 1 
>>> argument (the primary key) in the case their LDAPObject is an entry 
>>> and 2 arguments (the parent and primary key, in that order) if their 
>>> LDAPObject is a subentry:
>>>
>>> LDAPCreate
>>> LDAPRetrieve
>>> LDAPUpdate
>>> LDAPDelete
>>>
>>> Then there's LDAPSearch, which is a bit special. It takes only the 
>>> optional criteria argument if its LDAPObject is an entry. If its 
>>> LDAPObject is a sub-entry then it requires the parent key as the 
>>> first argument. The search logic works as follows:
>>>
>>> 1) If there is no parameters (except for the parent key in the case 
>>> of sub-entries), all entries of the given type are returned.
>>>
>>> 2) If there is only the criteria parameter, all entries of the given 
>>> type with one or more of their default attributes containing the 
>>> value of criteria are returned.
>>>
>>> 3) If there is an options specified, all entries of the given type 
>>> with the corresponding attribute EQUAL to the value specified are 
>>> returned. If more than one options is specified this way, only 
>>> entries matching ALL the conditions are returned.
>>>
>>> 2) and 3) can be combined, so for example, we can make a search for 
>>> entries who have a specific attribute equal to 'foo' and some of 
>>> their other attributes containing 'bar'.
>>>
>>> All of these base classes implement a 'callback' method, that is 
>>> called just before invoking python-ldap through the backend. 
>>> Subclasses can use it to execute their specific code after all the 
>>> boring stuff has been done for them.
>>>
>>> In the future I'm planning to write more base classes for other uses 
>>> cases such as adding/removing values from multivalued attributes.
>>>
>>>
>>> Patches descriptions:
>>> ---------------------
>>>
>>> 0001: Make it possible for subclasses of Command and co. to use their 
>>> own __public__.
>>>
>>> The problem was with PluginProxy only taking the base class' 
>>> __public__ into consideration.
>>>
>>>
>>> 0002: Add parent_key to keyword arguments of Param base class.
>>>
>>>
>>> 0003: Add support for parent key in Object base class.
>>>
>>>
>>> 0004: Make CRUD base classes take parent key parameters into 
>>> consideration when
>>> generating args.
>>>
>>>
>>> 0005: Add delete_tree method to ldap2 allowing recursive deletes of 
>>> whole LDAP branches.
>>>
>>> Some plugins were doing this "manually", so I figured it might as 
>>> well be provided by the LDAP API.
>>>
>>>
>>> 0006: Add new set of base classes for plugins using LDAP.
>>>
>>>
>>> To see the new bases classes in action, take a look at the next patch 
>>> I'm about to send to freeipa-devel.
>>
>> A few comments.
>>
>> I'd like Jason to comment on patch 0001 and 0002. It looks ok to me 
>> but he may balk at changing __public__.
>>
>> In LDAPObject you use a variable named keys. It isn't immediately 
>> clear what this is for (made clear after reading get_dn). If I 
>> understand parent_key and keys you are providing a general way of 
>> constructing the DN of the record, right? I'm not sure that parent_key 
>> and keys are very descriptive. It seems like primary_key is the rdn, 
>> parent_key is the next value in the DN and keys can contain either 
>> primary_key or both primary_key and parent_key. This is particularly 
>> confusing if you aren't using LDAPObject as a base class. Why would 
>> you want to return parent_key instead of primary_key when getting the 
>> arguments?
> I was trying to accomplish 2 things by introducing parent_key:
> 1) provide a generic way of constructing the DN
> 2) generate different arguments for CRUD methods of sub-entries
> 
> Some plugins have to deal with entries and their direct sub-entries (DNS 
> plugin: zone->resource, automount plugin: map->key). When accessing a 
> sub-entry, we need to know its RDN (primary_key), but also its parent 
> RDN (parent_key). parent_key is never returned instead of primary_key. 
> If present, parent_key is returned first followed by primary_key.

I think Jason picked primary_key as the main operating attribute both 
because of his background in SQL and his goal to make the framework not 
IPA-specific. At some point we may well extract the framework into a 
package that ships separately. At this early stage though it is simpler 
to have everything together.

I pondered on a better name before and punted, I couldn't think of 
anything either :-(

I'm wondering if we need a parent_primary_key variable though. Wordy as 
it is it does convey more clearly what it's for. Or it could be that as 
long as it is well-documented then it won't be a big deal.

Further, if you move it into the LDAPObject extension we can call it 
parent_rdn which is more descriptive.

> 
> You're right that it can be confusing when not using LDAPObject as a 
> base class, I'll change the CRUD base classes back and override 
> get_args() in LDAP* methods. 'parent_key' and 'keys' might not be the 
> most descriptive names, but I couldn't think of anything better - any 
> suggestions?
> 
>> I like the idea of a callback but is this limited by only returning 
>> the dn? What kinds of things do you envision this being used for? 
>> (e.g. what it if was smart enough to add in extra attributes the user 
>> didn't or couldn't ask for?).
> Actually they are smarter than they look. :) Since parameters in python 
> are passed by reference (well not exactly, it's actually pass by value, 
> but names are the values and names are like references...) we can modify 
> their contents in the callback. Unfortunately, this only applies to 
> mutable types, so DN/filter (immutable strings) have to be returned.

Ok, that's a good point. I think we'll need to document that this can 
happen so that installing some plugin later on doesn't cause unexpected 
side-effects because some expected keyword value is removed by the new 
plugin.

> 
>> BTW this sort of callback was always envisioned, I'm glad you're 
>> adding it. We had thought about having a pre- and post- callback so 
>> you could manage the data going in and coming out. What do you think?
> Sounds like a good idea.
> 
>> You define backend_name in LDAPObject but I don't see it used 
>> anywhere. What did you have in mind for this?
> backend_name is a forgotten feature I found in the depths of ipalib 
> code. When the Object instance is being initialized, its 'backend' 
> attribute is set to api.Backend.<backend_name>.

Ok, yeah we never could think what other backend there might be for us.

> 
>> If this is going to essentially be the base class for all LDAP-based 
>> plugins I think I'd like to see some more inline documentation so the 
>> output of epydoc will be helpful.
> I'll add it to my TODO list. I was thinking about writing a general 
> HOWTO for plugin authors anyway.

Sounds great.

> 
>> I wonder if delete_tree() isn't a bit dangerous. What if someone 
>> manages to pass in dc=example,dc=com?
> I can see your point. I'll remove delete_tree and rewrite LDAPDelete to 
> delete sub-entries "manually".

Ok.

> 
>> I think that ldap.passwd_s() will blow up if you pass in None as the 
>> old password.
> Probably, old pass is supposed to default to '' not None...

Right. You can try it but I seem to recall bad things happening either 
in python 2.4 or 2.5 or both when None was passed in.

> 
>> You made some changes to the modlist generator. Are you sure that this:
>>
>>     modlist.append((_ldap.MOD_DELETE, k, None))
>>
>> will work with python-ldap found on older systems (e.g. RHEL 5)?
> To be honest, I don't know. python-ldap docs say nothing about older 
> versions, but after some googling, I found code dating 2003 that was 
> using this.

Alright, this just looks really strange :-)

> 
>> I guess the basegroup class could be re-based to work on this and be 
>> even simpler :-)
> Yeah, most plugins could be re-based on this and would become a lot 
> simpler. :)
> 
>> Something that this will need at some point is a pointer to where in 
>> cn=ipaconfig the list of default objectclasses can be found. We may 
>> not end up defining this for every single object type but I think for 
>> many we will (users and groups at least). It would be nice if there 
>> was a way to just pass in an attribute name where this list can be found.
> Ok, that should be pretty straight forward.
> 
>> rob
> 
> I'm going to fix everything discussed in this e-mail and if it gets the 
> green light from Jason, we might finally be ready to make the LDAP 
> backend switch (at least for plugins).
> 
> Pavel

Have you run the tests against this backend? They may need to be tuned a 
bit I suppose but in general they should be able to identify problem areas.

rob

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 3245 bytes
Desc: S/MIME Cryptographic Signature
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20090608/9d5ed838/attachment.bin>