Hi Benjamin, Thanks for your comments. The following edits address most of them:
https://github.com/paololucente/draft-ietf-grow-bmp-adj-rib-out/commit/2134855587c0b98f24533aa66d76e05f3fa6ed29#diff-42c33450ab1d1f6d1ec35e9a12c7bf24 My comments inline: On 9 Jul 2019, at 00:40, Benjamin Kaduk via Datatracker <[email protected]<mailto:[email protected]>> wrote: Benjamin Kaduk has entered the following ballot position for draft-ietf-grow-bmp-adj-rib-out-06: 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/iesg/statement/discuss-criteria.html for more information about IESG DISCUSS and COMMENT positions. The document, along with other ballot positions, can be found here: https://datatracker.ietf.org/doc/draft-ietf-grow-bmp-adj-rib-out/ ---------------------------------------------------------------------- DISCUSS: ---------------------------------------------------------------------- The "Peer Up message Information" TLV type seems under-specified and under-motivated. (It is not mentioned in Abstract or Introduction.) Why does it need to be defined in this document, and what role is it expected to play? Who is the expected audience for it? Is it limited to the "group name"-like functionality described in Section 7.1? Why is cleartext appropriate, and are there any potential privacy considerations for any potential use cases? I’d +1 the reply from Jeff on this. ---------------------------------------------------------------------- COMMENT: ---------------------------------------------------------------------- Section 1 An example of pre-policy verses post-policy is when an inbound policy applies attribute modification or filters. Pre-policy would contain information prior to the inbound policy changes or filters of data. Post policy would convey the changed data or would not contain the filtered data. Can applying policy ever act as injecting new data? Yes. Jeff Haas proposed the edit below to draft-ietf-grow-bmp-loc-rib-04 — would the same/a similar edit address your point? https://github.com/paololucente/draft-ietf-grow-bmp-loc-rib/blob/master/draft-ietf-grow-bmp-local-rib.txt#L682-L684 Separately, I'd ask whether it's worth mentioning the new statistics types in the Introduction (skipping them from the Abstract is probably understandable). My feeling is that mentioning new statistics tyeps is too much detail for the introduction as well. While they integral part of the draft, they are not part of the core concept. Section 4 o Peer Address: The remote IP address associated with the TCP session over which the encapsulated PDU is sent. o Peer AS: The Autonomous System number of the peer from which the encapsulated PDU was sent. o Peer BGP ID: The BGP Identifier of the peer from which the encapsulated PDU was sent. I am not sure whether I'm reading these properly. Backing up, in regular RFC 7854 BMP, we have a BMP sender (router) that is speaking BGP to (presumably many) peers. These Peer Address/AS/BGP-ID fields are attributes of the BGP connections the router has to its peers, and are talking about how the data is coming in. For the Adj-RIB-Out version that we're defining in this document, we want to flip the sense, so that we are talking about what the router is sending on its outgoing BGP connections. In this way, the content being sent over BMP still reflects the origin of the BGP data, which makes sense. What's tripping me up is that we still talk about "the peer from which the encapsulated PDU was sent" -- IIUC this is "peer" in the "BGP peering" sense, so the data being sent over BMP is the local data from the router. Do we need to say "peer" at all, here, then? So, "The AS number for which the encapsulated PDU was sent", and "The BGP Identifier for which the encapsulated PDU was sent”? You are right. I catched this before in fact you see the “Peer Address” description actually makes sense; we forgot to edit accordingly the other two, Peer AS and Peer BGP ID. I did the edit now. o Timestamp: The time when the encapsulated routes were advertised (one may also think of this as the time when they were installed in the Adj-RIB-Out), expressed in seconds and microseconds since midnight (zero hour), January 1, 1970 (UTC). If zero, the time is unavailable. Precision of the timestamp is implementation- dependent. Are leap seconds included in this value? (Yes, I see that this language is basically taken directly from RFC 7854, which also left it underspecified.) I would propose to leave it underspecified as well, leaving it implementation specific. Section 5.1 Some attributes are set when the BGP message is transmitted, such as next- hop. Adj-RIB-Out Post-Policy MUST convey what is actually transmitted to the peer, next-hop and any attributes set during transmission should also be set and transmitted to the BMP receiver. I'm having a bit of trouble matching up "MUST convey" with "should also be set". (Also, we seem to say "MUST be what is actually sent to the peer" in Section 3; do we need the normative language in both places?) nit: this is a comma splice Ack, i removed the “should also be set” part. I have also normalised language among the two places. I feel it is good to stress the concept in the two places, despite the text repetition. Section 5.2 Some attributes are set only during transmission of the BGP message, i.e., Post-Policy. It is common that next-hop may be null, loopback, or similar during this phase. All mandatory attributes, such as nit: I suggest clarifying that "this phase" is pre-policy, not "during transmission”. Ack, fixed. Section 6.2 Do we need to say anything about the byte order of the 2- and 8-byte fields in these statistics structures? My take would be that it’s implied that bytes are transmitted as network byte order / big endian. rfc7854 does not specify this either. Nevertheless if it is felt a key detail, i can certainly add a remark about it. Section 6.3 BMP receiver implementations SHOULD ignore the O flag in Peer Up and Down notifications. BMP receiver implementations MUST use the per-peer header O flag in route monitoring and mirroring messages to identify if the message is for Adj-RIB-In or Adj-RIB-Out. Is this last MUST duplicating content elsewhere in the document? It doesn't seem related to the title of the section, "peer down and up notifications”. Agree, removed. Section 7.1 Do I interpret this correctly as saying that peer and update groups are not a defined protocol feature but rather something offered by implementations for convenience of administrators? The language about "should be simple to include a group name [...] but it is more complex than that" leaves me a little confuesd about what is current deployed reality, what is speculation about potential future work, and what is being defined as an actual new protocol feature. I’d +1 the reply from Jeff on this. Section 8 The administrative information TLV has some considerations about what kind of internal organizational information is shared to "the world". As Alvaro notes, publishing both pre- and post-policy outbound RIBs can give a new information channel into what the policy applied to outbound routes is, and presumably the usual BMP configuration considerations apply about only sending information to people authorized to receive it. Ack, edited. Please see my reply to Alvaro. Section 9.3 The registry where this seems to be listed claims to be the "BMP Initiation Message TLVs" registry; is the section heading of "Peer Up Information TLV" appropriate? There is a draft undergoing to split Initiation Message and Peer Up message TLV registries, see https://tools.ietf.org/html/draft-scudder-grow-bmp-peer-up-00 , which makes a lot of sense and is being adopted by the WG. I look for suggestions on what would be the nicest way to address this. Paolo
_______________________________________________ GROW mailing list [email protected] https://www.ietf.org/mailman/listinfo/grow
