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

Thu Jul 21 03:17:16 UTC 2011

JR Aquino wrote:
> 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?
>

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.

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'

Is there a way to validate the regex?

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.

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

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.

rob