# Andy Newton, ART AD, comments for draft-ietf-netconf-netconf-client-server-38
CC @anewton1998

* line numbers:
  - https://author-tools.ietf.org/api/idnits?url=https://www.ietf.org/archive/id/draft-ietf-netconf-netconf-client-server-38.txt&submitcheck=True

* comment syntax:
  - https://github.com/mnot/ietf-comments/blob/main/format.md

* "Handling Ballot Positions":
  - https://ietf.org/about/groups/iesg/statements/handling-ballot-positions/

## Discuss

## Comments

Thanks for your hard work on this document. I have just one nit.

## Nits

### Number of Yang models

This is minor, but can line 183 be changed to just "This document presents
YANG models" as it appears to have more than one as indicated by the other
introductory paragraph?

175        This document presents two YANG [RFC7950] modules, one module to
176        configure a NETCONF [RFC6241] client and the other module to
177        configure a NETCONF server.  Both modules support both NETCONF over
178        SSH [RFC6242] and NETCONF over TLS [RFC7589] and NETCONF Call Home
179        connections [RFC8071].

181     1.1.  Relation to other RFCs

183        This document presents one or more YANG modules [RFC7950] that are
184        part of a collection of RFCs that work together to, ultimately,
185        support the configuration of both the clients and servers of both the
186        NETCONF [RFC6241] and RESTCONF [RFC8040] protocols.

Thank you to Daniel Migault for the secdir review.

Section 4.1, 4.2, References:  Why is QUIC mentioned?  There are no configurations for either QUIC or UDP in the body of the draft.  Consider removing it from the SC and references.

Section 4.1 and 4.2, para 2:  Please replace the last sentence with "The YANG-based management protocols have to use a secure transport layer such as SSH [RFC4252], TLS [RFC8446], or QUIC [RFC9000].  The YANG-based management protocols also have to use mutual authentication."  [and if QUIC is removed, remove it here too.]

Section 4:  While RFC6241 doesn't reference BCP195 (because it predated it), RFC8040 does.  Those statements need to be updated to include RFC9523 and BCP195. Perhaps a statement like this: 'Implementations SHOULD also refer to [BCP195] for additional details.'

Hi Kent, 

Thank you for the perseverance to push this forward. 

Please find some comments below:

# Not only configuration but also state retrieval

The document reasons about “configuration”/”configure” while the model can also be used to read operations. Please consider updating through the document/module “configure” to “manage.”

Let me know if you want me to share my marked version with the suggested edits.

# It is a Data Model

Please change at least the title as follows: 

OLD: NETCONF Client and Server Models
NEW: NETCONF Client and Server Data Models

There are other places where some terminology alignment is needed.

# Not drafts anymore

Many of your RFC XXXX were already published as RFCs. Please update accoridnlgy.

# Check terms not used in the document 

CURRENT:
   Various examples in this document use "BASE64VALUE=" as a placeholder
   value for binary data that has been base64 encoded (per Section 9.8
   of [RFC7950]).  This placeholder value is used because real base64
   encoded structures are often many lines long and hence distracting to
   the example being presented.

I failed to find such uses in the document. Please double check.

# Don’t suffix data with their type

CURRENT:
   *  netconf-client-initiate-stack-grouping
   *  netconf-client-listen-stack-grouping
   *  netconf-client-app-grouping

No need to suffix all groupings with “-grouping”. Also, reading things such as “*-grouping" grouping” is heavy. I know that you used such constructs in other documents of your document sent, but NETCONF have this removed in recent reviewed documents (udp groupings, for example). I’d be consistent with latest practices in the WG here.

# Redundant data nodes

CURRENT:
          +--:(ssh) {ssh-initiate}?
          |  +-- ssh
          |     +-- tcp-client-parameters
          |     |  +---u tcpc:tcp-client-grouping
          |     +-- ssh-client-parameters
          |     |  +---u sshc:ssh-client-grouping
          |     +-- netconf-client-parameters
          |        +--u ncc:netconf-client-grouping
          +--:(tls) {tls-initiate}?
             +-- tls
                +-- tcp-client-parameters
                |  +---u tcpc:tcp-client-grouping
                +-- tls-client-parameters
                |  +---u tlsc:tls-client-grouping
                +-- netconf-client-parameters
                   +---u ncc:netconf-client-grouping

The non-expanded display does not reveal this, but there is IMO room to avoid optimize a bit the structure.

For example, any reason why we don’t factorize tcp-client-parameters and netconf-client-parameters for both ssh/tls?

At least for TCP, I understand this is because of a refine statement. Does it worth to have duplicated nodes here? Why no go for SSH port number as default given that this is the MTI for NETCONF?

Still, netconf-parameters part can be factorized between these transports.

The same comment applies for netconf-client-listen-stack-grouping, netconf-server-callhome-stack-grouping, etc.

# Broken trees

Keys are mandatory, not optional. Please check all your lists:

OLD:
     grouping netconf-client-app-grouping:
       +-- initiate! {ssh-initiate or tls-initiate}?
       |  +-- netconf-server* [name]
       |     +-- name?                 string
       |     +-- endpoints
       |     |  +-- endpoint* [name]
       |     |     +-- name?                                     string
       |     |     +---u netconf-client-initiate-stack-grouping
       |     +-- connection-type
       |     |  +-- (connection-type)
       |     |     +--:(persistent-connection)
       |     |     |  +-- persistent!
       |     |     +--:(periodic-connection)
       |     |        +-- periodic!
       |     |           +-- period?         uint16
       |     |           +-- anchor-time?    yang:date-and-time
       |     |           +-- idle-timeout?   uint16
       |     +-- reconnect-strategy
       |        +-- start-with?     enumeration
       |        +-- max-wait?       uint16
       |        +-- max-attempts?   uint8
       +-- listen! {ssh-listen or tls-listen}?
          +-- idle-timeout?   uint16
          +-- endpoints
             +-- endpoint* [name]
                +-- name?                                   string
                +---u netconf-client-listen-stack-grouping

NEW:
     grouping netconf-client-app-grouping:
       +-- initiate! {ssh-initiate or tls-initiate}?
       |  +-- netconf-server* [name]
       |     +-- name                 string
       |     +-- endpoints
       |     |  +-- endpoint* [name]
       |     |     +-- name                                   string
       |     |     +---u netconf-client-initiate-stack-grouping
       |     +-- connection-type
       |     |  +-- (connection-type)
       |     |     +--:(persistent-connection)
       |     |     |  +-- persistent!
       |     |     +--:(periodic-connection)
       |     |        +-- periodic!
       |     |           +-- period?         uint16
       |     |           +-- anchor-time?    yang:date-and-time
       |     |           +-- idle-timeout?   uint16
       |     +-- reconnect-strategy
       |        +-- start-with?     enumeration
       |        +-- max-wait?       uint16
       |        +-- max-attempts?   uint8
       +-- listen! {ssh-listen or tls-listen}?
          +-- idle-timeout?   uint16
          +-- endpoints
             +-- endpoint* [name]
                +-- name                                    string
                +---u netconf-client-listen-stack-grouping


OLD:
     grouping netconf-server-app-grouping:
       +-- listen! {ssh-listen or tls-listen}?
       |  +-- idle-timeout?   uint16
       |  +-- endpoints
       |     +-- endpoint* [name]
       |        +-- name?                                   string
       |        +---u netconf-server-listen-stack-grouping
       +-- call-home! {ssh-call-home or tls-call-home}?
          +-- netconf-client* [name]
             +-- name?                 string
             +-- endpoints
             |  +-- endpoint* [name]
             |     +-- name?                                     string
             |     +---u netconf-server-callhome-stack-grouping
             +-- connection-type
             |  +-- (connection-type)
             |     +--:(persistent-connection)
             |     |  +-- persistent!
             |     +--:(periodic-connection)
             |        +-- periodic!
             |           +-- period?         uint16
             |           +-- anchor-time?    yang:date-and-time
             |           +-- idle-timeout?   uint16
             +-- reconnect-strategy
                +-- start-with?     enumeration
                +-- max-wait?       uint16
                +-- max-attempts?   uint8

NEW:
     grouping netconf-server-app-grouping:
       +-- listen! {ssh-listen or tls-listen}?
       |  +-- idle-timeout?   uint16
       |  +-- endpoints
       |     +-- endpoint* [name]
       |        +-- name                                   string
       |        +---u netconf-server-listen-stack-grouping
       +-- call-home! {ssh-call-home or tls-call-home}?
          +-- netconf-client* [name]
             +-- name                 string
             +-- endpoints
             |  +-- endpoint* [name]
             |     +-- name                                     string
             |     +---u netconf-server-callhome-stack-grouping
             +-- connection-type
             |  +-- (connection-type)
             |     +--:(persistent-connection)
             |     |  +-- persistent!
             |     +--:(periodic-connection)
             |        +-- periodic!
             |           +-- period?         uint16
             |           +-- anchor-time?    yang:date-and-time
             |           +-- idle-timeout?   uint16
             +-- reconnect-strategy
                +-- start-with?     enumeration
                +-- max-wait?       uint16
                +-- max-attempts?   uint8

# Use terminology consistent with base specs

For example, 

•	s/Protocol-accessible Nodes/Protocol-accessible Data Nodes 
•	s/call-home/call home
•	s/Protocol-accessible nodes are those nodes/Protocol-accessible nodes are those data nodes

# Examples

JSON examples would be more convenient for readers, but I understand this is subjective.

# IETF Module template

Please update to follow the template at: https://datatracker.ietf.org/doc/html/draft-ietf-netmod-rfc8407bis-22#name-template-for-ietf-modules.

Also, update to 2025.

# Remove boilerplate from the module

CURRENT:
        The key words 'MUST', 'MUST NOT', 'REQUIRED', 'SHALL',
        'SHALL NOT', 'SHOULD', 'SHOULD NOT', 'RECOMMENDED',
        'NOT RECOMMENDED', 'MAY', and 'OPTIONAL' in this document
        are to be interpreted as described in BCP 14 (RFC 2119)
        (RFC 8174) when, and only when, they appear in all
        capitals, as shown here.";

Can be deleted as the only use is not adequate. Please consider:

OLD:
                 description
                   "NETCONF/TLS clients MUST pass some
                    authentication credentials.";

NEW:
                 description
                   "NETCONF/TLS clients must pass some
                    authentication credentials.";

This is a description, not a requirement. The protocol compliance is ensured by the ”must client-identity” statement.

# Avoid embedding reference in the description

Use reference statement for that. For example, I suggest to delete “described in RFC 8071” from the description of the netconf-client-listen-stack-grouping grouping.

# On periodic-connection  

I’m not asking to change this, but I guess reusing one of the groupings in draft-ietf-netmod-schedule-yang would be appropriate, but let’s not delay this spec further ;-)

# Authoritative reference for defaults

CURRENT:
                   leaf period {
                     type uint16;
                     units "minutes";
                     default "60"; 

Or

                   leaf idle-timeout {
                     type uint16;
                     units "seconds";
                     default 180; // three minutes

(there are many occurrences in the module)

Consider adding a reference where the default is defined. Thanks.

# Check enum is appropriate

CURRENT:
             leaf start-with {
               type enumeration {

Are we sure we won’t define any other strategy? For example, last connected for a given AF and the like.

# Please run pyang to have the modules in the canonical order

There are some places where I don’t think the canonical order is followed.

# Factorize among modules

CURRENT:
           container connection-type {
             description
               "Indicates the NETCONF server's preference for how the
                NETCONF connection is maintained.";

Wonder whether you considered factorizing among modules as this one, for example, has the same data nodes as netconf-client-app-grouping

# Security considerations

No need to repeat common considerations, one single section with specific cons for each model called out would be OK.

Also, please update to follow the latest version of the template: https://github.com/netmod-wg/rfc8407bis/blob/main/templates/sec-template.txt 

# Save a question from IANA and align with the guidance in the 8407bis

NEW:
      name:         ietf-netconf-client
      namespace:    urn:ietf:params:xml:ns:yang:ietf-netconf-client
      prefix:       ncc
      maintained by IANA? No
      reference:    RFC HHHH

      name:         ietf-netconf-server
      namespace:    urn:ietf:params:xml:ns:yang:ietf-netconf-server
      prefix:       ncs
      maintained by IANA? N
      reference:    RFC HHHH

Cheers,
Med

Thank you to Ines Robles for the GENART review.