On Sat, 2011-07-30 at 00:54 +0000, JR Aquino wrote:
> On Jul 21, 2011, at 8:53 AM, JR Aquino wrote:
> 
> > On Jul 21, 2011, at 7:31 AM, 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.
> >> 
> >> 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.
> >> 
> >> 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-default-group  Set a default group for auto membership
> >  --group/hostgroup=STR
> > 
> >> 
> >> 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
> 
> New Patch attached.
> 
> I believe I have addressed the issues highlighted in the thread.
> 

Hello JR,

Thanks for the patch, the new approach with automember as a separate
plugin is much better and more extensible. I reviewed it and have some
feedback:

1) I see that autoMemberScope in automember plugin configuration is set
to $SUFFIX. Why don't we use
cn=hostgroups,cn=accounts,dc=idm,dc=lab,dc=bos,dc=redhat,dc=com and
cn=hostgroups,cn=accounts,dc=idm,dc=lab,dc=bos,dc=redhat,dc=com. IMO
this would improve performance

2) Plugin is not configured correctly on a replica:
nsslapd-pluginConfigArea in cn=Auto Membership
Plugin,cn=plugins,cn=config is not created. Since cn=config is not
replicated, it need to be updated also on a replica. Plus, "Applying
LDAP updates" got stuck in my case for some reason.

3) I cannot use --inclusive-regex in ipa automember-add even though it
is stated in help that I can

# ipa automember-add --type=hostgroup --inclusive-regex=^web[1-9]\.example\.com 
tgroup
Usage: ipa [global-options] automember-add AUTOMEMBER-RULE [options]

ipa: error: no such option: --inclusive-regex

4) Error message when removing a condition is not clear:
# ipa automember-show --type=hostgroup tgroup
  Automember Rule: tgroup
  Inclusive Regex: fqdn=^web[1-9].example.com, fqdn=^www[1-9].example.com
  Exclusive Regex: fqdn=^www5.example.com

Can we detect this situation and change it to something like "Condition
not found"?

5) Having a rule with just an exclusive rule does not make sense - can
we handle it?
# ipa automember-show --type=hostgroup tgroup
  Automember Rule: tgroup
  Inclusive Regex: fqdn=^web[1-9].example.com, fqdn=^www[1-9].example.com
  Exclusive Regex: fqdn=^www5.example.com
# ipa automember-remove-condition --type=hostgroup tgroup 
--inclusive-regex=^web[1-9]\.example\.com 
--inclusive-regex=^www[1-9]\.example\.com
--------------------------------
Removed condition(s) to "tgroup"
--------------------------------
  Automember Rule: tgroup
  Exclusive Regex: fqdn=^www5.example.com

6) Command names for automember default group seems inconsistent:
  automember-add-default-group     Set default group for all unmatched entries.
  automember-default-group-show    Display information about the default 
automember groups.
  automember-remove-default-group  Remove default group for all unmatched 
entries.

If we would follow the same patter, "automember-default-group-show"
should be automember-show-default-group

7) Parameters of the automember default group seems inconsistent too:
a) --desc parameter present in automember-add-default-group and
automember-remove-default-group should not be here
b) Grouping Type of the automember type is passed as an argument in
automember-remove-default-group and automember-default-group-show
instead of --type=STR as in all other commands

8) automember.py: In
automember_add_condition/automember_remove_condition I see 2 almost
identical branches of code - a lot of redundancy. Couldn't we
consolidate them, for example to one "for attr in (INCLUDE_RE,
EXCLUDE_RE):" construct?

9) test_automember_plugin.py: the test class should be named
test_automember, not test_user

Martin

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to