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.

Martin

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

Reply via email to