Martin Kosek wrote:
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

I believe the scope defines where to find automatic members, so it should point to cn=computerss,cn=$SUFFIX.


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

I had him remove this. No other command lets you add an entry and members at the same time.


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

I think it should be automember-default-group-*. Since there is only one default for something I thing it should be set instead of add.

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


I'm also wondering about hardcoding a key. Is there a reason we can't ask the regex writer to simply include this themselves?

rob

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

Reply via email to