Re: [netmod] derived-from() vs derived-from-or-self() in acl model

[Per Hedeland <[e-mail]>]

On 2018-02-26 23:02, Mahesh Jethanandani wrote:
> 
> 
>> On Feb 26, 2018, at 1:31 PM, Per Hedeland <[e-mail]> wrote:
>>
>> On 2018-02-26 20:20, Mahesh Jethanandani wrote:
>>>> On Feb 26, 2018, at 9:01 AM, Per Hedeland <[e-mail] <mailto:[e-mail]>> wrote:
>>>>
>>>> [Adding Cc [e-mail] <mailto:[e-mail]>]
>>>>
>>>> On 2018-02-26 14:24, Ladislav Lhotka wrote:
>>>>> Per Hedeland <[e-mail] <mailto:[e-mail]>> writes:
>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> A customer of ours using one of the draft versions of the
>>>>>> ietf-access-control-list module reported that it was not possible to
>>>>>> configure an ethernet ace with type acl:eth-acl-type, due to the
>>>>>> derived-from() in
>>>>>>
>>>>>>               container eth {
>>>>>>                 when "derived-from(../../../../type, " +
>>>>>>                      "'acl:eth-acl-type')";
>>>>>>                 if-feature match-on-eth;
>>>>>>                 uses pf:acl-eth-header-fields;
>>>>>>                 description
>>>>>>                   "Rule set that matches ethernet headers.";
>>>>>>               }
>>>>>>
>>>>>> evaluating to "false". I pointed out that this is correct behavior of
>>>>>> our SW, since acl:eth-acl-type is not derived from acl:eth-acl-type, and
>>>>>> it would need to be derived-from-or-self() in order to evaluate to
>>>>>> "true". I also ventured a guess (not having followed the development of
>>>>>> the acl model in detail) that the intent was that vendors should define
>>>>>> their own identities, that actually *were* derived from acl:eth-acl-type
>>>>>> (and ditto for all the other *-acl-type identities, of course).
>>>>>>
>>>>>> However I'm not at all sure that the guess is correct, and if so why
>>>>>> this should be *enforced* by excluding the base identity. And having a
>>>>>> look at the example in section 4.3 of draft-ietf-netmod-acl-model-16, it
>>>>>> seems to be doing exactly what our customer tried, alhough with
>>>>>> ipv4-acl-type:
>>>>>>
>>>>>> <data xmlns="urn:ietf:params:xml:ns:netconf:base:1.0">
>>>>>>   <access-lists xmlns="urn:ietf:params:xml:ns:yang:ietf-access-control-list">
>>>>>>     <acl>
>>>>>>       <name>sample-ipv4-acl</name>
>>>>>>       <type>ipv4-acl-type</type>
>>>>>>       <aces>
>>>>>>         <ace>
>>>>>>           <name>rule1</name>
>>>>>>           <matches>
>>>>>>             <l3>
>>>>>>               <ipv4>
>>>>>>                 <protocol>tcp</protocol>
>>>>>>
>>>>>> As far as I can see, this snippet is invalid for the model, since the
>>>>>> 'ipv4' container has
>>>>>>
>>>>>>               container ipv4 {
>>>>>>                 when "derived-from(../../../../type, " +
>>>>>>                      "'acl:ipv4-acl-type')";
>>>>>>
>>>>>> - and ipv4-acl-type is *not* derived from ipv4-acl-type. (Of course
>>>>>> there shouldn't be any <l3> element either, but that's another thing.)
>>>>>>
>>>>>> So, is it the case that the derived-from()s should actually be
>>>>>> derived-from-or-self()s, or is the example wrong?
>>>>>
>>>>> This has to do with the irreflexivity property of identity derivation,
>>>>> which is, in my view, an unnecessary complication. It would be simpler
>>>>> but sufficient to define derivation as a reflexive relation, and have
>>>>> only one "derived-from()" XPath function.
>>>>
>>>> Be that as it may, it is obviously not what RFC 7950 says.
>>> I would agree that that is not how I am reading RFC 7950. For now my choice is to change all the derived-from() to derived-from-or-self().
>>
>> I think that is a useful change. FWIW, when I actually tried the example
>> as the payload of an <edit-config> towards our NETCONF server, I found
>> some other problems, which I believe are still relevant with the pull
>> request applied:
>>
>> 1) (Aldready mentioned) the <l3> element corresponding to the choice in
>> the model should not be present (RFC 7950 section 7.9.5).
> 
> Removed <l3>. Although I have say the tree diagram confused me. It identifies l3 as a node with a +rw next to it, while for ipv4 it says +.

Hm... - the tree diagram has

         |        |  +--rw (l3)?
         |        |  |  +--:(ipv4)
         |        |  |  |  +--rw ipv4 {match-on-ipv4}?

Where "(l3)?" is the choice, ":(ipv4)" is the case (not present in the
YANG module since it makes use of the "shorthand" per RFC 7950 section
7.9.2), and "ipv4" is the container (with its if-feature). The
latest/last tree diagram draft (-06) doesn't say anything in particular
about <flags> for choice/case, so it would seem that it should be "rw"
for both choice and case in configuration data. I.e. the case should be
shown as

         |        |  |  +--rw :(ipv4)

Latest pyang from github shows a 'case' as the first variant above
though, i.e. without both <flags> and whitespace before the name. I
guess Martin will fix...

--Per

>> 2) "tcp" is not a valid value for the 'protocol' leaf - from
>> [e-mail]:
>>
>>     leaf protocol {
>>       type uint8;
>>       description
>>         "Internet Protocol number. Refers to the protocol of the
>>          payload. In IPv6, this field is known as 'next-header.";
>>       reference "RFC 719, RFC 2460.";
>>     }
>>
>> I.e. it should be "6". And the reference for this leaf, as well as for
>> 'length' and 'ttl', should presumably be RFC 791, not RFC 719.
> 
> Changed it to 6, and 719 to 791.
> 
>>
>> 3) 11.11.11.1/24 (for 'destination-ipv4-network') and 10.10.10.1/24 (for
>> 'source-ipv4-network') are not valid (canonical) values for
>> inet:ipv4-prefix - from ietf-inet-types.yang:
>>
>>       The canonical format of an IPv4 prefix has all bits of
>>       the IPv4 address set to zero that are not part of the
>>       IPv4 prefix.";
>>
>> And of course it doesn't make much sense to have bits outside the prefix
>> length set for a match specification. It seems the pull request changes
>> these prefixes to RFC 1918 / RFC 5737 space, but those prefixes have
>> the same issue.
> 
> Made the host part of the network address 0.
> 
>>
>> I'm attaching the output from a <get-config> towards our NETCONF server
>> with these issues fixed (and the derived-from() ->
>> derived-from-or-self() change made).
> 
> Thanks.
> 
>>
>> --Per
>>
>>>>
>>>>> Identities that are considered "abstract" should not be instantiated, and
>>>>> then derived-from() and derived-from-or-self() give the same result.
>>>>
>>>> So I guess your take is that the *-acl-type identities derived from
>>>> acl:acl-base in the ietf-access-control-list module should be considered
>>>> "abstract", and thus the example should not use ipv4-acl-type for the
>>>> 'type' leaf, but instead some other identity derived from
>>>> acl:ipv4-acl-type. Do the authors agree?
>>>>
>>>> --Per
>>>>
>>>>> Lada
>>>>>
>>>>>>
>>>>>> --Per Hedeland
>>>>>>
>>>>>> _______________________________________________
>>>>>> netmod mailing list
>>>>>> [e-mail] <mailto:[e-mail]>
>>>>>> https://www.ietf.org/mailman/listinfo/netmod
>>> Mahesh Jethanandani
>>> [e-mail] <mailto:[e-mail]>
>>
>> <example.xml>
> 
> Mahesh Jethanandani
> [e-mail]
>