[Freeipa-devel] [PATCHES 247-259] ID views - management part

Tue Sep 16 11:21:33 UTC 2014

Petr,

thanks for the review, your input is, as always, much to the point.

My responses are inline. Also, I'm attaching a new patchset, rebased on
master, please, have a look at that.

Most of the patches have at least minor changes, since I rebased another
~15 patches into this patchset, and there are additional 9 patches on
top. Patch 254 was deprecated, as we decided to go with more explicit
way of having two separate commands for user and group ID overrides.

Also, the test suite (around 80 tests) for ID views is attached - patch
270. It depends on patch 269, sent in separate thread.

Tomas

On 09/10/2014 03:10 PM, Petr Viktorin wrote:
> On 08/01/2014 12:30 PM, Tomas Babej wrote:
>> Hi,
>>
>> the following set of patches implements the ID view creation and
>> management of views and ID overrides in IPA.
>>
>> Pending questions:
>> 1.) The patch 253 implements basic managed permissions for ID views and
>> ID overrides. Do we want to have a separate permission for assigning ID
>> views?
>> 2.) Performance: idview-apply command takes ~100 seconds for hostgroups
>> with 1000 member hosts. I am able to lower that by 20-30% using raw ldap
>> calls, is bypassing the framework worth the performance gain? We'll lose
>> the possiblity to hook on exception callbacks.
>>
>> The commands also need more helpful documentation, I am working on that.
>>
>
> Not tested yet, but here are my comments on the patches:
>
> 0247:
> The change to copy-schema-to-ca is not necessary. This is only used
> for servers installed with Dogtag 9; for such installs new schema is
> added online (to 99-user.ldif), so it will be replicated to the CA DS
> correctly.

Removed.

>
> Did you register the new OIDs?

Yep.

>
> 0248-0249 look good
>
> 0250:
> With so many names imported from one module, I find it more readable
> to `from ipalib.plugins import baseldap`, and use qualified names
> later. Same in the 2 other patches.
> This is personal preference/tip, feel free to disagree :)

I was just trying to be consistent here, rest of the IPA plugins seem to
use non-qualified names for plugins. Even tough the main reason is
probably that thay use evil star imports :) Let's keep it this way for now.

>
> 0521:
> Could you also kill the backslash in _hostname_validator?
Done.

>
> 0252:
> Typo in __doc__, should be "This functionality is primarily used"…
>
Done.

> {idview,idoverride}.takes_params: flags needs to be a set -- here
> you've specified 11 single-letter flags. (Don't we all love Python's
> iterable strings.)

No longer applies.

>
> Only one `ipa help` topic will be created for idview and idoverride.
> Is that intended?

Yes, but the documentation needs to be extended, making a note so that
we don't forget that.

>
> The pattern_errmsg in idoverride's uid should be expanded to fully
> describe the pattern.

If we want to expand it, then it should be corrected in user.py plugin
too (I have taken it from there). Still, it's a tradeoff between
conciseness and and exactness, do we really want to specify that user
login may be at most 253 letters long or the $ character may only be
used as terminal one? Or the fact that - can't be used at the beggining?
This is probably the way it was since forever in user plugin.

>
> 0253 looks good
>
> 0254:
> The DN refactoring is done; I don't think the asserts in pre_callbacks
> are necessary any more.

Fixed in other places. BTW, I killed this patch.

>
> 0258:
> idview_apply: typo in hostgroup doc, should be "hosts to apply the ID
> view to" and "after running the idview-apply command".
> Typos in output params, should be e.g. "this ID view"

Fixed the second and third complaint ( IMHO this is clear from context,
but I added "this" to be explicit), I'm not so sure about the first one
though. Can we get a native speaker input on that?

>
> idview_apply.execute: Is there a reason for calling idview_show
> instead of get_dn_if_exists?
Fixed.

>
> idview_apply.execute:
> failed['hostgroup'].append((hostgroup, str(e))) -- str(e) doesn't
> include the name of the exception, which is usually important

Fixed.

>
> Calling a Command (here, host_mod) from another Command is, at the
> very least, quite slow, with all the validation and detailed output.
> It's also a debugging nightmare when things go wrong. And the
> _exc_wrapper is an even worse debugging nightmare.
> If you need some functionality in the host plugin that you need, put
> it in a function and call that. Otherwise use ldap directly.
>
> Do we have some precedent for the 'No host was affected.' message?
> Consumers of the JSON API shouldn't need to cpecial-case this string.

I don't think so, this was just me being over user-friendly. I think we
can remove it, and I have done so.

>
> By subclassing idview_unapply from idview_apply you're violating
> Liskov's substitution principle. Make a common base class for them.

Done.

>
> 0259:
> show_id_overrides: cn is a MUST attribute in ipa*Override; use
> view.single_value['cn'] instead of get(). In enumerate_hosts, is None
> okay if cn is missing?

This was actually an error, fixed in updated version of patches. New
code uses 'ipaanchoruuid' (which is a MUST) and converts it (with your
objections addressed.)

As for the hosts, the 'cn' attribute is required by the nsHost objectclass.
>
>

-- 
Tomas Babej
Associate Software Engineer | Red Hat | Identity Management
RHCE | Brno Site | IRC: tbabej | freeipa.org 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140916/e8748a80/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0270-ipatests-Add-xmlrpc-tests-for-idviews-plugin.patch
Type: text/x-patch
Size: 48997 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140916/e8748a80/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0268-idviews-Resolve-anchors-to-object-names-in-idview-sh.patch
Type: text/x-patch
Size: 11025 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140916/e8748a80/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0267-idviews-Raise-NotFound-errors-if-object-to-override-.patch
Type: text/x-patch
Size: 1570 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140916/e8748a80/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0266-idviews-Change-format-of-IPA-anchor-to-include-domai.patch
Type: text/x-patch
Size: 2134 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140916/e8748a80/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0265-idviews-Alter-idoverride-methods-to-work-with-splitt.patch
Type: text/x-patch
Size: 5021 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140916/e8748a80/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0264-idviews-Split-the-idoverride-commands-into-iduserove.patch
Type: text/x-patch
Size: 17677 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140916/e8748a80/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0263-idviews-Split-the-idoverride-object-into-iduseroverr.patch
Type: text/x-patch
Size: 6644 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140916/e8748a80/attachment-0006.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0262-idviews-Support-specifying-object-names-instead-of-r.patch
Type: text/x-patch
Size: 7388 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140916/e8748a80/attachment-0007.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0261-trusts-Add-conversion-from-SID-to-object-name.patch
Type: text/x-patch
Size: 2997 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140916/e8748a80/attachment-0008.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0259-2-idviews-Extend-idview-show-command-to-display-assign.patch
Type: text/x-patch
Size: 8412 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140916/e8748a80/attachment-0009.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0258-2-idviews-Add-ipa-idview-apply-and-idview-unapply-comm.patch
Type: text/x-patch
Size: 9584 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140916/e8748a80/attachment-0010.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0257-2-hostgroup-Selected-PEP8-fixes-for-the-hostgroup-plug.patch
Type: text/x-patch
Size: 2851 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140916/e8748a80/attachment-0011.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0256-2-hostgroup-Remove-redundant-and-star-imports.patch
Type: text/x-patch
Size: 1279 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140916/e8748a80/attachment-0012.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0255-2-hostgroup-Add-helper-that-returns-all-members-of-a-h.patch
Type: text/x-patch
Size: 978 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140916/e8748a80/attachment-0013.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0253-2-idvies-Add-managed-permissions-for-idview-and-idover.patch
Type: text/x-patch
Size: 4094 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140916/e8748a80/attachment-0014.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0252-2-idviews-Create-basic-idview-plugin-structure.patch
Type: text/x-patch
Size: 17680 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140916/e8748a80/attachment-0015.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0251-2-ipalib-PEP8-fixes-for-host-plugin.patch
Type: text/x-patch
Size: 6958 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140916/e8748a80/attachment-0016.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0250-2-ipalib-Remove-redundant-and-star-imports-from-host-p.patch
Type: text/x-patch
Size: 2623 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140916/e8748a80/attachment-0017.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0249-2-idviews-Add-ipaAssignedIDVIew-reference-to-the-host-.patch
Type: text/x-patch
Size: 9760 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140916/e8748a80/attachment-0018.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0248-2-idviews-Create-container-for-ID-views-under-cn-accou.patch
Type: text/x-patch
Size: 1903 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140916/e8748a80/attachment-0019.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0247-2-idviews-Add-necessary-schema-for-the-ID-views.patch
Type: text/x-patch
Size: 4697 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140916/e8748a80/attachment-0020.bin>