Re: [Detnet] Last Call: <draft-ietf-detnet-yang-18.txt> (Deterministic Networking (DetNet) YANG Model) to Proposed Standard

[Don Fedyk <[e-mail]>]

Hi Florian 

Thank you for your review, 
Inline comments [Don] are my comments. 
There are three categories of my responses: 
- OK will adopt your change. 
- A comment - clarification - Not sure any action is necessary. See responses below.
- An update addressing a point in the text - action on me.  I will produce an update and respond with an email to this thread with the propose updated text for you and co-authors and others to review. 

That will take a couple days. 

Cheers
Don & co authors.

-----Original Message-----
From: Florian Kauer <[e-mail]> 
Sent: Wednesday, December 6, 2023 8:22 AM
To: [e-mail]; [e-mail]
Cc: [e-mail]; [e-mail]; [e-mail]; [e-mail]
Subject: Re: [Detnet] Last Call: <draft-ietf-detnet-yang-18.txt> (Deterministic Networking (DetNet) YANG Model) to Proposed Standard

Hi,
sorry for being this late, but there are several things that still confuse me when I try to apply the YANG model in a TSN over IP over TSN scenario (i.e. using DetNet to connect several TSNs over IP tunnels in the industrial automation or professional audio context).

It is likely that my confusions are only based on misunderstandings and missing background knowledge (and that's I why I did not speak up before and tried to do more research first), but I still would like to raise them now and let others decide if they are valid concerns or not. In both cases, I still would love to get support clarifying these points.

I also fixed some things that look like typos to me, but take them with a grain of salt since I am not a native speaker.

Greetings,
Florian

--- draft-ietf-detnet-yang-18.txt       2023-07-10 21:24:35.000000000 +0200
+++ draft-ietf-detnet-yang-18-fk-comments.txt   2023-12-06 13:49:42.692711093 +0100
@@ -192,14 +192,14 @@
    |  r  |Forwarding S-L   |Forwarding S-L   | Forwarding S-L|
    +-----+-----------------+-----------------+---------------+
 
-                   Figure 1: Detnet Layers and Node Types
+                   Figure 1: DetNet Layers and Node Types
[Don] OK.
 
    All of the layers have ingress/incoming and egress/outgoing
    operations, but any instance may be configured as only
    unidirectional.  Ingress refers to any DetNet layer where a DetNet
    context is applied.  Ingress allows functions such as switching,
    aggregation and encapsulation.  Likewise, egress refers to any DetNet
-   layer where a Detnet context is removed.  Egress allows functions
+   layer where a DetNet context is removed.  Egress allows functions
    such as switching, disaggregation and decapsulation.  This means that
    each unidirectional flow identifier configuration is programmed
    starting at the ingress and flow status is reported at ingress on @@ -454,7 +454,7 @@
[Don] OK.
    This YANG model imports typedefs from [RFC6991], [RFC8519],
    [RFC8294], [RFC8343], [IEEE8021Q], and [IEEE8021QCX].  This YANG
-   model also has the folowing references to RFCs that are not in the
+   model also has the following references to RFCs that are not in the
    document text body [RFC0791], [RFC4303], [RFC8349], [RFC8938],
    [RFC8960], [RFC8964], and [RFC8200].
[Don] OK.
 
@@ -583,7 +583,7 @@
      identity none {
        base app-status;
        description
-         "This Application has no status. This identity is
+         "This application has no status. This identity is
[Don] OK.
           expected when the configuration is incomplete.";
        reference
          "RFC 9016 Section 5.8";
@@ -600,7 +600,7 @@
      identity failed {
        base app-status;
        description
-         "Application ingres/egress failed.";
+         "Application ingress/egress failed.";\
[Don] OK.
        reference
          "RFC 9016 Section 5.8";
      }
@@ -608,7 +608,7 @@
      identity out-of-service {
        base app-status;
        description
-         "Application Administratively blocked.";
+         "Application administratively blocked.";
[Don] OK.
        reference
 
 
@@ -624,7 +624,7 @@
      identity partial-failed {
        base app-status;
        description
-         "This is an Application with one or more Egress ready, and one
+         "This is an application with one or more Egress ready, and one
[Don] OK.
           or more Egress failed.  The DetNet flow can be used if the
           Ingress is Ready.";
        reference
@@ -639,7 +639,7 @@
             + "/dnet:name";
        }
        description
-         "This is an Application Reference.";
+         "This is an application reference.";
      }
 
[Don] OK.
      typedef service-sub-layer-ref {
@@ -650,7 +650,7 @@
             + "/dnet:name";
        }
        description
-         "This is a Service sub-layer Reference.";
+         "This is a service sub-layer reference.";
      }
[Don] OK.
      typedef forwarding-sub-layer-ref { @@ -661,7 +661,7 @@
             + "/dnet:name";
        }
        description
-         "This is a Forwarding sub-layer Reference.";
+         "This is a forwarding sub-layer reference.";
      }
[Don] OK. 
      typedef traffic-profile-ref {
@@ -679,7 +679,7 @@
             + "/dnet:name";
        }
        description
-         "This is a Traffic Profile Reference.";
+         "This is a traffic profile reference.";
      }
[Don] OK. 
      typedef ipsec-spi {
@@ -1113,6 +1113,79 @@
          description
            "The Application flow type choices.";
          container tsn-app-flow {
+---
+fk: In case of a tsn-app-flow, how does the actual encapsulation take place?

[Don] This is a YANG specification that satisfies the operations in DetNet RFCs  
Listed in the references.  You can think of it as a configuration for an 
SDN controller that specifies encapsulation/decapsulation on various nodes/devices
along the data path. It is not specifying "how" it is specifying "what" aggregation,  
disaggregation functions are applied along the data path and which technology is used.

Also reading through your comments I think there is mis understanding.  

DetNet is not an any to any encapsulation. It is specifically what is covered in the DetNet architecture to support DetNet encapsulation flows for aggregation or replication.  We have specific type of encapsulations and behaviors that must be specified on ingress. The reverse operations don’t need as much specification because you are removing an outer encapsulation and exposing another header. I will try to see if this point is spelled out in the ID. 
  
+
+For reference, lets look at a few cases and how I have understood them:
+
+1. ip-app-flow, nothing MPLS related in service or forwarding sub-layers:
+   -> Just transmit unmodified packet with new L2 header.
++---------------+      +----------+
+| Payload       |      | Payload  |
++---------------+      +----------+
+|   UDP         |      |   UDP    |
++---------------+  ->  +----------+
+|   IP          |      |   IP     |
++---------------+      +----------+
+| L2 (original) |      | L2 (new) |
++---------------+      +----------+

[Don]  Is this is a DetNet Appflow?  Please see DetNet Architecture RFC 8655 Figure 3. Also Data Plane Framework RFC8938.
We are only concerned with DetNet.

The drawing is a bit ambiguous for DetNet.  
The Payload could be the TSN flow then the IP is a DetNet sub-network and you have a DetNet packet. The L2 is irrelevant.  
If the whole stack Payload/UDP/IP is a TSN payload then TSN is the subnetwork layer. It would be a TSN application frame in ingress and the SAME frame on egress.    

The YANG model aggregates (encapsulates) and disaggregates (decapsulates) DetNet packet but it does not change/translate a TSN L2.  The encapsulation layers for DetNet are IP/MPLS not Ethernet L2.   (we don’t specify egress L2 headers we can match L2 ingress headers at the Application interface).  
      

+
+2. ip-app-flow, but MPLS label is added:
+   -> Squeeze in MPLS header.
++---------------+      +----------+
+| Payload       |      | Payload  |
++---------------+      +----------+
+|   UDP         |      |   UDP    |
++---------------+  ->  +----------+
+|   IP          |      |   IP     |
++---------------+      +----------+
+| L2 (original) |      |   MPLS   |
++---------------+      +----------+
+                       | L2 (new) |
+                       +----------+
[Don] Again if this is an IP App flow you have this. L2 Not relevant.

++---------------+      +----------+
+| Payload       |      | Payload  |
++---------------+      +----------+
+|   UDP         |      |   UDP    |
++---------------+  ->  +----------+
+|   IP          |      |   IP     |
++---------------+      +----------+
                        |   MPLS   |
                        +----------+

[Don] Or if it is TSN app flow you have this: 

++---------------+      +----------+
+| Payload       |      | Original |
++---------------+      | L2 Frame |
+|   UDP         |      |          |
++---------------+  ->  |          |
+|   IP          |      |          |
++---------------+      |          |
+| L2 (original) |      |          |
++---------------+      +---------------------------+
+                       | DetNet Service Sublayer|  |
+                       +---------------------------+
+                       | DetNet Forwarding Sublayer|  |
+                       +---------------------------+                        
 

+
+3. mpls-app-flow:
+   -> Only optionally modify MPLS header
++-----------------+      +------------+
+| Payload         |      | Payload    |
++-----------------+      +------------+
+|   UDP           |      |   UDP      |
++-----------------+  ->  +------------+
+|   IP            |      |   IP       |
++-----------------+      +------------+
+| MPLS (original) |      | MPLS (new) |
++-----------------+      +------------+
+| L2 (original)   |      | L2 (new)   |
++-----------------+      +------------+


[Don] If it is an APP flow it gets encapsulated. We can encapsulate service sub-layer as application layer for example.  This header format could also be a service sub layer and we show examples of changing the labels as a serve sub layer and forwarding layer.   

+
+4. tsn-app-flow:
+   -> In order to be able to reconstruct the full original L2 packet, we need
+      to retain the L2 header. So we need something like:
+
++---------------+      +-----------------+
+| Payload       |      |    Payload      |
++---------------+  ->  +-----------------+
+| L2 (Original) |      |  L2 (Original)  |
++---------------+      +-----------------+
+                       |      ???        |
+                       +-----------------+
+                       |   L2 (new)      |
+                       +-----------------+

[Don] This is specified in IEEE TSN.    DetNet uses IP/MPLS as the service sub layer. 
+
+I know there are several different ways to realize this, including just 
+transmitting the L2 packet over MPLS (apparently not IETF 
+standardized?) or several others like L2TPv3 (RFC3931) or VXLAN (RFC 
+7348) if no MPLS labels are to be added (like in case 1 above).

+
+I would expect the YANG model (either this or a linked one) to somehow 
+specify which of these options shall actually be applied. Otherwise, 
+two DetNet routers might not be able to communicate with each other, 
+because one might use L2TPv3 and the other one VXLAN. And also for 
+example in the case of VXLAN, we would need the option to specify a VXLAN Network Identifier.

[Don] The scope of the YANG does what DetNet documents outline. There is no VXLAN or L2TP3 specified in the DetNet when this applied. 
The DetNet YANG works by specified the whole path and applying the appropriate encapsulation format at the appropriate places. Yes the encapsulation specified on each device must match just the way SDN controllers specify packet forwarding in several technologies.



+
+What do you think about that?
+
+---
            uses l2-header;
 
 
@@ -1143,7 +1216,7 @@
          "detnet-flow identification.";
        choice detnet-flow-type {
          description
-           "The Detnet flow type choices.";
+           "The DetNet flow type choices.";
          case ip-detnet-flow {
            uses ip-flow-id;
          }
[Don] OK
@@ -1254,7 +1327,7 @@
              case mpls {
                uses rt-types:mpls-label-stack;
                description
-                 "The MPLS Label stack next hop case.";
+                 "The MPLS label stack next hop case.";
              }
            }
          }
[Don] OK
@@ -1425,6 +1498,9 @@
            type string;
            description
              "An Aggregation group ID.";
+---
+fk: I would expect "The name of the traffic profile." here as it is similarly used for most other names.
+---
[Don] Looks like a copy paste error. Will fix. 
          }
          container traffic-requirements {
            description
@@ -1708,10 +1784,20 @@
            container egress {
              description
                "Route's next-hop attribute.";

[Don] This should read "Egress DetNet application flows or a compound flow
+---
+fk: This description does not seem right at this place!?
+---
              uses data-flow-spec;
              choice application-type {
                description
                  "This is the application type choices.";
+---
+fk: This part is quite confusing to me, especially since it does not directly mirror the ingress since.
+I guess one reason for that is there could be multiple interfaces that could be selected based on the packet headers?
+But for the same reason you could also have multiple ingress interfaces.
[Don]  For Egress the options are different because we get here by decapsulating the service sub-layer. We have either some type of TSN frame (Ethernet) or IP/MPLS but we are basically only associating the flow with an interface at the end of a service sub-layer.  
 
+Also the "ethernet" application type is confusing. I would expect that 
+it should mirror the "tsn-app-flow", but then it mentions "TSN unaware traffic"?
+---
[Don] We have used TSN and ethernet interchangeably - I will review for consistency. 

                container ethernet {
                  description
                    "This is TSN unaware traffic that maps to an @@ -1860,6 +1946,9 @@
                container forwarding-sub-layer {
                  description
                    "This service sub-layer is sent to the forwarding
+--
+fk: "sent" or "sending"?
+--
[Don] Sending is better. 
                     sub-layers of the lower layer for DetNet service
                     forwarding or service-to-forwarding aggregation at
                     the ingress node or relay node.  When the operation @@ -1978,7 +2067,11 @@
          list sub-layer {
            key "name";
            description
-             "The List is one or more DetNet Traffic types.";
+             "The list is one or more DetNet traffic types.";
+---
+fk: I don't get the meaning of this sentence in this context!?
+This is the forwarding sub-layer name and not a traffic type!?
[Don] I think "The list is one or more DetNet service/forwarding types". Is more accurate.
+---
            leaf name {
              type string;
              description
@@ -1996,6 +2089,9 @@
                 impose-and-forward, pop-and-forward,
                 pop-impose-and-forward, forward, pop-and-lookup.";
            }
+---
+fk: Since these are MPLS forwarding-operations, do we still use e.g. "forwarding" when using IP instead of MPLS or just leave this out?
+---
[Don] This is an optional operation that applies only to MPLS forwarding.  I suppose the type mpls-forwarding-operation or mpls-fwd-operation might be a more accurate name that just forwarding operation. 

            container incoming {
              description
                "The DetNet forwarding sub-layer incoming @@ -2206,9 +2302,18 @@
    be considered sensitive.
 
    /detnet/traffic-profile/member-app: This links traffic profiles to
-   applications, o this also could be considered more sensitive.  The
+   applications, so this also could be considered more sensitive.  The
    traffic profiles liked to service sub-layer and forwarding sub-layer
    are less sensitive.
+---
+fk: I am not so sure if that is the case. Being able to change a 
+traffic profile could mean that you can for example change a DetNet 
+flow that is specified as lossless into one that is lossy. Or one with 
+a very small latency into one with a huge latency.
+If the DetNet flow carries data of a time-critical industrial control 
+loop, I wouldn't be too happy if an attacker can occasionally crash my 
+system by dropping packets or inducing additional latencies.
+---
[Don] I will have to check this wording.
 
    /detnet/service/sub-layer/incoming/app-flow: This links applications
    to services.
@@ -2608,6 +2713,14 @@
         |     +--rw operation?            operation
         |     +--rw incoming
         |     |  +--rw (incoming)?
+---
+fk: One thing I raised before, but it either my comment got lost or i missed the response:
+Why is there no incoming forwarding sub-layer for the service sub-layer?
+An app-flow has outgoing service-sub-layer and incoming 
+service-sub-layer, a forwarding sub-layer has incoming 
+service-sub-layer and outgoing service-sub-layer, a service sub-layer 
+has incoming app-flow and outgoing app-flow and it has outgoing forwarding sub-layer, but NO incoming forwarding sub-layer.
[Don] Sorry I missed your comment before.   
The operation of Service-sub-layer to forwarding sub-layer is an association of an encapsulation operation. The reverse direction is simply a removal of the forwarding layer and a match in the now exposed service Sub-layer.  Therefore, you do not need the same level of description.


+---
         |     |     +--:(app-flow)
         |     |     |  +--rw app-flow
         |     |     |     +--rw flow*   app-flow-ref
@@ -2976,7 +3089,7 @@
       by the configuration.
 
    The following are examples of aggregation and disaggregation at
-   various points in Detnet.  Figures are provided in the PDF and HTML
+   various points in DetNet.  Figures are provided in the PDF and HTML
[Don] OK
    version of this document.
 
 B.1.  Example A-1 JSON Configuration/Operational