[Freeipa-devel] [PATCH] Add group plugin port to new LDAP backend.

[Rob Crittenden]

Pavel Zuna wrote:
> Jason Gerard DeRose wrote:
>> On Wed, 2009-05-13 at 16:04 -0400, Rob Crittenden wrote:
>>> Pavel Zuna wrote:
>>>> Rob Crittenden wrote:
>>>>> Pavel Zuna wrote:
>>>>>> Rob Crittenden wrote:
>>>>>>> Pavel Zuna wrote:
>>>>>>>> Rob Crittenden wrote:
>>>>>>>>> Pavel Zuna wrote:
>>>>>>>>>> By the way, there's a little bug I discovered while testing 
>>>>>>>>>> this plugin. It affects the old group plugin as well. When 
>>>>>>>>>> trying to modify a group into a posixGroup, gidNumber doesn't 
>>>>>>>>>> get generated automatically resulting in a object violation 
>>>>>>>>>> LDAP error. Solution is to generate it ourselves, but I didn't 
>>>>>>>>>> know how it works, so I commented that part out for now. 
>>>>>>>>>> (/FIXME in vim)
>>>>>>>>>>
>>>>>>>>> This should be fixed in FDS 1.2. Can you update and give it a try?
>>>>>>>>>
>>>>>>>>> rob
>>>>>>>> Sure, just updated and you're right, it works.  :)
>>>>>>>> Updated patch attached.
>>>>>>>>
>>>>>>>> Pavel
>>>>>>> nack. This won't handle someone using group-mod to set a specific 
>>>>>>> gidnumber. The posixGroup objectclass won't be added.
>>>>>>>
>>>>>>> rob
>>>>>> Fixed patch attached.
>>>>>>
>>>>>> Pavel
>>>>> The basegroup2 part looks ok but nack on group2.
>>>>>
>>>>> I think we should stick with using lower-case attribute names as a 
>>>>> rule of thumb rather than camel case. In any case you test for the 
>>>>> string posixGroup is in the list of objectclasses, this test needs 
>>>>> to be case insensitive.
>>>> When no attributes to retrieve are specified, python-ldap retrieves 
>>>> them all in the original form - camel case. If we specify them, then 
>>>> it returns them in the same form as we requested them. The new LDAP 
>>>> backend doesn't use CIDicts anymore, but only the normal python dict 
>>>> type, so everything is case sensitive. Of course I can make it 
>>>> return attribute names always as lowercase if that's what we want.
>>> I think we need consistent naming otherwise all sorts of odd bugs can 
>>> creep in.
>>>
>>>>> I also wonder if we should be using ldap.get_entry(). Why use this 
>>>>> over group-show?
>>>> It's faster, because we call get_entry directly and because we can 
>>>> request objectClass attribute only. Why invoke an IPA command 
>>>> instead of a making a direct call?
>>> Well, I felt the same way but Jason convinced me that by limiting the 
>>> places we do actual LDAP calls will be beneficial in the long-run. 
>>> The command is run internally, not over XML-RPC, so there isn't a 
>>> whole lot of additional overhead.
>>>
>>> Part of the idea, which we haven't really utilized much yet, is to 
>>> try to make the backend easily replacable.
>>
>> Well, first question, is this Backend.ldap.get_entry(), or a direct call
>> to the python-ldap bindings?  I feel very strongly that no plugins
>> should talk directly to python-ldap (except Backend.ldap).
>>
>> But if this is whether to retrieve the group entry via
>> Command.group_show() or Backend.ldap.get_entry(), I think it depends on
>> what attributes are needed.  If we want the same attributes as
>> Command.group_show() returns, we should call it so we aren't defining
>> the list of (or logic behind) the attributes to choose in multiple
>> places.  If we need all the attributes, calling Backend.ldap.get_entry()
>> is probably best.
> In this context, we need only the 'objectClass' attribute that 
> group_show doesn't return normally unless we pass it the '--all' option 
> and then we get a lot of attributes we don't need as side effect.

The biggest downside is that you'd get the members which really could be 
quite large. I think a better solution is to be able to pass into 
group-show (and really any/all of the plugins) the attributes you want 
to see, with a reasonable default and the --all options available too.

The problem with getting the entry within another plugin is you may 
require knowledge of the schema to do so. If the schema changes, like it 
did with host recently, you'd have to know to go and change it in other 
places. When I changed the DN of the host entries the only plugin that 
changed was host.

rob

> Generally speaking, I think the decision of what call is made should go 
> like this:
> We need to get attribute A,B,C,... (even if they happen to be the same 
> as group_show returns)? Call get_entry.
> We need to get the same attributes group_show returns (whatever those 
> may be)? Call group_show.
> 
>>>>> I'm not sure if the logic around setting gidnumber is right. If you 
>>>>> set the gidnumber but aren't using the --posix flag it looks like 
>>>>> it will always append posixgroup to the list of objectclasses. I'm 
>>>>> pretty sure the LDAP server is going to reject the update. I 
>>>>> suppose making a list(set(objectclasses)) would work for de-duping.
>>>> You're right, it's broken. I'll fix it.
>>>>
>>>> Pavel
>>> ok
>>>
>>> rob
> 
> Pavel

-------------- 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/20090515/0592c80e/attachment.bin>