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

[Martin Kosek]

On Thu, 2011-07-21 at 10:31 -0400, Rob Crittenden wrote:
> Martin Kosek wrote:
> > On Thu, 2011-07-21 at 03:37 +0000, JR Aquino wrote:
> >>>> Rob, I'm afraid I believe that ldap lookup is necessary. The user inputs a standard string to represent the possible host group… If i simply perform a get_dn it will indeed provide a dn, however, it won't verify that the host group actually exists…  (you don't want to create an assignment rule for a non existent target host group)
> >>>>
> >>>>
> >>>> Martin, (except for the name Clarity), I have addressed your observations in this latest patch.  Could you please have a look and let me know if there is anything else I need to take care of?
> >>>>
> >
> > Great, preparing the command parameters in pre_callback is much cleaner.
> >
> >>>
> >>> Good point about the LDAP lookup.
> >>>
> >>> This looks a lot better but there are still a few issues:
> >>>
> >>> If group_dn is in the object then you can use self.obj.handle_not_found(*keys) for the NotFound.
> >>
> >> Ok, I will give that a shot!
> >>
> >>>
> >>> Or if it can't be moved, in the calls to group_dn() you can use the ldap handle passed into pre_callback.
> >>>
> >>> I guess you are using the includetype tuple to avoid coding long variable names everywhere? Would a symbol be better, eg:
> >>>
> >>> INCLUDE_RE = 'automemberinclusiveregex'
> >>> EXCLUDE_RE = 'automemberexclusiveregex'
> >>
> >> That works, I'll swap em.
> >
> > I agree with Rob here, this will make the code better.
> >
> >>
> >>> Is there a way to validate the regex?
> >>
> >> Now that you mention it, I believe if I import re, we should be able to validate the initial regex and raise an exception if it is bogus.
> >>
> >>> If we were to add an equivalent user group handler would it be the same code in add_condition and remove_condition? It is sort of nice to have everything together at the moment, I suspect it will need to be generalized at some point.
> >>
> >> Well. For the groups, I was thinking it starts to get a little different.  I would still reuse the condition, but I believe I would pivot users into groups based upon something like their manager?
> >>
> >>> Adding a clarity with no rules won't let you add rules:
> >>>
> >>> # ipa hostgroup-add --desc=hg1 hg1
> >>> # ipa hostgroupclarity-add hg1
> >>> # ipa hostgroupclarity-add-condition --exclusive-hostname-regex=^web5\.example\.com hg1
> >>> ipa: ERROR: no modifications to be performed
> >>
> >> This ^ is deliberate, you cannot add an exclusion rule if there is no existing or simultaneous inclusive rule. :) Martin asked for that, and I think its wise.
> >
> > Yes, it is wise :-) But the error message is really not clear to the
> > user. We should tell him that there must be at least one inclusive rule.
> >
> > I wonder if we shouldn't force user to create a hostgroupclarity object
> > with at least one inclusive rule and than make sure that in all
> > operations at least one inclusive rule stays here. Or we could delete
> > the empty LDAP object after the last inclusive rule is removed, as we do
> > with DNS record LDAP objects in dnsrecord-del.
> >
> >>> The way you explained clarity today in IRC, how it brings clarity to managing membership with names no human can grok, I got it. Still, clarity is a bit awkward as a name. automember might be a better choice.
> >>
> >> Fair enough ;)  I tried, perhaps I can /allude/ to it in the help / docs.  automember it is.
> >>
> >> One final class I have been struggling with that I want to add…
> >>
> >> The object and attribute which defines the 'default group' is the parent of the actual rules… i.e. cn=hostgroup,cn=automember,cn=etc…
> >>
> >> The ipa cli seems to only want to let me make mods that assume I am specifying a target object on the cli… "ipa hostgroupautomember-default-group=foo<rulename>"<- in this scenario, we don't actually want or need a rule name because its the container above…  I have had success making the writes, but the cli syntax just doesn't lend itself to that level of abstraction…
> >>
> >> Any suggestions?
> >>
> >>
> >
> > I think the best shot would be to create a new command and overload the
> > execute method in that case. Like in hbacrule_enable. You would be able
> > to set dn correctly here and do the update. Does it makes sense? Rob?
> >
> > Martin
> >
> 
> I agree. We are better off abstracting things now so we can get the API 
> right.
> 
> I think we can stick more or less with the command names, just in a new 
> plugin and some new arguments.

Yes, this will make more flexible API for the future. We will be able to
implement new membership types easily if we want to.

> 
> I see the plugin with the following methods:
> 
> Each takes a single argument, the name of the rule. I don't think I'd 
> stick type into the DN so you wouldn't be able to use the same rule name 
> for different object types. If we want to allow that then we'd need to 
> add --type to a lot more commands.

I think we have to leave the type in the DN as it is now, i.e. 
1) cn=Hostgroups,cn=automembership,cn=etc,$SUFFIX
2) cn=Groups,cn=automembership,cn=etc,$SUFFIX
3) ...

Otherwise we wouldn't be able to configure the membership plugin
correctly (see automembership.ldif) and the rules would be mixed
together. Plus, it would be then easier to list rules for one type via
plain ldapsearch.

> 
> There is no mod to change types, you have to delete and re-add.
> 
> automember-add               Add an automember rule
>    --type=ENUM 			(hostgroup, group)
>    --desc=STR			description of this auto membership rule
>    --inclusive-regex=LIST	Inclusive Regex
>    --exclusive-regex=LIST	Exclusive Regex
> 
> automember-add-condition     Add conditions to automember rule
>    --inclusive-regex=LIST	Inclusive Regex
>    --exclusive-regex=LIST	Exclusive Regex
> 
> automember-del               Delete an automember rule
> 
> automember-find              Search for automember rules
>    --type=ENUM 			(hostgroup, group)
> 
> automember-mod               Modify an automember rule.
> 
> automember-remove-condition  Remove conditions from an automember rule
>    --inclusive-regex=LIST	Inclusive Regex
>    --exclusive-regex=LIST	Exclusive Regex
> 
> automember-show              Display an automember rule

This looks good, I only miss an option to set a default group (when no
rule matches). This would be located directly in
cn=automembership,cn=etc,$SUFFIX as JR mentioned. Therefore this needs a
different approach than the regular automembership rules - a new command
possibly.

Martin