[bess] John Scudder's Discuss on draft-ietf-bess-evpn-fast-df-recovery-10: (with DISCUSS and COMMENT)

[John Scudder via Datatracker <[e-mail]>]

John Scudder has entered the following ballot position for
draft-ietf-bess-evpn-fast-df-recovery-10: 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/about/groups/iesg/statements/handling-ballot-positions/ 
for more information about how to handle DISCUSS and COMMENT positions.


The document, along with other ballot positions, can be found here:
https://datatracker.ietf.org/doc/draft-ietf-bess-evpn-fast-df-recovery/



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

# John Scudder, RTG AD, comments for draft-ietf-bess-evpn-fast-df-recovery-10
CC @jgscudder

Thanks for this document. I appreciate the fact it was straightforward to read
and review, and of course, I appreciate the work on a useful protocol extension!

My earlier ballot, despite what I noted in the header, was actually a review of
version 9, not version 10. Looking at the document history, the version changed
out from under me sometime during the day, after I had mostly completed my
review but before I submitted it. I cut and pasted the document name without
noticing. I'm sorry for the oversight and hope I didn't cause anyone extra
work. I see that version 10 fixes many of the things I commented/discussed on
(probably due to Toerless Eckert's iotdir review which raised many of the same
issues). Having now read Toerless's review and Luc André's reply (I could have
saved myself some work if I'd read them earlier!) I've revised my ballot
accordingly. It also seems as though the conversation with Toerless is still in
flight, so my assumption is there may be more updates to the document as that
conversation unfolds.

I also hope you'll be addressing Ines Robles' RTGDIR review. It looks like Ines
has overlapping concerns with the ones Toerless and I raised, and some others
too.

## DISCUSS

I have two concerns I'd (still) like to discuss.

### "Unreasonable" SCT offsets

Apart from a few words in the Security Considerations section, you don't
provide any instructions to the implementer about what to do with unreasonable
SCT offsets, in particular unreasonable SCT future offsets (see also Ines'
review). The language from Section 5 is,

   It is left to implementations to
   decide what consists an "unreasonably large" SCT value.

The quotation marks imply that you think the implementor should do something
about unreasonably large SCT values, but the spec doesn't supply any clarity
about it. That's too bad, it should. I accept the argument in the preceding
Security Considerations sentences (not quoted) that the ability to signal
unreasonable SCT values doesn't introduce a new vulnerability vis-à-vis the
pre-existing specifications. Nonetheless, it does seem like this specification
should advise implementers to impose an upper limit on what SCT future offset
they will accept. The quoted sentence is a backhanded hint in that direction. I
think you should make it explicit, probably in Section 2 since that seems to be
where elements of procedure mostly go (see also my later remarks about this in
the COMMENTS section).

I see that you've revised Section 2.2 compared to my earlier review of version
9. It looks as though it could use a little more work, though, to capture two
things: First and most importantly, explicitly introduce the idea of a maximum
acceptable SCT offset (what I called max_SCT_offset in my earlier ballot).
Second, explicitly address the case where SCT minus skew is in the past. Sure,
the latter is "obvious", but one of our goals in writing specifications is to
be thorough, so IMO this is worth capturing (it isn't DISCUSS-worthy in itself,
I mention it here only because it's part of the same section).

### Assumptions about synchronized clocks

Although the document assumes synchronized clocks, it doesn't provide any
details about its assumptions, e.g. how close synchronization needs to be and
what the consequences are if the requirement isn't met. Normally,
specifications assuming synchronized clocks will provide at least some basic
analysis.

I think probably if you fix the max_SCT_delay thing, the analysis would be easy
because it would in effect bound the maximum effect of clock desynchronization,
and I bet the analysis would amount to "if you have badly desynchronized
clocks, it acts like RFC7432." (But this is just a guess, you are the experts.)

If you choose to add a short analysis, I could see it being a bullet in Section
1.4.

(I do see both Ines and Toerless raised some related concerns in their reviews,
although not this exact question AFAICT.)


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

## COMMENTS

### Section 3, peering timer

A new issue I noticed in version 10 is that you've added this paragraph:

   The peering timer is a configurable value where 3 seconds represents
   the default.  Configuring a timer value of 0, or so small as to
   expire during propagation of the BGP routes, is outside the scope of
   this document.  In reality, the use of the SCT approach presented in
   this documents encourages the use of larger peering timer values to
   overcome any sort of BGP route propagation delays.

However, this is the only place the document refers to a "peering timer".
Thanks for the clarifying paragraph, it's good. But I think you should do
something to clear up the terminology ambiguity. For example, everywhere you're
referring to this timer, you could use the term "peering timer" instead of just
"timer", and you could add it to your terminology section, along with a
reference to RFC 7432 Section 8.5 step (2).

(Also, s/this documents/this document/)

### Section 1.4, this sentence no verb

(fixed in 10)

### Section 1.4, "normalizes"

(fixed in 10)

### Sections 2 and 3 aren't well-divided

Section 2 is called "DF Election Synchronization Solution". Based on that, I
would assume that the entire solution would be specified in this section and
its subsections. Section 3 is called "Synchronization Scenarios". Based on
that, I would assume it contains examples, intended to help the reader
understand the operation of the solution.

Unfortunately, that's not entirely the case. Section 3 contains instances of
normative, or should-be-normative, text. I think you should restructure these
two sections to put the entire solution in Section 2 and keep Section 3 as
examples. By the way, I did find the examples very useful, thank you for
including them. I've flagged suggestions for restructuring in other more
specific comments.

For that matter, there's also normative text in Section 5: "the receiving PE
SHALL treat the DF Election at the peer as having occurred already, and proceed
without starting any timer to futher delay service carving". That's not a
consideration, that's a requirement, and I think it should also go into Section
2. The approach suggested in my DISCUSS would fix this issue. (Also
s/futher/further/.)

### Section 2, several things about skew

(fixed in 10)

### Section 2.1, that's not 8 octets

```
   A new BGP extended community is defined to communicate the Service
   Carving Timestamp for each Ethernet Segment.

   A new transitive extended community where the Type field is 0x06, and
   the Sub-Type is 0x0F is advertised along with the Ethernet Segment
   route.  The expected Service Carving Time is encoded as an 8-octet
   value as follows:

                        1                   2                   3
    0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   | Type = 0x06   | Sub-Type(0x0F)|      Timestamp Seconds        ~
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   ~  Timestamp Seconds            | Timestamp Fractional Seconds  |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
```

Did you mean six octets? Because that’s what the diagram shows... unless you're
calling 0x060F part of the service carving time, which doesn't make much sense.
I don't know, maybe you meant that it's an eight-octet value encoded in six
octets, but you explain that just fine in the text that follows. I think you'd
be better off simplifying the introductory text to something like this,

NEW:
   A BGP extended community, with Type 0x06 and Sub-Type 0x0F, is defined
   to communicate the Service Carving Timestamp for each Ethernet Segment:

                        1                   2                   3
    0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   | Type = 0x06   | Sub-Type(0x0F)|      Timestamp Seconds        ~
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   ~  Timestamp Seconds            | Timestamp Fractional Seconds  |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

Note I also elided "new" because fairly soon after publication as an RFC, the
community will no longer be "new". Then you let the following text supply the
additional detail that I've removed from my NEW text.

### Section 2.1, "outside of the scope of this document"

“A DF Election operation occurring exactly at the Era transition boundary some
time in 2036 is outside of the scope of this document.”

That’s fine as long as the consequences of such an incident wouldn’t be
serious. Have you analyzed this? A few words to this effect would be desirable;
the rollover is less than twelve years in the future and your specification is
likely to still be in use then.

I bet that, again, the change suggested in my DISCUSS would make this easy to
cover because even a naïve implementation would treat era rollover as nothing
worse than severe clock skew, and the window of vulnerability would be bounded
to max_SCT_delay. I'd still encourage you to say a few words about it, to
console any readers doing a Y2036 review sometime in the future.

### Section 3.1, concurrent elections

   In the case of multiple concurrent DF elections, each initiated by
   one of the recovering PEs, the SCTs must be ordered chronologically.
   All PEs shall execute only a single DF Election at the service
   carving time corresponding to the largest (latest) received timestamp
   value.  This DF Election will involve all active PEs in a unified DF
   Election update.

This looks and feels exactly like normative specification, which makes me think
it belongs in Section 2. I also wonder if you want SHALL instead of "shall".

## Notes

This review is in the ["IETF Comments" Markdown format][ICMF], You can use the
[`ietf-comments` tool][ICT] to automatically convert this review into
individual GitHub issues.

[ICMF]: https://github.com/mnot/ietf-comments/blob/main/format.md
[ICT]: https://github.com/mnot/ietf-comments