Subject: [Softwires] Benoit Claise's Discuss on draft-ietf-softwire-dslite-mib-12: (with DISCUSS and COMMENT)

From: "Benoit Claise" <[email-masked]>
Date: Wed, 02 Dec 2015 12:34:56 -0800

Benoit Claise has entered the following ballot position for
draft-ietf-softwire-dslite-mib-12: Discuss

When responding, please keep the subject line intact and reply to all
email addresses included in the To and CC lines. (Feel free to cut this
introductory paragraph, however.)

Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
for more information about IESG DISCUSS and COMMENT positions.

The document, along with other ballot positions, can be found here:
https://datatracker.ietf.org/doc/draft-ietf-softwire-dslite-mib/

----------------------------------------------------------------------
DISCUSS:
----------------------------------------------------------------------

As mentioned by Dave Thaler in his MIB doctor review:
I was assigned to do the MIB doctor review of this document, since I
previously
did an early review of -03.  My full comments are in the marked up copy
at
http://research.microsoft.com/~dthaler/draft-ietf-softwire-dslite-mib-12.pdf
Below is a summary of the issues called out therein.

Substantive issues:

1)      RFC 4001 requires each InetAddress object to explicitly state
which other InetAddressType object indicates the type.  None of the
objects
in this document do so.  RFC 7659 (the NATv2 MIB) does, and can be used
as
an example.

2)      dsliteNATBindEntry includes dsliteTunnelStartAddPreLen in the
INDEX.
To confirm this was intended: Can you really have 2 entries that have
all
other INDEX values the same and differ only in the prefix length?

3)      dsliteNATBindTable states that it extends natv2PortMapTable in
RFC 7659,
but rather than reusing the not-accessible objects from that table in its
own
INDEX clause, it defines its own.  That’s fine, but it is then not clear
whether
each such object in the dsliteNATBindTable INDEX needs to match
a corresponding value in the natv2PortMapTable INDEX,
or whether there can be additional entries that do not
appear in the natv2PortMapTable.   This should be clarified.

4)      Many objects in that table, such as
dsliteNATBindMappingIntRealm,
have very terse DESCRIPTIONs, whereas the DESCRIPTION of
the corresponding object in the natv2PortMapTable is quite
detailed.  Hence this draft is far less clear than RFC 7659,
since this draft has no such language.

5)      Objects of type InetAddress incorrectly have a REFERENCE
clause pointing to the definition of the InetAddress TC.
REFERENCE clauses should be used to point to the spec defining
the semantics, rather than the syntax.  For example,
dsliteNATBindMappingIntAddress is incorrect, whereas the
corresponding object in RFC 7659 (natv2AddressMapInternalAddressType)
is correct and points into the DS-Lite RFC (RFC 6333).

6)       I didn’t understand DsliteNATBindEntry at all.  Its
dsliteNATBindMappingMapBehavior object has a value
addressAndPortDependent(2) which “maps to a separate external
address and port combination for each different
destination address and port combination reached
through the same external realm”.   However, the external port
is in the INDEX clause and the destination address does not appear to be
in the table at all.  Since the 0 value for the external port already has
a different
special meaning, it can’t be 0 either.   So I don’t understand how this
table
can work.

7)      dsliteAFTRAlarmProtocolType is underspecified.  It’s a string,
but the description is very confusing as to what the legal string
values are, making it sound more like an INTEGER was intended.
(“This object indicate the protocol type of alarm,
                0:tcp,1:udp,2:icmp,3:total”)
E.g., does that mean the string is “0:tcp” or “0” or “tcp” or what?

8)      The dsliteStatisticTransmitted object seems to combine sent +
received
packets into a single counter with a name that implies only one
direction.
This is confusing, especially since most other MIB modules separate sent
vs received into different counters.

9)      dsliteAFTRUserSessionNumAlarm and
dsliteAFTRPortUsageOfSpecificIpAlarm
both refer to “the threshold” without stating what threshold that is.
There doesn’t seem to be any such threshold object in this MIB module or
elsewhere that I could find.

10)   dsliteAFTRAlarmScalarGroup is mandatory and requires read-write
access.
But a lesson learned from the NAT MIB (and many other MIBs) is that many
people don’t want write support in their MIB modules.  Does the WG
really
feel that write support is required in all implementations?  I’d
recommend
also having a read-only compliance statement, as is done in many other
MIB modules.

11)   The security considerations section uses the correct boilerplate
for
sensitive read-only objects, which includes “These are the tables and
objects and their sensitivity/vulnerability”.  However it then only
lists the tables/objects and contains no discussion of their
sensitivity/vulnerability.
This is required in order to comply with MIB review guidelines in RFC
4181.

12)   Per discussion on MIB Doctors, the root OID should probably not be
{ transmission xxx }, since that space usually implies that xxx is an
ifType
(not tunnel type) value.
BENOIT: as discussed with the MIB Doctors, it should be under mib-2.

13)   A number of undefined terms are used that I could not find in the
DS-Lite
RFC (6333) either, e.g., connection, session, etc.  As such, I couldn’t
tell what
was meant, and whether there were issues with the meaning.  At minimum,
REFERENCE clauses should be added to point to a specific section of a
document
that defines the terms.

----------------------------------------------------------------------
COMMENT:
----------------------------------------------------------------------

Editorial issues:

14)   The title, abstract, MIB module name, etc. all say the MIB module
is for
managing DS-Lite, but DS-Lite has two roles: AFTR and B4, and this
document
only instruments the AFTR.   Hence I would really recommend putting AFTR
in the title, abstract, and name.

15)   There are a bunch of editorial issues (grammar, typos) that should
be
cleaned up before publication.

16)   In a number of places, it seems to refer to some objects in some
MIB module,
whether this document or RFC 7659, without stating the name of the
object
so it is not clear which object was meant.

17)   The document still refers to draft-perrault-behave-natv2-mib
and should be RFC 7659.

And finally, I don't see the value of this sentence.
   "When these words are not in
   ALL CAPS (such as "should" or "Should"), they have their usual
   English meanings, and are not to be interpreted as [RFC2119] key
   words."
I believe we should favor text consistency across documents.

---

From: "Yu Fu" <[email-masked]>
Date: Fri, 11 Dec 2015 14:40:54 +0800

Dear Benoit and Dave,

Thanks for your extensive review. Please see my reply inline.

On 2/12/2015 5:54 am, "Softwires on behalf of Dave Thaler"
<[email-masked] on behalf of [email-masked]> wrote:

>did an early review of -03.  My full comments are in the marked up copy at
>1)     
>RFC 4001 requires each InetAddress object to explicitly state
>in this document do so.  RFC 7659 (the NATv2 MIB) does, and can be used as
>an example.

[Yu]: We will add the statement in the updated version according to the RFC 7659 as an example.

>2)     
>dsliteNATBindEntry
>includes
>dsliteTunnelStartAddPreLen in the INDEX.
>To confirm this was intended:
>Can you really have 2 entries that have all

[Yu]: Only the dsliteTunnelStartAddress would be enough to identify the IP address of the tunnel startpoint, so we will delete the dsliteTunnelStartAddPreLen in the INDEX of dsliteNATBindEntry.

>3)     
>dsliteNATBindTable states that it extends natv2PortMapTable in RFC 7659,
>INDEX clause, it defines its own.  That¹s fine, but it is then not clear
>whether
>each such object in the

[Yu]: Each entry in the dsliteNATBindTable not only need to match a corresponding entry in the natv2PortMapTable but also a corresponding entry in the dsliteTunnelTable, so each such object in the dsliteNATBindTable INDEX needs to match a corresponding value in the natv2PortMapTable INDEX and a correspongding value in the dsliteTunnelTable INDEX. We will add a clarification in the DESCRIPTION of the dsliteNATBindEntry.

>4)     
>Many objects in that table, such as dsliteNATBindMappingIntRealm,

[Yu]: We will add more detailed description in the defined object in the dsliteNATBindTable.

>5)     
>Objects of type InetAddress incorrectly have a REFERENCE

[Yu] We will correct it in the next version.

>6)     
>I didn¹t understand DsliteNATBindEntry at all.  Its
>addressAndPortDependent(2) which ³maps to a separate external
>through the same external realm².   However, the external
>port
>special meaning, it can¹t be 0 either.   So I don¹t understand how this
>table
>can work.

[Yu]: It need more clarification sentence here. Because the AFTR is also a CGN device (Carrier Grade NAT) which translates private IPv4 addresses to and from global IPv4 addresses according to a classical NAPT scheme configured by the Service Provider, the dsliteNATBindMappingMapBehavior object only has a value addressAndPortDependent(2) here in DS-Lite scenario. And the dsliteNATBindMappingExtPort must be a non-zero value. It also need some clarification in the DESCRIPTION of the natv2PortMapExternalPort. Let me take a simple example for the extended IPv4 NAT binding table according to the definition of dsliteNATBindTable: 
+------------------------------------------------------------------------------------------+---------------------------------------------------------------+
| TunnelStartAddress/IPv4InternalAddress/Protocol/InternalPort | IPv4ExternalAddress/Protocol/ExternalPort |
+-------------------------------------------------------------------------------------------+------------------------------------------------------------ -+
|  2001:db8:0:1::1 /      10.0.0.1    /  TCP  /  10000    |     192.0.2.1   /   TCP  /   5000   |
+-------------------------------------------------------------------------------------------+------------------------------------------------------ -------+
the translation tables in the AFTR are configured to forward between IP/TCP (10.0.0.1/10000) and IP/TCP
(192.0.2.1/5000). That is, a datagram received by the Dual-Stack Lite home router from the host at address 10.0.0.1, using TCP port 10000, will be translated to a datagram with IPv4 SRC address
192.0.2.1 and TCP SRC port 5000 in the Internet. This Entry is created when the AFTR receives datagram on the outbound direction. When the datagram is returned from the inbound direction, it will do a reverse lookup in the extended IPv4 NAT binding table to know how to reconstruct the IPv6 encapsulation back to the home router.

>7)     
>dsliteAFTRAlarmProtocolType is underspecified.  It¹s a string,
>(³This object indicate the protocol type of alarm,
>                0:tcp,1:udp,2:icmp,3:total²)
>E.g., does that mean the string is ³0:tcp² or ³0² or ³tcp² or what?

[Yu]: We will add a clarification to describe the string as 0:tcp.

>8)     
>The
>dsliteStatisticTransmitted object seems to combine sent + received
>packets into a single counter with a name that implies only one direction.

[Yu]: We will separate dsliteStatisticTransmitted object into two objects for sent and received packets.

>9)     
>dsliteAFTRUserSessionNumAlarm and dsliteAFTRPortUsageOfSpecificIpAlarm
>both refer to ³the threshold² without stating what threshold that is.
>There doesn¹t seem to be any such threshold object in this MIB module or

[Yu]: We will define these threshold objects in the next version.

>10)  
>dsliteAFTRAlarmScalarGroup
>is mandatory and requires read-write access.
>people don¹t want write support in their MIB modules.  Does the WG really
>feel that write support is required in all implementations?  I¹d recommend

[Yu]:The read-write object defined in this MIB is intended to set a threshold value used to trigger a trap. So I think the write support is required in DSLite-MIB as the object of natv2InstanceThresholdPortMapEntriesHigh defined in RFC 7659

>11)  
>The security considerations section uses the correct boilerplate for
>sensitive read-only objects, which includes ³These are the tables and
>objects and their sensitivity/vulnerability².  However it then only

[Yu]: We will add the discussion in the security considerations section.

>12)  
>Per discussion on MIB Doctors, the root OID should probably not be

[Yu]: We will move the DSLite-MIB under mib-2

>13)  
>A number of undefined terms are used that I could not find in the DS-Lite
>RFC (6333) either, e.g., connection, session, etc.  As such, I couldn¹t
>tell what

[Yu]: The session mentioned here is referred to the RFC 4008. We will add the REFERENCE clauses and some description for connection in the object definition.
> 
>Editorial issues:
>14)  
>The title, abstract, MIB module name, etc. all say the MIB module is for
>

[Yu]: We will putting the AFTR in the title, abstract, the name.
>15)  
>There are a bunch of editorial issues (grammar, typos) that should be

[Yu]: We will revise the editorial issues accordingly. 

>16)  
>In a number of places, it seems to refer to some objects in some MIB
>module,
>whether this document or RFC 7659, without stating the name of the object

[Yu]: We will add the name of the object.

>17)  
>The document still refers to
>draft-perrault-behave-natv2-mib
>and should be RFC 7659.

[Yu]: We will update it for RFC7659
>Dave

Thanks very much for you review. An updated version of the draft will be submitted soon. 

BR 
Yu

---

From: Dave Thaler <[email-masked]>
Date: Fri, 11 Dec 2015 17:08:41 +0000

>>7)     
>>dsliteAFTRAlarmProtocolType is underspecified.  It¹s a string, but the 
>>description is very confusing as to what the legal string values are, 
>>making it sound more like an INTEGER was intended.
>>                0:tcp,1:udp,2:icmp,3:total²) E.g., does that mean the 
>>string is ³0:tcp² or ³0² or ³tcp² or what?
>

Why is it a string rather than an INTEGER?

Dave

---

From: "Yu Fu" <[email-masked]>
Date: Mon, 14 Dec 2015 02:32:35 -0000

Hi Dave,

I think both string and INTEGER will be ok. If you think INTEGER is better, we will change it into INTEGER.

Thanks again.

BR
Yu

-----Original Message-----
From: Dave Thaler [mailto:[email-masked]] 
Sent: Saturday, December 12, 2015 1:09 AM
To: Yu Fu; 'Benoit Claise'; 'The IESG'
Cc: [email-masked]; [email-masked]; [email-masked]; [email-masked]
Subject: RE: Benoit Claise's Discuss on draft-ietf-softwire-dslite-mib-12: (with DISCUSS and COMMENT)

Dave