# Early YANGDOCTORS review of draft-ietf-lsr-ospf-flex-algo-yang

This document includes two IANA-maintained modules - "iana-igp-algo-types" and 
"iana-igp-metric-types". Further, it has two YANG modules for OSPF Application-Specific Link Attributes "ietf-ospf-link-attr" and Flex Algo "ietf-ospf-flex-algo".

## YANG

- Please run "pyang --ietf --lint -f yang --keep-comments --yang-line-length
69" to fix the formatting in the YANG modules. 

### iana-igp-algo-types and iana-igp-metric-types

- Please update the date

- In the description clause, please add
```
     All revisions of IETF and IANA published modules can be found
     at the YANG Parameters registry group
     (https://www.iana.org/assignments/yang-parameters).
```
- Please make this change
```
OLD:
     The latest version of this YANG module is available at
     <mailto:iana@iana.org>";
NEW:
     The latest version of this YANG module is available at
     https://www.iana.org/assignments/yang-parameters.";
END
```
- Remove the editor note about IANA_FOO_URL

- Please add a reference statement for various identities in iana-igp-metric-types. The description for some of them also includes RFC in text; would it not make sense to include those in the reference statement as well? 

### ietf-ospf-link-attr

- Remove BCP14 boilerplate as keywords are not used in the module.

- Please remove 'reference "RFC XXXX";' after description. 

- Please update the RFC title in the revision clause. 

- Please add a reference statement to identities. 

- Please add reference to grouping app-specific-link-attr-sub-tlvs

-    

### ietf-ospf-flex-algo

- Please add a reference statement for imports.

- Remove BCP14 boilerplate as keywords are not used in the module. 

- Please remove 'reference "RFC XXXX: YANG Data Model for OSPF Flexible Algorithm.";' after description. 

- Please update the RFC title in the revision clause. 

- Please add a reference statement to identities. 

- It is unclear why groupings are created for fa-ex-ag-sub-tlv, fa-in-any-ag-sub-tlv, fa-in-all-ag-sub-tlv, fad-flags-sub-tlv, fa-ex-srlg-sub-tlv, when they are only used once and can easily be defined inline in the grouping fad-tlvs. Please also add a reference statement in groupings. 

- For leaf flex-algo, consider adding "{ range "128..255" }". I note that you have it for leaf algo-number but not for flex-algo, any reason for it? 

- For extended-admin-groups, why unit64? RFC 7308 says EAG is a multiple of 4 bytes. Should it not be 32?

- In configuration for metric-type leaf, should it not have a default statement for IGP metric?  

- Why does OSPFv2 FAAM modeling differs from OSPFv3 wrt unknown-tlvs? If there is a good reason, perhaps describe it in text in the draft. 

## Draft

- Please add a brief paragraph that explains the tree in section 4. This can be done by listing the three places in OSPF YANG where the augmentation is being done and how to connect back to the ASLA feature. 

- Same for section 5.  

- It would be good to explain some design choices, such as why container prefix-metric with presence, use of unknown-tlvs in the model when ASLA subtlv can be known, the idea of key-less list to allow multiple instances, etc.  

- I was wondering why the IANA IGP modules that are common for both OSPF and ISIS are in an OSPF document. Perhaps state that explicitly to avoid confusion. 

- Please follow the security consideration template in https://www.ietf.org/archive/id/draft-ietf-netmod-rfc8407bis-28.html#section-3.7.1 

- Please update sections 7.2 and 7.3 to match the template at https://www.ietf.org/archive/id/draft-ietf-netmod-rfc8407bis-28.html#section-4.30.3.1; there are slight inconsistencies. 


## Nits

- s/which itself augments [RFC8349]/which itself augments the IETF Routing model [RFC8349]/

Thanks,
Dhruv