Subject: Yangdoctors last call review of draft-ietf-netconf-nmda-netconf-03

From: Ebben Aries <[email-masked]>
Date: Thu, 22 Feb 2018 16:01:16 -0800

Reviewer: Ebben Aries
Review result: Almost Ready

1 module in this draft:
- [email-masked]

YANG validation errors or warnings (from pyang 1.7.3 and yanglint 0.14.69)
- [email-masked]:171: warning: RFC 6087: 4.10,4.12:
  statement "enum" should have a "description" substatement (From pyang 1.7.3)

0 examples are provided in this draft (section 3.12 of
draft-ietf-netmod-rfc6087bis-18)

Module [email-masked]:
- Note: For the following imports, there are no references to the supporting
  documents as suggested in rfc6087bis however this item is currently under
  discussion on both usefulness/possible formatting
  - import "ietf-yang-types" should reference RFC 6991 per (not as a comment)
    https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis-18#section-4.7
  - import "ietf-inet-types" should reference RFC 6991 per (not as a comment)
    https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis-18#section-4.7
  - import "ietf-datastores" should reference I-D.ietf-netmod-revised-datastores per
    https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis-18#section-4.7
  - import "ietf-origin" should reference I-D.ietf-netmod-revised-datastores per
    https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis-18#section-4.7
  - import "ietf-netconf" should reference RFC 6241 per
    https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis-18#section-4.7
  - import "ietf-netconf-with-defaults" should reference RFC 6243 per
    https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis-18#section-4.7
- Module description, revision and feature definition should contain note to
  RFC Ed. to change placeholder reference to RFC when assigned
  https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis-18#appendix-C
- Security Considerations looks good and is adjusted to account for NETCONF
  only as well as addressing the additional RPCs introduced
- L82: suggested replacement of text:
  s/, i.e., the filter criteria are logically ANDed/. Multiple filters are
  processed as a logical AND operation/
- L127: suggested replacement of text:
  s/then the get-data/the get-data/

General comments/nits on draft text:
- As mentioned above, there are 0 examples in this draft.  There should be XML
  RPC examples of the 2 newly defined RPCs as well as usage examples of the
  augments of RFC6241 RPCs
- :with-origin    urn:ietf:params:netconf:capability:with-origin:1.0
  Wouldn't it make more sense to have this URN defined in
  I-D.ietf-netmod-revised-datastores rather?  In addition, should this
  capability and feature be rather defined as 'origin' in the 'ietf-origin'
  module?  As defined today, this feature is used as a clause for
  'origin-filter' as well as 'with-origin' inputs to get-data however these
  inputs appear to be mutually exclusive.
  IMHO, this should be detached from this specific draft which focuses on
  NETCONF specific base extensions for datastores as the returning and
  filtering of origin metadata can be transport independent.
- L308: suggested replacement of text:
  s/parameter is optional to support/parameter is optional and dependant off
  of the servers support for the 'origin' feature/ (dependant off the previous
  comment)
- L320: "ietf-netconf-nmda" (see Section 4). - This should be an actual
  reference as in line #306
- L345: "default-operation" parameter of the <edit-config> operation - This
  should at a bare minimum call reference to the appropriate section in
  RFC6241
- To the previous point, since there is a large amount of reference and
  comparison to RFC6241 definitions, any references should be tagged
  appropriately

---

From: Martin Bjorklund <[email-masked]>
Date: Fri, 23 Feb 2018 16:43:32 +0100 (CET)

Hi,

Thank you for this review.  Comments inline.

>   statement "enum" should have a "description" substatement (From pyang 1.7.3)

Yes, this is a warning b/c it is a SHOULD in 6087.  In this case, the
enum is described together with the rest in the main description.  I
think it gives a better overall description of the type.

>     https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis-18#section-4.7

I think we have to wait for the outcome of that discussion.  I would
prefer to add the reference statements.

>   https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis-18#appendix-C

Ok.

>   processed as a logical AND operation/

Hmm.  This misses the "i.e." - the original text explains the sentence
just before it.  What do others think?  Maybe leave this to the RFC
editor?

>   s/then the get-data/the get-data/

We have "if ... then ..." in several places.  Maybe also leave this to
the RFC editor?

>   RPC examples of the 2 newly defined RPCs as well as usage examples of the

This is a SHOULD in 6087.  I don't think that examples for these
operations would add much value.

>   filtering of origin metadata can be transport independent.

The capability is NETCONF-specific, and should be defined here, imo.
As for the feature, it is not as clear.  It is not obvious that it
should be defined in ietf-origin.  (and since that doc is at the RFC
editor, it might be late to change, unless it is really urgent).

>   of the servers support for the 'origin' feature/ (dependant off the previous

Do you mean that if we don't move the origin feature, this text should
not change?

>   reference as in line #306

I don't understand this comment.  It is a proper reference...?

>   should at a bare minimum call reference to the appropriate section in

There is a reference for <edit-config> to RFC 6241 in the first
paragraph of this section.  I think that is how references should be
done, but I might be wrong.  I am sure the RFC editor will fix this,
if needed.

>   comparison to RFC6241 definitions, any references should be tagged

I am not sure I understand what you mean with "tagged appropriately".
Could you provide OLD / NEW text for these occurences?

/martin

---

From: Ebben Aries <[email-masked]>
Date: Sat, 24 Feb 2018 20:14:37 -0700

Hi Martin - see inline...

> think it gives a better overall description of the type.

I'm fine w/ this - I agree

> >     https://urldefense.proofpoint.com/v2/url?u=https-3A__tools.ietf.org_html_draft-2Dietf-2Dnetmod-2Drfc6087bis-2D18-23section-2D4.7&d=DwICAg&c=HAkYuh63rsuhr6Scbfh0UjBXeMK-ndb3voDTXcWzoCI&r=GIehbDpQlo31lSi6WbnEkA&m=vjCdF0tPthdQp5vicuYUDK234W2DXJSvI9zSM0jaQS4&s=lYv-rQuwL9GaPVAIXkxrGgMI1AMjZg3yyhxbU2dUzZw&e=
> >     https://urldefense.proofpoint.com/v2/url?u=https-3A__tools.ietf.org_html_draft-2Dietf-2Dnetmod-2Drfc6087bis-2D18-23section-2D4.7&d=DwICAg&c=HAkYuh63rsuhr6Scbfh0UjBXeMK-ndb3voDTXcWzoCI&r=GIehbDpQlo31lSi6WbnEkA&m=vjCdF0tPthdQp5vicuYUDK234W2DXJSvI9zSM0jaQS4&s=lYv-rQuwL9GaPVAIXkxrGgMI1AMjZg3yyhxbU2dUzZw&e=
> >     https://urldefense.proofpoint.com/v2/url?u=https-3A__tools.ietf.org_html_draft-2Dietf-2Dnetmod-2Drfc6087bis-2D18-23section-2D4.7&d=DwICAg&c=HAkYuh63rsuhr6Scbfh0UjBXeMK-ndb3voDTXcWzoCI&r=GIehbDpQlo31lSi6WbnEkA&m=vjCdF0tPthdQp5vicuYUDK234W2DXJSvI9zSM0jaQS4&s=lYv-rQuwL9GaPVAIXkxrGgMI1AMjZg3yyhxbU2dUzZw&e=
> >     https://urldefense.proofpoint.com/v2/url?u=https-3A__tools.ietf.org_html_draft-2Dietf-2Dnetmod-2Drfc6087bis-2D18-23section-2D4.7&d=DwICAg&c=HAkYuh63rsuhr6Scbfh0UjBXeMK-ndb3voDTXcWzoCI&r=GIehbDpQlo31lSi6WbnEkA&m=vjCdF0tPthdQp5vicuYUDK234W2DXJSvI9zSM0jaQS4&s=lYv-rQuwL9GaPVAIXkxrGgMI1AMjZg3yyhxbU2dUzZw&e=
> >     https://urldefense.proofpoint.com/v2/url?u=https-3A__tools.ietf.org_html_draft-2Dietf-2Dnetmod-2Drfc6087bis-2D18-23section-2D4.7&d=DwICAg&c=HAkYuh63rsuhr6Scbfh0UjBXeMK-ndb3voDTXcWzoCI&r=GIehbDpQlo31lSi6WbnEkA&m=vjCdF0tPthdQp5vicuYUDK234W2DXJSvI9zSM0jaQS4&s=lYv-rQuwL9GaPVAIXkxrGgMI1AMjZg3yyhxbU2dUzZw&e=
> >     https://urldefense.proofpoint.com/v2/url?u=https-3A__tools.ietf.org_html_draft-2Dietf-2Dnetmod-2Drfc6087bis-2D18-23section-2D4.7&d=DwICAg&c=HAkYuh63rsuhr6Scbfh0UjBXeMK-ndb3voDTXcWzoCI&r=GIehbDpQlo31lSi6WbnEkA&m=vjCdF0tPthdQp5vicuYUDK234W2DXJSvI9zSM0jaQS4&s=lYv-rQuwL9GaPVAIXkxrGgMI1AMjZg3yyhxbU2dUzZw&e=
> 

Since we've been actively suggesting to add into the reference
statements, I suggest to just go ahead w/ the syntax you previously
described.  If we have a different outcome, then 6087bis can be updated
as well as modification to modules if time permits.

> >   https://urldefense.proofpoint.com/v2/url?u=https-3A__tools.ietf.org_html_draft-2Dietf-2Dnetmod-2Drfc6087bis-2D18-23appendix-2DC&d=DwICAg&c=HAkYuh63rsuhr6Scbfh0UjBXeMK-ndb3voDTXcWzoCI&r=GIehbDpQlo31lSi6WbnEkA&m=vjCdF0tPthdQp5vicuYUDK234W2DXJSvI9zSM0jaQS4&s=Bsh4kYXQ9T8sGblwrJnk41asnQCqSuUZjzvla2EgA9E&e=
> 
> Ok.
> 

We can leave this to the RFC editor

> We have "if ... then ..." in several places.  Maybe also leave this to

We can leave this to the RFC editor

> operations would add much value.

More examples is generally a good thing and provides clarity to
implementors.  While it is a SHOULD, RFC6241 carries examples across all
sections and propose this extension follow the same format.

> The capability is NETCONF-specific, and should be defined here, imo.

Understood.  I guess what I would have imagined is rather a capability
indicating that the operational datastore supports the population of
'origin' metadata.  Support of that capability would then indicate
support of the 'with-origin' parameter which is just a toggle to return
metadata or not.

Per draft-ietf-netmod-revised-datastores, it appears that the population
of origin metadata is mandatory however the wording appears pretty vague
around such.

This capability exists in both this draft as well as the RESTCONF draft
and am trying to understand if origin metadata is mandatory for support
in <operational> then why is it necessary to have a capability just for
support of this parameter vs. a capability of origin metadata support as
a whole?

If the capability is meant to convey 'origin' metadata support as a
whole, then I think it should be named 'origin' rather than
'with-origin' (which is just the parameter name we are discussing)

> editor, it might be late to change, unless it is really urgent).

It seems odd to me that we wouldn't define the feature along side the
annotation/identity definitions but rather here in a protocol specific
module.  I think that the population of 'origin' metadata support as a
whole should be the feature, this should exist in the ietf-origin module
and 'origin-filter' and 'with-origin' parameters would still be gated by
this feature that is defined elsewhere.

As is defined today, the feature description even indicates server
support for the annotation as a whole.

  feature origin {
    description
      "Indicates that the server supports the 'origin' annotation.";
    reference
      "RFC XXXX: Network Management Datastore Architecture";
  }

> Do you mean that if we don't move the origin feature, this text should

The suggestion above is merely to add a bit more description around this
parameter being gated off the server's support for populating origin
metadata.  If the server supports origin metadata, then it is optional
and the client can choose to use/not-use.  If the server does not
support origin metadata, then this parameter is not supported or
visible.

> I don't understand this comment.  It is a proper reference...?

I would assume reference to [I-D.ietf-netmod-revised-datastores] here
but there are a number of other spots where this exists as well.  This
can be left to the RFC editor as well

> done, but I might be wrong.  I am sure the RFC editor will fix this,

We can leave this to the RFC editor

> Could you provide OLD / NEW text for these occurences?

Same comment as above.  This can be left to the RFC editor and don't
believe all definitions need to be referenced.

Thx

/ebben

---

From: Benoit Claise <[email-masked]>
Date: Mon, 26 Feb 2018 23:21:37 +0100

>> operations would add much value.
Note that it's a SHOULD in a BCP (RFC6087bis)
I heard before the argument that 6087bis was not a standard. It's not 
the case any longer with RFC6087bis, on the next IESG telechat

Regards, Benoit

---

From: Martin Bjorklund <[email-masked]>
Date: Wed, 28 Feb 2018 13:17:38 +0100 (CET)

Hi,

> as well as modification to modules if time permits.

I have added references for now.

> sections and propose this extension follow the same format.

Ok, done.

> support of the 'with-origin' parameter which is just a toggle to return

Ok.  After some discussion with my co-authors, we suggest that we
remove the ":with-origin" capability, and just use the feature
"origin".  In fact, the YANG module ietf-netconf-nmda already uses
this feature to control both the "with-origin" and "origin-filter"
parameters, and the feature is advertised on servers that support the
"origin" annotation.  The YANG module doesn't even mention the
"with-origin" capability.

> of origin metadata is mandatory however the wording appears pretty vague

The architecure document defines this concept, but I think that
different protocols may or may not expose this; for example, a
protocol for constrained devices might choose to not support this at
all.

/martin