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

From: "Clyde Wildes (cwildes)" <[email-masked]>
Date: Sat, 12 Aug 2017 02:05:06 +0000

Kent,

Thanks for your exhaustive review. I will be publishing the revised model momentarily.

Comments inline as [clyde].

On 7/12/17, 2:55 PM, "netmod on behalf of Kent Watsen" <[email-masked] on behalf of [email-masked]> wrote:

    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?
    
[clyde] Agreed.

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

[clyde] Done.
    
    3. S1, P1, last sentence.  s/the messages/these messages/?

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

[clyde] Done.
    
    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?

[clyde] Suggestions appreciated. I wanted to provide a high level overview of the syslog process. I cleaned it up a little.
    
    6. S1.1: you should also reference RFC8174 here.

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

[clyde] Done
    
    8. S2: s/6020/7950/

[clyde] done
    
    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"?

[clyde] done
    
    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.

[clyde] done
    
    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?

[clyde] done
    
    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.

[clyde] S or F might not be present. 

       The operative sentence in the pseudocode is: 
       There is an element of facility-list (F, S)…
       or the message text matches the regex pattern (if it is present)
    
    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).

[clyde] I used to the Tree Diagram Notation embedded in the document and was
asked by another reviewer to use what is there now. I will change to your document’s 
notation.
    
    14. S3.1: is /syslog/actions/remote/destination/tls/ missing an
    'address' leaf?

[clyde] not as far as I know
    
    15. S4.1, P1: Doesn't the module import *groupings* from ietf-keystore
    and ietf-tls-client?

[clyde] done
    
    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";
      }

[clyde] done
    
    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?

[clyde] done
    
    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)?

[clyde] ‘none’ and ‘all’ are set to values that are not defined in RFC 5424. These values
were previously suggested by Martin Björklund
    
    20. S7: can you indent the two blocks of details so the whole thing
    reads better?

[clyde] I searched for an example that shows how to do this in XML and couldn’t find the keyword.
    
    21. S8: please rework so this section so it matches the new template
    at: https://trac.ietf.org/trac/ops/wiki/yang-security-guidelines

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

[clyde] done
    
Thanks,

Clyde

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

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

---

From: Kent Watsen <[email-masked]>
Date: Mon, 14 Aug 2017 13:53:46 +0000

> [clyde] Suggestions appreciated. I wanted to provide a high level overview
> of the syslog process. I cleaned it up a little.
 
Move Section 2 text to Section 1, replacing the text that's there?

> [clyde] S or F might not be present. 

In the YANG module, facility-list is keyed by [facility severity], which
means the values are always present, right?

> [clyde] not as far as I know

Looking at the tree-diagram, the 'tls' case doesn't seem to have the
address or port fields.  FWIW, the ietf-tls-client module doesn't 
provide these fields so that consuming modules can configure a normal
client versus a client listening for call-home connections...

	   +--:(tcp)
	   |  +--rw tcp
	   |     +--rw address?   inet:host
	   |     +--rw port?      inet:port-number
	   +--:(udp)
	      +--rw udp
	         +--rw address?   inet:host
	         +--rw port?      inet:port-number
	      +--:(tls)
	         +--rw tls
                  <address/port missing here, right?>
	            +--rw server-auth
                     <more ietf-tls-client grouping here>

> [clyde] ‘none’ and ‘all’ are set to values that are not defined in 
> RFC 5424. These values were previously suggested by Martin Björklund

Fine, but let's re-evaluate the values now.  Image having a variable x
and stepping through the selector list:

  if x >= facility-list/severity then foo.

Now imagine it read:

  if x >= 'all' then foo.

What integer value for 'all' would always ensure True?  MIN-INT
Likewise, you can see that MAX-INT is the best value for 'none'.

> and couldn’t find the keyword.

Assuming xml2rfc XML, then you could convert the contents to a figure,
or a list with style='empty'

Thanks,
Kent

---

From: "Clyde Wildes (cwildes)" <[email-masked]>
Date: Mon, 14 Aug 2017 14:47:17 +0000

Kent,

Comments inline as [clyde]…

On 8/14/17, 6:53 AM, "Kent Watsen" <[email-masked]> wrote:

     
    
[clyde] will do    

    
[clyde] There are two paths specifying a facility-filter in which case S or F are present, or specifying a pattern-match in which case they might not be present if facility-filter is not specified.   

    
    	   +--:(tcp)
    	   |  +--rw tcp

[clyde] Here is what the tree looks like in the latest draft…

                   |  +--:(tls)
                   |     +--rw tls
                   |        +--rw server-auth
                   |        |  +--rw trusted-ca-certs?       -> /ks:keystore/trusted-certificates/name
                   |        |  +--rw trusted-server-certs?   -> /ks:keystore/trusted-certificates/name
                   |        +--rw client-auth
                   |        |  +--rw (auth-type)?
                   |        |     +--:(certificate)
                   |        |        +--rw certificate?   -> /ks:keystore/keys/key/certificates/certificate/name
                   |        +--rw hello-params {tls-client-hello-params-config}?
                   |        |  +--rw tls-versions
                   |        |  |  +--rw tls-version*   identityref
                   |        |  +--rw cipher-suites
                   |        |     +--rw cipher-suite*   identityref
                   |        +--rw address?        inet:host
                   |        +--rw port?           inet:port-number
    
Address and port are there. Please clarify on what you think is missing.
  
This is what it looks like in the model:

            case tls {
              container tls {
                description
                  "This container describes the TLS transport options.";
                reference
                  "RFC 5425: Transport Layer Security (TLS) Transport 
                   Mapping for Syslog ";
                uses tlsc:tls-client-grouping;
                leaf address {
                  type inet:host;
                  description
                    "The leaf uniquely specifies the address of 
                     the remote host. One of the following must be 
                     specified: an ipv4 address, an ipv6 address, 
                     or a host name.";
                }
                leaf port {
                  type inet:port-number;
                  default 6514;
                  description
                    "TCP port 6514 has been allocated as the default 
                     port for syslog over TLS.";
                }
              }
            }

    

    Now imagine it read:
    
      if x >= 'all' then foo.

[clyde] I will be change these to:

'none' 2147483647 (so nothing can be greater than it)
'all' -2147483648 (so everything is greater than it)?

[clyde] ok I will follow this advice    

Thanks again for all your review comments!

Clyde
      
    Thanks,
    Kent

---

From: Kent Watsen <[email-masked]>
Date: Mon, 21 Aug 2017 14:18:16 +0000

Hi Clyde,

Trimming down to just the active discussion points.

    

<KENT> if this is what you're trying to say, then your pseudocode isn't right.
Regardless if you agree with me or not, you need to somehow make this section clearer.

    
    	   +--:(tcp)
    	   |  +--rw tcp

                   |  +--:(tls)
                   |     +--rw tls
                   |        +--rw server-auth

            case tls {
              container tls {
                description

<KENT> Sorry, it didn't see them way down at the bottom.  In all my drafts, I have these leafs before the tls-client-grouping.  Can you please switch the order around?

    
    
    Now imagine it read:
    
      if x >= 'all' then foo.

<KENT> I think so, but we should get Martin's confirmation.

K.