[Freeipa-devel] [PATCH] 0016 Fixes for{add, set, del}attr with managed attributes

Tue Mar 20 09:54:56 UTC 2012

On 03/16/2012 08:01 PM, Petr Viktorin wrote:
> On 03/16/2012 06:35 PM, Petr Viktorin wrote:
>> On 03/16/2012 06:33 PM, Rob Crittenden wrote:
>>> Rob Crittenden wrote:
>>>> Petr Viktorin wrote:
>>>>> On 03/15/2012 09:24 PM, Rob Crittenden wrote:
>>>>>> Petr Viktorin wrote:
>>>>>>> On 02/29/2012 04:34 PM, Petr Viktorin wrote:
>>>>>>>> On 02/29/2012 03:50 PM, Rob Crittenden wrote:
>>>>>>>>> Petr Viktorin wrote:
>>>>>>>>>> On 02/27/2012 11:03 PM, Rob Crittenden wrote:
>>>>>>>>>>> Petr Viktorin wrote:
>>>>>>>>>>>> Patch 16 defers validation & conversion until after
>>>>>>>>>>>> {add,del,set}attr is
>>>>>>>>>>>> processed, so that we don't search for an integer in a list of
>>>>>>>>>>>> strings
>>>>>>>>>>>> (this caused ticket #2405), and so that the end result of these
>>>>>>>>>>>> operations is validated (#2407).
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Patch 17 makes these options honor params marked no_create and
>>>>>>>>>>>> no_update.
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> https://fedorahosted.org/freeipa/ticket/2405
>>>>>>>>>>>> https://fedorahosted.org/freeipa/ticket/2407
>>>>>>>>>>>> https://fedorahosted.org/freeipa/ticket/2408
>>>>>>>>>>>
>>>>>>>>>>> NACK on Patch 17 which breaks patch 16.
>>>>>>>>>>
>>>>>>>>>> How is patch 16 broken? It works for me.
>>>>>>>>>
>>>>>>>>> My point is they rely on one another, IMHO, so without 17 the
>>>>>>>>> reported
>>>>>>>>> problem still exists.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> *attr is specifically made to be powerful. We don't want to
>>>>>>>>>>> arbitrarily
>>>>>>>>>>> block updating certain values.
>>>>>>>>>>
>>>>>>>>>> Noted
>>>>>>>>>>
>>>>>>>>>>> Not having patch 17 means that the problem reported in 2408
>>>>>>>>>>> still
>>>>>>>>>>> occurs. It should probably check both the schema and the
>>>>>>>>>>> param to
>>>>>>>>>>> determine if something can have multiple values and reject that
>>>>>>>>>>> way.
>>>>>>>>>>
>>>>>>>>>> I see the problem now: the certificate subject base is defined
>>>>>>>>>> as a
>>>>>>>>>> multi-value attribute in the LDAP schema. If it's changed to
>>>>>>>>>> single-value the existing validation should catch it.
>>>>>>>>>>
>>>>>>>>>> Also, most of the config attributes should probably be
>>>>>>>>>> required in
>>>>>>>>>> the
>>>>>>>>>> schema. Am I right?
>>>>>>>>>>
>>>>>>>>>> I'm a newbie here; what do I need to do when changing the
>>>>>>>>>> schema? Is
>>>>>>>>>> there a patch that does something similar I could use as an
>>>>>>>>>> example?
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> The framework should be able to impose its own single-value
>>>>>>>>> will as
>>>>>>>>> well. If a Param is designated as single-value the *attr should
>>>>>>>>> honor
>>>>>>>>> it.
>>>>>>>>
>>>>>>>> Here is the updated patch.
>>>>>>>> Since *attr is powerful enough to modify 'no_update' Params, which
>>>>>>>> CRUDUpdate forgets about, I need to check the params of the
>>>>>>>> LDAPObject
>>>>>>>> itself.
>>>>>>>>
>>>>>>>>> Updating schema is a bit of a nasty business right now. See
>>>>>>>>> 10-selinuxusermap.update for an example.
>>>>>>>>
>>>>>>>> I'll leave schema changes for after the release, then.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> _______________________________________________
>>>>>>>> Freeipa-devel mailing list
>>>>>>>> Freeipa-devel at redhat.com
>>>>>>>> https://www.redhat.com/mailman/listinfo/freeipa-devel
>>>>>>>
>>>>>>> Attached patch includes tests. Note that the test depends on my
>>>>>>> patches
>>>>>>> 12-13, which make ipasearchrecordslimit required.
>>>>>>
>>>>>> I gather that this eliminates the need for patch 17? It seems to work
>>>>>> as-is.
>>>>>
>>>>> Yes. Patch 17 made *attr honor no_create and no_update, which you said
>>>>> is not desired behavior.
>>>>>
>>>>>> The patch doesn't apply because of an encoding change Martin made
>>>>>> recently.
>>>>>>
>>>>>> It does seem to do the right thing though.
>>>>>>
>>>>>> rob
>>>>>
>>>>> Attaching rebased patch.
>>>>> This deletes Martin's change, but unless I tested wrong, his bug
>>>>> (https://fedorahosted.org/freeipa/ticket/2418) stays fixed. The
>>>>> tests in
>>>>> my patch should apply to that ticket as well.
>>>>>
>>>>> In another fork of this thread, there's discussion if this approach is
>>>>> good at all. Maybe we're overengineering a corner case here.
>>>>>
>>>>
>>>> Found another issue, a very subtle one.
>>>>
>>>> When using *attr and an exception occurs where the param name would
>>>> appear we want the name passed in to be used.
>>>>
>>>> For example:
>>>>
>>>> $ ipa pwpolicy-mod --setattr=krbpwdmaxfailure=xyz
>>>>
>>>> With this patch it will return:
>>>> ipa: ERROR: invalid 'maxfail': must be an integer
>>>>
>>>> It should return:
>>>> ipa: ERROR: invalid 'krbpwdmaxfailure': must be an integer
>>>
>>> On second thought, this may not be related...
>>
>> This is https://fedorahosted.org/freeipa/ticket/1418, I'll see if it
>> makes sense to fix it here.
>
> Okay, this is a different problem. A quick grep of ConversionError in
> parameters.py reveals that all of the "wrong datatype" errors are raised
> with the raw parameter name.
> Other errors are raised with either that or the cli_name or they
> auto-detect. I don't think they follow some rule in this.
>
> Furthermore, our test suite doesn't check exception arguments. Sounds
> like a major cleanup coming up here...
>
>>>
>>>>
>>>> The encoding problem does still exist too:
>>>>
>>>> $ ipa config-mod --setattr ipamigrationenabled=false
>>>> ipa: ERROR: ipaMigrationEnabled: value #0 invalid per syntax: Invalid
>>>> syntax.
>>>>
>>
>> Will fix.
>>
>
> Fixed in the attached update, which encodes the value.
>
> I was surprised to find that
> config_mod.params['ipamigrationenabled'].attribute is True, while
> config_mod.obj.params['ipamigrationenabled'].attribute is False (and so
> its encode() method does nothing). That's because 'attribute' is only
> set when the params are cloned from the LDAPObject to the CRUD method.
> Is there a reason behind this, or is it just that it was easier to do?
>
> For this case it means that params marked no_update will not be encoded
> properly -- getting to a working encode() would require either setting
> 'attribute' on the parent object or some ugly hackery.
>
>
>
> ---
> Petr³
>

Rebased to current master.

-- 
Petr³
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0016-06-Defer-conversion-and-validation-until-after-add-del-.patch
Type: text/x-patch
Size: 9585 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20120320/23219cd2/attachment.bin>