Hi Martin,
Sorry for the amazingly late response to get to these. I've provided comments
inline below. I've also raised issues and updated the YANG modules and draft
at https://github.com/netmod-wg/sub-intf-vlan-yang. The -07 version of this
draft that I've posted just now also contains markups (so far) to your comments
below.
> -----Original Message-----
> From: netmod <netmod-boun...@ietf.org> On Behalf Of Martin Bjorklund
> Sent: 22 August 2019 21:15
> To: netmod@ietf.org
> Subject: Re: [netmod] WG Last Call: draft-ietf-netmod-sub-intf-vlan-model-
> 05
>
> Hi,
>
> Here is my (late) review of draft-ietf-netmod-sub-intf-vlan-model-05.
>
> o 1
>
> The YANG module names are not correct; they are listed as:
>
> if-l3-vlan.yang - Defines the model for basic classification of
> VLAN tagged traffic to L3 transport services
>
> flexible-encapsulation.yang - Defines the model for flexible
> classification of Ethernet/VLAN traffic to L2 transport services
>
> Should be "ietf-if-l3-vlan" and "ietf-flexible-encapsulation".
>
> Or "ietf-if-l3-vlan" and "ietf-if-flexible-encapsulation".
>
> But I also wonder if these names should somehow be changed. What
> is a "l3-vlan"? And "flexible-encapsulation" sound a bit too
> generic.
>
[RW]
I thought about this, and what I propose (and what is in the latest draft) is
as follows:
ietf-if-vlan-encapsulation
- Adds "dot1q-vlan" to the encapsulation choice.
- Predominantly would be used for L3 sub-interfaces, but could also be used on
L2 interfaces (e.g. connected to L2VPNs) if only basic VLAN tag matching was
required/supported.
ietf-if-flexible-encapsulation
- Adds "flexible" to the encapsulation choice
- Supports matching untagged traffic, priority-tagged, VLAN-tagged with ranges
& wildcard, default match (i.e. match of last resort).
- Supports tag manipulations (push, pop, rewrite)
- Could be extended by vendors with more L2 classification (e.g. src/dst MAC
matching)
- Makes most sense for L2 interfaces, although there are other scenarios where
it could be used (e.g. PPPoE subscribers).
>
> o 1.1
>
> The text says:
>
> Sub-interface: A sub-interface is a small augmentation of a regular
> interface in the standard YANG module for Interface Management that
> represents a subset of the traffic handled by its parent interface.
>
> I think the augmentation is the YANG-realization of a sub-interface,
> but it is not what a sub-interface is. Also, this definition is
> mis-leading; it doesn't mention that a sub-interface has its own
> interface type and is represented as one separate entry in the
> interface list. I think it is better to import this term from
> draft-ietf-netmod-intf-ext-yang (section 3.6)
>
[RW]
Fixed, to pull in the reference from draft-ietf-netmod-intf-ext-yang.
> o 3
>
> The text says:
>
> The L3 Interface VLAN model provides appropriate leaves for
> termination of an 802.1Q VLAN tagged segment to a sub-interface based
> L3 service.
>
> There is a comment in the YANG model that says the same thing.
>
> But the YANG model itself augments not only to sub-interface-based
> interface, but also to ethernet-like interfaces.
>
[RW]
I've fixed this text to make it clear that the encapsulation can be applied to
an interface or sub-interface both in the description and the YANG model.
>
> o YANG modules
>
> Both modules lack the IETF Trust Copyright statement.
>
> We don't list WG Chairs anymore.
>
> The revision statements should be on the form: "RFC XXXX: <title>"
>
> Many descriptions are full sentences w/o the ending ".".
>
> The modules should be indented properly; a starting point can be
> pyang -f yang --yang-line-length 69
>
[RW]
Hopefully now fixed in the latest revision.
>
> o ietf-if-l3-vlan
>
> There is a comment:
>
> /*
> * Matches a single VLAN Id, or a pair of VLAN Ids to classify
> * traffic into an L3 service.
> */
>
> This should be moved into a description clause.
[RW]
I've removed this comment, and expanded the description clause to:
description
"Classifies 802.1Q VLAN tagged Ethernet frames to an
interface or sub-interface by exactly matching the number
of tags, tag type(s) and VLAN identifier(s).";
>
> o ietf-if-l3-vlan / container dot1q-vlan
>
> The must statement has:
>
> count(../../if-cmn:forwarding-mode) = 0
>
> This can be changed to not(../../if-cmn:forwarding-mode) which is
> more direct imo.
>
> The must statement's description statement seems to be a
> copy-and-paste error.
[RW]
This constraint has been removed. forwarding-mode was converted to a config
false node, and the consensus was that the configuration should not be
restricted.
>
>
> o ietf-if-l3-vlan / container dot1q-vlan
>
> The descriptions talk about "matching frames" and "classifying
> traffic", but it is not described anywhere how the matching and
> classifying is used.
>
> (also applies to ietf-flexible-encapsulation)
[RW]
I've added the following paragraph to both models (the basic VLAN model doesn't
contain the last sentence since it isn't so directly relevant):
"Flexibly classifies Ethernet frames to an interface or
sub-interface based on the L2 header fields.
Only frames matching the classification configured on an
interface/sub-interface are processed on that
interface/sub-interface.
Frames that do not match any sub-interface are processed
directly on the parent interface, if it is associated
with a forwarding instance, otherwise they are dropped.
If a frame could be classified to multiple
sub-interfaces then they get classified to the
sub-interface with the most specific match. E.g.,
matching two VLAN tags in the frame is more specific
than matching the outermost VLAN tag, which is more
specific than the catch all 'default' match.";
Do you think that this text is sufficient, or do you think that more is
required (e.g., in the document rather than the YANG model)?
>
>
> o ietf-if-l3-vlan / outer-tag / second-tag
>
> These names are a bit inconsistent. The description describes them
> as "outermost tag" and "second outermost tag". Perhaps use these
> names instead?
>
> (same names are found in ietf-flexible-encapsulation)
>
I prefer the existing names because they are shorter and have been agreed with
IEEE 802.1Q WG. The descriptions also make it clear which tags are being
matched, and I've changed the associated description text from "outermost tag"
to "outermost (first) tag" to hopefully make it a bit clearer.
I propose closing this issue without changing the names of the tags.
>
>
> o ietf-flexible-encapsulation / all features
>
> The features are described as:
>
> "This feature indicates whether the network element supports
> specifying flexible rewrite operations";
>
> Should this be s/whether/that ?
[RW]
Fixed.
>
>
> o ietf-flexible-encapsulation
>
> There is some descriptive text in comments that should be moved to
> description statements.
>
[RW]
Fixed. Descriptive text has been moved into description statements.
>
> o ietf-flexible-encapsulation
>
> The descriptions for pop/push are a bit terse. It seems to assume
> that readers already know what this (from somewhere) is so it
> doesn't need to be described. If this is intended, perhaps add a
> reference to where this is described.
>
[RW]
This isn't any standard reference for this functionality. I have tried to
expand the descriptions in the YANG model, please can you check the latest
version of the draft to see if you think these are sufficient.
Apologies again for being so slow to process these comments.
Regards,
Rob
>
>
> /martin
>
> _______________________________________________
> netmod mailing list
> netmod@ietf.org
> https://www.ietf.org/mailman/listinfo/netmod
_______________________________________________
netmod mailing list
netmod@ietf.org
https://www.ietf.org/mailman/listinfo/netmod
