Re: [Int-dir] Intdir telechat review of draft-ietf-rtgwg-vrrp-rfc5798bis-15

[Acee Lindem <[e-mail]>]

All, 

draft-ietf-rtgwg-vrrp-rfc5798bis-16 addresses Dave's INT area review comments. 

https://datatracker.ietf.org/doc/draft-ietf-rtgwg-vrrp-rfc5798bis/

Thanks,
Acee


> On Dec 30, 2023, at 4:13 PM, Acee Lindem <[e-mail]> wrote:
> 
> Hi Dave, 
> 
> Thanks for the review. 
> 
>> On Dec 27, 2023, at 6:30 PM, Dave Thaler via Datatracker <[e-mail]> wrote:
>> 
>> Reviewer: Dave Thaler
>> Review result: Ready with Issues
>> 
>> See https://1drv.ms/b/s!Aqj-Bj9PNivcn-MfckCWPYEAplaCJw?e=5TZtui for a copy with
>> my comments and editorial nits inline.
>> 
>> Summary:
>> 1. I am confused by the discussion of "forwarding" packets addressed to the
>> Active Router's address.  The Abstract and Introduction seem to talk about
>> doing it but then section 8.3.1 says not to.
> 
> The primary purpose of VRRP is to assume “forwarding” responsibility for the virtual addresses.
> I don’t see any compelling reason to change this now. I could change “sent to these IPv4 and IPv6
> addresses” to “routed to these IPv4 and IPv6 addresses” to avoid any confusion that this forwarding
> is tied to the packet header destination addresses. However, I don’t even see this as needed. 
> 
> 
>> 2. Missing discussion of DHCPv4. 
>> Section 1.3 seems to imply that static configuration of end hosts is the
>> primary mechanism for learning default routes, which is not the case for
>> clients or IoT devices as far as I know... DHCP is the default.  I believe VRRP
>> can still be used in a DHCP scenario and the document should say so.
> 
> Yes - but DHCP is really just another form of static route configuration. I’ll change
> this to “manual configuration” and include DHCPv4 [2131] and DHCPv6 [RFC8415]
> as well as static configuration. Any other suggested references? 
> 
> 
>> 3. Section
>> 4.2's discussion of IPv6 is confusing to me (and I wrote one of the relevant
>> RFCs).  If there are two routers sending RA's on the same LAN, then by default
>> all hosts learn _both_ of them.  The text implies half learned one and half
>> "are using" the other one.  This text needs to be clarified and then probably
>> reference RFC 4191 and RFC 4311 for more discussion.  Even better would be to
>> update the text to specifically discuss the interaction between VRRP and 4311
>> (which I think would be straightforward), and if needed mention different cases
>> for the different host types in RFC 4191 section 3 (it's also possible that the
>> interaction with VRRP is the same for all the types and the types need not be
>> mentioned except to say that the interaction is the same for all the host types
>> there).
> 
> For IPv6, I could change “learned” to “configured” since the purpose of section 4.2
> Is to demonstrate load-sharing and not specify IPv6 Router-Advertisement behavior.
> Alternately, I could change “learned” to “preferred” with a reference to RFC 4311. 
> 
> 
> 
>> 4. A couple places use "should" in cases where it's unclear whether it
>> means SHOULD or MUST (or even "MAY" when "may" occurs earlier in the text). 
>> This could adversely affect interoperability if it meant MUST and someone
>> interprets it as optional.
> 
> I’ll look at all of these. As long as the statement is specific to VRRP, I will consider
> changing these to normative. 
> 
> 
> 
>> 5. Section 8.3.2 says to log when multiple routers
>> advertise priority = 255, but doesn't say to log when multiple routers
>> advertise the same non-255 priority.  It says not to do that, so why wouldn't
>> you want to suggest logging any time the same priority is advertised by
>> multiple routers?  I.e., why is the logging recommendation limited to the 255
>> case?
> 
> The VRRP protocol handles this case with a tie-breaker of the VRRP router 
> with the VRRP router with the greater primary IP address taking precedence. The 
> reason for recommending that VRRP routers have different priorities is to minimize
> the churn do to them having the same advertisement skew time. It is not a protocol error. 
> 
>> 
>> . Various grammatical nits.
> 
> I’ll incorporate these. Note that the pseudo-code wasn’t meant to be grammatically
> correct. However, the changes you have suggested may not obscure the logic and I’ll consider
> them.
> 
> Thanks
> Acee