[Freeipa-devel] [PATCHES] 0068-0070 Automember rebuild membership

[Martin Kosek]

On 11/13/2013 04:56 PM, Ana Krivokapic wrote:
> On 11/13/2013 03:08 PM, Martin Kosek wrote:
>> On 10/29/2013 12:30 PM, Ana Krivokapic wrote:
>>> On 10/15/2013 06:09 PM, Ana Krivokapic wrote:
>>>> On 09/30/2013 10:02 AM, Petr Viktorin wrote:
>>>>> On 09/27/2013 03:12 PM, Martin Kosek wrote:
>>>>>> On 09/27/2013 03:00 PM, Jan Cholasta wrote:
>>>>>>> On 23.9.2013 19:41, Ana Krivokapic wrote:
>>>>>>>> On 09/19/2013 03:29 PM, Ana Krivokapic wrote:
>>>>>> ...
>>>>>>> Patch 69:
>>>>>>>
>>>>>>> I think the changes in the update file should be also done in the
>>>>>>> right LDIF
>>>>>>> files in install/share, though I don't know what is the recent
>>>>>>> consensus on this.
>>>>>>>
>>>>>>>
>>>>>>> Honza
>>>>>>>
>>>>>> Last time I checked, we used to do the change both in LDIF and update
>>>>>> file. Just to avoid the LDIF become obsolete.
>>>>>>
>>>>>> Martin
>>>>> Rob recently said his preference is to move everything from LDIF to updates,
>>>>> and out of the the LDIF files:
>>>>> http://www.redhat.com/archives/freeipa-devel/2013-September/msg00106.html
>>>>>
>>>>> I would agree, having two places with the same information is redundant and
>>>>> error-prone.
>>>>>
>>>> Thanks Honza for the review.
>>>>
>>>> I incorporated your suggestions in this updated patchset. I attached all the
>>>> patches for more convenient reviewing, but only patches 68 and 70 have changed.
>>>>
>>>> I haven't done any changes in the LDIF files since the consensus seems to be not
>>>> to do that.
>>> Patch 70 needed a rebase, attaching the whole patchset again.
>> This works pretty fine, I have few comments though:
>>
>> 1) 0068: the task should be run only for users/hosts base DN - this is where we
>> confine our automember and I think admin may be surprised that the rebuild call
>> is does not respect it.
> 
> Fixed.
> 
>>
>> 2) 0068: I am missing some examples for automember-rebuild in the help. At
>> least for running rebuild for all users/hosts and for running it for specified
>> user/host.
> 
> I added some examples, as well as a general description of the new command.
> 
>>
>> 3) 0068: I think that the labels/doc for the new command/options should be
>> improved. It is not obvious, that automember-rebuild can run for all
>> users/hosts, at least from following doc:
>>
>> # ipa help automember
>> ...
>>   automember-rebuild               Rebuild auto membership for specified entries.
>> ...
>>
>> Maybe we should remove the "for specified entries" part?
>>
>> As for the options, we now have this:
>>
>> # ipa help automember-rebuild
>> Usage: ipa [global-options] automember-rebuild [options]
>>
>> Rebuild auto membership for specified entries.
>> Options:
>>   -h, --help            show this help message and exit
>>   --type=['group', 'hostgroup']
>>                         Grouping to which the rule applies  <--completely stray
>>   --users=STR           Users for which the rebuild task will be run
>>   --hosts=STR           Hosts for which the rebuild task will be run
>>
>>
>> We should probably also do not mention specified entries here.
>>
>> As for option help, maybe the following would better show that it can be run
>> for all entries?
>>
>>   --type=['group', 'hostgroup']
>>                         Rebuild membership for all members of a grouping
>>   --users=STR           Rebuild membership for specified users
>>   --hosts=STR           Rebuild membership for specified hosts
> 
> Agreed, labels fixed as per your suggestions.
> 
>>
>> This makes me thinking we may want to forbid entering both --type and
>> --users/--hosts - i.e. either rebuild all or just selected ones - to make the
>> selection even more clear. But I am open to discussion on this one.
> 
> Validation prevents any invalid combination of options (e.g. --type=group and
> --hosts used together, or --type=hostgroup and --users used together). If, for
> example, --users is specified, then --type=group is allowed but not required. I
> think it's clear enough.
> 
>>
>> 4) 0069: Add Automember Export Updates Task is currently redundant. I think we
>> should either have permissions for all 3 possible tasks or for just the one we use.
> 
> I removed the unused permission.
> 
>>
>> 5) 0069: permissions should be of SYSTEM type as the ACI is out of SUFFIX, so
>> that user does not try to modify them (will be able to in future versions).
>> Adding Petr3 to CC for heads up on this one.
> 
> Fixed.
> 
>>
>> Martin
> 
> Thanks for the review, the updated patchset is attached.

Looks good. Last thing I would like to get fixed is this part in 0068:

+        types = {
+            'group': ('user', 'users', 'users'),
+            'hostgroup': ('host', 'hosts', 'computers'),
+        }
+
+        obj_name, opt_name, dn_part = types[gtype]
+        basedn = DN(('cn', dn_part),('cn', 'accounts'), api.env.basedn)
+        obj = self.api.Object[obj_name]

I know it works now, but if we sometime decide to maybe support different user
containers and not have it in cn=users,cn=accounts, it will help not user
hardcoded DNs like that, but rather standard

DN(api.env.container_users, api.env.basedn)
or
DN(api.env.container_hosts, api.env.basedn)

Martin