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

[Rob Crittenden]

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.

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

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

rob