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

[John Scudder <[e-mail]>]

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<http://trac.tools.ietf.org/area/rtg/trac/wiki/RtgDir>

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.


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

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?

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

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

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.

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.

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

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

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.

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.

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

13. Where you say “ambiguous and implementation dependent” I suggest dropping “and implementation dependent”, it seems redundant.

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

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.

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.


Nits:

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