Hi Rob/Scott, Going through the comments and responses I received, there is one comment that I do not see addressed. See inline with [mj].
> On Jul 29, 2025, at 1:00 PM, Mahesh Jethanandani <[email protected]> > wrote: > > Hi Rob, > > Thanks for considering some of my comments. See inline with some of my > follow-up comments. > > On Tue, Jul 15, 2025 at 9:48 AM Rob Wilton (rwilton) <[email protected] > <mailto:[email protected]>> wrote: >> Hi Mahesh, >> >> Thanks for the review! I’ve added some comments inline … >> >> >> >> From: Mahesh Jethanandani <[email protected] >> <mailto:[email protected]>> >> Date: Wednesday, 2 July 2025 at 07:25 >> To: [email protected] >> <mailto:[email protected]> >> <[email protected] >> <mailto:[email protected]>> >> Cc: NETMOD Group <[email protected] <mailto:[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? >> > Yes. That and having the text above would help. [mj] I do not see this in latest version of the draft. Thanks. >> >> >> >> >> 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? >> > You are right that most hardware capabilities are for two tags, outer and > inner tags. But if we want to keep the option of having one, two, or more > tags, it would help to have a consistent naming scheme, as in, first-tag, > second-tag, third-tag etc. >> >> >> 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? >> > Sounds good. > >> >> >> >> >> >> 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? >> > Ok. >> >> >> 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? >> > If we want to keep the example, make it clear that the example is for > validation purposes and that some of the fields are destined for operational > (ro) datastore. >> >> >> 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. >> > You can ignore this message. >> >> >> 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. >> > Ok. If it comes up in AUTH48, be prepared to explain it. > > Thanks. >> >> >> ------------------------------------------------------------------------------- >> 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 <http://tools/> with https://datatracker >> <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 <http://tools/> with https://datatracker >> <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 <http://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] <mailto:[email protected]> >> >> > > > > -- > Mahesh Jethanandani > [email protected] <mailto:[email protected]> Mahesh Jethanandani [email protected]
_______________________________________________ netmod mailing list -- [email protected] To unsubscribe send an email to [email protected]
