Early Review of draft-ietf-nvo3-gue-03
review-ietf-nvo3-gue-03-rtgdir-early-farrel-2016-07-07-00

Hi, 

I've been randomly selected from the Routing Directorate to perform a
QA review of this document. The philosophy behind QA reviews can be
found at 

https://trac.tools.ietf.org/area/rtg/trac/wiki/RtgDirDocQa



In short, the purpose of the review is to uncover issues or fixing or
wider discussion earlier in the process than sometimes happens with
RTG Dir reviews around or even after WG last call. It is not my 
intention to be overly critical or harsh n my review, but I have tried
to raise everything I could think of - my intention is to allow you to
be able to say "Yes, that was looked at, discussed, and agreed."

Please do follow up with questions or discussion, but don't feel that
you have to convince me of things - you need to convince the WG.

Cheers,
Adrian

---

Being forced to read this document, I'm afraid I was required to enter a
third-party IPR disclosure because of the IPR already disclosed against
draft-ietf-mpls-in-udp that became RFC 7510. This should show up on the
NVO3 mailing list.

---

It seems to me that there is some careful coordination needed with other
work on encapsulation of transport or network protocols in UDP. This
idea clearly has value in NVO3, but I should have thought it sat better
in the TSVWG. I hope the NVO3 chairs have discussed this with the TSVWG
chairs to ensure that there is no friction. This is particularly 
important because it will be important to recognise that only one of
this draft and draft-manner-tsvwg-gut is likely to make it to RFC.

You seem to have correctly addressed the three issues that have most 
worried the TSVWG (checksum, congestion and security), so that is all 
good, but I would recommend getting the TSVWG involved for a full and 
detailed review now and for each future revision of the document. In
fact, I would have tended towards making this a TSVWG document, but so
long as the chairs, the ADs, and the WGs are happy, that should be fine.

---

Overall, this work is a good idea and needed. When we did MPLS-in-UDP
there was a background proposal to generalise and only burn one port
number for al UDP encapsulations. This achieves that end.

However, I think this proposal may be too general and too extensible.
Future-proof is good, but there seem to be a lot of bells and whistles
defined here that have no specific use proposed, and no indication that
a future use might ever be defined. I think it is one thing that it
should be possible to extend a protocol, and another that it defines
multiple fields and extension mechanisms that might never be used.

I comment on some of these mechanisms in my notes below.

---

In section 3.1, please add a forward pointer to section 6 instead of
"below"

      o Source port (inner flow identifier): This should be set to a
        value that represents the encapsulated flow. The properties of
        the inner flow identifier are described below.

Probably add a forward pointer each time "inner flow identifier" is
mentioned.
           
---

In 3.1 when describing the contents of the Proto/ctype field it would 
be helpful to b crystal clear of which set of IP protocol numbers you
are using. Maybe a reference to


http://www.iana.org/assignments/protocol-numbers/protocol-numbers.xhtml



---

Maybe add forward pointers to 3.2, 3.3, and 3.4 from 3.1.

Similarly 3.5 from 3.2.

---

I wonder whether you have possibly overdone the future-proofing since
you have defined no uses for flags nor any possible fields or extension
fields. The mechanism you have defined could actually be added later if
needed (although it is possible to believe this to not happen for a 
number of years) by assigning a flag (the E flag as now), defining a new
field to contain extension flags, and proceeding as described.

There is nothing wrong with what you have written, but it does seem to
complicate the base protocol with a very long-term extensibility
horizon.

---

3.3 has 

   New flags should be
   allocated from high to low order bit contiguously.

I am pretty sure you want s/should be/are to be/
Also, this text needs to be in the IANA section as well otherwise IANA 
will not know that they are constrained.

---

3.3

   Flags may be paired together to allow different lengths for an
   optional field. For example, if two flag bits are paired, a field may
   possibly be three different lengths. Regardless of how flag bits may
   be paired, the lengths and offsets of optional fields corresponding
   to a set of flags must be well defined.

This works, of course, but aren't you again being too flexible and too
clever? For a field that might have two lengths, you are not saving any
bits. Why not simply allow a field with two possible lengths to simply
be defined as two different fields?

---

3.3

   Flags (or paired flags) are idempotent such that new flags should not

Is that s/should not/do not/ ?

---

3.4                                                                  

   An encapsulator and decapsulator MUST agree on the meaning
   of private data before using it.

How? Using an OID? Using a control message? 

---

I am not enthusiastic about allowing "private data" in the packet 
header. I can see its use for specific functions that you have called
out (security and identifiers), but even then I am not too comfortable.
Actually, wouldn't security and identifiers by standard fields rather
than private data?

Recall that these UDP packets will be exchanged by many implementations
and the ideal is that every speaker should be able to understand every
packet. Also recall that things that might be used as covert channels 
are best avoided.

---

Don't you need an IANA registry to track control messages?

---

Version 0x01 of GUE is very "clever". I wonder whether it is really
necessary.

In any case, you should discuss it in the Introduction with an
explanation of what it is, and some motivation.

---

5.4 has

   The decapsulator validates packets, including fields of the GUE
   header. If a packet is acceptable, the UDP and GUE headers are
   removed and the packet is resubmitted for IP protocol processing or
   control message processing if it is a control message.

...but, of course, the contents of the GUE packet that is not a control
packet may be any protocol as indicated by the version number and proto
fields. Passing anything other than IP for IP protocol processing might
be considered a mistake :-)

---

In 5.4

   ...otherwise malformed
   header, it must drop the packet and may log the event.

That is better as MUST and MAY according to other usage in the draft.
You might do well to check all uses of must/should/may to check whether
they could/should be in upper case.

When saying "may log the event" it is traditional (and probably good) to
also give advice about thresholds and risks of log-swamping when under
attack.

Although "otherwise malformed" might cover it, I think you should call
out "unknown or unsupported payload protocol".

---

In 5.5

   It
   may encapsulate a GUE packet in another GUE packet, for instance to
   implement a network tunnel.

Doesn't that require a protocol number to be assigned for GUE?

---

In 5.6

   A middle box may interpret some flags and optional fields of the GUE
   header for classification purposes, but is not required to understand
   all flags and fields in GUE packets. 

I think you mean s/all/any of the/

---

5.6.1 has

   The source port set in the UDP
   header must be the destination port the peer would set for replies.

But 3.1 has

      o Source port (inner flow identifier): This should be set to a
        value that represents the encapsulated flow. The properties of
        the inner flow identifier are described below.

      o Destination port: The GUE assigned port number, 6080.

You can't achieve both and it would seem that the only way GUE can be 
"symmetrical" is to use the same source port in both directions.

---

The text in 5.6.2
   This method
   is problematic since ports numbers do not have global meaning
   ([RFC7605]) and a packet which is not GUE but destined to the same
   port number could be misinterpreted.
...sent me scurrying to 7605. I think the point is not that the port
   number does not have global meaning, but that "It is important to
   recognize that any interpretation of port numbers -- except at the
   endpoints -- may be incorrect, because port numbers are meaningful
   only at the endpoints," and "Ultimately, port numbers indicate 
   services only to the endpoints, and any intermediate device that
   assigns meaning to a value can be incorrect."

Maybe similar enough, but I think that the intent of 7605 is to say 
that a service may be run over many different port numbers so you can't
guarantee to find all instances of the service by looking for the port
number. I don't think the intent of 7605 is to say that something in the
network seen using port 6080 might not be GUE.

However, if you proceed with this you'll need to:
- resurrect draft-herbert-udp-magic-numbers
- make it  normative reference
- explain where the GUE magic number comes from

I think you would do well to reduce 5.6.2 to just an observation on
middlebox behavior, and remove all reference to draft-herbert-udp-magic-
numbers.

---

I suspect that discussing NAT as you do in 5.7 will not make you very
popular. It is true that NAT exists, and it is worth observing what 
would happen if a GUE packet went through a NAT. But I am not sure that
this is a problem to be solved in this document.

Indeed, since you don't actually solve it but only make suggestions 
about how it might be solved, I suggest reducing the text and saying 
that another document could be written in the future to describe NAT-
traversal for GUE packets.

BTW, where you say...
   connection semantics must be applied to a
   GUE tunnel as described above
... I think you are probably referring to section 5.6.2. You should be
explicit if that is the case, but consider my comments about 5.6.2.

---

5.8.2 has

    The GUE header checksum (in version 0x0) provides a UDP-lite
    [RFC3828] type of checksum capability as an optional field of the
    GUE header.

This is confusing! At first read we might assume you mean the checksum
field in the GUE header as shown in 3.1, but I think you are actually
calling that the UDP checksum. Reading between the lines, you are 
describing an optional field called the GUE Checksum that could be
included in the GUE header (if the corresponding flag is set). You need
to:

- fix the broken reference ([GUECSUM] or [REMCSUM]?)
- make the reference normative
- consider simply moving the description to this document.

---

5.9
Pay attention to the current discussion on the softwire and nvo3 lists.

---

6.2
      o An encapsulator may occasionally change the inner flow
        identifier used for an inner flow per its discretion (for
        security, route selection, etc). Changing the value should
        happen no more than once every thirty seconds.

I assume the limitation is because statistical load balancing will not
work if there is too much variance in hashable fields. However, 30 
seconds may be a very large number of packets. Is there any science
behind that value?

---

6.2
      o Decapsulators, or any networking devices, should not attempt any
        interpretation of the inner flow identifier, nor should they
        attempt to reproduce any hash calculation. They may use the
        value to match further receive packets for steering decisions,
        but cannot assume that the hash uniquely or permanently
        identifies a flow.

I agree with "should not attempt". But then you give an example of 
applying (limited) interpretation.

If the source port can change (even only once every 30 seconds) then 
what does it mean to "match further packets"? After all, between 29.9
and 30.1 seconds is only a short window, but during such time, any
such matching for steering would be invalid.

---

Hooray for section 7!
Could you please point to it from the Introduction because it will 
significantly help the reader.

Probably you should add 7510 to the long list of references.

---

Section 7 says
      o GUE permits encapsulation of arbitrary IP protocols, which
        includes layer 2 3, and 4 protocols. This potentially allows
        nearly all traffic within a data center to be normalized to be
        either TCP or UDP on the wire.
How so "normalized to TCP"? That doesn't seem to be mentioned anywhere
in this document, and so either needs a reference or to be dropped.

---

I'm not convinced that punting security to a separate document is the
best idea, and it will make progressing this document hard unless you
can get that other document adopted and well advanced.

---


Could you please get IANA to update the reference for port 6080 to
point to this document. And could you please update section 9 so that
when this document is published as an RFC IANA will update the registry
to point to the RFC.
                                                           
---

The IANA section will need some more work

   IANA is requested to create a "GUE flag-fields" registry to allocate
   flags and optional fields for the primary GUE header flags and
   extension flags. This shall be a registry of bit assignments for
   flags, length of optional fields for corresponding flags, and
   descriptive strings. There are sixteen bits for primary GUE header
   flags (bit number 0-15) where bit 15 is reserved as the extension
   flag in this document. There are thirty-two bits for extension flags.

I think you need to separate out the new registries rather than try to
put all of the stuff into one registry which wold then have an 
ambiguous name.

You should also;
- set out what you want IANA to track in tabular form so that they can
  reproduce it in the registry without any confusion
- pre-populate the registries with any values you have defined (such as
  the E flag)
- describe the allocation policies for each registry

---

I think a good number of your references are normative. These include...
[GUESEC]
[UDPMAG]
[REMCSUM] or [GUECSUM] if it exists

---

It is always helpful to state at the top of an Appendix "This appendix
is informational and does not constitute a normative part of this
document."

Yeah! I'm always grumpy when I'm asked to do that, but it does actually
help get it past the IESG, and it might even help the reader.