Hi Paolo, On Tue, Jul 16, 2019 at 04:19:43PM +0000, Paolo Lucente wrote: > > 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:
Thanks. The quoting seems a bit odd, but hopefully we can figure it out... > 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. While Jeff's reply does help me personally to understand the motivation, I'm left wondering what role I expect the final RFC itself to play in this matter. I think I'd like to see a brief mention in the Introduction that this TLV is added in order to convey administrator-defined information about the rib-out; is that consistent with your stance on the matter? > ---------------------------------------------------------------------- > 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 I'm not 100% sure I'm looking at the right link -- the highlighted lines are about requiring the Table Name to be sent when multiple filters are applicable. My question was intended to be about the wording here, about whether "post policy would convey" should mention the possibility of new, injected, data (in addition to changed or removed data). > 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. Fair enough. > 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. Thanks; it's reassuring to know that the problem was not on my end. > 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. I can understand that decision, even if I'm not fully comfortable actively endorsing it. > 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. Thanks! > 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. I think that it's probably best to just have the implicit "do what you did for rfc7854" and not try to get super-precise here. > 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. If this is well-understood by the target audience, I'm happy to live with my minor confusion. > 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. IIRC that looked fine. > 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. I think the responsible AD can handle this with the IANA interactions during the publication/AUTH48 process. Thanks for letting me know about the other work in progress! -Ben _______________________________________________ GROW mailing list [email protected] https://www.ietf.org/mailman/listinfo/grow
