Subject: [netconf] Yangdoctors last call review of draft-ietf-netconf-ssh-client-server-24

From: Andy Bierman via Datatracker <[email-masked]>
Date: Tue, 25 May 2021 12:21:12 -0700

Reviewer: Andy Bierman
Review result: Ready

Comments:

1) Measuring Interoperability for groupings and identities

[same comment for SSH and TLS drafts]

These modules are intentionally abstract.
There are no protocol-accessible objects defined at all.
Interoperability is usually measured in the context of a
specific protocol (e.g., NETCONF).

There is an assumption that interoperability will be achieved
by some other RFCs that will have "uses" statements to create
protocol-accessible or otherwise implementable objects.

There is also an assumption that the groupings will be used the
same everywhere, and the only difference will be the
path from root to the objects in these groupings.
In fact, the "refine" statement allows each usage to be
different.

Perhaps the drafts should mention these interoperability issues.

2) same feature names in 2 modules

  - feature userauth-hostbased
  - feature userauth-none
  - feature userauth-password
  - feature userauth-publickey

The ietf-ssh-client and ietf-ssh-server modules both use these
feature names. IMO users will not expect this, and this will cause
confusion.

Why can't these features be defined once in ietf-ssh-common.yang?
Seems like client and server will advertise the feature for
implementing their relevant values.

---

From: Kent Watsen <[email-masked]>
Date: Thu, 27 May 2021 19:05:21 +0000

[moving all alias except “netconf” to the BCC list]

Hi Andy,

Thank you for your review!

> On May 25, 2021, at 3:21 PM, Andy Bierman via Datatracker <[email-masked]> wrote:
> 
> Reviewer: Andy Bierman
> Review result: Ready
> 
> Comments:
> 

True, and we have, e.g., the "netconf-client-server" and "restconf-client-server" drafts, and the PCE WG has the "pcep-yang” draft.   There’s a leap of faith, but also some running code, so hopefully within the intentions of an “RFC”.

> In fact, the "refine" statement allows each usage to be

What assumption?  Do you mean the description statements in the various "[ssh/tls]-[client/server]-grouping” groupings.  For instance, see the 2nd paragraph below:

     grouping ssh-client-grouping {
       description
         "A reusable grouping for configuring a SSH client without
          any consideration for how an underlying TCP session is
          established.

          Note that this grouping uses fairly typical descendent
          node names such that a stack of 'uses' statements will
          have name conflicts.  It is intended that the consuming
          data model will resolve the issue (e.g., by wrapping
          the 'uses' statement in a container called
          'ssh-client-parameters').  This model purposely does
          not do this itself so as to provide maximum flexibility
          to consuming models.";

While it is true that “stacks” (or sibling “uses” statements) are expected (from a "best practice” POV), I’m unaware of any Xpath expressions enforcing this.

> Perhaps the drafts should mention these interoperability issues.

Could you be more specific in what text you’d like to see in which sections?

> implementing their relevant values.

Well, these were named differently in just the previous version of the ssh draft…from the change log:

 *  Renamed "client-auth-*" to "userauth-*"
 *  Renamed "client-identity-*" to "userauth-*”

Juergen felt that these were more natural names for SSH.   I agreed, mostly just to get the “monkey off my back”, but it breaks with the convention used by the collection if client-server drafts (where all the variations of "[client/server]-[auth/identity]” are typically used).  

Yes, “userauth” is the term used in the SSH drafts, and I didn’t think is was a big deal or, really, any deal at all, for the same feature names to be used in both drafts, such that I'm surprised by your "users will not expect this, and this will cause confusion” comment...

Moving the definitions to the “common” module is possible, of course, but tricky since the description statements say different things, to address the different contexts in which they’re used.  The description statement could be expanded to address each context, but this will get wordy…I wonder if it wouldn’t be better to just go back to the previous “client-[auth/identity]-*” prefixes - thoughts?

K.

---

From: Andy Bierman <[email-masked]>
Date: Fri, 28 May 2021 16:59:11 -0700

On Thu, May 27, 2021 at 12:05 PM Kent Watsen <[email-masked]> wrote:

> True, and we have, e.g., the "netconf-client-server" and
> "restconf-client-server" drafts, and the PCE WG has the "pcep-yang” draft.
>  There’s a leap of faith, but also some running code, so hopefully within
> the intentions of an “RFC”.
>
>
> What assumption?  Do you mean the description statements in the various
> "[ssh/tls]-[client/server]-grouping” groupings.  For instance, see the 2nd
> paragraph below:
>
> While it is true that “stacks” (or sibling “uses” statements) are expected
> (from a "best practice” POV), I’m unaware of any Xpath expressions
> enforcing this.
>
>

I was re-reading RFC 7950 and there is nothing to add in these drafts about
it.
Groupings do not define any nodes in the schema tree.

In the old days the IETF used to be concerned with Interoperability Reports
in order to advance an RFC from Proposed Standard to Draft Standard.
Now that that is no longer done, it is not that important to point out that
interoperability can only be evaluated for the module with the "uses"
statement,
since that is how schema tree nodes are defined from a grouping template.

> Could you be more specific in what text you’d like to see in which
see bove -- no changes requested

> Juergen felt that these were more natural names for SSH.   I agreed,
> mostly just to get the “monkey off my back”, but it breaks with the
> convention used by the collection if client-server drafts (where all the
> variations of "[client/server]-[auth/identity]” are typically used).
>
> Yes, “userauth” is the term used in the SSH drafts, and I didn’t think is
> was a big deal or, really, any deal at all, for the same feature names to
> be used in both drafts, such that I'm surprised by your "users will not
> expect this, and this will cause confusion” comment...
>
> Moving the definitions to the “common” module is possible, of course, but
> tricky since the description statements say different things, to address
> the different contexts in which they’re used.  The description statement
> could be expanded to address each context, but this will get wordy…I wonder
> if it wouldn’t be better to just go back to the previous
> “client-[auth/identity]-*” prefixes - thoughts?
>
>
OK -- not a big deal except possibly when the feature name is used outside
of YANG,
without any prefix to tell which one is meant.  I agree the different
description-stmts
make it difficult to write a common feature definition, so this is not
important to change either.

> K.
Andy