I2NSF Registration Interface YANG Data Model for NSF Capability Registration
draft-ietf-i2nsf-registration-interface-dm-26

# John Scudder, RTG AD, comments draft-ietf-i2nsf-registration-interface-dm-23
CC @jgscudder

## COMMENTS

### Section 3, misused RFC 2119 keyword

In "After an NSF is registered with Security Controller, some modifications on the capability of the NSF MAY be required later" I suspect you really meant "may" in the ordinary English sense as a synonym for "might" or "could", and not MAY in the RFC 2119 sense? If you really mean to express a protocol requirement here, please say more, otherwise please change to use one of the normal English options.

### Section 4.2, missing text, misused RFC 2119 keyword

The first paragraph of Section 4.2 ends in what appears to be a sentence fragment. It looks like something's missing there.

The second paragraph begins with "Security Controller MAY require some additional capabilities to serve the security service request from an I2NSF User, but none of the registered NSFs has the required capabilities." The same comment applies as for Section 3.

### Section 5.1.1 wrong reference

When you reference RFC 8431, did you mean RFC 8340?

## Notes

This review is in the ["IETF Comments" Markdown format][ICMF], You can use the
[`ietf-comments` tool][ICT] to automatically convert this review into
individual GitHub issues. 

[ICMF]: https://github.com/mnot/ietf-comments/blob/main/format.md
[ICT]: https://github.com/mnot/ietf-comments

The shepherd writeup seems to have skipped answering part of the first question: Why is this the right document status?

I'm also confused by the answer to question 18.

[copied from my otherwise-resolved DISCUSS]

It also makes me wonder if this shouldn't be updating some other document if you're extending required behavior of an I2NSF component that was defined someplace else.  I looked around for a document defining "security controller" and couldn't find an obvious one, so I'm at a loss for what to suggest.

Thanks for working on this specification. Thanks for Brian Trammell for the TSVART review.

I have two comments and I think it will improve the document if addressed -

# Section 4.1.2 : it says -

      "Note that TCP is used as a transport layer protocol due to either NETCONF or RESTCONF. "

   I am not sure I understand this, seems like the sentence is incomplete. Specially when UDP, SCTP and DCCP is also included in the capabilities.. Why was TCP called out in this section alone. what am I missing?

# QUIC was not mentioned here, but then shouldn't is also have some text describing why it is not included like other i2nfs documents does now?

Thank you for the work put into this document. 

Please find below some non-blocking COMMENT points (but replies would be appreciated *especially* for the use of "input" and "output", i.e., I was about to ballot a DISCUSS but this does not fit the DISCUSS criteria), and some nits.

Special thanks to Linda Dunbar for the shepherd's write-up including the WG consensus *but* it lacks the justification of the intended status. 

The IPR section of the shepherd's write-up is *incorrect* as it claims that only 2 IPR disclosures have been done while there are 5 in the data tracker. At least, the IETF Last Call has a reference to the five disclosures, so this did not cause any problem.

I hope that this review helps to improve the document,

Regards,

-éric


# DISCUSS (blocking)

As noted in https://www.ietf.org/blog/handling-iesg-ballot-positions/, a DISCUSS ballot is a request to have a discussion on the following topics:

## 

# COMMENTS (non blocking)

## IPR disclosure

Should the shepherd's write-up be updated ? See above.

## NMDA

I have often seen in abstracts and introductions sentences like `The YANG data model in this document conforms to the Network Management Datastore Architecture (NMDA) defined in RFC 8342.`Did the author consider using the same sentence ?

## Section 4.1

Should NSF names include platform or version ? I.e., a NSF name such as "firewall-example" sounds like really broad.

## Section 4.1.2

s/an IP address /one or several IP address(es) / ?

## Section 5.1.2.1

I find the use of 'input' for the writable part and 'output' for the read-only part confusing... Should "status" be used rather than "output", esp. for a network function.

The "ip" cardinality is "?" while it should rather be "*".

Redefining "nsf-specification" rather than importing another module with similar capabilities would be easier. Also, "model" is wide open and underspecified.

## Section 5.2

Having a leaf named "ip" being an union with "inet:domain-name" seems really weird.

# NITS (non blocking / cosmetic)

## Section 1

s/It also describes the operations which SHOULD be performed/It also describes the operations that SHOULD be performed/ ?

## Comments

### Section 5.2, paragraph 20
```
           leaf speed {
             type uint32;
             units "MHz";
             description
               "The data transfer rate of the memory in MegaHertz (MHz).";
           }
```
Why does RAM bandwidth matter but not RAM latency? Also, modern packet-processing software is written to avoid going over the memory bus and is trying to execute in L3 as much as possible - so why does this matter?

### Section 5.2, paragraph 20
```
           leaf capacity {
             type uint32;
             units "MB";
             description
               "The disk or storage maximum capacity in Megabytes (MB).";
           }
```
This only scales to ~4000TB, which seems too small.

### Section 5.2, paragraph 22
```
         container bandwidth {
           description
             "Network bandwidth available on an NSF
              in the unit of Bps (Bytes per second)";
  
           leaf outbound {
             type uint64;
             units "Bps";
             description
               "The maximum outbound network bandwidth available to the
                NSF in bytes per second (Bps)";
           }
  
           leaf inbound {
             type uint64;
             units "Bps";
             description
               "The maximum inbound network bandwidth available to the
                NSF in bytes per second (Bps)";
           }
         }
       }
```
Is this per interface or the aggregate ingress/egress bandwidth across all interfaces?

## Nits

All comments below are about very minor potential issues that you may choose to
address in some way - or ignore - as you see fit. Some were flagged by
automated tools (via https://github.com/larseggert/ietf-reviewtool), so there
will likely be some false positives. There is no need to let me know what you
did with these suggestions.

### Typos

#### Section 5.2, paragraph 6
```
-      //RFC Ed.: replace occurences of XXXX with actual RFC number and
+      //RFC Ed.: replace occurrences of XXXX with actual RFC number and
+                             +
```

#### Section 5.2, paragraph 21
```
-              "The number of independent CPU in a single computing
+              "The number of independent CPUs in a single computing
+                                            +
```

### Uncited references

Uncited references: `[RFC7348]` and `[I-D.ietf-nvo3-vxlan-gpe]`.

## Notes

This review is in the ["IETF Comments" Markdown format][ICMF], You can use the
[`ietf-comments` tool][ICT] to automatically convert this review into
individual GitHub issues. Review generated by the [`ietf-reviewtool`][IRT].

[ICMF]: https://github.com/mnot/ietf-comments/blob/main/format.md
[ICT]: https://github.com/mnot/ietf-comments
[IRT]: https://github.com/larseggert/ietf-reviewtool

Thanks to Brian Trammell for the TSVART review.

Hi,

I found this document harder to read, and this probably isn't quite the approach that I would have taken.  Specifically, why not just export the capabilities as a regular operational data model that returns the current capabilities?

A couple more specific comments/suggestions below.

(1) p 18, sec 5.2.  YANG Data Module

     rpc nsf-capability-registration {
       description
         "Description of the capabilities that the
          Security Controller requests to the DMS";
       input {
         container query-nsf-capability {
           description
             "Description of the capabilities to request";

I'm not a big fan of this approach, but if please be very clear in the description here for the input exactly what the input means.  E.g., does it restrict, or filter, the output that is returned?


(2) p 18, sec 5.2.  YANG Data Module

           uses i2nsfcap:nsf-capabilities;
           reference "RFC YYYY: I2NSF Capability YANG Data Model";
         //RFC Ed.: replace YYYY with actual RFC number of
         //draft-ietf-i2nsf-capability-data-model and remove this note.
         }
       }
       output {
         list nsf {
           key "nsf-name";
           description
             "Network access information of an NSF
              with the requested capabilities";

In the description here, please be very clear about exactly what capabilities are returned.  E.g., are they filtered by the input query?


(3) p 19, sec 5.2.  YANG Data Module

       output {
         container nsf {
           description
             "The update for the NSF";

Does this just return the current capabilities (which would probably be the obvious choice), or is this filtered relative to what was returned previously in the last nsf-capability-registration request?  If so, what happens if another nsf-capability-registration request has been made?  E.g., it feels like you are possibly holding on to some adhoc state between the RPC calls which may prove to be fragile, particularly if you have more than one client that is requesting the information.  I don't really understand why you don't have return the current capabilities, and perhaps define a YANG notification to notify clients if the capabilities have changed.



Nit level comments:

(4) p 6, sec 4.1.1.1.  NSF Specification

   temporarily, i.e., Random Access Memory (RAM).  The information
   consists of RAM maximum capacity and RAM speed.  The disk

I don't think that RAM speed is specified in the model, so perhaps this text should be updated.

Thanks,
Rob