[Freeipa-devel] Command instantiation

[Petr Viktorin]

On 01/14/2013 07:17 PM, Rob Crittenden wrote:
> Petr Viktorin wrote:
>> On 01/14/2013 05:48 PM, John Dennis wrote:
>>> On 01/14/2013 11:06 AM, Petr Viktorin wrote:
>>>>
>>>> IPA Command objects sometimes need to pass some data between their
>>>> various methods. Currently that's done using the thread-local context.
>>>> For an example see dnsrecord_del, which sets a "del_all" flag in
>>>> pre_callback and then checks it in execute.
>>>>
>>>> While that works for now, it's far from best practice. For example, if
>>>> some Command can call another Command, we need to carefully check that
>>>> the global data isn't overwritten.
>>>>
>>>>
>>>> The other way data is passed around is arguments. The callback methods
>>>> take a bunch of arguments that are the same for a particular Command
>>>> invocation (ldap, entry_attrs, *keys, **options). By now, there's no
>>>> hope of adding a new one, since all the callbacks would need to be
>>>> rewritten. (Well, one could add an artificial option, but that's
>>>> clearly
>>>> not a good solution.)
>>>> In OOP, this problem is usually solved by putting the data in an object
>>>> and passing that around. Or better, putting it in the object the
>>>> methods
>>>> are called on.
>>>>
>>>> This got me thinking -- why do we not make a new Command instance for
>>>> every command invocation? Currently Command objects are only created
>>>> once and added to the api, so they can't be used to store per-command
>>>> data.
>>>> It seems that having `api.Command.user_add` create a new instance would
>>>> work better for us. (Of course it's not the best syntax for creating a
>>>> new object, but having to change all the calling code would be too
>>>> disruptive).
>>>> What do you think?
>>>>
>>>
>>> Just a few thoughts, no answers ... :-)
>>>
>>> I agree with you that using thread local context blocks to pass
>>> cooperating data is indicative of a design flaw elsewhere.
>>>
>>> See the discussion from a couple of days ago concerning api locking and
>>> making our system robust against foreign plugins. As I understand it
>>> that design prohibits modifying the singleton command objects. Under
>>> your proposal how would we maintain that protection? The fact the
>>> commands are singletons is less of an issue than the fact they lock
>>> themselves when the api is finalized. If the commands instead acted as a
>>> "factory" and produced new instances wouldn't they also have to observe
>>> the same locking rules thus defeating the value of instantiating copies
>>> of the command?
>>
>> Good point. Actually, making api a factory would eliminate the reason
>> for locking Commands: the foreign plugin could do whatever it wanted to
>> its instance of the Command, and all would be well unless it modifies
>> the class itself (which is about as bad as using setattr).
>
> I still think this would invite people to do even more dangerous things,
> like changing the API on-the-fly.

Then let them. We're all adults here.
You can't stop people from writing bad plugins. You can't stop attackers 
from killing your machine if you let them run any code. What's the point?

The current design invites *us* to do things like setattr and 
thread-local state, just to get out of the boundaries we've set for 
ourselves.
And the boundaries we need will grow over time, see the discussion of 
bolted-on things below.

Sharp knives are safer than dull ones.

>>> I think the entire design of the plugin system has at it's heart
>>> non-modifiable singleton command objects which does not carry state.
>>
>> Yes. I guess I'm just trying to find ways to get out of that trap...
>
> context is your state, right? It is sort of bolted on but it is
> per-request and guaranteed not to leak into anything else (except batch,
> as noted below).

We have and support the batch plugin, so that guarantee isn't worth much.

Thread-local state works for now but as more things are built on it, 
it'll become unmanageable.

>>> FWIW, I was never convinced the trade-off between protecting our API and
>>> being able to make smart coding choices (such as your suggestion) struck
>>> the right balance.
>>>
>>> Going back to one of your suggestions of passing a "context block" as a
>>> parameter. Our method signatures do not currently support that as you
>>> observe. But given the fact by conscious design a thread only executes
>>> one *top-level* command at a time and then clears it state the thread
>>> local context block is effectively playing the almost exactly same role
>>> as if it were passed as a parameter.
>>
>> Almost. But, lots of Commands call other Commands, and the caller needs
>> to know the internals of the callee to make sure the context attributes
>> don't clash. Not to mention the other things, such as connection
>> backends, that use the thread-local object. It's fragile.
>
> No argument there. We could clean this up somewhat by imposing a
> namespace into variable naming.
>>
>> As for clearing the state, you already can't rely on that: the batch
>> plugin doesn't do it.
>>
>
> Yes, speaking of bolted on, that defines the batch plugin pretty well.
> It should be fairly straightforward to clear the state between
> executions though (or at least parts of it, there may be some
> batch-specif cthings we'd want to maintain).
>

There are also connection-specific things to watch out for. But I'd 
rather refactor than bolt on more things as we need them, because the 
code will only get worse with each thing that's bolted on.

-- 
Petr³