On ke, 09 kesä 2021, Rob Crittenden via FreeIPA-devel wrote:
Rob Crittenden via FreeIPA-devel wrote:
> Alexander Bokovoy wrote:
>> On ti, 13 huhti 2021, Rob Crittenden via FreeIPA-devel wrote:
>>> Currently the framework has four types of methods:
>>>
>>> - pre_callback: called before any real work is done. Usually used to
>>> tune up incoming options
>>> - execute: does the brute work. For most CRUD plugins this is all done
>>> in baseldap.py in the LDAP* classes
>>> - post_callback: supposed to be data cleanup for presentation to the
>>> caller but in some cases actual work is being done. user-add calls
>>> ldap.add_entry_to_group for example.
>>> - exc_callback: special case handling for exceptions
>>>
>>> If you have "work" to be done that would normally be done in
execute
>>> there is no way to do it other than to abuse post_callback to get it
>>> done.
>>>
>>> Overriding execute is only an option if you want to completely replace
>>> the call, otherwise order execution is going to be strange.
>>>
>>> Say for example in user_add you want to do work after the execute() is
>>> done so you'd do something like:
>>>
>>> class user_add()
>>> def execute(stuff):
>>> result = super(user_add, self).execute(stuff)
>>> # some extra work
>>> return result
>>>
>>> The order of execution is:
>>>
>>> user_add:pre_callback
>>> user_add:execute()
>>> LDAPCreate:execute()
>>> user_add:post_callback()
>>> user_add:execute()
>>>
>>> This is because the LDAP* baseldap methods handle the pre/post/exc
>>> callbacks directly. So in order to be able to insert more "work"
in
>>> execute itself we'd need another callback type.
>>>
>>> Why do I want this?
>>>
>>> One feature of post_callback is that for add/mod methods
>>> ldap.get_entry() is called so we can prepare the final results to be
>>> returned to the user for display.
>>>
>>> There are cases where the user doesn't need or case to see that result.
>>> We could save an least one LDAP query by returning only errors in this
>>> case.
>>>
>>> The use case is mostly around bulk operations where the user doesn't
>>> care about seeing the UID of all users added, for example.
>>>
>>> To do this I think we can skip post_callback altogether, assuming it
>>> does no real "work".
>>>
>>> In the case of user_add/mod skipping post_callback would save three
>>> queries: fetch the user, has_keytab and has_password.
>>>
>>> But before re-architecting this I'd like to get some opinions on it.
>>>
>>> I can do a full-blown design if desired.
>>
>> So your proposal is multi-fold:
>>
>> - add a callback type that can be used to perform operations that could
>> always be done as a part of execute() after the main processing of
>> add/mod was done but before post_callbacks were to be called
>>
>> - use the result of executing new type callbacks to decide whether to
>> do or skip post_callback() execution
>>
>> - move content of post_callback that does not depend on the
>> availability of the final results into this new callback type
>>
>> - leave in the post_callback() state only those operations that rely on
>> the final results
>>
>> Is this right?
>
> Mostly. I don't expect to skip any execute callbacks but you do raise an
> interesting point. What gets passed into and out of each result callback
> is still TBD. I think that the baseldap/top-level execute should create
> the final result and dn, entry_attrs, etc will be passed around.
>
>> Assuming it is right, the only issue I have is an impact to third-party
>> plugins. We can communicate such change of course but it is going to
>> break someone's plugin, for sure. So a better documentation would be
>> needed. ;)
>>
>
> Good point. I wonder if we can set a context variable for when in
> post_callback that gets used in any baseldap method that does a write so
> that an exception is raised. That would certainly get admins attention.
>
> It would also be a bit harsh. We could log something instead but that
> would rely on admins to read the logs.
Ping. Any additional feedback would be great.
Frankly, that's was the only point I was able to come with. Definitely
should log the warning/suggestion but other than this, I have no idea
what else can we add.
--
/ Alexander Bokovoy
Sr. Principal Software Engineer
Security / Identity Management Engineering
Red Hat Limited, Finland