Subject: Re: Yangdoctors last call review of draft-ietf-ospf-yang-09

From: "Acee Lindem (acee)" <[email-masked]>
Date: Mon, 8 Jan 2018 19:28:53 +0000

Hi Lada,

Apologies for the delay. We somewhat got hung up on 4 and 6. See inline.

On 12/6/17, 6:26 AM, "Ladislav Lhotka" <[email-masked]> wrote:

>Reviewer: Ladislav Lhotka
>Review result: Ready with Issues
>
>The data model defined in this document is a massive piece of work: it
>consists of 11 YANG modules and defines around 1200 schema nodes. The
>"ietf-ospf@2017-10-30" module is compatible with the NMDA architecture.
>
>**** Comments
>
>1. Unless there is a really compelling reason not to do so, the
>   "ietf-ospf" should declare YANG version 1.1. For one,
>   "ietf-routing" that is being augmented by "ietf-ospf" already
>   declares this version. Some of my suggestions below also assume
>   version 1.1.

We will add this. 

>
>2. The "ietf-ospf" can work only with the new NMDA-compatible
>   revisions of some modules, such as "ietf-interfaces" and
>   "ietf-routing". I understand it is not desirable to import such
>   modules by revision, but at least it should be mentioned in a
>   description attached to every such import.

We will add comments to the description.
>
>3. Maybe the draft could mention that implementations should supply a
>   default routing domain as a system-controlled resource.

Isn’t this more of an RFC8022BIS statement? I guess we could state this as
an assumption. 
>
>4. In "when" expressions, the module uses literal strings for
>   identities. This is known to be problematic, the XPath functions
>   derived-from() or derived-from-or-self() should be used instead.

Why is this problematic? Is it because the types can be extended?

>
>5. Some enumerations, such as "packet-type" and "if-state-type"
>   define enum identifiers with uppercase letters and/or underscores,
>   for example "Database-Description" or "LONG_WAIT". RFC6087bis
>   recommends that only lowercase letters, numbers and dashes. I think
>   this convention should be observed despite the fact that the
>   current names are traditionally used in OSPF specs. The
>   "ietf-routing" module also defines "router-id" even though the
>   documents use "Router ID".

I agree - we will follow the RFC 6087BIS guidelines.

>
>6. The types of LSA headers are modelled as integers. While OSPF gurus
>   probably know these numbers by heart, it is not very
>   reader-frienly. So at least some references to documents defining
>   these numbers should be provided, but my suggestion is to consider
>   implementing them with identities. It seems it might also be useful
>   to define some "abstract" identities for these types. For example,
>   if "opaque-lsa" is defined, then the definition of container
>   "opaque" could simply use
>
>     when "derived-from(../../header/type, 'ospf:opaque-lsa')";
>
>   instead of
>
>      when "../../header/type = 9 or "
>              + "../../header/type = 10 or "
>              + "../../header/type = 11";

I guess I don’t see the identities as always being better. We will
consider this one. 

>
>7. The title of sec. 2.9 should be "OSPF Notifications" rather than
>   "OSPF notification".

Sure. 

Thanks,
Acee

---

From: Ladislav Lhotka <[email-masked]>
Date: Tue, 09 Jan 2018 08:51:35 +0100

Hi Acee,

please see inline.

> > Review result: Ready with Issues

...

> Isn’t this more of an RFC8022BIS statement? I guess we could state this as

Probably, but it is not a YANG issue, so I'd leave it to you routing folks to
decide.

> Why is this problematic? Is it because the types can be extended?

That's one reason: derived identities should often also satisfy the constraint.

But the more serious problem is that things like

    when "../../../../../../../rt:type = 'ospf:ospfv3'"

rely on plain string comparison that depends od the actual prefix used for the
"rt:type" value. For one, according to RFC 7951 the JSON encoding of this value
would be "ietf-ospf:ospfv3" so the above expression is always false. 

> I guess I don’t see the identities as always being better. We will

In what situations could the numbers be better?

...

Thanks, Lada

-- 
Ladislav Lhotka
Head, CZ.NIC Labs
PGP Key ID: 0xB8F92B08A9F76C67

---

From: Martin Bjorklund <[email-masked]>
Date: Tue, 09 Jan 2018 09:06:48 +0100 (CET)

Hi,

> would be "ietf-ospf:ospfv3" so the above expression is always false. 

This is not correct; the when expression is not evaluated on the JSON
encoding.  See the last paragraph of section 9.10.3 in RFC 7950:

   The string value of a node of type "identityref" in a "must" or
   "when" XPath expression is the referred identity's qualified name
   with the prefix present.  If the referred identity is defined in an
   imported module, the prefix in the string value is the prefix defined
   in the corresponding "import" statement.  Otherwise, the prefix in
   the string value is the prefix for the current module.

So the equality test of the identityref is correct.

However, I agree that in most cases 'derived-from-or-self' should be
used, in order to handle derived identities.

/martin

---

From: Ladislav Lhotka <[email-masked]>
Date: Tue, 09 Jan 2018 16:08:56 +0100

>    the string value is the prefix for the current module.

This is weird, to say the least. The leafref instance may have an identity value
that is defined in a module that (has to be implemented by the server but)
needn't be imported in the module that contains the XPath expression. So I don't
know what 'corresponding "import" statement' this paragraph is talking about.

Also, potentially there can be a collision in prefixes and then this also breaks
down.

A moral of the namespace/prefix story in XML was that relying of namespace
prefixes having a particular value is a really bad idea. I know that the cited
paragraph was intended to make such XPath string comparisons more deterministic,
but it is also problematic and should be avoided if possible. 

Lada

---

From: Ladislav Lhotka <[email-masked]>
Date: Tue, 09 Jan 2018 16:23:03 +0100

On Tue, 2018-01-09 at 16:08 +0100, Ladislav Lhotka wrote:
> > > Probably, but it is not a YANG issue, so I'd leave it to you routing folks
> > 
> > to
> > 
> > > decide.
> > > 
> > > >  
> > > > 
> > > That's one reason: derived identities should often also satisfy the
> > 
> > constraint.
> > 
> > > 
> > > rely on plain string comparison that depends od the actual prefix used for
> > 
> > the
> > 
> > > "rt:type" value. For one, according to RFC 7951 the JSON encoding of this
> > 
> > value
> > 
> This is weird, to say the least. The leafref instance may have an identity 

Sorry, I meant "identityref" here --------^

Lada

> value

---

From: Martin Bjorklund <[email-masked]>
Date: Tue, 09 Jan 2018 16:23:11 +0100 (CET)

> know what 'corresponding "import" statement' this paragraph is talking about.

It has to import the module in order to give a prefix, which then can
be used in the XPath expression.

> Also, potentially there can be a collision in prefixes and then this also breaks

No, two modules cannot be imported with the same prefix.

> but it is also problematic and should be avoided if possible.

Note that this prefix is under the control of the module designer
writing the XPath expression.  The same identityref value might use a
different prefix in some other module.

/martin

---

From: Ladislav Lhotka <[email-masked]>
Date: Tue, 09 Jan 2018 17:08:21 +0100

> be used in the XPath expression.

In the XPath expression above, do you mean the "rt" prefix of "rt:type"? If so,
it is irrelevant for the string comparison, what's important is the *value* of
the "rt:type" instance, which can be an identity defined in a module that
needn't be imported by ietf-routing, ietf-ospf or whatever. Sec. 9.10.2:

   On a particular server, the valid values are further restricted to the set of
     identities defined in the modules implemented by the server.

> No, two modules cannot be imported with the same prefix.

I have to disagree. An identity derived from the "ietf-routing:control-protocol-
type" base identity can be defined in a module that is not imported anywhere. If
a server declares such a module as implemented, then "rt:type" may have this
value per sec. 9.10.2.

And, consequently, there may be two different modules with conflicting prefixes
defining identities that are derived from "ietf-routing:control-protocol-type". 

> writing the XPath expression.  The same identityref value might use a

No, it is not. The prefixes appear in instance data.

Lada

---

From: Martin Bjorklund <[email-masked]>
Date: Wed, 10 Jan 2018 10:34:28 +0100 (CET)

Hi,

I think we can agree that the model in this I-D should use
derived-from-or-self() instead of string comparison, and conclude this
discussion here.  I suggest that if we need to further discuss the
representation of identityrefs, then we start a new thred on the
NETMOD ML.

/martin

---

From: "Acee Lindem (acee)" <[email-masked]>
Date: Mon, 22 Jan 2018 18:42:07 +0000

Hi Martin, Lada, 

In experimenting with this, I get YANGLINT validation errors. I’m not sure what I’m missing but the first argument to derived-from()/derive-from-or-self() is the schema node and the second is the identity – correct?
For example, the following YANG leaf:

identity ospf-protocol {
    base "rt:routing-protocol";
    description "Any version the OSPF protocol";
  }

  identity ospfv2 {
    base "ospf-protocol";
    description "OSPFv2";
  }

  identity ospfv3 {
    base "ospf-protocol";
    description "OSPFv3";
  }

leaf routing-protocol-name {
      type leafref {
        path "/rt:routing/rt:control-plane-protocols/"
           + "rt:control-plane-protocol/rt:name";
      }
      must "derived-from( "
	+ "/rt:routing/rt:control-plane-protocols/"
        + "rt:control-plane-protocol[rt:name=current()]/"
        + "rt:type, ospf:ospf-protocol)";
      description
       "OSPF routing protocol instance name.";
    }

Gives me: 

warn: Schema node "ietf-ospf:ospf-protocol" not found (derived-from( /ietf-routing:routing/ietf-routing:control-plane-protocols/ietf-routing:control-plane-protocol[ietf-routing:name=current()]/ietf-routing:type, ietf-ospf:ospf-protocol) with context node "/ietf-ospf:if-state-change/routing-protocol-name".

Thanks
Acee 

On 1/10/18, 4:36 AM, "Martin Bjorklund" <[email-masked]> wrote:

    Hi,

    /martin