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.