Thank you Dhruv for your thorough review.
We greatly appreciate it.
We will shortly submit a new version (v5) that addresses comments from you and from other reviewers.
Please see our comments inline.
Cheers,
Sanjay and Nir
On Sun, Dec 4, 2022 at 8:51 AM Dhruv Dhody via Datatracker <noreply@xxxxxxxx> wrote:Reviewer: Dhruv Dhody
Review result: Has Issues
# OPSDir review of draft-ietf-cdni-additional-footprint-types-04
Reviewer: Dhruv Dhody
Review Result: Minor issues
I have reviewed this document as part of the Operational directorate's ongoing
effort to review all IETF documents being processed by the IESG. These
comments were written with the intent of improving the operational aspects of
the IETF drafts. Comments that are not addressed in Telechat may be included in
AD reviews during the IESG review. Document editors and WG chairs should treat
these comments just like any other Telechat review comments.
Also at https://notes.ietf.org/draft-ietf-cdni-additional-footprint-types
## Summary
This document defines two new CDNI Footprint Types (one for ISO3166-2
Subdivision Codes and one for union footprints). The document is easy to read
and clear. The use of union could have some additional operational complexity.
## Major
- None
## Minor
- Abstract should explicitly state that it is updating RFC 8006 and 8008.[N/S] We have updated the draft to be only referencing 8006, but updating 8008.
- I think the I-D can use BCP 14 keywords (e.g. MAY), request authors to check.
I see the use of "required" and "may" but in lowercase![N/S] We replaced the relevant "may" with a "MUST".
The "required" was not in a "definitive" context of the RFC, it was more referring to best practice. Therefore we replaced it with "expected"
- Section 3.1, while defining new footprint types in ALTO, RFC 9241 also
explicitly states "Hierarchy and Inheritance" which this I-D skips. Just to be
consistent, please add it and state that "There is no hierarchy or inheritance
for properties associated with subdivision codes."[N/S] Done
- Section 4.1, Looking at the IANA registry
https://www.iana.org/assignments/cdni-parameters/cdni-parameters.xhtml#metadata-footprint-types
, I am not sure why FCI prefix ("FCI.xxx") have been added in this document.
- Also the description for subdivisioncode could be aligned to how the
countrycode is defined "ISO 3166-1 alpha-2 code".[N/S] We had changed the description to:ISO 3166-2 country subdivision code: alpha-2 country code, followed by a hyphen-minus, and up to 3 characters from A-Z;0-9 as a code within the country.- Should there be an explicit statement that these are optional and not
mandatory?[N/S] Can you please clarify what section this comment refers to?
- Are there any constraints to union that one needs to state? The use of the
same type? Presence of multiple unions? Conflicts? Any operational issues? I
could not think of any explicit one but wanted to highlight if others can.
Perhaps adding some text on this could be beneficial.[N/S] The only limitation we have is that a union cannot include another "union" element.See section 2.2.1
## Nits
- Fix the english "below is introduced the" in````
To overcome the described limitation and allow a list of footprint
constraints that match both IPv4 and IPv6 client addresses, below is
introduced the "footprintunion" footprint type.
````[N/S] Fixed- In table 1, change "Specification" to "Reference" to align with the IANA[N/S] Doneregistry. In table 2 also add a "Reference" column at the end.[N/S]: This table is consistent with the one in section 7.4 of RFC 9241
Thanks!
Dhruv
-- last-call mailing list last-call@xxxxxxxx https://www.ietf.org/mailman/listinfo/last-call
