Subject: Re: [netmod] WG Last Call for draft-ietf-netmod-syslog-model-15

From: Kent Watsen <[email-masked]>
Date: Wed, 12 Jul 2017 21:55:35 +0000

As shepherd, yang doctor, and individual contributor, following is 
my LC/YD review.

1. Because I know this draft will not be presented in Prague, I first
checked to see if it was NMDA-compatible.  The draft contains just
one module, and it only contains config true nodes (no config false
nodes).  There is no companion "-state" module in the Appendix.  As
far as I can tell, all this is accurate, as I don't believe this 
module needs to do anything special to be NMDA compatible.  Agreed?

2. the abstract seems just a little bland.  Is there any way to beef
it up with a sentence or two?

3. S1, P1, last sentence.  s/the messages/these messages/?

4. S1, P3, 1st sentence: "and processes those"?  - rewrite sentence?

5. S1 as a whole.  I'm a bit unclear what this section is doing.  It
seems to be a general summary of Syslog (RFC5424).  Do we need this here?

6. S1.1: you should also reference RFC8174 here.

7. S1.2: three terms come from 5424, but only one has its definition
   provided.  This seems inconsistent...

8. S2: s/6020/7950/

9. S3, P3: this paragraph is hard to read due to the previous paragraph
talking about proprietary features.  Maybe replace the beginning of the 
sentence to read "Some optional features are defined in this document
to specify"?

10. S3, P4: The diagram appears to show multiple originators, not 
just one, so s/an originator/originators/?  Also, I don't think 
either of the commas are needed.

11. S3, P6: This paragraph starts a new aspect of the design, right?
This is likely just a text-rendering issue, but the transition from
the diagram above (Figure 1) to this line is not visible.  Can you
provide a transition sentence?

12. S3, P8: I'm having trouble understanding the pseudocode.  What
happens if S and/or F are not present?  Can S or F ever not be
present? - looking at the tree diagram, it seems like they might
always be set to something in the model.

13. S3.1, P1: RFC 6087 did not define tree diagram notation, and
rfc6087bis references the tree-diagram draft.  I don't think that
it is safe for this draft to reference the tree-diagram draft, as
that draft is unstable (the notation may change).  You should 
probably copy/paste the Tree Diagram Notation section found in
other drafts today (especially mine).

14. S3.1: is /syslog/actions/remote/destination/tls/ missing an
'address' leaf?

15. S4.1, P1: Doesn't the module import *groupings* from ietf-keystore
and ietf-tls-client?

16. S4.1, though it's not in 6087bis, I think that it is best
practice for 'import' statements to include a 'reference'
substatement:

  import ietf-keystore {
    prefix ks;
    reference
      "RFC YYYY: Keystore Model";
  }

17. S4.1, imports that are used for groupings only should use a
revision statement:

  import ietf-tls-client {
    prefix tlsc;
    revision-date YYYY-MM-DD; // stable grouping definitions
    reference
      "RFC ZZZZ: TLS Client and Server Models";
  }

18. S4.1, can you put the beginning of the 'organization' (i.e. "IETF")
on the next line, s/NETCONF Data Modeling Language/Network Modeling/,
and put a blank line in after the 'organization' line?

19. S4.1, in the 'severity-filter' grouping, why does leaf 'severity'
have values set for enums 'none' and 'all'?  When would these values
be used, as opposed to the enum's name string?  If you do need values,
then shouldn't 'none' be 2147483647 (so nothing can be greater than it)
and 'all' be -2147483648 (so everything is greater than it)?

20. S7: can you indent the two blocks of details so the whole thing
reads better?

21. S8: please rework so this section so it matches the new template
at: https://trac.ietf.org/trac/ops/wiki/yang-security-guidelines

22. S8.1: it would be better if the third paragraph was moved up to
become the first paragraph.

DISCLAIMER: I'm not a syslog expert, but have interacted with it,
including structured-syslog, over the years.

Kent

---

From: "Clyde Wildes (cwildes)" <[email-masked]>
Date: Wed, 19 Jul 2017 17:48:05 +0000

Hi Alex,

Answers inline as [clyde]…

On 7/17/17, 4:20 PM, "netmod on behalf of Alex Campbell" <[email-masked] on behalf of [email-masked]> wrote:

    I am considering to implement the data model in this draft. (dependent on business priorities of course)
    I have reviewed this draft and found the following issues.
    
    * I see pattern-match is specified to use POSIX 1003.2 regular expressions. This is presumably for compatibility with existing implementations; however it is inconsistent with most of YANG (which is specified to use XPath regular expressions) - unless these are the same.

[clyde] I believe that my answer in the other thread explains why we used Posix 1003.2 – it is commonly used. 

    * pattern-match is inside the facility-filter container; common sense says this is wrong as pattern-match has nothing to do with facilities.

[clyde] I will move pattern-match up one level in the next version of the draft. Thanks for catching this!

    * The advanced-compare container groups together two nodes that share a common "when" and "if-feature" statement, but don't seem to have any semantic relation to each other. Are there general guidelines on when to use a container?

[clyde] The confusion may come as a result of the when clause appearing before the if-feature clause which is set by the IETF statement order recommendation.

The when construct was suggested by Martin Björklund as a way of solving the case that advanced-compare does not apply for the ‘all’ and ‘none’ case.

The if-feature applies to the entire container – it is either supported or not.

    * The advanced-compare container has a description starting with "This leaf ..." even though it is not a leaf.

[clyde] This will be fixed in the next draft.

    * The examples are missing <facility-filter> nodes.

[clyde] This will be fixed in the next draft.

    * Perhaps there should be more consistent terminology for receivers of syslog messages; both "collectors" and "actions" are used in the draft. RFC 5424 uses "collector" for the ultimate recipient of a log message - which might not be applicable, because the sending system has no idea whether the receiving system is a collector or a relay.
    
[clyde] The definition of “collector” in RFC 5424 is: A "collector" gathers syslog content for further analysis.

actions relate to the “further analysis” taken by the “collector”. 

“Collectors” appears in the model under the remote action and I believe the usage is correct:
      container remote {
        if-feature remote-action;
        description
          "This container describes the configuration parameters for 
           forwarding syslog messages to remote relays or collectors.";

I will revise the description of these terms in the next draft.
    
Thanks,

Clyde  
    
    ________________________________________
    From: netmod <[email-masked]> on behalf of Kent Watsen <[email-masked]>
    Sent: Saturday, 8 July 2017 6:34 a.m.
    To: [email-masked]
    Subject: [netmod] WG Last Call for draft-ietf-netmod-syslog-model-15
    
    This is a notice to start a three week NETMOD WG last call for the
    document:
    
        A YANG Data Model for Syslog Configuration
        https://tools.ietf.org/html/draft-ietf-netmod-syslog-model-15
    
    Note: Three weeks is more than needed, especially given this
          draft has been through Last Call before, but we understand
          folks are busy these days.
    
    Please indicate your support or concerns by Friday, July 28, 2017.
    
    We are particularly interested in statements of the form:
      * I have reviewed this draft and found no issues.
      * I have reviewed this draft and found the following issues: ...
    
    As well as:
      * I have implemented the data model in this draft.
      * I am implementing the data model in this draft.
      * I am considering to implement the data model in this draft.
      * I am not considering to implement the data model in this draft.
    
    Thank you,
    NETMOD WG Chairs

    
    _______________________________________________
    netmod mailing list
    [email-masked]
    https://www.ietf.org/mailman/listinfo/netmod
    
    _______________________________________________
    netmod mailing list
    [email-masked]
    https://www.ietf.org/mailman/listinfo/netmod