Subject: Re: [netmod] I-D Action: draft-ietf-netmod-factory-default-09.txt

From: Kent Watsen <[email-masked]>
Date: Sun, 9 Feb 2020 19:16:15 +0000

[catching up on an old shepherd action item]

Authors,

While processing the shepherd writeup, I discovered some issues that should be fixed before this draft is forwarded to the IESG.  Please submit an update so that I can continue the review.

WG,

How is it that so many issues got through the Last-Call process?!

Comments by Section:

1) The abstract and Introduction both say “When resetting a datastore, …”, but this isn’t the case anymore, I mean, we only reset the device, right? 

2) The Abstract says “…datastore…that contains data that will be copied over to the running datastore at reset.”  This is inconsistent with what is stated elsewhere about the contents going into all conventional datastores.  Recommendation is to instead say that it contains the factory default configuration for the device.

3) The Introduction says "This datastore can also be used in <get-data> operation” - missing the word “the” before <get-data>.

4) The Introduction’s 4th paragraph serves no purpose and asymmetrically calls out NETCONF.  Please delete.

6) Section 1.1 says "A read-only configuration datastore holding a preconfigured minimal initial configuration that can be used to initialize the configuration of a server.”  Remove the word “minimal” and replace “can be” with “is”.

7) Section 1.1 says "The content of the datastore is usually static, but MAY depend on external factors like available HW.”  This is not true.  The contents are static, full stop.  However, the application of the contents may vary based on factors such as HW present.   But since this is always true, I recommend the entire sentence be removed.  

8) Section 2 says "The contents of the <operational> datastore MUST be reset back to an appropriate factory-default state.”  I think words “reset” and "factory-default state” are misleading.  Better would be to say “…MUST reflect the operational state of the device after applying the factory default configuration."

9) Section 2 says “...MUST restore storage to factory condition, including remove log files, remove temporary files (from datastore or elsewhere).”  This is both grammatically incorrect and confusing.  Recommend replacing with “...MUST restore non-volatile storage to factory condition.  Depending on the system, this may entail deleting dynamically generated files, such as those containing keys (e.g., /etc/ssl/private), certificates (e.g., /etc/ssl), and logs (e.g., /var/log), temporary files (/tmp/*).

10) After applying the fix from (9) above, the following two sentences could be replaced with “All security sensitive data (i.e., private keys, passwords, etc.) SHOULD be overwritten with zeros or a pattern before deletion."

11) Section 3: "Following guidelines for defining...” should be "Following the guidelines for defining…"

12) Section 3: replace 'factory-default’ with "factory-default”.  That is, use double-quotes. The only time single-quotes are used is when needing to quote something inside a YANG string.

13) Section 3:apply the same change described in (6) above wrt “minimal” and “can be”.

14) Section 3: remove the word “only” from "A device MAY only implement the <factory-reset> RPC without implementing the 'factory-default’ datastore”, or replace “without” with “, i.e., without”.

15) Section 3: replace "which make it lose the ability to see what configuration the device would be reset back to” with “which would only eliminate the ability to programmatically determine the factory default configuration”.

16) Section 3; replace "RESTCONF,the CLI etcunless” with “RESTCONF, the CLI, etc. unless” 

17) Section 3 needs to also mention that <candidate>, if present, is initialized to the contents of <factory-default>

18) Section 3: the word “usually” in the sentence "The datastore content is usually defined by the device vendor.” Should either be removed or explained how it could ever not be the case.  (I recommend the former)

19) Section 3: apply the same change described in (7) above.

20) both of the ‘import’ statements are missing ‘reference’ statements.”

21) Section 4: the module’s “description” statement is both incomplete and hard to read. How about replacing it with “This YANG module defines an RPC called ‘factory-reset’, a datastore identity called ‘factory-default’, and and identity called 'factory-default-datastore’."

22) Section 4, in the "factory-reset” RPC: replace "(i.e.,<running> , <startup>,and <candidate>)” with  "(i.e., <running> , <startup>, and <candidate>)”.  

23) Section 4, in the description statement for the "factory-default” identity, apply the same change from (2) above.  Also, “of the” should be “of the”.

24) Section 5, why are the registrations triple-spaced?

25) Section 6: replace the 2nd paragraph with “Access to the <factory-reset> RPC operation is considered sensitive in and therefore has been restricted using the 'default-deny-all’ access control defined in [RFC8341].”

Comments Not Bound to Specific Sections:

A) The ‘<factory-default>’ notation should be introduced before first use, which occurs inside a parentheses in Section 2.

B) “factory-default” and “factory default” (without the hyphen) are used inconsistently.  Recommend using the hyphenated form when referring to YANG node, i.e., "the ‘factory-default’ datastore” and "<factory-default>”.

C) “factory-reset” and “factory reset" (without the hyphen) are used inconsistently.  Recommend using the hyphenated form when referring to YANG node, i.e., "the ‘factory-reset’ RPC” and "<factory-reset>”.

D) Replace “HW” with “hardware” (occurs three places in the draft).

Thanks,
Kent // as shepherd

---

From: Qin Wu <[email-masked]>
Date: Mon, 10 Feb 2020 03:44:15 +0000

Thanks Kent for shepherd review comments and will issue new version soon to address these comments.

发件人: Kent Watsen [mailto:[email-masked]]
发送时间: 2020年2月10日 3:16
收件人: draft-ietf-netmod-factory-default <[email-masked]>
抄送: [email-masked]
主题: Re: I-D Action: draft-ietf-netmod-factory-default-09.txt

Authors,

WG,

[Qin]: I believe many of them are wordsmith input, I am happy to accept them, thanks Kent.
Comments by Section:

[Qin]:Yes, will update the draft to reflect this.
2) The Abstract says “…datastore…that contains data that will be copied over to the running datastore at reset.”  This is inconsistent with what is stated elsewhere about the contents going into all conventional datastores.  Recommendation is to instead say that it contains the factory default configuration for the device.

[Qin]:Good suggestion to address inconsistency issue. Thanks.
[Qin]: Okay.
[Qin]: Works for me.
[Qin]:Okay.
[Qin]: Works for me.
[Qin]:Good suggestion.
[Qin]: Make sense, thanks.
[Qin]: Okay.
[Qin]:Okay.
[Qin]:Okay.
[Qin]:Okay.
[Qin]:Okay.
[Qin]:Okay.
[Qin]:Good catch for the typo, will fix.
[Qin]:Okay.
[Qin]:Okay.
[Qin]:Okay.
[Qin]:Okay.
[Qin]:Okay.
[Qin]:Okay.
[Qin]:Okay.
[Qin]:Okay.
[Qin]:Okay.

[Qin]: Will add a term in the terminology section.
[Qin]:Okay.
[Qin]:Okay.
[Qin]:Okay.

Thanks,
Kent // as shepherd