[Freeipa-devel] [PATCH] 34 Create FreeIPA CLI Plugin for the 389 Auto Membership plugin
JR Aquino
JR.Aquino at citrix.com
Thu Jul 21 00:20:58 UTC 2011
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?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jraquino-0034-Create-FreeIPA-CLI-Plugin-for-the-389-Auto-Membershi.patch
Type: application/octet-stream
Size: 29053 bytes
Desc: freeipa-jraquino-0034-Create-FreeIPA-CLI-Plugin-for-the-389-Auto-Membershi.patch
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20110721/d914b0f0/attachment.obj>
More information about the Freeipa-devel
mailing list