On Mon, 2011-08-08 at 16:16 +0000, JR Aquino wrote:
> On Aug 8, 2011, at 2:04 AM, Martin Kosek wrote:
> 
> > On Fri, 2011-08-05 at 18:36 +0000, JR Aquino wrote:
> >> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >> Jr Aquino, GCIH | Information Security Specialist
> >> Citrix Online | 7408 Hollister Avenue | Goleta, CA 93117
> >> T:  +1 805.690.3478
> >> jr.aqu...@citrixonline.com
> >> http://www.citrixonline.com
> >> 
> >> On Aug 2, 2011, at 5:55 AM, Rob Crittenden wrote:
> >> 
> >>> JR Aquino wrote:
> >>>> On Aug 1, 2011, at 5:56 AM, Rob Crittenden wrote:
> >>>> 
> >>>>> 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.
> >>>> 
> >>>> autoMemberScope needs to point to the lowest common denominator where 
> >>>> the plugin will detect the insertion of new ldap objects that match the 
> >>>> rules:
> >>>> (http://directory.fedoraproject.org/wiki/Auto_Membership_Design#Example_Configuration_Entries)
> >>>> 
> >>>> With that in mind, it seems that we need to point it to 
> >>>> cn=accounts,$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.
> >>>> 
> >>>> I will be correcting this with Rob's suggestion in irc of moving the 
> >>>> operation into __common_setup
> >>>> 
> >>>>>> 
> >>>>>> 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.
> >>>> 
> >>>> I will be removing reference to this in the help/doc. Thank you for 
> >>>> catching it.
> >>>> 
> >>>>> 
> >>>>>> 
> >>>>>> 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"?
> >>>> 
> >>>> It appears as though this situation is tricker than just 'condition not 
> >>>> found'.
> >>>> 
> >>>> Since we can provide a list of both inclusive and exclusive regex, and 
> >>>> any one of the provided entries could be /not found/ it looks as though 
> >>>> I will need to replicate the functionality of 'failed' similar to how 
> >>>> they are treated when removing a list of users/hosts from a 
> >>>> group/hostgroup where some of the members are non-existant.
> >>>> 
> >>>> It looks like I need to return a failed list, but it is not currently 
> >>>> clear how to accomplish that in an LDAPUpdate pre/post_callback, as 
> >>>> those are really only looking for the return of a 'dn'
> >>>> 
> >>>>>> 
> >>>>>> 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
> >>>> 
> >>>> I am adding additional checks to verify that we don't create/leave a 
> >>>> rule with a sole exclusive regex.
> >>>> 
> >>>>>> 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.
> >>>> 
> >>>> I agree with Rob:
> >>>> ipa  automember-default-group-set
> >>>> ipa  automember-default-group-remove
> >>>> ipa  automember-default-group-show
> >>>> 
> >>>>> 
> >>>>>> 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
> >>>> 
> >>>> This is deliberate.
> >>>> 
> >>>> The framework has a mandatory requirement of at least 1 argument.
> >>>> 
> >>>> Since these 3 operations require us to operate on parent object's, it 
> >>>> does not make sense to force it to provide a 'rule' as the modification 
> >>>> does not take place on any 1 rule, but rather the container which they 
> >>>> are apart of.
> >>>> I am open to suggestions...
> >>>> 
> >>>>>> 
> >>>>>> 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?
> >>>> 
> >>>> I will see what I can do to accommodate this.  It is likely that there 
> >>>> will need to be several pivot points to provide the logic required to 
> >>>> prevent the addition of an exclusive regex without an inclusive one 
> >>>> present.  Likewise with the deletion of the last inclusive regex when 
> >>>> there is at least 1 exclusive regex present.
> >>>> 
> >>>>>> 
> >>>>>> 9) test_automember_plugin.py: the test class should be named
> >>>>>> test_automember, not test_user
> >>>> 
> >>>> Ah. Thanks!
> >>>> 
> >>>>>> 
> >>>>>> 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?
> >>>> 
> >>>> I am fairly opposed to removing 'default' attrs which the rules are 
> >>>> applied to...  I am happy to provide a means to override them.
> >>>> 
> >>>> While it may be second nature for all of us to know that there is an 
> >>>> fqdn attribute, etc, our consumers are likely not going to intrinsically 
> >>>> know our schema.  We also deliberately mask the real attribute names in 
> >>>> the framework. (fqdn = Host name)
> >>>> 
> >>>> Providing a default feels like a happy medium which allows for ease of 
> >>>> use and somewhat of a safety belt against users defining an incorrect 
> >>>> attribute name.
> >>>> 
> >>>> It also might get somewhat tiring to constantly provide --key=fqdn every 
> >>>> time you add a hostname regex?
> >>> 
> >>> Ok, but when you display rules fqdn is displayed. How are users to know
> >>> they shouldn't include fqdn= when removing existing rules?
> >>> 
> >>>>> 
> >>>> 
> >>>> Stand by for a follow up patch later today after I figure out how to 
> >>>> better account for the failed manipulation of conditions.
> >>>> 
> >> 
> >> Final Draft Patch attached, Please review.
> >> 
> >> The API should all be straightened out.
> >> The handling of adding/removing of conditions has been greatly improved.
> >> We now require type and key for conditionals.
> >> A schema check is performed to catch any bad attributes.
> >> 
> >> Exclusive Conditions: I've thought more about it, and it is a benign 
> >> condition that does not effect the state of the system.
> >> Similarly, Sudo rules can contain deny commands with no additional data 
> >> which are also benign.  I would like to propose that those all cases where 
> >> this is possible in FreeIPA be reviewed in the future rather than 
> >> excluding this patch from being committed to master.
> >> 
> > 
> > I think you attached a wrong patch, its same as the previous one. Btw.
> > if you use freeipa-jraquino-PATCH_NO-PATCH_VER-description.patch naming,
> > it is simpler to recognize the version of the patch.
> 
> Opps!
> 
> Attached is the corrected patch.
> Tripled checked that it has the correct modifications.
> 

Ah, this one's better. I checked the new API, seems consistent to me. I
was thinking about the new --key attribute, looks OK. It would be great
to have some default setting here, but since it is common for all
grouping types, we cannot do that. Its good its at least covered in
documentation and checked in schema.

Sending the issues I have found:

1) freeipa.spec.in: you changed 389-ds-base Requires to a version that
does not even exists:

-Requires(pre): 389-ds-base >= 1.2.8.0-1
+Requires(pre): 389-ds-base >= 1.2.9.0.2

Please change it to 1.2.9.5-1 which has been released yesterday and
which should fix our reported DS issues (BZ 723937, 725743). If we don't
push this patch before 2.1 release I will update the 389-ds-base
Requires myself as it contains the fixes for us.

2) Plugin is still not being configured correctly on a replica:

This change need to be executed on both master and replica:

+dn: cn=Auto Membership Plugin,cn=plugins,cn=config
+changetype: modify
+add: nsslapd-pluginConfigArea
+nsslapd-pluginConfigArea: cn=automember,cn=etc,$SUFFIX

The initial cn=etc,$SUFFIX population should be done only on master.

3) I see that autoMemberScope in automember plugin configuration is
still set to $SUFFIX. I would suggest to set it for hostgroups and
groups to cn=hostgroups,cn=accounts,$SUFFIX and cn=groups,cn=accounts,
$SUFFIX, respectfully, to improve plugin search performance

4) Automember help is not correct for ipa
automember-default-group-set/remove commands. The API has changed there:

+ Set the default target group:
+    ipa automember-default-group-set --type=hostgroup webservers
+    ipa automember-default-group-set --type=group ipausers
+
+ Set the default target group:
+    ipa automember-default-group-remove --type=hostgroup webservers
+    ipa automember-default-group-remove --type=group ipausers
+


5) I would fix examples for condition manipulating commands:

+ Add another condition to the rule:
+   ipa automember-add-condition --inclusive-regex=^web[1-9+]\.example\.com 
webservers
+
+ Add an exclusive condition to the rule to prevent auto asignment:
+   ipa automember-add-condition --exclusive-regex=^web5\.example\.com 
webservers
+
+ Remove a condition from the rule:
+   ipa automember-remove-condition --inclusive-regex=^www[1-9+]\.example\.com 
webservers
+

Currently, the framework asks for both Attribute Key and Grouping type
in these commands. I think it is better to have those required
attributes already filled, so that user can just simply copy&paste

6) I got internal error when trying to add an duplicate exclusive regex:
# ipa automember-show --type=hostgroup webservers
  Automember Rule: webservers
  Inclusive Regex: fqdn=^web[1-9+].example.com
  Exclusive Regex: fqdn=^web5.example.com
# ipa automember-add-condition --exclusive-regex=^web5\.example\.com 
--type=hostgroup --key=fqdn webservers
ipa: ERROR: an internal error has occurred

Martin

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

Reply via email to