Subject: Re: [pim] AD Review of draft-ietf-pim-drlb-10

From: "Mankamana Mishra (mankamis)" <[email-masked]>
Date: Mon, 26 Aug 2019 15:40:02 +0000

Hi Alvaro, 
Me & Stig would go over your comments and address it soon. 

Thanks 
Mankamana 

-----Original Message-----
From: Alvaro Retana <[email-masked]>
Date: Monday, August 26, 2019 at 7:57 AM
To: "[email-masked]" <[email-masked]>
Cc: "[email-masked]" <[email-masked]>, "[email-masked]" <[email-masked]>, Mike McBride <[email-masked]>
Subject: Re: AD Review of draft-ietf-pim-drlb-10
Resent-From: <[email-masked]>
Resent-To: <[email-masked]>, <[email-masked]>, <[email-masked]>, <[email-masked]>, <[email-masked]>, <[email-masked]>
Resent-Date: Monday, August 26, 2019 at 7:57 AM

    Hi!
    
    It’s been a while since this review…any thoughts?
    
    Thanks!
    
    Alvaro.
    
    On June 21, 2019 at 12:46:30 PM, Alvaro Retana ([email-masked])
    wrote:
    
    Dear authors:
    
    First of all, thank you for reviving this work!!
    
    I just finished reading the document and have significant concerns (please
    see details in line).  In fact, I am pointing at the same general issue
    that the original AD review [1] did: under-specification.  Adding text in
    6.2 has resulted in behavior being specified (inconsistently!) more than
    once.
    
    Also, the specification of the Hash Algorithm is included in §4 (Functional
    Overview).  Not being in an appropriately named section is significant.
    Please move this part to §6 (Protocol Specification), or maybe its own
    section.
    
    Even though there is much work needed, I don't want to return this document
    to the WG and risk more delay.  While some major issues will require
    changes, I think that many of them are about the structure of the
    document.  If needed, I will rely on Mike (Chair/Shepherd) to coordinate
    further WG review.
    
    Thanks!
    
    Alvaro.
    
    [1] https://mailarchive.ietf.org/arch/msg/pim/K61q4--5ZBb9RTMkeud-5MnmuOw

    [Line numbers from id-nits.]

    ...
    90 1.  Introduction
    
    92   On a multi-access LAN such as an Ethernet, one of the PIM routers is
    93   elected as a DR.  The PIM DR has two roles in the PIM-SM protocol.
    94   On the first hop LAN, the PIM DR is responsible for registering an
    95   active source with the Rendezvous Point (RP) if the group is
    96   operating in PIM-SM.  On the last hop LAN, the PIM DR is responsible
    97   for tracking local multicast listeners and forwarding to these
    98   listeners if the group is operating in PIM-SM.
    
    [minor] The first reference to rfc7761 is below, when talking about DR
    election.  Consider moving that reference to this first paragraph so that
    the stage is set early.
    
    [nit] I know DR was extended in the Abstract, the acronym is used above and
    the extended version again below...  Please extend DR above and use the
    contracted form elsewhere.

    ...
    114   Assume R1 is elected as the Designated Router.  According to
    115   [RFC7761], R1 will be responsible for forwarding traffic to that LAN
    116   on behalf of any local members.  In addition to keeping track of IGMP
    117   and MLD membership reports, R1 is also responsible for initiating the
    118   creation of source and/or shared trees towards the senders or the
    119   RPs.
    
    [minor] We need Informative references to IGMP and MLD.
    
    121   Forcing sole data plane forwarding responsibility on the PIM DR
    122   uncovers a limitation in the protocol.  In comparison, even though an
    123   OSPF DR or an IS-IS DIS handles additional duties while running the
    124   OSPF or IS-IS protocols, they are not required to be solely
    125   responsible for forwarding packets for the network.  On the other
    126   hand, on a last hop LAN, only the PIM DR is asked to forward packets
    127   while the other routers handle only control traffic (and perhaps drop
    128   packets due to RPF failures).  Hence the forwarding load of a last
    129   hop LAN is concentrated on a single router.
    
    [minor] "In comparison..."  I think that comparing the PIM DR to an OSPF
    DR/IS-IS DIS is completely out of place since the justification for this
    work is to share forwarding responsibilities...and the IGP DR/DIS are not
    responsible for forwarding at all!  IOW, the comparison doesn't make sense.
    
    [minor] RPF failures are only mentioned here.  Either expand on them and
    explain why they are relevant to this work, or remove the text to avoid
    confusion.
    
    131   This leads to several issues.  One of the issues is that the
    132   aggregated bandwidth will be limited to what R1 can handle towards
    133   this particular interface.  It is very common that the last hop LAN
    134   consists of switches that run IGMP/MLD or PIM snooping.  This allows
    135   the forwarding of multicast packets to be restricted only to segments
    136   leading to receivers who have indicated their interest in multicast
    137   groups using either IGMP or MLD.  The emergence of the switched
    138   Ethernet allows the aggregated bandwidth to exceed, sometimes by a
    139   large number, that of a single link.  For example, let us modify
    140   Figure 1 and introduce an Ethernet switch in Figure 2.
    
    [nit] "this particular interface"  Which interface?  I'm sure you mean the
    one to the core networks...but please be explicit.
    
    [minor] "IGMP/MLD or PIM snooping"  We need a reference.

    ...
    184   There is a limitation in the hash algorithm used in this document,
    185   but this document provides the option to have different and more
    186   consistent hash algorithms in the future.
    
    [minor] I think that the limitations discussion should be left to §4.3.1.
    IOW, I suggest taking this paragraph out.

    ...
    193 2.  Terminology
    
    195   The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT",
    196   "SHOULD", "SHOULD NOT", "RECOMMENDED", "MAY", and "OPTIONAL" in this
    197   document are to be interpreted as described in [RFC2119].
    
    [major] Please use the rfc8174 template.

    ...
    204   o  GDR: GDR stands for "Group Designated Router".  For each multicast
    205      flow, either a (*,G) for ASM, or an (S,G) for SSM, a hash
    206      algorithm (described below) is used to select one of the routers
    207      as a GDR.  The GDR is responsible for initiating the forwarding
    208      tree building process for the corresponding multicast flow.
    
    [nit] s/GDR: GDR stands for "Group Designated Router"./GDR: Group
    Designated Router.
    
    [minor] ASM needs to be expanded
    
    210   o  GDR Candidate: a last hop router that has the potential to become
    211      a GDR.  A GDR Candidate must have the same DR priority and must
    212      run the same GDR election hash algorithm as the DR router.  It
    213      must send and process new PIM Hello Options as defined in this
    214      document.  There might be more than one GDR Candidate on a LAN,
    215      but only one can become GDR for a specific multicast flow.
    
    [major] This paragraph uses "must" several times.  Should that be Normative
    language?
    
    [??] Why is it a requirement to have the same DR priority?  It seems to me
    that there would be more options and even better load sharing if more
    routers could be GDR.

    ...
    229 4.  Functional Overview
    ...
    256   Hash Masks are defined for Source, Group and RP separately, in order
    257   to handle PIM ASM/SSM.  The masks, as well as a sorted list of GDR
    258   Candidate Addresses, are announced by the DR in a new DR Load
    259   Balancing GDR (DRLBGDR) PIM Hello Option.
    
    [nit / personal opinion] The abbreviations for the new Options are awful --
    I keep having to think about the meaning and whether they are spelled
    correctly at every step.  Perhaps something reader-friendly acronyms would
    be better.  Suggestions:
    
    - DR Load Balancing Capability PIM Hello Option: LB Option
    - DR Load Balancing GDR PIM Hello Option: GDR List Option (maybe even GL)
    
    261   A hash algorithm based on the announced Source, Group, or RP masks
    262   allows one GDR to be assigned to a corresponding multicast state.
    263   And that GDR is responsible for initiating the creation of the
    264   multicast forwarding tree for multicast traffic.
    
    [major] Is there a possibility that the assigned GDR for a specific
    multicast state can't fulfill its duties?  This document assumes that all
    the GDRs are able to service all states...but that may not be true.
    Because every GDR candidate calculates who the GDR is for a specific state,
    it may never know what an alternate GDR is not able to forward traffic.
     [Does this make sense?]
    
    266 4.1.  GDR Candidates
    
    268   GDR is the new concept introduced by this specification.  GDR
    269   Candidates are routers eligible for GDR election on the LAN.  To
    270   become a GDR Candidate, a router MUST support this specification,
    271   have the same DR priority and run the same GDR election hash
    272   algorithm as the DR on the LAN.
    
    [nit] There is a lot of repetition in the text...for example, all the
    contents on this paragraph were already mentioned in the last section...
    Cleaning up the text would be nice.
    
    [major] "To become a GDR Candidate, a router MUST support this
    specification..."  First of all, this is an obvious statement: the GDR is
    defined in this document.  Second, there's no Normative value in using MUST

---

From: Stig Venaas <[email-masked]>
Date: Tue, 27 Aug 2019 14:54:36 -0700

Hi

On Sat, Jun 22, 2019 at 8:08 AM Alvaro Retana <[email-masked]> wrote:
> I just finished reading the document and have significant concerns (please see details in line).  In fact, I am pointing at the same general issue that the original AD review [1] did: under-specification.  Adding text in 6.2 has resulted in behavior being specified (inconsistently!) more than once.
>
> Also, the specification of the Hash Algorithm is included in §4 (Functional Overview).  Not being in an appropriately named section is significant.  Please move this part to §6 (Protocol Specification), or maybe its own section.
>
> Even though there is much work needed, I don't want to return this document to the WG and risk more delay.  While some major issues will require changes, I think that many of them are about the structure of the document.  If needed, I will rely on Mike (Chair/Shepherd) to coordinate further WG review.

Thanks, I agree with most of your comments and will try to address
them. There are a couple of technical points I would like to discuss
though. Please see below.
I only kept the points I'm commenting out.

> [??] Why is it a requirement to have the same DR priority?  It seems to me that there would be more options and even better load sharing if more routers could be GDR.

An administrator can choose exactly which routers should take part by
giving them all the highest priority. The hashing will cause all of
the ones with the highest priority to share the burden more or less
equally, there is no prioritization needed among the routers in the
set. An administrator could potentially give all the routers the
highest priority.

> [major] Is there a possibility that the assigned GDR for a specific multicast state can't fulfill its duties?  This document assumes that all the GDRs are able to service all states...but that may not be true.  Because every GDR candidate calculates who the GDR is for a specific state, it may never know what an alternate GDR is not able to forward traffic.  [Does this make sense?]

It is assumed that a router is only configured as GDR if it is
expected to have the necessary connectivity etc to fulfill its role.
This is no different than expecting today that any router elected as
the DR has the necessary connectivity. Implementations could
potentially have some ways of automatically decrease the priority in
failure scenarios to avoid being the DR. This is not specific to DRLB
though.

> [minor] Can you provide any guidance on how to configure the hash masks?  The example below shows a mask of 0.0.255.0, which seems both "unnatural" (for the average person used to set masks) and very specific (in order for N to have a specific value).  Maybe it's just me, but I would have set the mask to something similar to 255.255.0.0/0.0.255.255.  Guidance should be added to §8 (Manageability Considerations).

The reason for allowing such masks rather than just prefix lengths is
to allow load-balancing regardless of how service providers utilize
their space. I will try to think of examples. It would allow for
ensuring that certain groups always are hashed to the same router
(that the hash value computed is exactly the same for the groups).
That is, H(G1) == H(G2) if G1 and G2 are the same after the mask is
added. Hence the same router will be selected as the GDR for both G1
and G2.

> [major] What does "provide" mean in this context?  I guess that it means that if no hash mask is configured, then the default should be used.  Is that correct?
>
> [major] When would an implementation not provide those masks?  IOW, why aren't you using MUST?

Just using default masks would be sufficient for most deployments.

> [?] I couldn't figure out from rfc7761 whether all Options have to be included in all Hellos, all the time. Do they?

In general every hello messages would contain the exact same options,
unless some state (typically config), changes.

> [major] "The addresses are sorted in descending order." Should this be Normative?  Does the ordering (ascending/descending/random) really matter?  It seems to me that because the DR is the only one making the advertisement then it could do whatever and the hash would still work, am I missing something?

The listed routers are the GDR and when you use modulo, you need all
the GDRs to agree on the order for the modulo to indicate which router
 is responsible.

> [major] Should the receiving router verify that the DR announced the same hash algorithm in its DRLBC?  What should happen if the receiver's address is listed in the DRLBGDR, but the advertised hash algorithms don't match?  Even though it is specified in several places that the hash algorithms must be the same, there's a possibility (bug, attack) that the algorithms don't match.  If the DRLBGDR is used (when no match in the algorithms is verified), then it may result in an unexpected state...  [See also my comments in the Security Considerations section.]

In the worst case if one of the GDRs don't support the listed
algorithm it cannot take part. There is no way for it to compute which
groups it should be responsible for. So if the DR violated the spec
(or there is an attack) this would then result in no GDR acting as GDR
for some groups.

> [major] Why would R2 never relinquish its role?  I can imagine the corner case you identified here where R2 doesn't receive R1s Hello...but that could result in even R2 thinking it is the DR.  Is there a chance that the interpretation of the algorithm could end up producing inconsistent results?  I haven't thought about this too much, but it seems that the only possibility is if the candidate GDRs have a different view of the (*,G)/(S,G) state:  for example, if they think the RP is different for a specific group...but I don't know how likely that is?  Is there an opportunity for someone upstream to alter the information so that the local LAN don't consistently converge on who the GDRs should be?
>
> Is this a potential security issue?  Am I being paranoid?

Let me think about this some more. But there is a risk of getting
duplication or no forwarder if hellos are lost. What is essential is
that whenever he DR sends a list of participating routers, all routers
receive the hello (If the list didn't change, there is no issue if 1-2
hellos are lost). It should not be a problem if other hellos are lost.
Note that with regular PIM, there are also possible issues with loss
and two routers thinking they are DRs, or all routers thinking for a
while that they are not DRs.

It will take a while for us to revise the document.

Thanks for good and detailed comments,
Stig