Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixing nits suggested by Mahesh #89

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Fixing nits suggested by Mahesh #89

wants to merge 5 commits into from

Conversation

sbarguil
Copy link
Collaborator

@sbarguil sbarguil commented Jan 3, 2025

COMMENT:

"Abstract", paragraph 2

RFC 8519 defines a YANG data model for Access Control Lists (ACLs).
This document discusses a set of extensions that fix many of the
limitations of the ACL model as initially defined in RFC 8519.

The document also defines IANA-maintained modules for ICMP types and
IPv6 extension headers.

This document provided important extensions to the base ACL YANG model defined in RFC8519.

However, nowhere in the document does it mention that these extensions augment the base model, except in the model itself. Reading the abstract and the introduction, a reader would be left wondering whether this model is replacing RFC 8519 or augmenting it. Can that be made clear?

Section 2, paragraph 4

Defined set: Refers to reusable description of one or multiple
information elements (e.g., IP address, IP prefix, port number, or
ICMP type).

Is it just a description? A better way to define it would be to say:

Elements in a defined set typically share a logical purpose or function, such as IP address, IP prefixes, port number, or ICMP type.

Similarly definition of 'payload-based filtering' could be added to define the term or referenced if defined somewhere else.

Section 3.5, paragraph 1

The augmented ACL structure (Figure 1) includes new leafs
'ipv4-fragment' and 'ipv6-fragment' to better handle fragments.

The document comments that RFC8519 does not do a good job of handling fragments and that this document addresses that issue. It would help to explain how the model defined in the document allows for better handling of fragments. For example, describe how the combination of operator and fragment type eases fragment handling.

Section 4, paragraph 1

This model imports types from [RFC6991], [RFC8519], and [RFC8294].

Thanks first of all to Per for providing YANG doctor review comments. I also tend to agree with him that as a practice reference to RFC 9293, 4443, 3032, 792, and IEEE 802.1Q, IEEE 802.1ah need to be added here, even if they are referenced or mentioned elsewhere.

"Appendix E.", paragraph 1

This section provides a few examples to illustrate the use of the
enhanced ACL module ("ietf-acl-enh").

Thank you for providing examples along with the module. The added feature in this module is payload-based filtering. An example configuration reflecting that, say for HTTP headers, would greatly enhance the understanding of this module.

No reference entries found for these items, which were mentioned in the text:
[RFC3692].


NIT

All comments below are about very minor potential issues that you may choose to
address in some way - or ignore - as you see fit. Some were flagged by
automated tools (via https://github.com/larseggert/ietf-reviewtool) so there
will likely be some false positives. There is no need to let me know what you
did with these suggestions.

Section 3.6, paragraph 2

A new feature, called 'match-on-payload', is defined in the document.
This can be used, for example, for QUIC [RFC9000] or for tunneling
protocols. This feature requires configuring a data offset, a
length, and a binary pattern to macth data against using a specified
operator.

s/macth/match/

Section 6.3.1, paragraph 15

IANA is requested to updated the "Reference" in the "ICMP Type
Numbers" registry as follows:

s/updated/update/

Section 6.3.2, paragraph 15

IANA is requested to updated the "Reference" in the "ICMPv6 "type"
Numbers" registry as follows:

s/updated/update/

Section 6.3.3, paragraph 15

IANA is requested to updated the "Reference" in the "IPv6 Extension
Header Types" registry as follows:

s/updated/update/

Section 1.1, paragraph 10

ixes, port number, or ICMP type. Similarly definition of 'payload-based filt
^^^^^^^^^
A comma may be missing after the conjunctive/linking adverb "Similarly".

Section 4, paragraph 3

{ base tcp-flag; description "Acknowledgment TCP flag bit."; reference "RFC
^^^^^^^^^^^^^^
Do not mix variants of the same word ("acknowledgment" and "acknowledgement")

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants