From: rtgwg <[email protected]> on behalf of Yingzhen Qu
<[email protected]>
Sent: 07 July 2020 06:14
Hi John and all,
Thanks for the comments and suggestions.
Regarding question #16, the leaf “ip-prefix” type includes both the address and
mask length as a string, so it’s hard to add a “must” statement using mask
length. If we separate the prefix definition into address and mask length, we
can have something like:
<tp>
Inserting my comment before all the YANG, just because it is technically
possible to specify something formally in a DDL does not always make it a good
idea. Sometimes the DDL becomes so complex that it gets misunderstood. I did
comment earlier about the YANG requiring lower and upper to be present and to
have upper >= lower but I do not suggest going beyond that. For example in the
YANG below there is a contradiction between length zero and length one;-)
Sometimes a line of text is the better engineering IMHO.
Tom Petch
grouping prefix {
description
"Configuration data for a prefix definition.";
leaf ip-address {
type inet:ip-address;
mandatory true;
description
"An IPv6 or IPv4 address. A prefix is defined using
both ip-address and mask-length. All members of the
prefix set should be of the same address family as
the prefix-set mode.";
}
leaf mask-length {
type uint8 {
range "1..128";
}
mandatory true;
description
"The mask-length, is in range from 0 to 32 for IPv4,
0 to 128 for IPv6.";
}
leaf mask-length-lower {
type uint8;
must "../mask-length-lower >= ../mask-length" {
error-message "The lower bound should not be less"
+ "than the mask-length.";
}
description
"Mask length range lower bound.";
}
leaf mask-length-upper {
type uint8 {
range "1..128";
}
must "../mask-length-upper >= ../mask-length-lower" {
error-message "The upper bound should not be less"
+ "than lower bound.";
}
description
"Mask length range upper bound.
The combination of mask-length-lower and mask-length-upper
define a range for the mask length, or single 'exact'
length if mask-length-lower and mask-length-upper are
equal.
Example: 192.0.2.0/24 through 192.0.2.0/26 would be
expressed as prefix: 192.0.2.0/24,
mask-length-lower=24,
mask-length-upper=26
Example: 192.0.2.0/24 (an exact match) would be
expressed as prefix: 192.0.2.0/24,
mask-length-lower=24,
mask-length-upper=24
Example: any prefix of length 28 in the range from
192.0.2.0/28 to 192.0.2.240/28.
expressed as prefix: 192.0.2.0/24,
mask-length-lower=28,
mask-length-upper=28
A route prefix is said to be matched by a prefix list
when the route prefix length is less than or equal to
the mask-length-upper, and is greater than or equal to
the mask-length-lower.";
}
}
A “must” is added to the mask-length-lower to constrain it’s not less than the
mask length. I also added the “match” explanation at the end of the
description and one more example.
Please let me know your comments.
Thanks,
Yingzhen
From: "Acee Lindem (acee)" <[email protected]>
Date: Monday, July 6, 2020 at 2:56 PM
To: Yingzhen Qu <[email protected]>, John Scudder <[email protected]>
Cc: "[email protected]" <[email protected]>, "[email protected]"
<[email protected]>, "[email protected]"
<[email protected]>, RTGWG <[email protected]>
Subject: Re: [RTG-DIR] RtgDir review: draft-ietf-rtgwg-policy-model-16
Hi John, Yingzhen, Co-authors, et al,
With respect to #16, I think we should explicitly state that it is a most
specific prefix match within the prefix-set. Unfortunately, that is not enough
since the mask range limits are also a part of the prefix-set list key:
container prefixes {
description
"Container for the list of prefixes in a policy
prefix list.";
list prefix-list {
key "ip-prefix mask-length-lower mask-length-upper";
description
"List of prefixes in the prefix set.";
uses prefix;
}
}
However, I really don’t see the utility of multiple prefix entries with the
same prefix but different mask ranges. In fact, if they overlap there could be
ambiguity without the missing rules. We could easily make up some rules
involving mask-length-lower and mask-length-upper but my feeling is that we’d
just be adding complexity without solving any real problem. My proposal would
be to remove mask-length-lower and mask-length-upper from the list key.
Comments??
Thanks,
Acee
From: rtg-dir <[email protected]> on behalf of John Scudder
<[email protected]>
Date: Monday, July 6, 2020 at 5:25 PM
To: Yingzhen Qu <[email protected]>
Cc: Routing ADs <[email protected]>, Routing Directorate <[email protected]>,
"[email protected]"
<[email protected]>, Routing WG <[email protected]>
Subject: Re: [RTG-DIR] RtgDir review: draft-ietf-rtgwg-policy-model-16
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
<[email protected]<mailto:[email protected]>> 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 <[email protected]<mailto:[email protected]>>
Date: Tuesday, June 30, 2020 at 11:14 AM
To: "[email protected]<mailto:[email protected]>"
<[email protected]<mailto:[email protected]>>
Cc: "[email protected]<mailto:[email protected]>"
<[email protected]<mailto:[email protected]>>,
"[email protected]<mailto:[email protected]>"
<[email protected]<mailto:[email protected]>>,
RTGWG <[email protected]<mailto:[email protected]>>
Subject: RtgDir review: draft-ietf-rtgwg-policy-model-16
Resent-From: <[email protected]<mailto:[email protected]>>
Resent-To: <[email protected]<mailto:[email protected]>>,
<[email protected]<mailto:[email protected]>>,
<[email protected]<mailto:[email protected]>>,
<[email protected]<mailto:[email protected]>>,
<[email protected]<mailto:[email protected]>>,
<[email protected]<mailto:[email protected]>>,
<[email protected]<mailto:[email protected]>>,
<[email protected]<mailto:[email protected]>>,
<[email protected]<mailto:[email protected]>>
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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Furldefense.com%2Fv3%2F__https%3A%2Fnam11.safelinks.protection.outlook.com%2F%3Furl%3Dhttp*3A*2F*2Ftrac.tools.ietf.org*2Farea*2Frtg*2Ftrac*2Fwiki*2FRtgDir%26data%3D02*7C01*7Cyingzhen.qu*40futurewei.com*7Cc87e948839554e7786e308d81d216ffc*7C0fee8ff2a3b240189c753a1d5591fedc*7C1*7C1*7C637291376752227134%26sdata%3DzixUU1xImjqWnN8jg7F2Qg4DaQkGNyn89M3TGl25Rg8*3D%26reserved%3D0__%3BJSUlJSUlJSUlJSUlJSUlJSU!!NEt6yMaO-gk!XZuh9ykx2jpIDjT5R-BWU-N9PMpRCh_gjjLWQ-JGr5mSxXCIoz7BCHcCRqXLpQ%24&data=02%7C01%7Cyingzhen.qu%40futurewei.com%7Cf7d05d58ffac4064cc4508d821f7601e%7C0fee8ff2a3b240189c753a1d5591fedc%7C1%7C0%7C637296693645977131&sdata=dB9UN0wZ5Ip1hwRGGBAHWdNqZwiMqd%2F2abeH1YO3p6o%3D&reserved=0>
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
_______________________________________________
rtgwg mailing list
[email protected]
https://www.ietf.org/mailman/listinfo/rtgwg
