On 04/11/2012 03:42 AM, Jan Cholasta wrote: > On 11.4.2012 09:27, Martin Kosek wrote: >> On Wed, 2012-04-11 at 09:18 +0200, Jan Cholasta wrote: >>> On 10.4.2012 19:56, Dmitri Pal wrote: >>>> On 04/10/2012 01:48 PM, Rob Crittenden wrote: >>>>> Petr Viktorin wrote: >>>>>> On 04/10/2012 07:07 PM, Martin Kosek wrote: >>>>>>> On Tue, 2012-04-10 at 17:03 +0200, Jan Cholasta wrote: >>>>>>>> On 10.4.2012 16:00, Petr Viktorin wrote: >>>>>>>>> I'm aware that we have backwards compatibility requirements so we >>>>>>>>> have >>>>>>>>> to stick with unfortunate decisions, but I wanted you to know >>>>>>>>> what I >>>>>>>>> think. Please tell me I'm wrong! >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> It is not clear what --{set,add,del}attr and friends should >>>>>>>>> do. On >>>>>>>>> the >>>>>>>>> one hand they should be powerful -- presumably as powerful as >>>>>>>>> ldapmodify. On the other hand they should be checked to ensure >>>>>>>>> they >>>>>>>>> can't be used to break the system. These requirements are >>>>>>>>> contradictory. >>>>>>>>> And in either case we're doing it wrong: >>>>>>>>> - If they should be all-powerful, we shouldn't validate them. >>>>>>>>> - If they shouldn't break the system we can just disable them for >>>>>>>>> IPA-managed attributes. My understanding is that they were >>>>>>>>> originally >>>>>>>>> only added for attributes IPA doesn't know about. People can >>>>>>>>> still >>>>>>>>> use >>>>>>>>> ldapmodify to bypass validation if they want. >>>>>>>>> - If somewhere in between, we need to clearly define what they >>>>>>>>> should >>>>>>>>> do, instead of drawing the line ad-hoc based on individual >>>>>>>>> details we >>>>>>>>> forgot about, as tickets come from QE. >>>>>>>>> >>>>>>>>> >>>>>>>>> I would hope people won't use --setattr for IPA-managed >>>>>>>>> attributes. >>>>>>>>> Which would however mean we won't get much community testing >>>>>>>>> for all >>>>>>>>> this extra code. >>>>>>>>> >>>>>>>>> >>>>>>>>> Then, there's an unfortunate detail in IPA implementation: >>>>>>>>> attribute >>>>>>>>> Params need to be cloned to method objects (Create, Update, >>>>>>>>> etc.) to >>>>>>>>> work properly (e.g. get the `attribute` flag set). If they are >>>>>>>>> marked >>>>>>>>> no_update, they don't get cloned, so they don't work properly. >>>>>>>>> Yet --setattr apparently needs to be able to update and validate >>>>>>>>> attributes marked no_update (this ties to the confusing >>>>>>>>> requirements on >>>>>>>>> --setattr I already mentioned). This leads to doing the same work >>>>>>>>> again, >>>>>>>>> slightly differently. >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> tl;dr: --setattr work on IPA-managed attributes (with validation) >>>>>>>>> is a >>>>>>>>> mistake. It adds no functionality, only complexity. We don't want >>>>>>>>> people >>>>>>>>> to use it. It will cost us a lot of maintenance work to support. >>>>>>>>> >>>>>>>>> >>>>>>>>> Thank you for listening. A patch for the newest regression is >>>>>>>>> coming >>>>>>>>> up. >>>>>>>>> >>>>>>>> >>>>>>>> I wholeheartedly agree. >>>>>>> >>>>>>> This is indeed a mine field and we need to make a look from at the >>>>>>> issue >>>>>>> from all sides before accepting a decision. >>>>>> >>>>>> Yes. >>>>>> >>>>>>>> >>>>>>>> Like you said above, we should either not validate >>>>>>>> --{set,add,del}attr >>>>>>>> or don't allow them on known attributes. >>>>>>> >>>>>>> IMHO, validating attributes we manage in the same way for both >>>>>>> --setattr >>>>>>> and standard attrs is not that wrong. It is a good precaution, >>>>>>> because >>>>>>> if we let an unvalidated value in, it can make even a bigger >>>>>>> mess later >>>>>>> in our pre_callbacks or post_callbacks where we assume that at this >>>>>>> point everything is valid. >>>>>> >>>>>> Then we should validate *exactly* the same way, including not >>>>>> allowing >>>>>> no_update attributes to be updated. >>>>>> >>>>>>> If somebody wants to modify attributes in an uncontrolled, >>>>>>> unvalidated >>>>>>> way, he is free to use ldapmodify or other tool to play with raw >>>>>>> LDAP >>>>>>> values. Without our guarantee of course. >>>>>> >>>>>> That's clear. >>>>>> >>>>>>> But if he chooses to use our --{set,add,del}attr we should at least >>>>>>> help >>>>>>> him to not shoot himself to the leg and >>>>>>> validate/normalize/encode the >>>>>>> value. I don't know how many users use this API, but removing a >>>>>>> support >>>>>>> for all managed attributes seems as a big compatibility break to >>>>>>> me. >>>>>> >>>>>> Well, it was broken (see >>>>>> https://fedorahosted.org/freeipa/ticket/2405, >>>>>> 2407, 2408), so I don't think many people used it. >>>>>> >>>>>> Anyway, what's the use case? Why would the user want to use >>>>>> --setattr >>>>>> for validated attributes? Is our regular API lacking something? >>>>>> >>>>> >>>>> I think Honza had the best suggestion, enhance the standard API to >>>>> handle multi-valued attributes better and reduce the need for >>>>> set/add/delattr. At some future point we can consider dropping them >>>>> when updating something already covered by a Param. We're stuck with >>>>> them for now. >>>>> >>>> >>>> The use case I would see is the extensibility. Say a customer wants to >>>> extend a schema and add an attribute X to the user object. He would >>>> still be able to manage users using CLI without writing a plugin for >>>> the new attribute. Yes plugin is preferred but not everybody would go >>>> for it. So in absence of the plugin we can't do validation but we >>>> still >>>> should function and be able to deal with this attribute via CLI >>>> (and UI >>>> if this attribute is enabled for UI via UI configuration). >>>> >>>> I am generally against dropping this interface. But expectations IMO >>>> should be: >>>> 1) If the attribute is managed by us with setattr and friends it >>>> should >>>> behave in the same way as via the direct add/mod/del command >>>> 2) If attribute is not managed it should not provide any guarantees >>>> and >>>> act in the same way as via LDAP >>>> >>>> Hope this helps. >>>> >>> >>> This might be the best thing to do, but IMO it is still no good, >>> because >>> the behavior of --{set,add,del}attr for a particular attribute might >>> change between API versions, when that attribute changes from unmanaged >>> to managed. >>> >>> Honza >>> >> >> I think this is OK and expectable - user may use setattr option to set >> an attribute before it is officially supported by IPA and still have it >> working when he upgrades. > > There's no guarantee the user will still have it working. User > application might break if it depends on the unmanaged behavior and we > are too strict in validation of the managed attribute, for example. I > don't known how much of an issue is this actually, but this kind of > unpredictability is not a good thing IMHO. > >> Though we should make sure that we describe >> this API well in our documentation to make sure this expectation is >> shared with users. >> >> Martin >> > > Honza > Honza,
I do not agree. The difference in behavior will be only in validation. So instead of letting an invalid data in we would start to prevent it. IMO it is a bug in the application in the first place so breakage is justified especially if we document this expectation. -- Thank you, Dmitri Pal Sr. Engineering Manager IPA project, Red Hat Inc. ------------------------------- Looking to carve out IT costs? www.redhat.com/carveoutcosts/ _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel