Hi Ladislav,
Thank you very much for the review. I'll respond to your comments below.
On 21/05/2025 11:46, Ladislav Lhotka via Datatracker wrote:
Document: draft-ietf-grow-yang-bgp-communities
Title: A YANG Data Model for BGP Communities
Reviewer: Ladislav Lhotka
Review result: Ready with Nits
The ietf-bgp-communities module represents an interesting use case where YANG
is used outside of its standard server-client domain for publishing information
about configured BGP communities. YANG essentially plays a role of a (JSON)
schema language.
**** General comments
- Section 6 (Data Elements) contains a hierarchy of subsections, one for each
data node of the schema tree. These subsections (i) repeat information that is
formally specified in the YANG module, and (ii) sometimes add extra
explanations. Unless there is a good reason for having this section in the
document, I'd suggest to remove it. Duplicated information invites troubles if
this section and the YANG module get out of sync, and the extra information, if
it is substantial, should be added to descriptions in the YANG module.
Ok
- Running code seems to exist, which is always good, and the document refers to
two autonomous systems that already use the schema defined in the YANG module
(well, almost, see below). In both cases, only an entire read-only JSON file
can be retrieved from an URL. I think it might be useful to consider a more
granular service, e.g. using a RESTCONF server, and perhaps even a possibility
of remote edits.
This certainly a possibility.
The JSON files published by the listed autonomous systems are meant as
an input for the looking glass implementation in section 10.2. For this
use case, reading and caching a complete file at once is more suitable.
**** Specific comments
- The two examples of BGP community data published by AS197000 and AS25152 are
based on an old revision of the YANG module – words in multi-word identifiers,
such as "email-address" are now separated by a hyphen. Both JSON files also
contain other trivial errors:
* namespace identifiers should be "ietf-bgp-communities" rather than
"draft-ietf-grow-yang-bgp-communities", see RFC 7951
* lists "contacts" and "fields" shoud in fact be "contact" an "field".
These issues were identified during a review of the previous revision,
which triggered the request for the YANG Doctors review.
- At several places, regular expressions are used for specifying the set of
permitted Unicode characters. For example, the definition of
"community-description" typedef contains the following statement:
pattern '[^\p{C}]+';
I am not sure that this matches the intent – are parsers expected to handle
combining and extender characters? If so, a positive definition using the
"\c" character class (XML 1.0 name characters) might be preferable.
Ok
- Section 8.1 instructs operators to order overlapping community definitions
from most to least preferred. Shouldn't then the "regular", "extended" and
"large" lists be defined as "ordered-by user"? Alternatively, a "preference"
leaf could be added to make this explicit.
Good catch!
- In Appendix A, it would be useful to provide an example of extended community
definition.
Ok
I will publish an updated draft shortly.
Kind regards,
Martin
_______________________________________________
GROW mailing list -- [email protected]
To unsubscribe send an email to [email protected]
