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

[Martin Kosek]

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?


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.


3) I miss hostgroupclarity-mod module. What would I do if I want to
update Description?


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.


5) Test test_clarityrule_plugin.py: reference to inexistent python
module:
+Test the `ipalib/plugins/clarityrule.py` module.


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


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.


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.


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


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


Martin