[Freeipa-devel] [PATCH 0021] Forbid overlapping rid ranges for the same id range

Tomas Babej tbabej at redhat.com
Fri Dec 14 12:52:49 UTC 2012


On 12/13/2012 02:48 PM, Martin Kosek wrote:
> On 12/13/2012 11:52 AM, Tomas Babej wrote:
>> On 12/12/2012 04:32 PM, Martin Kosek wrote:
>>> On 10/26/2012 03:43 PM, Tomas Babej wrote:
>>>> Hi,
>>>>
>>>> creating an id range with overlapping primary and secondary
>>>> rid range using idrange-add or idrange-mod command now
>>>> raises ValidationError. Unit tests have been added to
>>>> test_range_plugin.py.
>>>>
>>>> https://fedorahosted.org/freeipa/ticket/3171
>>>>
>>>> Tomas
>>>>
>>> 1) Add command can cause crash:
>>>
>>> # ipa idrange-add range9 --base-id=1000 --rid-base=1000 --secondary-rid-base=
>>> --range-size 1000
>>> ipa: ERROR: an internal error has occurred
>>>
>>> 2) I don't like this construct very much:
>>>
>>> updated_values = dict(zip(rid_range_attributes,[None]*3))
>>>
>>> This would look better, IMO:
>>> updated_values = dict((attr, None) for attr in rid_range_attributes)
>>>
>>> Why do you need this dict pre-created anyway? You overwrite all keys here:
>>>
>>> +            for attr in rid_range_attributes:
>>> +                if attr in entry_attrs:
>>> +                    updated_values[attr] = entry_attrs[attr]
>>> +                else:
>>> +                    updated_values[attr] = int(old_attrs[attr][0])
>>>
>>>
>>> 3) [nitpick] We don't end ValidationError with '.':
>>>
>>> +                   raise errors.ValidationError(name='ID Range setup',
>>> +                       error=_("Primary rid range and secondary rid range"\
>>> +                               " cannot overlap."))
>>>
>>> There is also a duplication of the same error message...
>>>
>>> 4) The -mod operation will also crash:
>>>
>>> # ipa idrange-add range9 --base-id=1000 --rid-base=1000
>>> --secondary-rid-base=2000 --range-size 1000
>>> # ipa idrange-mod range9 --secondary-rid-base=
>>> ipa: ERROR: an internal error has occurred
>>>
>>> Martin
>> New patch version as well as diff between
>> patch versions (for your convenience) attached.
>>
>> Tomas
> 1) You introduced mixed spaces and tabs - Python gods do not like that!
Oops, I'd rather send a fixed patch sooner than they bring down their 
wrath on me. :)
> 2) This is a nitpick, but there are too many redundant parens and brackets in
> this statement:
>
> +	if(any([attr is None for attr in [rid_base,secondary_rid_base, size]])):
> +            return False
>
> This would look nicer and would not create unnecessary list:
>
> +	if any(attr is None for attr in (rid_base, secondary_rid_base, size)):
> +            return False
Brackets reduced.
> 3) Another construct I did not like very much:
>
> +	is_set = lambda x : (x in entry_attrs) and not (x is None)
>
> a) "x is not None" reads better than "not (x is None)"
> b) I would rather replace all is_set lambdas with use of "if
> entry_attrs.get('attribute')" which is also used in other places in ipalib
I don't think this approach would be beneficial. If any of rid_base,
secondary_rid_base, base_id, e.g. would be set to 0, the expression like

/entry_attrs.get('base_id')

/would be evaluated as False both in case that there is no key 'base_id'
in the dictionary and in the case that the value associated with the key
is 0. To avoid these problems, we would have to complicate conditions
used in if-s, and therefore make the readability worse.
/
/
> 4) I see a suspicions check
>
> +            if (is_set('ipasecondarybaserid') != is_set('ipabaserid')):
>
> I though that ipasecondarybaserid is optional. With your change it is not:
>
> # ipa idrange-add range9 --base-id=1000 --rid-base=1000 --range-size 1000
> ipa: ERROR: invalid 'ID Range setup': Options secondary_rid_base and rid_base
> must be used together
>
> It is also quite ugly condition, I would do something like:
>
> if entry_attrs.get('ipasecondarybaserid') and not entry_attrs.get('ipabaserid'):
> ... raise error
It's not a bug. It's a feature :)

Secondary base RID indeed is mandatory when primary RID base has been 
defined.
Its purpose is to prevent collisions when user and group share the same 
POSIX ID number.

 From the documentation of idrange.py:

/To create an ID range for the local domain it is not necessary to 
specify a//
//domain SID. But since it is possible that a user and a group can have 
the same//
//value as Posix ID a second RID interval is needed to handle conflicts.

/
> 5) I would not create a list when it is not necessary, a tuple is more
> efficient I think:
>
> +            rid_range_attributes =
> ['ipabaserid','ipasecondarybaserid','ipaidrangesize']
Fixed.
> 6) If we want to check user does not create secondary RID range without a
> primary RID range, we should also do it in -mod operation:
>
> # ipa idrange-mod range9 --rid-base=
> --------------------------
> Modified ID range "range9"
> --------------------------
>    Range name: range9
>    First Posix ID of the range: 1000
>    Number of IDs in the range: 1000
>    First RID of the secondary RID range: 2000
>    Range type: local domain range
This is fixed as part of my patch 0024 as it falls under the scope of
http://fedorahosted.org/freeipa/ticket/3170

I will send a rebased version later today.
> 7) I am sorry I did not come with this in my previous review, but I have one
> more nitpick for the error message:
> +                       error=_("Primary rid range and secondary rid range"\
> +                               " cannot overlap"))
>
> I would do s/rid/RID/ as we also refer it as RID in our help...
>
> Martin
Fixed. However, lower-case rid is used in ipa_range_check.c 389 plugin.
We might want to consider filing a naming convention ticket then.

Tomas

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20121214/1a7faeb3/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0021-3-Forbid-overlapping-rid-ranges-for-the-same-id-range.patch
Type: text/x-patch
Size: 11761 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20121214/1a7faeb3/attachment.bin>


More information about the Freeipa-devel mailing list