[Freeipa-devel] [PATCH] 34 Create FreeIPA CLI Plugin for the 389 Auto Membership plugin

[JR Aquino]

On Aug 23, 2011, at 2:43 PM, Rob Crittenden wrote:

> JR Aquino wrote:
>> 
>> On Aug 19, 2011, at 2:16 AM, Martin Kosek wrote:
>> 
>>> Hi JR,
>>> 
>>> I get to your plugin again. You can see my findings below.
>>> 
>>> On Tue, 2011-08-09 at 22:41 +0000, JR Aquino wrote:
>>> ...
>>>> Ok New Patch attached.
>>>> 
>>>> I believe this addresses the above.
>>>> 
>>>> 1. Requires(pre): 389-ds-base>= 1.2.9.5-1
>>> 
>>> 1) Please, remove the change to FreeIPA spec, its no longer needed since
>>> we shipped version 2.1 and it already requires sufficient 389-ds-base
>>> version.
>> 
>> Done.
>> 
>>> 
>>>> 
>>>> 2. replica-automember.ldif added for dsinstance to install during replica installs:
>>>> +dn: cn=Auto Membership Plugin,cn=plugins,cn=config
>>>> +changetype: modify
>>>> +add: nsslapd-pluginConfigArea
>>>> +nsslapd-pluginConfigArea: cn=automember,cn=etc,$SUFFIX
>>> 
>>> 2) OK. I would do it a bit different - have one LDIF for
>>> nsslapd-pluginConfigArea setting and second for creating the base
>>> automember structure. Master would then use both LDIFs and a replica
>>> both of them. We would then be without duplicates in LDIF. But your way
>>> acceptable.
>> 
>> Please allow the 2 ldif's in as they are.
>> 
>> I tried to split them to leverage cn=config change in common, however, I encountered a 389 ds bug.
>> I will be opening a bug with Nathan in BZ to address the bug.  If you feel strongly, we can either:
>> 
>> A: Accept the two LDIFs as is and revisit after a newer version of 389 ds is available.
>> B: Wait until 389 ds addresses the bug and make the minor modification you suggested above.
>> 
>>> 
>>>> 
>>>> 3. autoMemberScope is now set for each:
>>>> groups: cn=users,cn=accounts,$SUFFIX
>>>> hostgroups: cn=computers,cn=accounts,$SUFFIX
>>> 
>>> OK
>>> 
>>>> 
>>>> 4. Corrected examples
>>>> Set the default target group:
>>>>    ipa automember-default-group-set --default-group=webservers hostgroup
>>>>    ipa automember-default-group-set --default-group=ipausers group
>>>> 
>>>> Set the default target group:
>>>>    ipa automember-default-group-remove hostgroup
>>>>    ipa automember-default-group-remove group
>>>> 
>>>> Show the default target group:
>>>>    ipa automember-default-group-show hostgroup
>>>>    ipa automember-default-group-show group
>>>> 
>>>> 5. Corrected examples
>>>> Add a condition to the rule:
>>>>   ipa automember-add-condition --key=fqdn --type=hostgroup --inclusive-regex=^web[1-9+]\.example\.com webservers
>>> 
>>> 3) Please fix the regex to ^web[1-9]+\.example\.com. I think its just a
>>> mistake - right now for example a host web11.example.com does not match.
>> 
>> Fixed
>> 
>>> 
>>>>   ipa automember-add-condition --key=manager --type=group --inclusive-regex=^mscott admins
>>>> 
>>> 
>>> 4) I think you wanted to use devel rule instead of non-existent "admins"
>>> automember rule.
>>> 
>> 
>> You are correct, this has been fixed.
>> 
>>>> Add an exclusive condition to the rule to prevent auto asignment:
>>>>   ipa automember-add-condition --key=fqdn --type=hostgroup --exclusive-regex=^web5\.example\.com webservers
>>>> 
>>>> Remove a condition from the rule:
>>>>   ipa automember-remove-condition --key=fqdn --type=hostgroup --inclusive-regex=^www[1-9+]\.example\.com webservers
>>> 
>>> 5) The same as in 3)
>> 
>> Fixed
>> 
>>> 
>>>> 
>>>> 6. Correct bug for adding duplicate conditions. Included test for it in the test suite.
>>>> 
>>> 
>>> OK. Here are my additional findings:
>>> 
>>> 6) There some more example commands in doc which are not complete and
>>> require some user typing:
>>> 
>>> Display a automember rule:
>>>    ipa automember-show webservers
>>> 
>>> Delete an automember rule:
>>>    ipa automember-del webservers
>>> 
>>> Grouping type option is missing
>> 
>> Fixed.  Added the appropriate flags in the examples
>> 
>>> 
>>> 7) I get internal error when running examples from the automember doc:
>>> # ipa automember-add --type=group devel
>>> -----------------------------
>>> Added automember rule "devel"
>>> -----------------------------
>>>  Automember Rule: devel
>>> # ipa automember-add-condition --key=manager --type=group --inclusive-regex=^mscott admins
>>> ipa: ERROR: an internal error has occurred
>> 
>> Fixed.
>> 
>>> 
>>> 
>>> That's all. The plugin gets better with every version, I think we may
>>> soon be ready for pushing - when all of the issues are resolved.
>>> 
>>> Martin
>>> 
>> 
>> Please let me know how it looks now.
>> 
> 
> Looks lots better, just a couple of nits:
> 
> * The default-group api has type as an arg and everywhere else it is --type, can we make it consistent? We can argue about this with Martin tomorrow if you'd like.

This has now been fixed with some help from Rob removing 'cn' as a primary key.

> 
> * The tests focus mainly on bucket allocation, it also needs to test adding/removing conditions and rules. I wonder if there should actually be two test suites, one to test the basics of the plugin and one to make sure it operates properly when creating entries.

I have added many new tests in the xml test for automember.  It now verifies the functionality of multiple entries, as well as the logic behind exclusive and inclusive regex.

> 
> * Can you document in the ldifs and the installer why there are separate ones for master and replicas (for dsinstance.py I think you can just say # see ldifs for details).

The ldifs and dsinstance have now been commented.

> 
> rob
> 

As per Rob via IRC, I have made a very minor modification to user.py which allows the test suite to wait for memberof to finish so that it will provide consistent output with automember assignment.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jraquino-0034-Create-FreeIPA-CLI-Plugin-for-the-389-Auto-Membershi.patch
Type: application/octet-stream
Size: 73441 bytes
Desc: freeipa-jraquino-0034-Create-FreeIPA-CLI-Plugin-for-the-389-Auto-Membershi.patch
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20110830/99d46e02/attachment.obj>