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

Wed Aug 31 07:59:24 UTC 2011

On Wed, 2011-08-31 at 00:59 +0000, JR Aquino wrote:
> 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

OK then. I did not find any major issues preventing me from acking the
ticket. If we find an issue later, we can create a ticket.

So ACK and pushed to master, ipa-2-1. Before pushing I fixed commit
message and few minor nits in automember examples:
- changed automember-remove-condition to match automember-add-condition
before it
- changed automember-add-condition --key=manager so that it actually
matches the manager
- automember-mod indentation

Martin