Hi Mahesh, Thanks for the review! I’ve added some comments inline …
From: Mahesh Jethanandani <[email protected]> Date: Wednesday, 2 July 2025 at 07:25 To: [email protected] <[email protected]> Cc: NETMOD Group <[email protected]> Subject: AD review of draft-ietf-netmod-sub-intf-vlan-model-15 I want to thank the authors for plugging in a key piece of the (sub)interface model. The document is very well written. I would also like to thank Per Andersson for providing an early YANG doctor's review. Please find below a few comments that I hope will improve the document further. Section 1, paragraph 0 > This document defines two YANG [RFC7950] modules that augment the > encapsulation choice YANG element defined in Interface Extensions > YANG [I-D.ietf-netmod-intf-ext-yang] and the generic interfaces data > model defined in [RFC8343]. The two modules provide configuration > nodes to support classification of Ethernet/VLAN traffic to sub- > interfaces, that can have interface based feature and service > configuration applied to them. Perhaps further qualify that the document defines a YANG 1.1 model. Yes, okay, will change to “two YANG 1.1 [RFC 7950] modules”. Section 1, paragraph 3 > ietf-if-vlan-encapsulation.yang - Defines the model for basic > classification of VLAN tagged traffic, normally to L3 packet > forwarding services Is it for L3 or L3 and L2 services? So, it could be used for both, but the core idea is that this module is mainly used for L3 services, whilst the other, more flexible matching, is normally used for L2 services. Section 3, paragraph 4 The document talks about dropping packets if, for example, on ingress if the VLAN tag does not match. Where are these statistics being maintained? Are there other statistics that would be helpful to debug issues? The interface extensions draft module augments the parent (trunk) interface with a generic unknown encaps counter that should be incremented if the package cannot be demultiplexed to any of the child sub-interfaces, i.e., if the packet doesn’t match any of them. In the generic case doesn’t necessarily have to just be because of VLAN Id, if more sophisticated matches were being performed. +--ro in-discard-unknown-encaps? yang:counter64 Do you think this would be sufficient? Section 3, paragraph 4 > augment /if:interfaces/if:interface/if-ext:encapsulation > /if-ext:encaps-type: > +--:(dot1q-vlan) > +--rw dot1q-vlan > +--rw outer-tag > | +--rw tag-type dot1q-tag-type > | +--rw vlan-id vlanid > +--rw second-tag! > +--rw tag-type dot1q-tag-type > +--rw vlan-id vlanid Interesting choice of name for the second tag as 'second-tag'. I would have thought that since the outer tag is called 'outer-tag', the inner tag would have been called 'inner-tag'. Or even the use of 's-tag' and 'c-tag’ would be familiar. The package might have more than two tags, even though this model only allows for matching on two tags, which I think matches common current hardware capabilities. We didn’t want to use s-tag and c-tag because that has other connotations (e.g., what the ethertype of the tag would be). We would prefer to keep this as is, if that is okay? Section 4, paragraph 0 > The Interface Flexible Encapsulation model is designed to allow for > the flexible provisioning of layer 2 services. It provides the > capability to classify and demultiplex Ethernet/VLAN frames received > on an Ethernet trunk interface to sub-interfaces based on the fields > available in the layer 2 headers. Once classified to sub-interfaces, > it provides the capability to selectively modify fields within the > layer 2 frame header before the frame is handed off to the > appropriate forwarding code for further handling. The forwarding > instance, e.g., L2VPN, VPLS, etc., is configured using a separate > YANG configuration model defined elsewhere. While it is not the responsibility of this draft to define how the other models define the appropriate forwarding behaviour, it is not clear how this model is supposed to interoperate with these other models. Take the L2VPN YANG module draft as an example. How are the tags defined here mapped to a particular instance of L2VPN? Is there any guidance in this document to help those other models? The draft does contain an example (in 7.2) of interacting with the IETF L2VPN YANG model, but that draft hasn’t progressed for quite some time, and hence it is possible that this example will end up being out of date. If the L2VPN model does get published then perhaps bis’ing this document to update the example might be worth thinking about. I’m slightly worried about saying too much in this document because there is quite a lot of flexibility, e.g., some deployments may choose to strip the tags across the L2VPN service, whereas other deployments may want to keep the tags intact, or even do a combination of the two. I propose that we add some generic text about services being bound to an interface or sub-interface as the mechanism to pass traffic to/from interface/sub-interface to service. Okay? Section 4, paragraph 0 > The model supports a common core set of layer 2 header matches based > on the 802.1Q tag type and VLAN Ids contained within the header up to > a tag stack depth of two tags. Can a diagram (or reference to a diagram) that shows an Ethernet frame with one and two VLAN tags be added here? I think it would help to understand some of the terms like outer tag, EtherType, and others. Good idea. Yes, we can add a diagram illustrating an Ethernet Frame. Section 5, paragraph 11 > "Specifies the VLAN tag values to match against the > second outermost 802.1Q VLAN tag in the frame."; What is "second outermost 802.1Q VLAN tag"? Till now the reference has been to "second tag". Calling it "inner tag" or "C-VLAN tag" may make more sense. The idea is that may be more than two tags, and this text is trying to clarify that it is the second tag starting with the outermost, rather than counting from the innermost tag. In the YANG itself, we just refer to it as second-tag and only use “outermost” in the description to give additional clarity. So, I propose that we leave this text unchanged. Okay? Section 7.1, paragraph 4 > { > "ietf-interfaces:interfaces": { > "interface": [ > { > "name": "eth0", > "type": "iana-if-type:ethernetCsmacd", > "oper-status": "up", > "statistics": { > "discontinuity-time": "2023-12-15T09:04:00-05:00" > } If this is a configuration example, why is 'oper-status' being configured? We had included oper-status and statistics so that the examples validated with YANG lint. So, the choice is between examples that validate or examples that are minimal. Do you have a preference as to which way we go? Section 10, paragraph 0 > The "ietf-if-flexible-encapsulation" and "ietf-if-vlan-encapsulation" > YANG modules define data models that are designed to be accessed via > YANG-based management protocols, such as NETCONF [RFC6241] and > RESTCONF [RFC8040]. These protocols have to use a secure transport > layer (e.g., SSH [RFC6242], TLS [RFC8446], and QUIC [RFC9000]) and > have to use mutual authentication. Please make sure this template matches the template in 8407bis. Yes, we will update the template in this document and the other way to match 84097bis. No reference entries found for these items, which were mentioned in the text: [draft-ietf-netmod-intf-ext-yang]. I think you meant [I-D.ietf-netmod-intf-ext-yang]. Possible DOWNREF from this Standards Track doc to [IEEE_802.1Q_2022]. If so, the IESG needs to approve it. Is this really a DOWNREF? If so, I assume that this doesn’t matter. Found terminology that should be reviewed for inclusivity; see https://www.rfc-editor.org/part2/#inclusive_language for background and more guidance: * Term "native"; alternatives might be "built-in", "fundamental", "ingrained", "intrinsic", "original" I would like to keep “native” because it is a well-known term in 802.1Q and moving away from this would likely only cause confusion. ------------------------------------------------------------------------------- NIT ------------------------------------------------------------------------------- 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. Section 5, paragraph 4 > "WG Web: <http://tools.ietf.org/wg/netmod/> Substitute http://tools with https://datatracker Will fix. Section 5, paragraph 9 > "VLAN encapsulations YANG model"; s/VLAN encapsulations YANG model/Initial version of the model./ Will fix. Section 6, paragraph 4 "WG Web: <http://tools.ietf.org/wg/netmod/> Substitute http://tools with https://datatracker Will fix. Section 6, paragraph 10 > "Interface flexible encapsulations YANG model"; s/Interface flexible encapsulation YANG model/Initial version of the module/ Will fix. Section 6, paragraph 9 > "This feature indicates that the network element supports > specifying flexible rewrite operations."; One extra space in the indent. Will fix. Section 6, paragraph 12 > "For IEEE 802.1Q interoperability, when matching two > tags, it is required that the outermost (first) tag > exists and is an S-VLAN, and the second outermost > tag is a C-VLAN."; Please clarify what is "second outermost tag". Please see reply above. These URLs point to tools.ietf.org, which has been taken out of service: * http://tools.ietf.org/wg/netmod/ Yes, will fix. These URLs in the document did not return content: * https://www.rfc-editor.org/info/rfcBBBB Yes, will fix. Section 1.3, paragraph 2 > that they can be cleanly extended in future. 2.1. Interoperability with IEE > ^^^^^^^^^ The phrase "in future" is British English. Did you mean: "in the future"? Sorry for being British 😉. Will fix. Section 6, paragraph 14 > of other fields in the L2 header in future."; container dot1q-tag-rewrite { > ^^^^^^^^^ The phrase "in future" is British English. Did you mean: "in the future"? Ditto. Section 6, paragraph 14 > , which is more specific than the catch all 'default' match."; uses flexible- > ^^^^^^^^^ It seems that a hyphen in the noun or adjective "catch-all" is missing. Will fix. Section 6, paragraph 14 > ng a VLAN tag, or rewriting the VLAN Id carried in a VLAN tag."; choice dire > ^^ Possible spelling mistake found. Will leave. Section 6, paragraph 16 > c with a single S-VLAN tag, with VLAN Id 11. COMMENT: { "ietf-interfaces:inte > ^^ Possible spelling mistake found. Will leave. Section 6, paragraph 20 > 0.3' configured to match traffic with a S-VLAN tag of 10, and C-VLAN tag of 2 > ^ Use "an" instead of "a" if the following word starts with a vowel sound, e.g. "an article", "an hour". Will fix. Section 6, paragraph 25 > and Dan Romascanu for their help progressing this draft. The authors would a > ^^^^^^^^^^^ The verb "help" is used with an infinitive. I think that this is fine, will leave to RFC editor. Section 9.1, paragraph 17 > l, the classification of traffic arriving on an interface to a given sub-inte > ^^^^^^^^^^^ The usual preposition after "arriving" is "at", not "on". Did you mean "arriving at"? We think “on” is better in this context. Will leave to the RFC editor. Section 10.2, paragraph 5 > the 802.1Q bridge model can also be use to implement L2 services in some sce > ^^^ Did you mean "used"? Will fix. Section 10.2, paragraph 9 > ment scenarios for which they are optimized. Devices may choose which of the > ^^^^^^^^^ Do not mix variants of the same word ("optimize" and "optimise") within a single text. Will fix. I’ll defer to Scott for the right spelling to use here since my British English is no good ;-) Many thanks for your thoughtful review. Kind regards, Scott & Rob Mahesh Jethanandani [email protected]
_______________________________________________ netmod mailing list -- [email protected] To unsubscribe send an email to [email protected]
