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

Wed Apr 4 14:51:49 UTC 2012

Petr Viktorin wrote:
> On 03/30/2012 03:22 PM, Rob Crittenden wrote:
>> Petr Viktorin wrote:
>>> 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.
>>>
>>
>> There is a test failure. I ran out of time last night to investigate
>> whether the test is valid or it is an issue with the patch, forgot to
>> send this out before I left for the day yesterday.
>>
>> ======================================================================
>> FAIL: test_attr[23]: pwpolicy_mod: Try setting non-numeric
>> krbpwdmaxfailure
>> ----------------------------------------------------------------------
>> Traceback (most recent call last):
>> File "/usr/lib/python2.7/site-packages/nose/case.py", line 187, in
>> runTest
>> self.test(*self.arg)
>> File
>> "/home/rcrit/redhat/freeipa-beta1/tests/test_xmlrpc/xmlrpc_test.py",
>> line 247, in <lambda>
>> func = lambda: self.check(nice, **test)
>> File
>> "/home/rcrit/redhat/freeipa-beta1/tests/test_xmlrpc/xmlrpc_test.py",
>> line 260, in check
>> self.check_exception(nice, cmd, args, options, expected)
>> File
>> "/home/rcrit/redhat/freeipa-beta1/tests/test_xmlrpc/xmlrpc_test.py",
>> line 277, in check_exception
>> UNEXPECTED % (cmd, name, args, options, e.__class__.__name__, e)
>> AssertionError: Expected 'pwpolicy_mod' to raise ConversionError, but
>> caught different.
>> args = []
>> options = {'setattr': u'krbpwdmaxfailure=abc'}
>> ValidationError: invalid 'krbpwdmaxfailure': must be an integer
>
> Fixed. I should have caught this on Friday.
>
>

ACK, pushed to master and ipa-2-2

rob