Reviewer: Ebben Aries
Review result: Not Ready
1 module in this draft:
- ietf-sip-auto-peering@xxxxxxxxxxxxxxx
YANG compiler errors or warnings (pyang 2.6.1, yanglint 3.7.8)
# pyang
--------------------
yang/ietf-sip-auto-peering@xxxxxxxxxxxxxxx:50: warning: RFC 8407: 3.1: The IETF
Trust Copyright statement seems to be missing or is not correct (see pyang
--ietf-help for details). yang/ietf-sip-auto-peering@xxxxxxxxxxxxxxx:144:
error: unexpected keyword "mandatory"
yang/ietf-sip-auto-peering@xxxxxxxxxxxxxxx:151: error: keyword "uses" not in
canonical order, expected "type" (see RFC 6020, Section 12)
yang/ietf-sip-auto-peering@xxxxxxxxxxxxxxx:151: error: unexpected keyword
"uses" yang/ietf-sip-auto-peering@xxxxxxxxxxxxxxx:152: error: keyword
"max-elements" not in canonical order, expected "type" (see RFC 6020, Section
12) yang/ietf-sip-auto-peering@xxxxxxxxxxxxxxx:200: error: keyword "uses" not
in canonical order, expected "type" (see RFC 6020, Section 12)
yang/ietf-sip-auto-peering@xxxxxxxxxxxxxxx:200: error: unexpected keyword
"uses" yang/ietf-sip-auto-peering@xxxxxxxxxxxxxxx:201: error: keyword
"max-elements" not in canonical order, expected "type" (see RFC 6020, Section
12) yang/ietf-sip-auto-peering@xxxxxxxxxxxxxxx:236: error: keyword "uses" not
in canonical order, expected "type" (see RFC 6020, Section 12)
yang/ietf-sip-auto-peering@xxxxxxxxxxxxxxx:236: error: unexpected keyword
"uses" yang/ietf-sip-auto-peering@xxxxxxxxxxxxxxx:245: error: keyword
"description" not in canonical order, expected "type" (see RFC 6020, Section 12)
# yanglint
--------------------
libyang err : Invalid keyword "mandatory" as a child of "llist". (line 144)
libyang err : Parsing module "ietf-sip-auto-peering" failed.
Overall Comments
--------------------
My review is solely based off the YANG model and the included instance-data
and I had started the review off -18 but later switched to -23. The comments
below are mostly against -18 since version -23 has so many fundamental issues
that I would not consider ready for YANG Doctor review. Please ensure your
model can compile and instance-data can validate prior to further submissions.
Disclaimer: With that said, I have not fully reviewed if prior -18 items
below have been addressed in version -23
YANG Module
--------------------
- Missing `yang-version "1.1"; statement. See other published modules as well
as RFC8407
- Missing reference statement for import of ietf-inet-types
- Revision statement description and reference should be set accordingly
- Are the typedefs declaring IP address + port best served in a common model
vs. local definition?
- For the enum variants defined under the 'transport' leaf, suggest using an
alternate delimeter vs. ';'. '-' or '_' are both valid options
- In addition for the enum variants, looking across published models to date,
suggest lower case with '-' delimter for consistency
- Node naming: casing is inconsistent and not inline with many published
models to date. Suggest replacing camelcasing w/ snake-case throughout
- __REF1__: There are many special string encodings used throughout. Does
this have some significance to be modeled and represented this way as this
ultimately creates a bad API contract at the end of the day. Can these
encoded values be split into their own distinct strongly typed nodes?
- Regarding the overall tree structure, I would recommend
- A more descriptive top-level node vs. `peering-info`
- Spell out full words as node names
- Do not place lists at the top-level
- Is there any reason `/peering-info[index]` is an int32 vs. uint32?
- How is a consumer/implementor of the YANG module supposed to know how the
`/peering-info[index]/variant` leaf is encoded? Where will this be
described?
- Node: `/peering-info[index]/revision`
- notBefore: This is a plain string. Suggest using a proper time type
- location: Your instance data indicates this is a URL. Suggest narrowing
this in to a more distinct type.
- Node: `/peering-info[index]/transport-info/transport`
- See: __REF1__ above
- Rather than creating special encodings of enum variants, suggest making
this a leaf-list of type enumeration (or identityref if you forsee wider
expansion)
- Node: `/peering-info[index]/transport-info/realms[name]
- The instance-data provides an encoded host/domain-name. Is this always
the case for a realm name or can the string be arbitrary?
- Node: `/peering-info[index]/transport-info/realms[name]/password
- What are all the encodings/hash algorithms in play here and what are the
implications if one were to access this data? Is this hash used as-is
in the wire protocol for SIP peering? (Does it matter if one does not
have the clear-text variant?)
- Do the max-elements stmts for the `callControl` and `dns` leaf-lists have
a corresponding protocol limitation?
- Node: `/peering-info[index]/call-specs/supportedMethods`
- See: __REF1__ above
- Same comment as the `transport` leaf above. There is no description of
this string encoding and should be better served by distinct
identifiable values (enums or identities) it seems. Is this how the
wire protocol encodes such?
- Node: `/peering-info[index]/call-specs/callerId/preferredMethod`
- Same as above. What are all the possible values here? Can they be
encoded in an enum or identity?
- Node: `/peering-info[index]/call-specs/numRanges[index]`
- Can there be negative indices? If not, use unsigned
- Node: `/peering-info[index]/call-specs/numRanges[index]/type`
- Similar comment to previous. What are all the possible values?
(enum/identity)
- Node: `/peering-info[index]/call-specs/numRanges[index]/count`
- You are restricting via special strings in the when statement. How does
one know these? Adjust the `type` leaf to enum/identity and use
well-known values to restrict against
- This is a signed integer. Can the value rise or decrease? Should this
be of yang:gauge or yang:counter type rather?
- Node: `/peering-info[index]/call-specs/numRanges[index]/value`
- This appears to be a list of integer values but is a string?
- Is this always a number? Can this be of a more appropriate type?
- Node: `/peering-info[index]/media/mediaTypeAudio/mediaFormat`
- See: __REF1__ above
- Same comment as above on loose encoding. Can this be broken apart into
distinct values w/ a tighter data types?
- Node: `/peering-info[index]/dtmf/payloadNumber`
- Signed w/ a restriction on unsigned only - is there a reason behind
this? What does this number signal exactly?
- Node: `/peering-info[index]/security/signaling/version`
- See: __REF1__ above
- Same comment as above. Do not create special encodings like this. Make
this a leaf-list from known values (enum/identity)
- General: If you have to reference other RFCs in any descriptions, suggest
using a proper `reference` statement (See other published YANG models for
examples)
Instance Data
--------------------
- asap-example.json has a missing "index" list key in the instance data
--
last-call mailing list -- last-call@xxxxxxxx
To unsubscribe send an email to last-call-leave@xxxxxxxx
