On Jul 20, 2011, at 8:37 AM, Rob Crittenden wrote: > JR Aquino wrote: >> On Jul 15, 2011, at 7:55 AM, Rob Crittenden wrote: >> >>> Martin Kosek wrote: >>>> On Thu, 2011-07-14 at 23:05 +0000, JR Aquino wrote: >>>>> On Jul 14, 2011, at 11:55 AM, wrote: >>>>> >>>>>> https://fedorahosted.org/freeipa/ticket/1272 >>>>>> >>>>>> * Added new container in etc to hold the automembership configs. >>>>>> * Modified constants to point to the new container >>>>>> * Modified dsinstance to create the container >>>>>> * Modified hostgroup.py to add the new commands >>>>>> * Added xmlrpc test to verify functionality >>>>> >>>>> Minor adjustment: >>>>> Auto Membership Plugin isn't available until 1.2.9-0.2+ >>>>> >>>>> Modified freeipa.spec.in: >>>>> BuildRequires: 389-ds-base-devel>= 1.2.9-0.2 >>>> >>>> I have reviewed your patch. Basic functionality is OK but I have some >>>> concerns. >>>> >>>> 1) I am not sure with the command name, it is not really clear to me >>>> what this command does. But I know from my experience that inventing a >>>> cool name for something new may be the most difficult task at all :-) >>>> Maybe command name "hostgrouprule" or "hostgroupauto" would be more >>>> clear? >> >> Perhaps my example docs were too soft with fqdn=^www[1-9]+\.example\.com >> etc.. >> I should 'clarify'... perhaps what I need to do is add more useful >> information to the doc, for example If I were to add to the doc area >> examples where hostnames are like: w[1-9]+s2r8\.example\.com >> >> The real reason for the usefulness of this technology, is in SaaS, Cloud, >> and Cluster environments, where the hostnames tend to be non-human readable, >> and more like a serial number detailing their function, their rack location, >> or their vm-instance, etc... >> >> It is because of those scenarios that caused me so much grief as a security >> engineer trying to assign rights that it became clear that I could just >> define the reproducible pattern to match assignment into a host group. The >> hostnames needed clarity in order to understand where they belonged in the >> network. >> >> I'll give it one more chance to pass the censors since I've been internally >> calling it clarity for the last 2 1/2 years that I've been using it... >> >>>> >>>> >>>> 2) Overloading execute method in functions >>>> hostgroupclarity_add_condition and hostgroupclarity_remove_condition is >>>> an over-kill for me. I think we could just read current >>>> inclusive/exclusive regexes in pre_callback, modify them and let >>>> LDAPUpdate class do the standard LDAP operations. >> >> I'll recode to perform the actions in a pre_callback. >> >>>> >>>> >>>> 3) I miss hostgroupclarity-mod module. What would I do if I want to >>>> update Description? >> >> Thank you for catching this, I will add it. >> >>>> >>>> >>>> 4) I didn't like this construct in the code, its error prone to >>>> potential future parameter changes. >>>> + if len(options) == 2: # 'all' and 'raw' are always sent >>>> + raise errors.EmptyModlist() >>>> I know it's in baseldap.py but I still wouldn't like to see this in >>>> plugins. >> >> I should be able to omit that once the code is located in the pre_callback. >> >>>> >>>> >>>> 5) Test test_clarityrule_plugin.py: reference to inexistent python >>>> module: >>>> +Test the `ipalib/plugins/clarityrule.py` module. >> >> Thank you, that is left over from a previous attempt. I will remove it. >> >>>> >>>> >>>> Then I did some real testing of the new command: >>>> >>>> 6) Invalid examples, fqdn is not supposed to be a part of regex >>>> $ ipa hostgroupclarity-add >>>> --inclusive-hostname-regex=fqdn=^www[1-9]+\.example\.com webservers >>>> Hostgroup Clarity Rule: webservers >>>> Inclusive Regex: fqdn=fqdn=^www[1-9]+.example.com >> >> Also an oversight, thanks, I will correct it. >> >>>> >>>> >>>> 7) It does not make sense to have a rule with only an exclusive regex: >>>> $ ipa hostgroupclarity-add --exclusive-hostname-regex=^www5+\.example\.com >>>> webservers >>>> Hostgroup Clarity Rule: webservers >>>> $ ipa host-add --force foo.example.co >>>> $ ipa hostgroup-show webservers >>>> Host-group: webservers >>>> Description: Web Servers >>>> Member hosts: www1.example.com >>>> >>>> I think we should 1) hide exclusive regex option in hostgroupclarity-add >>>> and 2) check that there is at least one inclusive regex in the rule when >>>> running hostgroupclarity-add-condition and >>>> hostgroupclarity-remove-condition. >> >> I agree, I'll hide it during the creation, and force it to require an >> inclusive prior to adding an exclusive. >> >>>> >>>> >>>> 8) Plugin incorrectly handles a situation when both inclusive and >>>> exclusive regex-es are being added: >>>> >>>> $ ipa hostgroupclarity-add >>>> --inclusive-hostname-regex=^www[1-9]+\.example\.com webservers >>>> Hostgroup Clarity Rule: webservers >>>> Inclusive Regex: fqdn=^www[1-9]+.example.com >>>> $ ipa hostgroupclarity-add-condition >>>> --inclusive-hostname-regex=^web[1-9]+\.example\.com >>>> --exclusive-hostname-regex=www5\.example\.com webservers >>>> Inclusive Regex: fqdn=^web[1-9]+.example.com, fqdn=^www[1-9]+.example.com >>>> Exclusive Regex: www5.example.com >>>> >>>> Exclusive regex misses fqdn. >> >> Will look into this. >> >>>> >>>> >>>> 9) Removing multiple conditions also works incorrectly: >>>> >>>> $ ipa hostgroupclarity-show webservers >>>> Hostgroup Clarity Rule: webservers >>>> Inclusive Regex: fqdn=^www[1-9]+.example.com, fqdn=^web[1-9]+.example.com >>>> Exclusive Regex: fqdn=www5.example.com >>>> $ ipa hostgroupclarity-remove-condition webservers >>>> --inclusive-hostname-regex=^www[1-9]+\.example\.com >>>> --exclusive-hostname-regex=www5\.example\.com >>>> Inclusive Regex: fqdn=^web[1-9]+.example.com >>>> $ ipa hostgroupclarity-show webservers >>>> Hostgroup Clarity Rule: webservers >>>> Inclusive Regex: fqdn=^web[1-9]+.example.com >>>> Exclusive Regex: fqdn=www5.example.com >> >> Same as above likely. >> >>>> >>>> >>>> 10) When removing nonexistent regex I would expect more explaining error >>>> message: >>>> >>>> $ ipa hostgroupclarity-show webservers >>>> Hostgroup Clarity Rule: webservers >>>> Inclusive Regex: fqdn=^web[1-9]+.example.com >>>> Exclusive Regex: fqdn=www5.example.com >>>> $ ipa hostgroupclarity-remove-condition webservers >>>> --inclusive-hostname-regex=foo >>>> ipa: ERROR: no modifications to be performed >> >> I'll see what better exception can be thrown. Thanks! >> >>> >>> I think that with group_dn() you should use the api to get the entry rather >>> than calling LDAP directly (I'd stick it into the clarity object). >>> >>> This is untested but I think it will work: >>> >>> def hostgroup_dn(self, hostgroup): >>> entry = self.api.Command.user_show(hostgroup, all=True)['result'] >>> return entry['dn'] >>> >>> rob >> >> I'll try this instead, thanks Rob! >> >> -JR >> > > And on second thought you may be able to hook right into the hostgroup object > get_dn() function.
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?
binvmQf0xNHo9.bin
Description: freeipa-jraquino-0034-Create-FreeIPA-CLI-Plugin-for-the-389-Auto-Membershi.patch
_______________________________________________ Freeipa-devel mailing list [email protected] https://www.redhat.com/mailman/listinfo/freeipa-devel
