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

Wed Aug 31 00:59:08 UTC 2011

On Aug 30, 2011, at 12:44 PM, Jr Aquino wrote:

> 
> 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.

Confirmed with Rob that there is a bug with the list compare function in the tests (users + memberof + automember can result in unpredictable order. He will be adding a separate patch for that.

Additional fixes suggested by Rob via IRC:

Added additional info in help to demonstrate the user/host being auto assigned to their respective groups
Added additional testing for mod / show / find
Added summaries for the automember-default-group set of commands
Correct behavior for find to return the cn label
Added spelling corrections
-------------- 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: 80511 bytes
Desc: freeipa-jraquino-0034-Create-FreeIPA-CLI-Plugin-for-the-389-Auto-Membershi.patch
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20110831/b1d2cd04/attachment.obj>