A YANG Data Model for Routing Policy
draft-ietf-rtgwg-policy-model-31

In the acks you have a misspelling: “Chris Bower” should be “Chris Bowers”.

I don't think you need the BCP 14 boilerplate in the prose part of this document.  You have exactly one of its keywords, at the bottom of Section 4.4, and you could find a way to say that normatively without bringing in BCP 14.  Your choice, of course.

Thanks to Dan Harkins for the SECDIR review.

** Section 5.

   If none of the policy statement conditions
   are satisfied, then evaluation of the current policy definition
   stops, and the next policy definition in the chain is evaluated.

Is it worth mentioning in this paragraph that various implementation specific optimizations may be possible.  For example, Section 4.2 notes policy match conditions.  If the match condition is ALL and the first condition is not satisfied, is it necessary to evaluate the next policy statement?

** Section 8.  The text helpful notes the read sensitivity of “/routing-policy/policy-definitions/policy-definition” with “Additionally,       policies and their attendant conditions and actions should be considered proprietary and disclosure could be used to ascertain partners, customers, and supplies.”  It seems like “defined-sets/prefix-sets” could also reveal these relationships with partners, customers or suppliers.

Thank you for this document; it seems like it will be useful.

Thank you for the work put into this document. I really admire the 4 authors managing to reach a consensus even while having different affiliations: IETF at its best!

Please find below some non-blocking COMMENT points (but replies would be appreciated).


I hope that this helps to improve the document,

Regards,

-éric

== COMMENTS ==

-- Section 2 --
Having "Policy chain: A policy chain is a sequence of policy definitions (described in Section 4)." in the terminology section does not really help the reader...

-- Section 4.1 --
While I am not a YANG expert, I wonder about the "*" (usually meaning 0 or more) for address in the neighbor-set container ? How can a neighbor exist w/o an address ? Why not using the "min-elements' YANG statement ?

Section 4.2

   Comparison conditions may similarly use options to change how route
   attributes should be tested, e.g., for equality or inequality,
   against a given value.

I'm not sure if this is stale text, or if still current, which
operations it refers to (e.g., where is an "inequality" option
offered?).  Perhaps it is referring to the potential behavior of future
augmentations?

Section 4.4

   Note that the called policy may itself call other policies (subject
   to implementation limitations).  The model does not prescribe a
   nesting depth because this varies among implementations.  For
   example, an implementation may only support a single level of
   subroutine recursion.  [...]

Is there a useful way to programmatically determine the nesting limit of
a given implementation, or is this more of just a "read the
documentation" thing?

Section 5

   Whether or not the route's pre-policy attributes are used for testing
   policy statement conditions is dependent on the implementation
   specific value of the match-modified-attributes leaf.  If match-
   modified-attributes is false and actions modify route attributes,
   these modifications are not used for policy statement conditions.
   Conversely, if match-modified-attributes is true and actions modify
   the policy application-specific attributes, the attributes as
   modified by the policy are used for policy condition statements.

Are these "actions" in question only for the current policy statement,
all policy statements in the current policy definition, or all policy
definitions?  (I see on second read that this is an "ro" node at
effectively global scope, and so likely to be at the broadest "all
policy statements" scope, but I would still welcome some clarification.

Section 7.2

      enum  add-metric {
        description
          "Add the specified value to the existing metric.
           If the result would overflow the maximum metric
           (0xffffffff), set the metric to the maximum.";
      }

Is the parenthetical always fully generic?  I seem to recall that, e.g.,
the Babel YANG model uses a 16-bit field for the metric.

    container apply-policy {

It slightly surprised me that the import-policy and export-policy lists
both required an instance, but I am rather distanced from what is
actually done in practice, and it doesn't seem too hard to put in a
"reject everything" policy if needed.  OTOH, there are *also* separate
default-{import,export} policy leaves, that do have a default reject, so
the need to explicitly set at least one policy in both direction may be
overkill.

          leaf-list tag-value {
            type tag-type;
            description
              "Value of the tag set member.";

The tag-type is a union of uint32 and hex-string; does the notion of
"equivalence" for matching tags do any type conversion between those?
If not, a warning that the value set here must be of the correct type to
match the received (or generated?) route might be in order.

              container match-interface {
                leaf interface {
                  type leafref {
                    path "/if:interfaces/if:interface/if:name";

Is it always the case that the interface over which we receive a route
and the interface that traffic on that route is sent will be the same?
Or do we have to worry about potential skew between the two ways that
the interface comes into play?

              container match-neighbor-set {

I don't understand why there's no "match-set-options" analogue for
match-neighbor set.  Does the neighbor set use "ANY" matching semantics,
then?

              leaf set-route-preference {
                type uint16;
                description
                  "Set the preference for the route. It is also
                   known as 'administrative distance', allows for
                   selecting the preferred route among routes with
                   the same destination prefix. A smaller value is
                   more preferred.";

Thank you for including the last sentence!

Section 8

We should probably mention the apply-policy grouping as being
security-relevant (and how).

Section 11.1

Since RFCs 6241 and 8040 are only referenced from the security
considerations boilerplate (as examples of how YANG modules can be
used), they may not need to be classified as normative.

Appendix A,B

I didn't do much of a review here, since I'd need to understand
draft-ietf-idr-bgp-model in order to have very much useful to say, and
any such comments would accordingly be better directed at that draft.

NITS

Section 4.4

As an editorial matter, I'd consider adding a statement along the lines
of "when call-policy elements are present, a given policy statement
matches if all called policies return accept-route and all the other
conditions in the policy statement also match".  There don't seem to be
any other ways to read the current text that are reasonable, but it took
me a bit of thinking to work it out.

Section 7.2

  identity isis-level-1-2 {
    base route-level;
    description
      "Identity for IS-IS Level 1 and Level 2 area importation. It
       is only applicable to routes imported into the IS-IS
       protocol.";

The "importation into both" phrasing from ospf-normal-nssa reads much
more clearly to me than what's here.

          container prefixes {
            description
              "Container for the list of prefixes in a policy
               prefix list. Since individual prefixes do not have
               unique actions, the order in which the prefix in
               prefix-list are matched has no impact on the outcome
               and is left to the implementation. A given prefix-set
               condition is satisfied if the input prefix matches
               any of the prefixes in the prefix-set.";

The last sentence seems like it could be removed, since the
match-set-options leaf is what actually specifies the matching behavior
(and is not always "any").

              leaf set-application-tag {
                type tag-type;
                description
                  "Set the application tag for the route.
                   The application-specific tag is an additional tag
                   that can be used by applications that require
                   semantics and/or policy different from that of the
                   tag. For example, the tag is usually automatically
                   advertised in OSPF AS-External Link State
                   Advertisements (LSAs) while this application-specific
                   tag is not advertised implicitly.";

This seems to make more sense if I apply s/implicitly/explicitly/.

Section 8

      /routing-policy/defined-sets/prefix-sets -- Knowledge of these
      data nodes can be used to ascertain which local prefixes are
      susceptible to a Denial-of-Service (DoS) attack.

      /routing-policy/defined-sets/prefix-sets -- Knowledge of these
      data nodes can be used to ascertain local neighbors against whom
      to mount a Denial-of-Service (DoS) attack.

The second one reads like it should be "neighbor-set" rather than
"prefix-sets".

      /routing-policy/policy-definitions/policy-definition /statements/

Spurious space.

No reference entries found for: [draft-ietf-idr-bgp-model-09]

-------------------------------------------------------------------------------
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.

"Table of Contents", paragraph 2, nit:
>  manage policy configuration in a consistent way in environments with routers
>                              ^^^^^^^^^^^^^^^^^^^
Consider replacing this phrase with the adverb "consistently" to avoid
wordiness.

Section 3. , paragraph 4, nit:
> ations, and similarly, actions may effect multiple changes to route attribut
>                                    ^^^^^^
Did you mean "affect" (have an effect upon)?

Section 4.2. , paragraph 10, nit:
>  a reject-route action returns false and the calling policy evaluation will
>                                     ^^^^
Use a comma before "and" if it connects two independent clauses (unless they
are closely connected and short).

Section 4.3. , paragraph 4, nit:
> eating policies nested beyond a small number of levels (e.g., 2-3) is discou
>                               ^^^^^^^^^^^^^^^^^
Specify a number, remove phrase, use "a few", or use "some".

Section 4.3. , paragraph 4, nit:
> o ensure that there is no recursion amongst nested routing policies. 5. Polic
>                                     ^^^^^^^
Do not mix variants of the same word ("amongst" and "among") within a single
text.

Section 4.4. , paragraph 2, nit:
> n is specified for the chain). Whether or not the route's pre-policy attribut
>                                ^^^^^^^^^^^^^^
Consider shortening this phrase to just "Whether". It is correct though if you
mean "regardless of whether".

Section 7.2. , paragraph 31, nit:
> existing metric. If the result would overflow the maximum metric (0xffffffff
>                                ^^^^^^^^^^^^^^
Consider removing "would". (Usually, "would" does not occur in a conditional
clause, unless to make a request or give a polite order.).

Section 7.2. , paragraph 33, nit:
> he existing metric. If the result would be less than 0, set the metric to 0.
>                                   ^^^^^^^^
Consider removing "would". (Usually, "would" does not occur in a conditional
clause, unless to make a request or give a polite order.).