Re: [Last-Call] Genart last call review of draft-ietf-netconf-ssh-client-server-37

[Kent Watsen <kent+ietf@xxxxxxxxxx>]

Hi Elwyn,

Thank you for your valuable comments.

Please find responses below.

PS: language used that implies that something has been fixed (e.g., Fixed!)

Should always be read to mean “fixed in my local copy (in git) and will be

part of the next published update, which will likely occur early next week."

Kent

On Feb 14, 2024, at 1:52 PM, Elwyn Davies via Datatracker <noreply@xxxxxxxx> wrote:

Reviewer: Elwyn Davies
Review result: Almost Ready

I am the assigned Gen-ART reviewer for this draft. The General Area
Review Team (Gen-ART) reviews all IETF documents being processed
by the IESG for the IETF Chair.  Please treat these comments just
like any other last call comments.

For more information, please see the FAQ at

<https://wiki.ietf.org/en/group/gen/GenArtFAQ>.

Document: draft-ietf-netconf-ssh-client-server-37
Reviewer: Elwyn Davies
Review Date: 2024-02-14
IETF LC End Date: 2024-02-12
IESG Telechat date: 2024-02-29

Summary:Almost ready.  The majority of points are editorial nits, however the
proposed mechanism for  generating YANG modules to provide automated access to
certain sets of options defined in IANA registries is not explained at all in
the introduction and needs to be.

I added this sentence to the Introduction (and a similar sentence to the “tls-client-server”

draft, that also defines an IANA-maintained module…

    This document only defines that the four IANA modules exist, and

    presents a script in Appendix A that IANA may use to generate the YANG

    modules.  This document does not publish initial versions of these

    four modules.  IANA publishes these modules.

Good?

I also feel that the authors need to consider
(and may have already done this) whether they should supply automation scripts
that will assist IANA in creating and checking the updated YANG modules that
they are to be asked to generate when the relevant registry entries are
updated.

Your very last comment below refers the "the program”.  Is this program the

"automation script" you seek?

I have not attempted to check that the IETF YANG modules are correct
or complete as I do not have the SSH knowledge at my fingertips.

True, they are too unwieldy to inspect manually.

I wonder if
the mechanism needs a designated expert to deal with naming queries and help
with any issues in creating the IANA YANG modules rather than passing this
burden to the NETCONF WG chair - which rather assumes the NETCONF WG will be
around for ever.

I believe that you are correct in that it should not point to the NETCONF WG chairs.

We similarly removed “NETCONF WG” from the IANA-registrations defined in Section

6.1 to instead point to "The IESG”.

Should we point to "The IESG” here as well?  Or do you mean that the registry should

define an “expert” to be appointed?

Major issues:
None

Minor issues:

General: The document contains a number of references to the NETCONF WG chairs
and the NETCONF WG mailing list. Is this adequately future proof?

Being discussed above.

General: Although this is not directly relevant to the draft, it occurs to me
that since IANA is being requested to edit YANG modules in response to registry
changes and the resulting modules would be expected to be read by protocol
implementations, it would be desirable if IANA was supplied with scripts that
could be used to automatically update the IANA modules and run the YANG checker
script to ensure the syntactical correctness of the updates.  The changes
resulting from these updates should be automatically backwards compatible so
updating the modules should not be problematic.

This document defines an “automation script” in the Appendix.  It is nearly exactly

as you seek.  The only delta is that it doesn’t try to incorporate the “revision” history

from previously defined modules.  This is a difficult thing to code in advance of the

IANA publishing initial versions of the modules, and yet easy for IANA representative

familiar with text-editors to do.

There are two choices:

1. Add text explain to IANA how to copy the revision section from the previously

published YANG module.

2. Somehow enhance the script to analyze the previously published module…but

this entails at least *knowing* the URLs that will be used...

Thoughts?

S2.1.1:  The last paragraph of
this section reads:

The diagram above uses syntax that is similar to but not defined in
  [RFC8340].

In that case had the syntax better be defined in this document?

The syntax is pretty obvious, right?  - especially after looking at RFC 8340?

FYI, there are 11 instances of that phrase in this document, and many more

instances spread across all nine documents.  Even just adding a second

sentence as small as “The structure represents the hierarchical relationship

between elements.” would take time…

Is this a must-have?

Nits/editorial comments:

Abstract, para 2:  I suggest s/enabling/supporting/ since the YANG modules
provide a standardised framework rather than actually providing the only way of
configuration of SSH entities.

Fixed, and also a similar paragraph in the “tls-client-server” draft.

Abstract, para 3:  Similarly, s/for an IANA-maintained/providing support for an
IANA-maintained/.

Fixed, and also a similar paragraph in the “tls-client-server” draft.

Introduction, s1:  This section is very bald.  In particular, the introduction
is silent about the purpose of the IANA modules.  It should, in the way of
convention, contain similar words to paragraphs 2 and 3 of the abstract to
explain the purpose of the document.

I copied the abstract text into the Introduction.  It’s better.

Similar edit in the “tls-client-server” draft.

The section should also  contain a subsection similar to s1.1 to explain the
purposes of the IANA modules and how they are created and maintained since the
draft only defines the format and not the exact contents of the YANG modules.

Right. As mentioned above, it now contains this text:

    This document only defines that the four IANA modules exist, and

    presents a script in Appendix A that IANA may use to generate the YANG

    modules.  This document does not publish initial versions of these

    four modules.  IANA publishes these modules.

Good?

s1.1, para 1: s/more so than/rather than/, s/advance/extended/

Fixed, and also a similar paragraph in the “tls-client-server” draft.

s1.2, para 1: as with the Abstract s/enable/support/

Fixed.  This update affects all nine drafts.

Table 1:  This table should have a RFC Editor note to clarify that the right
hand column needs to be updated with the references to the RFCs that will
hopefully result from the approval of the referenced I-Ds.  For convenience of
readers, I hope that the keys will be of the form RFCxxxx to reduce reader
effort in working out what documents are reference.

I don’t understand.  This is a normal reference that will automatically be

converted to its final published RFC reference without any extra instruction.

Is there anything more to do here?

That said, this draft does also use placeholder values like RFC AAAA, RFC 

BBBB, etc., and there is a note to the Editor (at top, immediately after the 

Abstract) to make those substitutions.

s1.4: The jargon <operational> should be replaced by 'operational state
datastore' with a reference to Section 5.3 of RFC 8342.

“Jargon" is a bit overstated.   It is a formal term in the YANG ecosphere.

It’s just a terminology-import away from being proper.  Far from jargon...

That said, I can’t fault your proposed text, as it seems easier to do than

importing a term, and plausibly more proper.

Even still, I’m beginning to question the need for that section at all.  See

https://mailarchive.ietf.org/arch/msg/netmod/pL5Mb3S1e9UEqzepPtNGwMUBQ1o/

Stay tuned!

s2.3:
OLD:
rpc generate-asymmetric-key-pair {
      if-feature "asymmetric-key-pair-generation";
      description
        "Requests the device to generate an public key using
         the specified key algorithm.";
NEW:
rpc generate-asymmetric-key-pair {
      if-feature "asymmetric-key-pair-generation";
      description
        "Requests the device to generate a public key using
         the specified key algorithm.";
END

Fixed.   No need to also fix in the “ietf-tls-common” module.

ss6.3-6.6:  In the 5th para of the boilerplate text in each of these 4 sections:
s/is a invalid for a YANG identity/is not a lexically valid name for a YANG
identity/

Fixed, and also in the “tls-client-server” draft.

Also 4 paragraphs from the end of each section:
s/that points to the where the module was  published./that points to the
document defining the registry update that resulted in this change./\

Fixed, and also in the “tls-client-server” draft.

Appendix A:  I think the intention of the instruction to remove Appendix A
before publication only applies to the program in the header section rather
than the whole Appendix which contains the initial creations of the IANA
modules.  I take it that the program will be rerun if necessary after the draft
has been approved in case there have been registry updates since the last draft
update.

Oops, I had the “removeInRFC” attribute set.

That said, the intention is to remove the 4 subsections, while leaving just the top-level section defining the code.   The is very strong guidance that initial IANA modules SHOULD NOT be published in RFCs.  Whether the code is published also likely doesn’t matter.

I updated the note to the Editor (at top, after the Abstract) to remove the four subsections.

Thanks again!

Kent

-- 
last-call mailing list
last-call@xxxxxxxx
https://www.ietf.org/mailman/listinfo/last-call