Application-Specific Link Attributes Advertisement Using the Border Gateway Protocol - Link State (BGP-LS)
draft-ietf-idr-bgp-ls-app-specific-attr-16

Thanks for resolving my secdir comments.

Thank you to Paul Wouters for the SECDIR review.

** Section 2.  There is no text describing the Reserved bits.  Perhaps:

NEW
Reserved: This field is reserved.  It MUST be set to zero on transmission and MUST be ignored on receipt.

** Section 8.
It is assumed that the IGP instances
   originating these TLVs will support all the required security (as
   described in [RFC8919] and [RFC8920]) to prevent any security issues
   when propagating the TLVs into BGP-LS.

I concur with John Scudder’s analysis of this text and where it could use improvement.  He eloquently described it already so I won’t repeat it here.

Thank you for the work put into this document.

Special thanks to Keyur Patel for the shepherd's write-up including the WG consensus but I would have appreciated to see more about the intended status though.

I've chosen to put these in as comments rather than discuss positions for now, since I don't necessarily think these comments should be blocking in nature, just there for due consideration.

In Section 2 second paragraph you have:

The format of this TLV is as follows and is similar to the
   corresponding ASLA sub-TLVs defined for OSPF and IS-IS in [RFC8920]
   and [RFC8919] respectively.

This gets a little confusing where further down only one of these RFC's is referenced as follows:

 o  SABM Length : 1 octet field carrying the Standard Application
      Identifier Bit Mask Length in octets as defined in [RFC8920].
  o  UDABM Length : 1 octet field carrying the User-Defined Application
      Identifier Bit Mask Length in octets as defined in [RFC8920].
 o  User-Defined Application Identifier Bit Mask : An optional set of
      bits (of size 0, 4, or 8 octets as indicated by the UDABM Length),
      where each bit represents a single user-defined application as
      defined in [RFC8919].

Various instances of this - Would the authors consider referencing both RFC's at each point so that readers are aware that the same thing applies across both ISIS and OSPF.

Thank you for the work on this document.

Many thanks to Kirsty Paine for her ART ART review: https://mailarchive.ietf.org/arch/msg/art/Gp7TtdCUrbmf6WJy4ATcmN_DYQI/, and thanks to the authors for addressing her comment.

Francesca

Thanks for all the work to help clear up my DISCUSS! Version 14 looks good, modulo some questions about §4.1 I've sent in a separate reply (but which are only COMMENT level).

--

Previous DISCUSS:

I'd like to DISCUSS some questions I have about the content of Section 4's IS-IS rules. I don't expect it will be difficult to resolve these, I just want to be sure we have the conversation.

1. In §4 (2.C),

       C.  The SRLGs advertised in IS-IS SRLG ASLA TLVs and the other
           link attributes advertised in IS-IS ASLA sub-TLVs are
           REQUIRED to be collated, on a per-application basis, for all
           applications that have their bit set in the SABM/UDABM in at
           least one of the aforementioned TLV types.
           
Is there some reason that there's no need to consider the case where both of the TLV types in question have zero-length application bit masks?

2. Later in the same paragraph,

                                                       When performing
           this collation, only the TLVs with the application's bit set
           in SABM/UDABM MUST be used when such TLVs are available from
           either TLV types.

I suspect you aren't saying what you mean, and the "only... MUST" construct is hard to puzzle out. Do you perhaps mean TLVs that don’t have the application's bit set MUST NOT be used? Because as you've written it, 

- All TLVs with the bit set MUST be collated,
- Any TLVs without the bit set MAY be collated (by implication). Which doesn't seem very sensible AFAICT.

A possible rewrite, if I've understood your intention correctly, might be "When performing this collation, every TLV with the application's bit set in SABM/UDABM MUST be included, when such TLVs are available from either TLV type."

(Note also a minor fix for agreement in number, s/types/type/ in the last word.)

3. And still later in the same paragraph,

                              If the bit for an application is set in
           the SABM/UDABM of only one of the TLV types, then the
           attributes from the other TLV type with zero-length
           application bit mask MUST be also collated for that
           application, if such TLV is available.

I'm confused by the reference to a zero-length application bit mask. Can't the other TLV type have a nonzero-length application bit mask, but still have the application bit in question not set within the bit mask?

I guess probably what you mean is, if the other TLV type is present and has a zero-length application bit mask, since a zero-length SABM/UDABM basically is a wildcard, the equivalent of all bits being set (as I read RFC 8920). 

If that's right, then a change seems in order, along the lines of

                              If the bit for an application is set in
           the SABM/UDABM of only one of the TLV types, and if the 
           other TLV type has a zero-length application bit mask, then the
           attributes from the other TLV type with zero-length
           application bit mask MUST be also collated for that
           application, if such TLV is available.

I just interpolated the "and" clause. It's a bit wordy and could probably be further condensed, but I think it clarifies sufficiently.

On the other hand, if I'm not right about this (very possible) then I'd appreciate further discussion on what I got wrong, so we can figure out if the text needs clarification or if I just need a clue bat.

Previous COMMENT:

4. In §2 you reference "SABML". I was going to ask you to expand it on first use, but you only use it once. So, please just write it out in words as "SABM Length" and don't introduce an extra acronym.

5. In §3:

   All the BGP-LS Attribute TLVs defined in the table above are REQUIRED
   to be advertised as a top-level TLV in the BGP-LS Attribute for
   carrying link attributes specific to RSVP-TE.

I suggest “... when used to carry” instead of “... for carrying”. Also, “listed in the table”, not defined in it. 

6. Much of §4 is dedicated to a special case for IS-IS:

   2.  In the case of IS-IS, the following specific procedures are to be
       followed:

It would be nice to spend a few words mentioning why IS-IS needs a special case and OSPF doesn't, so that the reader doesn't have to wonder. I'm guessing that this relates to the way IS-IS uses sub-TLVs for various things whereas OSPF doesn't, so you need rules to reconcile the values in IS-IS but not OSPF?

7. §4 (2.B) needs some adjectives, "IS-IS" and "BGP-LS", respectively, to make clear which flavor of "ASLA sub-TLV" you're talking about. I *think* that here,

       B.  When the ASLA sub-TLV has the RSVP-TE application bit set,
           then the link attributes for the corresponding ASLA sub-TLV
           MUST be encoded using the respective BGP-LS top-level TLVs as
           per [RFC7752] [RFC8571] [RFC9104].
           
the first reference to "ASLA sub-TLV" is the IS-IS flavor, and the second is to the BGP-LS flavor --  but please be specific.

8. In §4, 

   These rules ensure that a BGP-LS originator performs the
   advertisement for all application-specific link attributes from the
   IGP nodes that support or do not support the ASLA extension.

I don't understand this sentence. :-( For one thing, applying simple logic to "IGP nodes that support or do not support the ASLA extension" we can rewrite it as "IGP nodes", full stop, since "support or do not support" covers the entire universe by definition. That would imply the sentence could be rewritten as

   These rules ensure that a BGP-LS originator performs the
   advertisement for all application-specific link attributes from the
   IGP nodes.
   
Which doesn't really help me. :-(

9. In §5,

   It is recommended that nodes supporting this specification are
   selected as originators of BGP-LS information when advertising the
   link-state information from the IGPs.

Do you mean only such nodes should be selected? Or is it that at least one such should be among those selected? Either is supportable, I think. Whatever the answer, please clarify the text correspondingly. Although, more generally I guess BGP-LS originators ought to support all the extensions in use in a given IGP, not limited to this one, so this is really a special case of a general principle. 

10. In §5,

                                     They would, however, not be able to
   support neither the application-specific link attributes nor newer
   applications that may be encoded only using the ASLA TLV.
   
Drop the "not". Also, again this seems like a special case of a general principle -- of course a BGP-LS consumer that doesn't understand a given extension can't do the thing that the extension covers. But, it's fine to remind the reader of this.

11. In §8,

                             It is assumed that the IGP instances
   originating these TLVs will support all the required security (as
   described in [RFC8919] and [RFC8920]) to prevent any security issues
   when propagating the TLVs into BGP-LS.
   
I think purporting to "prevent any security issues" is a case of seriously overpromising. A very similar issue existed in draft-ietf-idr-bgp-ls-sbfd-extensions-08. Perhaps a similar fix could be applied, although in this case, when I took a quick look at RFCs 8919 and 8920 I didn't find any "required security" of note, so I'm really not sure what if anything is being actually promised by this sentence? Can you help me understand what properties I should be expecting?

In any case, for the most part it seems fine to say your security model is the same as that of RFCs 8919 and 8920, since as we know BGP-LS tries to be just a re-rendering of the data from those suppliers. But, we should keep in mind this consideration from RFC 7752:

   Additionally, it may be considered that the export of link-state and
   TE information as described in this document constitutes a risk to
   confidentiality of mission-critical or commercially sensitive
   information about the network.  

Might it be worth reminding the reader that if there's any sensitive information contained in the new stuff you're propagating, they should take this warning to heart, since the scope across which the information is being propagated can increase considerably? I don't insist on this, I'm only raising it as a point to consider.


Nits and very minor comments --

12. The document talks about new this, new that. This probably won't age well -- keep in mind that the RFC this document will become will be around for decades, at which point none of this stuff will be "new". 

It's obviously not a big deal but if it were my document I'd find a way to rewrite those "new" in a way that won't be instantly obsolete.

13. In §4,

                                 The application-specific attribute
   value received via sub-TLVs within the ASLA TLV have preference over
   the value received via the top-level TLVs.
   
There's an agreement in number problem here -- perhaps rewrite as "An application-specific attribute value received via a sub-TLV within the ASLA TLV has precedence over the value received via a top-level TLV"?

# GEN AD review of draft-ietf-idr-bgp-ls-app-specific-attr-11

CC @larseggert

Thanks to Russ Housley for the General Area Review Team (Gen-ART) review
(https://mailarchive.ietf.org/arch/msg/gen-art/T9RNz1ar99diDYpCMY75m1asL2w).

## Comments

### Section 2, paragraph 5
```
     where:
```
It might be helpful to state the length of the (fixed-length) fields in
the bullet list below, so readers don't have to count bits in Figure 1.

### Inclusive language

Found terminology that should be reviewed for inclusivity; see
https://www.rfc-editor.org/part2/#inclusive_language for background and more
guidance:

 * Term `traditional`; alternatives might be `classic`, `classical`, `common`,
   `conventional`, `customary`, `fixed`, `habitual`, `historic`,
   `long-established`, `popular`, `prescribed`, `regular`, `rooted`,
   `time-honored`, `universal`, `widely used`, `widespread`

## Nits

All comments below are about very minor potential issues that you may choose to
address in some way - or ignore - as you see fit. Some were flagged by
automated tools (via https://github.com/larseggert/ietf-reviewtool), so there
will likely be some false positives. There is no need to let me know what you
did with these suggestions.

### Boilerplate

Document still refers to the "Simplified BSD License", which was corrected in
the TLP on September 21, 2021. It should instead refer to the "Revised BSD
License".

### Grammar/style

#### Section 2, paragraph 14
```
rlying IGPs are also encoded in a similar manner in BGP-LS. The following ta
                             ^^^^^^^^^^^^^^^^^^^
```
Consider replacing this phrase with the adverb "similarly" to avoid wordiness.

## 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. Review generated by the [`ietf-reviewtool`][IRT].

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

Thanks to Kirsty Paine for the ARTART review.

Hi,

Thanks for the manageability considerations section.

Only one minor nit in section 5.  Deployment Considerations

   They would, however, not be able to
   support neither the application-specific link attributes nor newer
   applications that may be encoded only using the ASLA TLV.

I think that the "not" here is incorrect and should be deleted.

Rob

Huge thanks to Scott Bradner for his OpsDir review of this document ( https://datatracker.ietf.org/doc/review-ietf-idr-bgp-ls-app-specific-attr-04-opsdir-early-bradner-2021-02-16/ ).
Due to a confluence of events, I was not able to do my regular level of review, and so, other than a quick skim of the document, I'm balloting based almost entirely on his review.