[Freeipa-devel] [PATCH] 34 Create FreeIPA CLI Plugin for the 389 Auto Membership plugin

[JR Aquino]

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