Subject: [Netconf] draft-ietf-netconf-notification-messages: YANG validation issues

From: Benoit Claise <[email-masked]>
Date: Mon, 2 Oct 2017 10:46:26 +0200

Dear authors,

See http://www.claise.be/IETFYANGPageCompilation.html
Note: you can download the extracted YANG module on the web page, to 
make sure the line # are similar

Regarding the pyang error:

    [email-masked]:220: error: the
    'yang-data' extension must have exactly one child that is a container
    [email-masked]:237: error: the
    'yang-data' extension must have exactly one child that is a container

https://tools.ietf.org/html/rfc8040#section-8

    It MUST contain data definition statements
    that result in exactly one container data node definition.

Note that a similar issue (choice with yang-data) was discussed on the 
NETCONF mailing list.
See the "yang-data cannot encode a choice statement?" email thread.

For the yumadum-pro errors, I'll let Andy investigate:

    Error: Got 'anydata', Expected: right brace
    [email-masked]:227.5: error(245): wrong
    token type

    Error: Got 'semicolon', Expected: body statement
    [email-masked]:232.25: error(245): wrong
    token type

    Error: Extra input after end of module starting on line 237
    [email-masked]:237.3: error(281): extra node

    ***
    *** 3 Errors, 0 Warnings

Regards, B.

---

From: Andy Bierman <[email-masked]>
Date: Mon, 2 Oct 2017 11:58:08 -0700

On Mon, Oct 2, 2017 at 1:46 AM, Benoit Claise <[email-masked]> wrote:

> Note: you can download the extracted YANG module on the web page, to make
> sure the line # are similar
>
> [email-masked]:220: error: the 'yang-data'
> extension must have exactly one child that is a container
> [email-masked]:237: error: the 'yang-data'
> extension must have exactly one child that is a container
>
> [email-masked]:227.5: error(245): wrong token
> type
>

The error handled code needs to be fixed so it is more clear that it is
reporting the same error:

 /* Message to a receiver containing one notification.*/
  rc:yang-data message {
    container message-header {
      description
        "delineates header info from content for easy parsing.";
      uses notification-header;
      uses message-header;
    }
    anydata notification-contents {
      description
        "Non-header info of what actually got sent to receiver.  This
        includes after any content based security filtering.  Need to
        determine if/how it is viable not to include tag encapsulation
        for this object.";
    }
  }

IMO the entire special-case handling of yang-data should be redone.
Comments on example above pushing yang-data past its intended usage:

1) The "message" parameter does not get used anywhere, ant it gives
    people the impression they can declare duplicate siblings in the same
namespace

     rc:yang-data foo {
       container X;
     }

     rc:yang-data bar {
       container X;
     }

This is actually invalid because the assigned XML namespace is the same for
both /X nodes.

2) Using anydata as the notification contents hides the schema for the real
notification.
    How does a client know which 'real' schema to apply to parse each
specific event type?

> Regards, B.

Andy

---

From: "Eric Voit (evoit)" <[email-masked]>
Date: Mon, 2 Oct 2017 21:57:09 +0000

Hi Andy,
Hi Benoit,

From: Andy Bierman, October 2, 2017 2:58 PM

On Mon, Oct 2, 2017 at 1:46 AM, Benoit Claise <[email-masked]<mailto:[email-masked]>> wrote:
Dear authors,

See http://www.claise.be/IETFYANGPageCompilation.html
Note: you can download the extracted YANG module on the web page, to make sure the line # are similar

Regarding the pyang error:
[email-masked]:220<mailto:[email-masked]:220>: error: the 'yang-data' extension must have exactly one child that is a container
[email-masked]:237<mailto:[email-masked]:237>: error: the 'yang-data' extension must have exactly one child that is a container
https://tools.ietf.org/html/rfc8040#section-8

<eric> Yep, missed that, will fix.

Note that a similar issue (choice with yang-data) was discussed on the NETCONF mailing list.
See the "yang-data cannot encode a choice statement?" email thread.

For the yumadum-pro errors, I'll let Andy investigate:
Error: Got 'anydata', Expected: right brace
[email-masked]:227.5<mailto:[email-masked]:227.5>: error(245): wrong token type

Error: Got 'semicolon', Expected: body statement
[email-masked]:232.25<mailto:[email-masked]:232.25>: error(245): wrong token type

Error: Extra input after end of module starting on line 237
[email-masked]:237.3<mailto:[email-masked]:237.3>: error(281): extra node

***
*** 3 Errors, 0 Warnings

The error handled code needs to be fixed so it is more clear that it is reporting the same error:

1) The "message" parameter does not get used anywhere, ant it gives
    people the impression they can declare duplicate siblings in the same namespace

     rc:yang-data foo {
       container X;
     }

     rc:yang-data bar {
       container X;
     }

This is actually invalid because the assigned XML namespace is the same for both /X nodes.

<eric> In the case for notification-messages, there need not be the same top level nodes used for within a container.  But in general, I see how this could get confusing for other users of yang-data.

2) Using anydata as the notification contents hides the schema for the real notification.
<eric> As there will be many notifications, and more will be added, I think this is a required part of a solution.  We need a level of indirection to pull in other elements of the schema.

    How does a client know which 'real' schema to apply to parse each specific event type?
<eric> The namespace and message name are the first elements of after the header.  If that is not sufficient, we could pull these out as additional notification headers.  (That isn’t shown now.)

Eric

Regards, B.

Andy

---

From: Andy Bierman <[email-masked]>
Date: Tue, 3 Oct 2017 10:11:05 -0700

On Mon, Oct 2, 2017 at 2:57 PM, Eric Voit (evoit) <[email-masked]> wrote:

> Hi Andy,
>
> Hi Benoit,
>
>
>
> *From:* Andy Bierman, October 2, 2017 2:58 PM
>
> <eric> In the case for notification-messages, there need not be the same
> top level nodes used for within a container.  But in general, I see how
> this could get confusing for other users of yang-data.
>
>
>

The problem in this draft is that the defined structures describe 2
consecutive containers,
which would be an invalid XML representation.

So a compliant yang-data def would be :

rc:yang-data message {
  container message {
     container message-header { ... }
     anydata notification-contents { ... }
  }
}

IMO YANG is mostly useless for what you are trying to do here.
YANG has no substitutionGroup like XSD.
You cannot say "place a node with any name here, that matches the
replacement tag".
Instead YANG provides a named container (notification-contents) and inside
that container
there is an arbitrary subtree.

The simple mapping from "child of <notification>" to "YANG
notification-stmt" seems to be missing.

Andy

> <eric> As there will be many notifications, and more will be added, I
> think this is a required part of a solution.  We need a level of
> indirection to pull in other elements of the schema.
>
>
>
> <eric> The namespace and message name are the first elements of after the
> header.  If that is not sufficient, we could pull these out as additional
> notification headers.  (That isn’t shown now.)
>
>
>
> Eric
>
>
>
>
>
>
>
> Regards, B.
>
>
>
>
>
> Andy
>
>
>

---

From: "Eric Voit (evoit)" <[email-masked]>
Date: Tue, 3 Oct 2017 17:24:27 +0000

From: Andy Bierman, October 3, 2017 1:11 PM

On Mon, Oct 2, 2017 at 2:57 PM, Eric Voit (evoit) <[email-masked]<mailto:[email-masked]>> wrote:
Hi Andy,
Hi Benoit,

***
*** 3 Errors, 0 Warnings

     rc:yang-data foo {
       container X;
     }

     rc:yang-data bar {
       container X;
     }

The problem in this draft is that the defined structures describe 2 consecutive containers,
which would be an invalid XML representation.

rc:yang-data message {
  container message {

<eric> exactly.  I was hoping to remove an unnecessary level of encapsulation from the perspective of the updates, but that doesn’t seem possible with our available constructs.  New version out shortly will adjust this back.

IMO YANG is mostly useless for what you are trying to do here.
YANG has no substitutionGroup like XSD.
You cannot say "place a node with any name here, that matches the replacement tag".

<eric> I have been poking around the original XSD for 5277, and don’t see comparable mapping available.

Instead YANG provides a named container (notification-contents) and inside that container
there is an arbitrary subtree.

<eric> That seems to be what we have.  Per my earlier comment above, I am going to extract the notification name and yang model name from the namespace and place into separate headers at least for now.  Logic used for matching to the individual notification schemas shouldn’t be too hard for developers to figure out from there.

The simple mapping from "child of <notification>" to "YANG notification-stmt" seems to be missing.

<eric> Agree.  I have been working on that today too.

Eric

Andy

Eric

Regards, B.

Andy

---

From: "Eric Voit (evoit)" <[email-masked]>
Date: Tue, 3 Oct 2017 23:11:49 +0000

YANG Model embedded now matches Andy’s suggestions below on container structure, other small tweaks embedded.  More to be done.  Compile has returned ok, clearing that issue.

Eric

draft-ietf-netconf-notification-messages-02.txt:
xym 0.4:
Extracting 'ietf-notification-messages.yang'

[email-masked]:
pyang 1.7.3: pyang --verbose --ietf -p {libs} {model}:
No validation errors

yanglint 0.13.69: yanglint --verbose -p {rfclib} -p {draftlib} -p {tmplib} {model}:
No validation errors

From: Andy Bierman, October 3, 2017 1:11 PM
To: Eric Voit (evoit) <[email-masked]>
Cc: Benoit Claise (bclaise) <[email-masked]>; [email-masked]; NETCONF <[email-masked]>
Subject: Re: draft-ietf-netconf-notification-messages: YANG validation issues

***
*** 3 Errors, 0 Warnings

     rc:yang-data foo {
       container X;
     }

     rc:yang-data bar {
       container X;
     }

rc:yang-data message {
  container message {

Andy

Eric

Regards, B.

Andy

---

From: Benoit Claise <[email-masked]>
Date: Wed, 4 Oct 2017 14:34:45 +0200

Thanks Eric,

One last error, reported by yumadump-pro

Error: No key present for list 'notifications' on line 286
[email-masked]:301.7: error(272): data missing

***
*** 1 Errors, 0 Warnings

   rc:yang-data bundled-message {
     container bundled-message {
       presence
         "Indicates attempt to communicate notifications to a receiver.";
       description
         "Message to a receiver containing many bundled notifications";
       container bundled-message-header {
         description
           "Header info for messages.";
         uses message-header {
           refine message-time {
             mandatory true;
           }
         }
         leaf notification-count {
             type uint16;
             description
                 "Quantity of notification records in a bundled-message
                 specific receiver.";
         }
       }
       list notifications { <========== line 286
         description
           "Set of notifications to a receiver.";
         container notification-header {
           description
             "Header info for each bundled notification.";
           uses notification-header;
         }
         anydata notification-contents {
           description
             "Encapsultates objects following YANG's notification-stmt
             grammar of RFC-7950 section 14.  Within are the notified
             objects the publisher actually generated in order to be
             passed to a receiver after all filtering has completed.";
         }
       }
     }
   }

Regards, Benoit
>
> YANG Model embedded now matches Andy’s suggestions below on container 
> structure, other small tweaks embedded.  More to be done.  Compile has 
> returned ok, clearing that issue.
>
> Eric
>
> *From:*Andy Bierman, October 3, 2017 1:11 PM
> *To:* Eric Voit (evoit) <[email-masked]>
> *Cc:* Benoit Claise (bclaise) <[email-masked]>; 
> [email-masked]; NETCONF 
> <[email-masked]>
> *Subject:* Re: draft-ietf-netconf-notification-messages: YANG 
> validation issues
>
>     *From:*Andy Bierman, October 2, 2017 2:58 PM
>
>         Note: you can download the extracted YANG module on the web
>         page, to make sure the line # are similar
>
>             [email-masked]:220
>             <mailto:[email-masked]:220>:
>             error: the 'yang-data' extension must have exactly one
>             child that is a container
>             [email-masked]:237
>             <mailto:[email-masked]:237>:
>             error: the 'yang-data' extension must have exactly one
>             child that is a container
>
>             [email-masked]:227.5
>             <mailto:[email-masked]:227.5>:
>             error(245): wrong token type
>
>             [email-masked]:232.25
>             <mailto:[email-masked]:232.25>:
>             error(245): wrong token type
>
>             [email-masked]:237.3
>             <mailto:[email-masked]:237.3>:
>             error(281): extra node
>
>             ***
>             *** 3 Errors, 0 Warnings
>
>     <eric> In the case for notification-messages, there need not be
>     the same top level nodes used for within a container.  But in
>     general, I see how this could get confusing for other users of
>     yang-data.
>
>     <eric> As there will be many notifications, and more will be
>     added, I think this is a required part of a solution. We need a
>     level of indirection to pull in other elements of the schema.
>
>     <eric> The namespace and message name are the first elements of
>     after the header.  If that is not sufficient, we could pull these
>     out as additional notification headers. (That isn’t shown now.)
>
>     Eric
>
>         Regards, B.
>
>     Andy
>

---

From: "Eric Voit (evoit)" <[email-masked]>
Date: Wed, 4 Oct 2017 13:47:41 +0000

Hi Benoit,

Per RFC7590 section 7.8.2, the list statement only needs a key when representing config data.  These elements in a notification/yang-data certainly aren’t traditional config data nodes.

The immediate answer you might have is that we could require “config false” for containers under notifications or yang data.   But when I do that for this model, pyang gives me warnings.
[email-masked]:273: warning: explicit config statement is ignored
So I don’t really want to upload a new version of this model that gives this warning.

I think that a more flexible answer would be that the test tool can safely assume “config false” for notifications or yang-data.  This should get rid of this class of yang errors.

Eric

P.S.: If you are including after-the-upload YANG error checking by yumadump-pro as part of compliance, is there any way we could make it part of the during-the-upload validation?  This will help avoid unnecessary draft version churn.

From: Benoit Claise, October 4, 2017 8:35 AM
To: Eric Voit (evoit) <[email-masked]>; Andy Bierman <[email-masked]>
Cc: [email-masked]; NETCONF <[email-masked]>
Subject: Re: draft-ietf-netconf-notification-messages: YANG validation issues

Thanks Eric,

Error: No key present for list 'notifications' on line 286
[email-masked]:301.7<mailto:[email-masked]:301.7>: error(272): data missing

***
*** 1 Errors, 0 Warnings

Regards, Benoit

Eric

[email-masked]<mailto:[email-masked]>:
pyang 1.7.3: pyang --verbose --ietf -p {libs} {model}:
No validation errors

From: Andy Bierman, October 3, 2017 1:11 PM
To: Eric Voit (evoit) <[email-masked]><mailto:[email-masked]>
Cc: Benoit Claise (bclaise) <[email-masked]><mailto:[email-masked]>; [email-masked]<mailto:[email-masked]>; NETCONF <[email-masked]><mailto:[email-masked]>
Subject: Re: draft-ietf-netconf-notification-messages: YANG validation issues

***
*** 3 Errors, 0 Warnings

     rc:yang-data foo {
       container X;
     }

     rc:yang-data bar {
       container X;
     }

rc:yang-data message {
  container message {

Andy

Eric

Regards, B.

Andy

---

From: Benoit Claise <[email-masked]>
Date: Wed, 4 Oct 2017 18:04:38 +0200

Hi Eric,
>
> Hi Benoit,
>
> Per RFC7590 section 7.8.2, the list statement only needs a key when 
> representing config data.  These elements in a notification/yang-data 
> certainly aren’t traditional config data nodes.
>
> The immediate answer you might have is that we could require “config 
> false” for containers under notifications or yang data.   But when I 
> do that for this model, pyang gives me warnings.
>
> I think that a more flexible answer would be that the test tool can 
> safely assume “config false” for notifications or yang-data.  This 
> should get rid of this class of yang errors.
>
This one is for Andy's tool.
>
> Eric
>
> P.S.: If you are including after-the-upload YANG error checking by 
> yumadump-pro as part of compliance, is there any way we could make it 
> part of the during-the-upload validation? This will help avoid 
> unnecessary draft version churn.
>
And this one is for Andy as well.

Regards, Benoit
>
> *From:*Benoit Claise, October 4, 2017 8:35 AM
> *To:* Eric Voit (evoit) <[email-masked]>; Andy Bierman 
> <[email-masked]>
> *Cc:* [email-masked]; NETCONF 
> <[email-masked]>
> [email-masked]:301.7 
> <mailto:[email-masked]:301.7>: error(272): 
> data missing
>
> ***
> *** 1 Errors, 0 Warnings
>
>
>     YANG Model embedded now matches Andy’s suggestions below on
>     container structure, other small tweaks embedded.  More to be
>     done.  Compile has returned ok, clearing that issue.
>
>     Eric
>
>     [email-masked]
>     <mailto:[email-masked]>:
>
>     *To:* Eric Voit (evoit) <[email-masked]> <mailto:[email-masked]>
>     *Cc:* Benoit Claise (bclaise) <[email-masked]>
>     <mailto:[email-masked]>;
>     [email-masked]
>     <mailto:[email-masked]>;
>     NETCONF <[email-masked]> <mailto:[email-masked]>
>         On Mon, Oct 2, 2017 at 1:46 AM, Benoit Claise
>         <[email-masked] <mailto:[email-masked]>> wrote:
>
>             Dear authors,
>
>             Note: you can download the extracted YANG module on the
>             web page, to make sure the line # are similar
>
>         <eric> In the case for notification-messages, there need not
>         be the same top level nodes used for within a container.  But
>         in general, I see how this could get confusing for other users
>         of yang-data.
>
>         added, I think this is a required part of a solution.  We need
>         a level of indirection to pull in other elements of the schema.
>
>         <eric> The namespace and message name are the first elements
>         of after the header.  If that is not sufficient, we could pull
>         these out as additional notification headers.  (That isn’t
>         shown now.)
>
>         Eric
>
>             Regards, B.
>
>         Andy
>

---

From: Andy Bierman <[email-masked]>
Date: Wed, 4 Oct 2017 09:40:39 -0700

On Wed, Oct 4, 2017 at 9:04 AM, Benoit Claise <[email-masked]> wrote:

> false” for containers under notifications or yang data.   But when I do
> that for this model, pyang gives me warnings.
>
> I think that a more flexible answer would be that the test tool can safely
> assume “config false” for notifications or yang-data.  This should get rid
> of this class of yang errors.
>

I will fix the tool so it does not think yang-data is config=true.

> yumadump-pro as part of compliance, is there any way we could make it part
> of the during-the-upload validation?  This will help avoid unnecessary
> draft version churn.
>

I will discuss this one off-line with you.

Andy

>
>
>
>
> *From:* Benoit Claise, October 4, 2017 8:35 AM
> *To:* Eric Voit (evoit) <[email-masked]> <[email-masked]>; Andy Bierman
> <[email-masked]> <[email-masked]>
> <[email-masked]> <[email-masked]>
> *Subject:* Re: draft-ietf-netconf-notification-messages: YANG validation
> issues
>
>
>
> Thanks Eric,
>
> *From:* Andy Bierman, October 3, 2017 1:11 PM
> *To:* Eric Voit (evoit) <[email-masked]> <[email-masked]>
> *Cc:* Benoit Claise (bclaise) <[email-masked]> <[email-masked]>;
> <[email-masked]> <[email-masked]>
> *Subject:* Re: draft-ietf-netconf-notification-messages: YANG validation
> issues
>
>
>
>
>
>
>