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
_______________________________________________
Freeipa-devel mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/freeipa-devel
