Re: [lmap] AD evaluation: draft-ietf-lmap-yang-10

[Alissa Cooper <[e-mail]>]

Hi Juergen,

Responses below. I trimmed out areas that don’t need a response.

> On Jan 24, 2017, at 3:23 PM, Juergen Schoenwaelder <[e-mail]> wrote:
> 
>> = Section 4.2 =
>> 
>> (1) "It is generally a good idea to always configure
>>            an end time and to refresh the configuration
>>            of event object as needed to ensure that agents
>>            that loose connectivity to their controller
>>            do not continue their tasks forever."
>> 
>> This is more of a comment for the information model, but if this is worth saying here, shouldn't it (also) be mentioned in the corresponding information model definition in draft-ietf-lmap-information-model? This doesn't seem specific to this particular data model.
> 
> I see your point. I think it makes sense to note this in the second
> item 4. of section 3 of the information model. (I will change the
> numbering of the second list to use letters to make it easier to refer
> to things.)
> 
>       [...] For Event objects specifying a
>       series of events, it is generally a good idea to configure an end
>       time and to refresh the end time as needed to ensure that MAs
>       that loose connectivity to their controller do not continue
>       executing Schedules forever.

Sounds good.

> 
>> (2) For all the counters defined here, I think you need to specify the time period over which the counts are to be calculated.
> 
> We use counters as defined in RFC 6991 and I think the definition in
> RFC 6991 takes care of things.

Ok.

> 
>> (3) 'list action {
>>             key name;
>>             description
>>               "An action describes a task that is invoked by the
>>                schedule. Multiple actions are invoked sequentially.";'
>> 
>> I don't get this -- doesn't the execution mode define whether the actions are executed sequentially, in parallel, or pipelined?
> 
> Good catch. Changed this to:
> 
>           list action {
>             key name;
>             description
>               "An action describes a task that is invoked by the
> 		schedule. Multiple actions are invoked according to
>                the execution-mode of the schedule.”;

Sounds good.

> 
>> (4) "A queue is internally used to pass
>>                  results to another schedule."
>> 
>> I thought it was up to the implementation to decide how to implement this?
> 
> OK. I meanwhile understand that the word 'queue' has too many
> connotations. Is 'buffer' a less problematic term? The key here is
> that the data producer and the data consumer are in general not
> running at the time and hence data needs to be stored temporarily
> somewhere.

Isn’t this implied though? Even with “buffer” it still seems like specifying an implementation detail. I’d prefer to see something like your last sentence above instead.

> I will replace queue with buffer for now.
> 
>> (5) I don't understand why the 'program' elements are included in the task configurations and capabilities. It doesn't seem wise to allow the controller to tell the MA which program it needs to use to complete a task, and I don't understand why the MA would need to communicate that information to the controller either. I thought the recent list discussion indicated that if an MA was not capable of performing an action, that action would simply fail, which seems like all that is needed.
> 
> There are multiple things:
> 
> (a) How do I tell which task I want to have executed? The information
>    model assumes that this can be done with the help of a registry.
>    The YANG data model, in addition, allows to use a simple 'program
>    name'. Note that this is a choice, i.e., you either use a registry
>    or a program name, but not both.
> 
>    The registry at the end is just some level of indirection - but
>    this indirection also requires to have tasks registered in a
>    registry. Right now, the implementation I know of only supports
>    program names.
> 
> (b) The task list in the capabilities branch serves as an inventory,
>    i.e., it tells the controller which tasks are supported by a given
>    implementation. The other task list defined options that are used
>    when a certain task is invoked (the Task Configuration in the
>    information model). If a controller configures a task that the
>    agent does support (i.e., it is not listed in the capability
>    tasks), it will not be executed.
> 
> Note that a capability task name 'traceroute' exposed by the LMAP
> agent does not necessarily mean that there is a program called
> traceroute at the operating system level. In fact, an implementation
> could choose to run traceroute internally without an explicit system
> level process (like RIPE Atlas did everything in a big event loop, not
> sure whether this is still the case).

Ok, I can understand that under circumstances where there is no registry, there needs to be some identifier to indicate which task to run, and which tasks the MA is capable of running. But the examples seem to imply that ‘program’ is the path and file name of the actual executable on the MA, which is what seems dangerous and unnecessary to me. Why would the controller necessarily even know where such executables reside on the file system? And I know there are a lot of things that could go wrong if a Controller gets compromised, but it just seems like making it so trivial for an MA implementation to literally just run the executable name specified by the Controller creates unnecessary risk.

If the task name on its own is not sufficient for the MA to be able to figure out which program is suitable to run, why not have the additional field defined as ‘program-name’ with some guidance about how to populate it, so that, e.g., what you end up with in that field is “mtr” or “fping” instead of "/usr/bin/mtr” or "/usr/bin/fping”?

> 
>> Nits and minor comments:
> 
>> (2) "Implementers MUST taken care that
>>   option names and values are passed literally to programs.  In
>>   particular, it MUST be avoided that any shell expansions are
>>   performed that may alter the option names and values."
>> 
>> This text strikes me as a bit odd. Surely there are a whole selection of good programming practices that are necessary to ensure that things don't go haywire when implementing LMAP -- why call out these two with normative recommendations? Why does this guidance only apply to options? I would recommend having this text be non-normative, but if you do keep it I would suggest the following:
>> 
>> Implementers MUST take care that
>>   option names and values are passed literally to programs.  In
>>   particular, shell expansions that may alter the option names and values MUST NOT be performed.
> 
> I have no strong opinion on MUST vs must here and I usually happily
> follow the advice of IESG members on RFC 2119 keywords. ;-)

Ok, my suggestion is to use non-normative “ought to” rather than 2119 MUST.

Thanks,
Alissa


> 
> Yes, there are many ways to write broken code but this is such a
> common problem that I think can't hurt to remind implementors.
> 
> A diff of -10 to my working copy (of -11) can be found here:
> 
>  http://beadg.de/lmap/
> 
> /js
> 
> -- 
> Juergen Schoenwaelder           Jacobs University Bremen gGmbH
> Phone: [phone]         Campus Ring 1 | 28759 Bremen | Germany
> Fax:   [phone]         <http://www.jacobs-university.de/>