Re: RtgDir review: draft-ietf-rtgwg-policy-model-16

[John Scudder <[e-mail]>]

Hi Yingzhen,

Other than the question of prefix definition (my #16) everything looks good, just one nit,


                                                        creating
   policies nested beyond a small number of levels (e.g., 2-3) are
   discouraged.


Should be “is discouraged”. Sorry, I know I suggested that wording but I didn’t think about it in context. I was thinking “nested policies… are discouraged” but in fact it’s “creating” that’s being discouraged, so “is”.

Regarding #16, you propose changing from your current, which looks like

198.51.100.0/24 mask-length-lower 24 mask-length-upper 28

to be like this:

198.51.100/0/24 mask-length-max 28

The first form is more expressive and matches what’s in the field more closely, so I’d suggest retaining it or something like it. I just think the definition needs to be tightened. One thing to mention is the constraint that mask-length <= mask-length-lower <= mask-length-upper. You’ve made a start at that in your update to the description of mask-length-lower, but shouldn’t that be a “must” clause and not just a description? I guess maybe that would require you to change your prefix encoding so that you can break the mask length out as an atom, but it seems worth it. I also notice that -lower and -upper aren’t mandatory: should you specify what their defaults are if not specified? I presume -lower defaults to be equal to mask-length, and -upper defaults to be equal to -lower.

And finally, I think you should say a little more about the semantics of what a prefix match is. I suppose you can argue that this is common knowledge by now, but it seems to me it’s worth laying out. Given the way your document is structured, I’m not sure where I’d suggest putting the procedure, but for what it’s worth here’s what we did in RFC 6811 to define a similar matching procedure:


   o  Covered: A Route Prefix is said to be Covered by a VRP when the
      VRP prefix length is less than or equal to the Route prefix
      length, and the VRP prefix address and the Route prefix address
      are identical for all bits specified by the VRP prefix length.
      (That is, the Route prefix is either identical to the VRP prefix
      or more specific than the VRP prefix.)

   o  Matched: A Route Prefix is said to be Matched by a VRP when the
      Route Prefix is Covered by that VRP, the Route prefix length is
      less than or equal to the VRP maximum length, and the Route Origin
      ASN is equal to the VRP ASN.


Obviously this text doesn’t work verbatim for your document, but it probably isn’t that hard to adapt, e.g. something like:


   o  Covered: A Route Prefix is said to be Covered by a prefix filter

      when the mask-length is less than or equal to the Route prefix
      length, and the ip-prefix address and the Route prefix address
      are identical for all bits specified by the mask-length.
      (That is, the Route prefix is either identical to the prefix filter
      or more specific than the prefix filter.)

   o  Matched: A Route Prefix is said to be Matched by a prefix filter when the
      Route Prefix is Covered by that prefix filter, the Route prefix length is
      less than or equal to the max-length-upper, and the Route prefix

      length is greater than or equal to the max-length-lower.


I’m sure that needs some work, or of course you may not choose to adopt it at all, but you get the idea. Now that I think about it, this text doesn’t capture the idea of a best match, because we didn’t need that for RFC 6811. I’m not sure you need it either since a prefix-list either matches a route, or doesn’t, and it doesn’t have the potential for different dispositions depending on which leaf matched. If it did have that potential, you’d need to capture the idea of best-match.

$0.02,

—John

On Jul 5, 2020, at 8:22 PM, Yingzhen Qu <[e-mail]<mailto:[e-mail]>> wrote:



Hi John,

Thank you so much for your thorough review, really appreciate. I’ve uploaded version -17 to address your comments and please see detailed response below starting with [YQ].

Question 16 still remains open, and please let me know your comments.

Thanks,
Yingzhen

From: John Scudder <[e-mail]<mailto:[e-mail]>>
Date: Tuesday, June 30, 2020 at 11:14 AM
To: "[e-mail]<mailto:[e-mail]>" <[e-mail]<mailto:[e-mail]>>
Cc: "[e-mail]<mailto:[e-mail]>" <[e-mail]<mailto:[e-mail]>>, "[e-mail]<mailto:[e-mail]>" <[e-mail]<mailto:[e-mail]>>, RTGWG <[e-mail]<mailto:[e-mail]>>
Subject: RtgDir review: draft-ietf-rtgwg-policy-model-16
Resent-From: <[e-mail]<mailto:[e-mail]>>
Resent-To: <[e-mail]<mailto:[e-mail]>>, <[e-mail]<mailto:[e-mail]>>, <[e-mail]<mailto:[e-mail]>>, <[e-mail]<mailto:[e-mail]>>, <[e-mail]<mailto:[e-mail]>>, <[e-mail]<mailto:[e-mail]>>, <[e-mail]<mailto:[e-mail]>>, <[e-mail]<mailto:[e-mail]>>, <[e-mail]<mailto:[e-mail]>>
Resent-Date: Tuesday, June 30, 2020 at 11:14 AM

Hello,
I have been selected as the Routing Directorate reviewer for this draft. The Routing Directorate seeks to review all routing or routing-related drafts as they pass through IETF last call and IESG review, and sometimes on special request. The purpose of the review is to provide assistance to the Routing ADs. For more information about the Routing Directorate, please see ​http://trac.tools.ietf.org/area/rtg/trac/wiki/RtgDir<https://urldefense.com/v3/__https://nam11.safelinks.protection.outlook.com/?url=http*3A*2F*2Ftrac.tools.ietf.org*2Farea*2Frtg*2Ftrac*2Fwiki*2FRtgDir&data=02*7C01*7Cyingzhen.qu*40futurewei.com*7Cc87e948839554e7786e308d81d216ffc*7C0fee8ff2a3b240189c753a1d5591fedc*7C1*7C1*7C637291376752227134&sdata=zixUU1xImjqWnN8jg7F2Qg4DaQkGNyn89M3TGl25Rg8*3D&reserved=0__;JSUlJSUlJSUlJSUlJSUlJSU!!NEt6yMaO-gk!XZuh9ykx2jpIDjT5R-BWU-N9PMpRCh_gjjLWQ-JGr5mSxXCIoz7BCHcCRqXLpQ$>
Although these comments are primarily for the use of the Routing ADs, it would be helpful if you could consider them along with any other IETF Last Call comments that you receive, and strive to resolve them through discussion or by updating the draft.
Document: draft-ietf-rtgwg-policy-model-16
Reviewer: John Scudder
Review Date: June 30, 2020
IETF LC End Date: ?
Intended Status: Standards Track

Summary: I have one significant concern about this document and recommend that the Routing ADs discuss this issue further with the authors.

Comments:
The draft is clear and readable, thank you for putting in the effort to produce a high-quality document.

Major Issues:
1. On page 17 you say:

        Policy 'subroutines' (or nested policies) are supported by
        allowing policy statement conditions to reference another
        policy definition which applies conditions and actions from
        the referenced policy before returning to the calling policy
        statement and resuming evaluation.  If the called policy
        results in an accept-route (either explicit or by default),
        then the subroutine returns an effective true value to the
        calling policy.  Similarly, a reject-route action returns
        false.  If the subroutine returns true, the calling policy
        continues to evaluate the remaining conditions (using a
        modified route if the subroutine performed any changes to the
        route).

I read this as saying further evaluations should consider the modified route, not the original route. But on page 10 you say:

   Note that the route's pre-policy attributes are always used for
   testing policy statement conditions.  In other words, if actions
   modify the policy application specific attributes, those
   modifications are not used for policy statement conditions.

Which is it? I think this has to be resolved one way or the other before progressing the document.

[YQ] Thanks for catching this. For nested policies, original data should always be used. I’ve changed the description in the model (the page 17 one) to match page 10.

Minor Issues:

2. For an outsider reading the document, the precise meanings of “import” and “export”, as well as “routing context” (and similar) are not clear. The first place I noticed this was in section 6:

   Policy chains are sequences of policy
   definitions (described in Section 4) that have an associated
   direction (import or export) with respect to the routing context in
   which they are defined.

Possibly all this would be obvious to someone with the necessary YANG expertise, I guess. I can pretty much intuit it because I’m familiar with routing policy, however I think it’s worth putting in the effort to make it clearer to the reader and not require them to use their intuition. Would it be accurate to rewrite the quoted sentence something like this?

“Policy chains are sequences of policy definitions (described in Section 4). They can be referenced from different contexts. For example, a policy chain could be associated with a routing protocol and used to control its interaction with its protocol peers. Or, it could be used to control the interaction between a routing protocol and the local routing information base. A policy chain has an associated direction (import or export), with respect to the context in which it is referenced."

[YQ]: thanks for suggesting the text. It’s modified as what you suggested.

3. This sentence rubbed me wrong:

   Nested policies are a
   convenience in many routing policy constructions but creating
   policies nested beyond a small number of levels (e.g., 2-3) should be
   discouraged.


By whom should they be discouraged? Maybe you mean “are discouraged”? By the way, *why* are they discouraged?

[YQ]: changed to “are discouraged”. The reason is also explained in the same paragraph, please let us know if you think this is not clear or enough.

4. A question about the next sentence:


   Also, implementations should have validation to assure
   that there is no recursion amongst nested routing policies.

I guess the concern about recursion is that given the route data is treated as immutable by policy and there’s no way to pass results or parameters, there’s no way to have a termination condition? This makes sense, or doesn’t, depend on how you resolve my question #1. (Also I think you mean “ensure”.)

[YQ]: A recursion might happen if you have policy A calling policy B, then policy B is calling policy A without proper termination condition. Bottom line is when a policy gets more complicated, it’s easier to make mistakes, and the operators need to be more careful.

5. Mostly in this document it’s clear that you’re using “operator” in the sense of “network operator” and not in its algebraic sense, but I found it a little ambiguous in this sentence:

   Match conditions may be further modified using the match-set-options
   configuration which allows operators to change the behavior of a
   match.  Three options are supported:


It would remove ambiguity if you changed this to say “network operators”.

[YQ]: Done.

6. Some of the types you describe are suffixed with “-type”, for example “ospf-external-type”. Others aren’t suffixed, for example “ospf-external-t1”. Is there any reason for this lack of consistency? It hurts my eyes but is otherwise harmless I suppose.
[YQ]: added “-type” to a few identities to make them consistent.

7. It’s unclear to me why the draft defines “bgp-local” and “bgp-external”, since it otherwise doesn’t deal with BGP in any way, leaving that for a separate document. Furthermore, I don’t even understand what the semantics of these are. I suppose “bgp-external” is likely to be a route received from an EBGP peer. As for “bgp-local”, I don’t care to guess.

[YQ]: I changed “bgp-local” to “bgp-internal”, also changed descriptions.

8. The description of ip-prefix says “The IP prefix represented as an IPv6 or IPv4 network number followed prefix length with an intervening slash character a delimiter”. I think maybe you are missing a “by a” and a “as”? As in, "The IP prefix represented as an IPv6 or IPv4 network number followed by a prefix length with an intervening slash character as a delimiter.”
[YQ]: fixed.

9. Speaking of IP addresses and prefixes, you use net 10 in one of your examples. It’s my understanding that this is poor form, that you should be using the “documentation” addresses given in RFC 6890. (Thanks for doing that with the rest of the document, BTW.)
[YQ]: Changed net 10 to 198.51.100.0/24, one of the documentation addresses.

10. Also speaking of prefixes, all your examples are nicely-formed. Here’s a less-nicely formed one: 192.0.2.0/8 mask-length-lower=24 mask-length-upper=24. Does this match 192.0.2/24? Does it match 192.0.0/24? How about 192.255.1/24? Does it match something else, if so what? Is it a syntax error? I think this relates to my later question #16.
[YQ]: Please see answer to question #16.

11. You describe export-policy as:

           "List of policy names in sequence to be applied on
            sending a routing update in the current context, e.g.,
            for the current peer group, neighbor, address family,
            etc.";


Is export-policy really restricted only to the formation of routing updates? This makes perfect sense for BGP, but I don’t think the link-state IGPs really work this way, for example.

[YQ]: you’re right, the descriptions regarding peer group etc. are more BGP-centric. I’ve changed the description for both import-policy and export-policy. Please let me know if you have more comments. Cop&paste below for your convenience.
Import-policy:
      description
        "List of policy names in sequence to be applied on
         receiving redistributed routes from another routing protocol
         or receiving a routing update in the current context, e.g.,
         for the current peer group, neighbor, address family,
         etc.";
Export-policy:
      description
        "List of policy names in sequence to be applied on
         redistributing routes from one routing protocol to another
         or sending a routing update in the current context, e.g.,
         for the current peer group, neighbor, address family,
         etc.";

12. You describe apply-policy as:

           "Anchor point for routing policies in the model.
            Import and export policies are with respect to the local
            routing table, i.e., export (send) and import (receive),
            depending on the context.";


This is not clear to me (sorry). If I expand “i.e.” as “in other words” it doesn’t help I’m afraid. Since I don’t understand it I can’t suggest a fix. :-(

[YQ]: this grouping is meant to be used by other models. There is also the following description at the grouping level:
    description
      "Top level container for routing policy applications. This
       grouping is intended to be used in routing models where
       needed.";
This model supports both import and export mode, so it’s up to an implementation to choose the right one.

13. Where you say “ambiguous and implementation dependent” I suggest dropping “and implementation dependent”, it seems redundant.
[YQ]: done.

14. I feel sad that all the examples are IPv4, poor IPv6 remains the red-headed stepchild. ¯\_(ツ)_/¯
[YQ]: ¯\_(ツ)_/¯

15. In the final example on p. 37, I scratched my head a little that the operation being done is set-import-level. I guess the policy is going to be applied in the IS-IS context, and therefore it’s not OSPF exporting the route, but rather IS-IS importing the route into its LSPDB? This is what led to my comment #2.
[YQ]: yes, it’s IS-IS to import OSPF routes. Vendors have different implementations when redistributing routes, and the model support both import and export.

16. I don’t think you ever define the semantics of a prefix set. Of course “everyone knows” that this should be a best match, and not (say) sequential match or something else — but please say so. See also my comment #10, you also seem to be underspecified with respect with what to do if mask-length-lower is greater than the given mask length.

[YQ]: Thanks for bringing up the prefix length issue. How about we change it to only ip-prefix and “mask-length-max”? similar to “ipAddrBlocks” defined in RFC 6482. The “mask-length-max” should not be less than the prefix length.
Open to suggestions on this.
Nits:
[YQ]: all fixed.
17. I’ve attached an edited version of the draft with a few small typo and grammar corrections. Here’s a diff as well, vs. the base -16:

jgs-mbp:Downloads jgs$ diff draft-ietf-rtgwg-policy-model-16.txt draft-ietf-rtgwg-policy-model-16-jgs-edits.txt
155c155
<    exported, modified, and advertised between routing protocols
---
>    exported, modified, and advertised between routing protocol
302c302
<    The models provides a set of generic sets that can be used for
---
>    The models provide a set of generic sets that can be used for
777c777
<    elsewhere in the draft.
---
>    elsewhere in this document.
876c876
<         or comparison operations, and similarly actions may be
---
>         or comparison operations, and similarly actions may be a
996c996
<          "RFC 5302 - Domain-Wide Prefix Distributino with
---
>          "RFC 5302 - Domain-Wide Prefix Distribution with
1014c1014
<          "RFC 5302 - Domain-Wide Prefix Distributino with
---
>          "RFC 5302 - Domain-Wide Prefix Distribution with
1059c1059
<          "RFC 5302 - Domain-Wide Prefix Distributino with
---
>          "RFC 5302 - Domain-Wide Prefix Distribution with
1077c1077
<          "RFC 5302 - Domain-Wide Prefix Distributino with
---
>          "RFC 5302 - Domain-Wide Prefix Distribution with
1088c1088
<          "RFC 5302 - Domain-Wide Prefix Distributino with
---
>          "RFC 5302 - Domain-Wide Prefix Distribution with
1103c1103
<          "RFC 5302 - Domain-Wide Prefix Distributino with
---
>          "RFC 5302 - Domain-Wide Prefix Distribution with
1113c1113
<          "RFC 5302 - Domain-Wide Prefix Distributino with
---
>          "RFC 5302 - Domain-Wide Prefix Distribution with
1483c1483
<              "Condition to check the protocol specific type
---
>              "Condition to check the protocol-specific type
2208c2208
<    The routing policy module defined in this draft is based on the
---
>    The routing policy module defined in this document is based on the