Subject: Re: [bess] Shepherd review on draft-ietf-bess-mvpn-mib-01

From: "Jeffrey (Zhaohui) Zhang" <[email-masked]>
Date: Mon, 25 Jan 2016 21:41:42 +0000

Mach,

Thank you very much for the review. Please see zzh> below.

> -----Original Message-----
> From: BESS [mailto:[email-masked]] On Behalf Of Mach Chen
> Sent: Tuesday, December 22, 2015 3:18 AM
> To: [email-masked]
> Cc: [email-masked]; [email-masked]
> Subject: [bess] Shepherd review on draft-ietf-bess-mvpn-mib-01
> 
> Hi Authors,
> 
> I am requested (by the WG chairs) to shepherd this draft, here are my
> shepherd review comments on this document.
> 
> 
> 1. To make the document more readable, expect for the well-known
> abbreviations, there are a lot of abbreviations (e.g., MVRF, I-/S-PMSI,
> etc.) that should be expanded when first use.

Zzh> Will do.

> 
> 2. Abstract and introduction section
> 
> s/ multicast in MPLS/BGP-based Layer-3 VPN (MVPN)/ Multicast in BGP/MPLS
> in IP VPN (MVPN),  to align with the definition in RFC6513

Zzh> will do.

> 
> 3.
> Section 2.1
> 
>    - mvpnIpmsiTable/Entry
> 
>      This table contains all advertised or received intra-as I-PMSIs.
>      With PIM-MVPN, it is applicable only when BGP-Based Autodiscovery
>      of MVPN Membership is used.
> 
>    - mvpnInterAsIpmsiTable/Entry
> 
>      This table contains all advertised or received inter-as I-PMSIs.
>      With PIM-MVPN, it is applicable only when BGP-Based Autodiscovery
>      of MVPN Membership is used.
> 1) For each table, why is there a "/Entry" followed?  To avoid confusion,
> I'd suggest to remove them if there is no special intention or meaning.

Zzh> It is meant to say that xxxEntry are xxxTable are associated and their usage is explained in the paragraphs. I will remove the "/Entry"

> 
> 2 )In addition, from the above text, I have the feeling that a
> mvpnIpmsiTable or mvpnInterAsIpmsiTable can only contain advertised or
> received I-PMSIs, but cannot contain both. Is this the intention?

Zzh> Both. I'll change "or" to "and".

> 
> 
> 4. Section 2.2, the LAST-UPDATED, ORGANIZATION, CONTACT-INFO and the
> Revision history should be updated to reflect the latest status.

Zzh> I had expected this to be updated at the last moment (e.g. at publication), but I am not sure. Will do whatever is the right procedure.

> 
> 5. mvpnMvrfNumber OBJECT-TYPE
>      SYNTAX        Unsigned32
>      MAX-ACCESS    read-only
>      STATUS        current
>      DESCRIPTION
>          "The total number of MVRFs for IPv4 or IPv6 or mLDP
>           C-Multicast that are present in this device."
> 
> Should the "or" be changed to "and"?

Zzh> It's for any of those types. Someone said we should use "or". I am open to suggestions.

> 
> 6.
> There are several places in draft say the following or similar:
> "An entry in this table is created for every MVRF in the device."
> I'd suggest to replace the "every" as "each".

Zzh> Will do.

> 
> 7.
> Page 10:
> mvpnGenCmcastRouteProtocol OBJECT-TYPE
>      SYNTAX        INTEGER { pim (1),
>                              bgp (2)
>                            }
>      MAX-ACCESS    read-write
>      STATUS        current
>      DESCRIPTION
>          "Protocol used to signal C-multicast states across the
>           provider core.
> 
> s/ Protocol /The protocol is

Zzh> Will do.

> 
> 8.
> Page 13:
> mvpnBgpGenVrfRtImport OBJECT-TYPE
>      SYNTAX             MplsL3VpnRouteDistinguisher
>      MAX-ACCESS         read-write
>      STATUS             current
>      DESCRIPTION
>          "The VRF Route Import Extended Community that this device
>           adds to unicast vpn routes that it advertises for this mvpn."
>      ::= { mvpnBgpGeneralEntry 2}
> 
>   mvpnBgpGenSrcAs      OBJECT-TYPE
>      SYNTAX            Unsigned32
>      MAX-ACCESS        read-only
>      STATUS            current
>      DESCRIPTION
>          "The Source AS number in Source AS Extended Community that this
>           device adds to the unicast vpn routes that it advertises for
>           this mvpn."
> 
> Why should the "Source", "Extended", and "Community" be upper case? If
> there is no special intention, suggest to change to lower case. You may
> need to look through the whole document to do a text clean up.

Zzh> That's per RFC 6513.

> 
> 9.
> Section 3, Security consideration.
> 
> Given that the document introduces some read-write objects, I don't think
> that the current statement of "This document does not introduce new
> security risks." will pass the IESG review. I'd suggest the authors to
> enhance the security consideration section. For the mib security
> consideration, you may refer to some existing mid documents (e.g,
> https://tools.ietf.org/html/draft-ietf-mpls-tp-oam-id-mib-11) that have
> already passed the IESG review. Also, you may refer to some history
> discussions on a mib security consideration (e.g.,
> https://datatracker.ietf.org/doc/draft-ietf-trill-oam-mib/history/ ).

Zzh> This seems to be no different from CLI configuration/monitoring? But I will study those past discussions.

> 
> 10.Please make sure that the MIB Modules are compiled cleanly.

Zzh> I did use http://www.simpleweb.org/ietf/mibs/validate/. Did you notice any problem?

> 
> 11. BTW, I saw the WG chairs had issued the IPR poll, and only Jeffery
> replied, to progress this draft, the authors and contributors have to
> respond to the IPR poll.

Zzh> Working on that.

> Happy Holidays!

Zzh> Thanks! Part of the reason for this late response :-)

Jeffrey

> 
> Best regards,
> Mach
> 
> 
> 
> 
> 
> _______________________________________________
> BESS mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/bess

---

From: Mach Chen <[email-masked]>
Date: Tue, 26 Jan 2016 06:40:08 +0000

Hi Jeffery,

Some responses inline...

Snipped.

> Zzh> It's for any of those types. Someone said we should use "or". I am open to

If the mvpnMvrfNumber is designed to be the number of each type of the VRFs, then "or" should be used and it's better to add some clarification text here. Otherwise, "and" should be used. When I read the mib, it gives my feeling that mvpnMvrfNumber is the number of all MVRFs. 

> > there is no special intention, suggest to change to lower case. You
> > may need to look through the whole document to do a text clean up.
> 

If that's the case, I'm fine with it.

> > Given that the document introduces some read-write objects, I don't
> > think that the current statement of "This document does not introduce
> > new security risks." will pass the IESG review. I'd suggest the
> > authors to enhance the security consideration section. For the mib
> > security consideration, you may refer to some existing mid documents
> > (e.g,
> > https://tools.ietf.org/html/draft-ietf-mpls-tp-oam-id-mib-11) that
> > have already passed the IESG review. Also, you may refer to some
> > history discussions on a mib security consideration (e.g.,

Please do so and update the security consideration section.

> Zzh> I did use http://www.simpleweb.org/ietf/mibs/validate/. Did you notice

I did not run any validation, if you did, that's great.

Once you update the document, we can start the MIB doctor review. 

Best regards,
Mach