Paul Wouters comments with DISCUSS

[xorrkaz]

1. The layout is completely broken / wrapped, making the document fairly unreadable. Can this be fixed somehow ?

2. I don't see a method for syslog using TCP without TLS ? In fact, I am using that on my personal home router to my collection server, without TLS. Could the document not simply use:

+--rw (transport)
              |  +--:(udp)
              |  |  +--rw udp
              |  |     +--rw address?   inet:host
              |  |     +--rw port?      inet:port-number
              |  |  +--rw tcp
              |  |     +--rw address?   inet:host
              |  |     +--rw port?      inet:port-number
              |  +--:(tls)

3. Also, how do ports and addresses combine. Can I specify "1.2.3.4:80 and 5.6.7.8:443" ? It looks like that is not the case here?

4. I don't see an option for configuring ratelimits, which are commonly available as options for syslog services?

5. Could the TLS configuration parameters not use another model (one the IESG reviewed a little while ago?). It seems odd to define these within the syslog module. It would be nice if any kind of certificate options with TLS could be included from another module so if there are no TLS options, this module/RFC does not require updating?

6. Maybe the same applies for the "signers" section but perhaps that is differently formatted data signed and unrelated to the TLS layer ?

Other Comments:

7. Related question: Is it one certificate+key that used for the TLS connection as well as to sign data within the payload of packets?

8. facility-filter seems badly named as it also filters for severity ? Maybe syslog-filter ?

[xorrkaz]

On points 1, 5 and 6, I think we can fix this by using the pyang option --tree-no-expand-uses. I know I told Éric we didn't want to do this, but this is now the second comment that gets confused. I think we can clean up the tree and avoid further confusion. Thoughts?

I need to look at points 2 and 3 more closely. I haven't had a chance to break this down. Certainly the intent is to be able to do this without TLS as that is a common case (as is clear text UDP).

On point 4, my comment would be that the reason this draft languished so long is that it kept growing in terms of options and complexity. I don't think we want to capture all available options something like syslog-ng supports. The initial goal was a least-common-denominator approach IIRC. Various implementations are free to augment and add their own capabilities. Thoughts?

I need to look at point 7 more.

For point 8, I agree. The facility-filter uses the severity-filter grouping:

|  +--rw facility-filter
        |  |  +--rw facility-list* [facility severity]
        |  |     +--rw facility            union
        |  |     +--rw severity            union
        |  |     +--rw advanced-compare {select-adv-compare}?
        |  |        +--rw compare?   enumeration
        |  |        +--rw action?    identityref

I think we should call this syslog-filter or just filter. Thoughts?

[mjethanandani]

On points 1, 5 and 6, I think we can fix this by using the pyang option --tree-no-expand-uses. I know I told Éric we didn't want to do this, but this is now the second comment that gets confused. I think we can clean up the tree and avoid further confusion. Thoughts?

As I mentioned in the e-mail, the issue with the tree diagram is not with the length, which is what you would address if you went with '--tree-no-expand-uses' option. It is with the width.

The secondary point of whether we are using groupings from other drafts, or defining our own comes from a lack of understanding how the trees are generated. In my mind, that is more education.

Having said that, we can print the main tree diagram with reference to groupings in them, and then print the grouping (that we are importing) separately. I will have to check how much work that is going to be. Or maybe leave out the printing of groupings completely?? Readers then have to go figure what all parameters need to be configured for TLS.

I need to look at points 2 and 3 more closely. I haven't had a chance to break this down. Certainly the intent is to be able to do this without TLS as that is a common case (as is clear text UDP).

For point 2, yes, currently, there is no support for clear text (TCP). We can add that.

For point 3, I am not clear on the ask. Is the configuration saying that there are two collectors, one clear text on port 80, and another on TLS enabled port of 443? If yes, we need to make each of the transports a list.

On point 4, my comment would be that the reason this draft languished so long is that it kept growing in terms of options and complexity. I don't think we want to capture all available options something like syslog-ng supports. The initial goal was a least-common-denominator approach IIRC. Various implementations are free to augment and add their own capabilities. Thoughts?

Agree. We do not want to expand the scope of the draft.

I need to look at point 7 more.

I am not an expert in TLS, but isn't it the case that the initial certificate+key is used to negotiate a symmetric key which is used to encrypt the subsequent payload?

For point 8, I agree. The facility-filter uses the severity-filter grouping:

|  +--rw facility-filter
        |  |  +--rw facility-list* [facility severity]
        |  |     +--rw facility            union
        |  |     +--rw severity            union
        |  |     +--rw advanced-compare {select-adv-compare}?
        |  |        +--rw compare?   enumeration
        |  |        +--rw action?    identityref

I think we should call this syslog-filter or just filter. Thoughts?

I am ok with that change.

[xorrkaz]

I can make some changes when I'm back from this conference.

[mjethanandani]

Let me know if the PR associated with this issue addresses the questions Paul had. Combined with some of the other changes, and our responses, I believe we should have.

[mjethanandani]

I see that you have provided comments. Let me address them.

[xorrkaz]

Yeah. I'd be happy to write some additional text if you want to merge this. Then I can do a PR on top of it.