Dear authors: Thank you for a well written document!
I have some comments inline. I would like to at least see you address the Security Considerations-related issues before starting the IETF LC. Thanks! Alvaro. [Line numbers from idnits.] ... 54 Table of Contents ... 69 7. Security Considerations . . . . . . . . . . . . . . . . . . . 11 70 8. IANA Considerations . . . . . . . . . . . . . . . . . . . . . 12 71 9. YANG module . . . . . . . . . . . . . . . . . . . . . . . . . 12 72 9.1. Routing policy model . . . . . . . . . . . . . . . . . . 12 [minor] Please move the module to *before* the Security Considerations. ... 212 3. Model overview ... 220 o A structure that allows routing protocol models to add protocol- 221 specific policy conditions and actions though YANG augmentations. 222 There is a complete example of this for BGP [RFC4271] policies in 223 the proposed vendor-neutral BGP data model 224 [I-D.ietf-idr-bgp-model]. [minor] As far as I can tell, the augmentation in I-D.ietf-idr-bgp-model is not an example -- it is the real augmentation. Maybe you meant to write "the augmentation in I-D.ietf-idr-bgp-model can be used as an example for other protocols". See also my comment below about Appendix A. ... 259 4.1. Defined sets for policy matching ... 270 o neighbor sets - Each neighbor set define a set of neighboring 271 nodes by their IP addresses. A neighbor set is used for selecting 272 routes based on the neighbors advertising the routes. [nit] s/neighbor set define/neighbor set defines ... 300 4.2. Policy conditions 302 Policy statements consist of a set of conditions and actions (either 303 of which may be empty). Conditions are used to match route 304 attributes against a defined set (e.g., a prefix set), or to compare 305 attributes against a specific value. [minor] "conditions and actions (either of which may be empty)" This got me thinking, what is the default? If the action set is empty, what should be the default: accept or deny? I latter found, buried in §5 that the default is reject-route. It might be a good idea to mention that here. ... 356 4.3. Policy actions ... 371 +--rw actions ... 381 +--rw set-preference? uint16 [?] What is the preference? Is this intended to be related to "administrative distance", or local-pref, or what? Or maybe it is something completely different. 382 +--rw set-tag? tag-type 383 +--rw set-application-tag? tag-type [?] What is an application tag? What is the difference between that an a tag? 385 4.4. Policy subroutines 387 Policy 'subroutines' (or nested policies) are supported by allowing 388 policy statement conditions to reference other policy definitions 389 using the call-policy configuration. Called policies apply their 390 conditions and actions before returning to the calling policy 391 statement and resuming evaluation. The outcome of the called policy 392 affects the evaluation of the calling policy. If the called policy 393 results in an accept-route, then the subroutine returns an effective 394 boolean true value to the calling policy. For the calling policy, 395 this is equivalent to a condition statement evaluating to a true 396 value and evaluation of the policy continues (see Section 5). Note 397 that the called policy may also modify attributes of the route in its 398 action statements. Similarly, a reject-route action returns false 399 and the calling policy evaluation will be affected accordingly. When 400 the end of the subroutine policy statements is reached, the default 401 route disposition action is returned (i.e., boolean false for reject- 402 route). Consequently, a subroutine cannot explicitly accept or 403 reject a route. Rather it merely provides an indication that 'call- 404 policy' condition returns boolean true or false indicating whether or 405 not the condition matches. Route acceptance or rejection is solely 406 determined by the top-level policy. [nit] That's a long and complex paragraph. Consider breaking it up -- maybe along true and false results. [] Just to make sure I understand: Let's assume I have a prefix-set and a neighbor-set, and I want to accept the routes in the prefix-set when received from a neighbor-set. If I have a single policy definition (match-prefix-set and match-neighbor-set under a single policy statement) then it should work as I want it to, right? Assuming that I configure accept-route as the policy-result. OTOH, if I create two policy definitions (one with match-prefix-set and the other with match-neighbor-set), and no actions...and then use call-policy to call both of them in order (prefix and neighbors), then re result will always be a deny, even if I configure acce-route in the top-level policy. If this correct? I hope that made sense. It would be great it you provide an example showing the two different scenarios and their result while using the same prefix/neighbor-sets. 408 Note that the called policy may itself call other policies (subject 409 to implementation limitations). The model does not prescribe a 410 nesting depth because this varies among implementations. For 411 example, an implementation may only support a single level of 412 subroutine recursion. As with any routing policy construction, care 413 must be taken with nested policies to ensure that the effective 414 return value results in the intended behavior. Nested policies are a 415 convenience in many routing policy constructions but creating 416 policies nested beyond a small number of levels (e.g., 2-3) is 417 discouraged. Also, implementations should have validation to ensure 418 that there is no recursion amongst nested routing policies. [major] "should have validation" Why is this not a normative recommendation? s/should/SHOULD Recursion doesn't sound like a good idea, ever! Why would the validation only be recommended and not required? Why SHOULD and not MUST? 420 5. Policy evaluation 422 Evaluation of each policy definition proceeds by evaluating its 423 corresponding individual policy statements in order. When all the 424 condition statements in a policy statement are satisfied, the 425 corresponding action statements are executed. If the actions include 426 either accept-route or reject-route actions, evaluation of the 427 current policy definition stops, and no further policy statement is 428 evaluated. If there are multiple policies in the policy chain, 429 subsequent policies are not evaluated. Policy chains are sequences 430 of policy definitions (described in . (Section 4)). [nit] s/described in . (Section 4)/as described in Section 4 ... 472 7. Security Considerations ... 487 There are a number of data nodes defined in this YANG module that are 488 writable/creatable/deletable (i.e., config true, which is the 489 default). These data nodes may be considered sensitive or vulnerable 490 in some network environments. Write operations (e.g., edit-config) 491 to these data nodes without proper protection can have a negative 492 effect on network operations. These are the subtrees and data nodes 493 and their sensitivity/vulnerability: 495 /routing-policy 497 /routing-policy/defined-sets/prefix-sets 499 /routing-policy/defined-sets/neighbor-sets 501 /routing-policy/defined-sets/tag-sets 503 /routing-policy/policy-definitions [major] It is expected that each data node include a description of the sensitivity or vulnerability. https://trac.ietf.org/trac/ops/wiki/yang-security-guidelines 505 Unauthorized access to any data node of these subtrees can disclose 506 the operational state information of routing policies on this device. [major] This paragraph talks about access to the data, but a more complete coverage is expected: https://trac.ietf.org/trac/ops/wiki/yang-security-guidelines ... 531 9. YANG module [major] The trees shows before represent parts of the module. But there's no place where the whole tree is shown -- as it usually is in most YANG documents (at least it seems to me). Please include a full tree somewhere. 533 The routing policy model is described by the YANG modules in the 534 sections below. [RFC2328], [RFC3101], [RFC5130], and [RFC5302] are 535 referenced here since they are referenced in the YANG model but not 536 elsewhere in this document. [minor] I think that the common practice is to list all the documents reference inside the module, and not just the ones not referenced elsewhere. 538 9.1. Routing policy model ... 587 "WG Web: <http://tools.ietf.org/wg/rtgwg/> [major] s/<...>/<https://datatracker.ietf.org/wg/rtgwg/> 588 WG List: <Email: [email protected]> [minor] s/Email:/mailto:/g ... 599 description 600 "This module describes a YANG model for routing policy 601 configuration. It is a limited subset of all of the policy 602 configuration parameters available in the variety of vendor 603 implementations, but supports widely used constructs for 604 managing how routes are imported, exported, modified and 605 advertised across different routing protocol instances or 606 within a single routing protocol instance. This module is 607 intended to be used in conjunction with routing protocol 608 configuration modules (e.g., BGP) defined in other models. [minor] Does the rest of this description need to be included inside the module? It seems like overkill to me -- not a showstopper. I didn't check if the text matches the rest of the document; if keeping it, please do. ... 668 This version of this YANG module is part of RFC XXXX 669 (https://www.rfc-editor.org/info/rfcXXXX); see the RFC itself 670 for full legal notices. [minor] This paragraph is repeated below. Remove this one. 672 The key words 'MUST', 'MUST NOT', 'REQUIRED', 'SHALL', 'SHALL 673 NOT', 'SHOULD', 'SHOULD NOT', 'RECOMMENDED', 'NOT 674 RECOMMENDED', 'MAY', and 'OPTIONAL' in this document are to be 675 interpreted as described in BCP 14 (RFC 2119) (RFC 8174) when, 676 and only when, they appear in all capitals, as shown here. 678 This version of this YANG module is part of RFC XXXX; 679 see the RFC itself for full legal notices."; ... 733 identity route-level { 734 description 735 "Base identity for route import or export level."; 736 } [minor] This base identity is described for "import or export", but the other identities (below) are only described in terms of importation. Because route-level is only used as an action, then it seems that the "export" part is not applicable. Am I interpreting that correctly? If so, the set-route-level container also includes "export" in the description. ... 907 typedef default-policy-type { 908 type enumeration { 909 enum accept-route { 910 description 911 "Default policy to accept the route."; 912 } 913 enum reject-route { 914 description 915 "Default policy to reject the route."; 916 } 917 } 918 description 919 "Type used to specify route disposition in 920 a policy chain. This typedef retained for 921 name compatibility with default import and 922 export policy."; 923 } [?] "retained"? From where? ... 980 typedef metric-modification-type { 981 type enumeration { 982 enum set-metric { 983 description 984 "Set the metric to the specified value."; 985 } 986 enum add-metric { 987 description 988 "Add the specified value to the existing metric. 989 If the result would overflow the maximum metric 990 (0xffffffff), set the metric to the maximum."; [major] Not all metrics have the same length! How should shorter metric fields be handled? Is that expected to be solved by an augmentation, if/when needed? ... 1009 leaf ip-prefix { 1010 type inet:ip-prefix; 1011 mandatory true; 1012 description 1013 "The IP prefix represented as an IPv6 or IPv4 network 1014 number followed by a prefix length with an intervening 1015 slash character as a delimiter. All members of the prefix 1016 set MUST be of the same address family as the prefix-set 1017 mode."; 1018 } [nit] s/prefix set/prefix-set [major] "MUST be of the same address family" This normative text should be placed in the prefix-sets container because that is where the prefix-sets are defined. Note that the text there is as follows: leaf mode { ... description "Indicates the mode of the prefix set, in terms of which address families (IPv4, IPv6, or both) are present. The mode provides a hint, but the device MUST validate that all prefixes are of the indicated type, and is expected to reject the configuration if there is a discrepancy."; The normative requirements are not the same: "MUST be" vs "MUST validate". The "expected to reject" action is more akin to "MUST be". 1020 leaf mask-length-lower { 1021 type uint8; [minor] Shouldn't a range be defined here too? 1022 description 1023 "Mask length range lower bound. It MUST NOT be less than 1024 the prefix length defined in ip-prefix."; 1025 } 1026 leaf mask-length-upper { 1027 type uint8 { 1028 range "1..128"; 1029 } 1030 must "../mask-length-upper >= ../mask-length-lower" { 1031 error-message "The upper bound MUST NOT be less" 1032 + "than lower bound."; 1033 } [minor] An error message is printed if the mask-length-upper doesn't meet the expectation. But no error related to mask-length-lower. Why? [major] The text in the error message contains a normative requirement that is not specified. IOW, "upper bound MUST NOT be less" just appears in the error message and not as part of the specification: add it to the description field. 1034 description 1035 "Mask length range upper bound. 1037 The combination of mask-length-lower and mask-length-upper 1038 define a range for the mask length, or single 'exact' 1039 length if mask-length-lower and mask-length-upper are 1040 equal. 1042 Example: 192.0.2.0/24 through 192.0.2.0/26 would be 1043 expressed as prefix: 192.0.2.0/24, 1044 mask-length-lower=24, 1045 mask-length-upper=26 1047 Example: 192.0.2.0/24 (an exact match) would be 1048 expressed as prefix: 192.0.2.0/24, 1049 mask-length-lower=24, 1050 mask-length-upper=24"; 1051 } [minor] Please move the explanation about the combination and the example to the description field for the prefix grouping. [] Please also add an IPv6 example. ... 1067 grouping match-set-options-restricted-group { 1068 description 1069 "Grouping for a restricted set of match operation 1070 modifiers."; 1072 leaf match-set-options { 1073 type match-set-options-type { 1074 enum any { 1075 description 1076 "Match is true if given value matches any 1077 member of the defined set."; 1078 } 1079 enum invert { 1080 description 1081 "Match is true if given value does not match 1082 any member of the defined set."; 1083 } 1084 } 1085 description 1086 "Optional parameter that governs the behavior of the 1087 match operation. This leaf only supports matching on 1088 'any' member of the set or 'invert' the match. 1089 Matching on 'all' is not supported."; 1090 } [minor] "Matching on 'all' is not supported." I was wondering about this and noticed that match-set-options-restricted-group is used by match-prefix-set and match-tag-set. I can see how a prefix would probably never match 'all', but why not offer that option for tags? BTW, also realized (from §4.2) that prefix-set and tag-set are the only places where match-set-options is applied. If neither uses 'all' then why even define it? ... 1097 container match-interface { 1098 leaf interface { 1099 type leafref { 1100 path "/if:interfaces/if:interface/if:name"; 1101 } 1102 description 1103 "Reference to a base interface. If a reference to a 1104 subinterface is required, this leaf MUST be specified 1105 to indicate the base interface."; 1106 } 1107 leaf subinterface { 1108 type leafref { 1109 path "/if:interfaces/if:interface/if-ext:encapsulation" 1110 + "/if-flex:flexible/if-flex:match" 1111 + "/if-flex:dot1q-vlan-tagged" 1112 + "/if-flex:outer-tag/if-flex:vlan-id"; 1113 } 1114 description 1115 "Reference to a subinterface -- this requires the base 1116 interface to be specified using the interface leaf in 1117 this container. If only a reference to a base interface 1118 is required, this leaf SHOULD NOT be set."; [major] "SHOULD NOT be set" If set, what should it be set to? When is it ok to set it? Why is this action recommended and not required? ... 1147 container match-prefix-set { 1148 leaf prefix-set { 1149 type leafref { 1150 path "../../../../../../../defined-sets/" + 1151 "prefix-sets/prefix-set/name"; 1152 } 1153 description 1154 "References a defined prefix set."; 1155 } 1156 uses match-set-options-restricted-group; 1158 description 1159 "Match a referenced prefix-set according to the logic 1160 defined in the match-set-options leaf."; 1161 } 1162 } 1164 grouping neighbor-set-condition { 1165 description 1166 "This grouping provides neighbor-set conditions."; 1168 container match-neighbor-set { 1169 leaf neighbor-set { 1170 type leafref { 1171 path "../../../../../../../defined-sets/neighbor-sets/" + 1172 "neighbor-set/name"; 1173 require-instance true; [nit] Is require-instance needed? The default is true, so... This caught my attention because it shows up here, but not under match-prefix-set (above) for example. ... 1351 container neighbor-sets { 1352 description 1353 "Data definition for a list of IPv4 or IPv6 1354 neighbors which can be matched in a routing policy."; [minor] Can this set contain both IPv4/IPv6 neighbors? ... 1681 Appendix A. Routing protocol-specific policies [minor] Please add a reference to this appendix from the main body text. ... 1689 The example below provides an illustration of how another data model 1690 can augment parts of this routing policy data model. It uses 1691 specific examples from draft-ietf-idr-bgp-model-09 to show in a 1692 concrete manner how the different pieces fit together. This example 1693 is not normative with respect to [I-D.ietf-idr-bgp-model]. The model 1694 similarly augments BGP-specific conditions and actions in the 1695 corresponding sections of the routing policy model. In the example 1696 below, the XPath prefix "bp:" specifies import from the ietf-bgp- 1697 policy sub-module and the XPath prefix "bt:" specifies import from 1698 the ietf-bgp-types sub-module [I-D.ietf-idr-bgp-model]. [] If I understand correctly, the tree below was built from the information in I-D.ietf-idr-bgp-model. To avoid potentially running out of sync (i.e. the bgp module deciding to do something else), would it be easier to just point people at that draft, and eliminate this appendix? Just thinking out loud. Even though I-D.ietf-idr-bgp-model is not a Normative reference, we can also hold publication until both are ready. ... 1807 Appendix B. Policy examples [minor] Please add a reference to this appendix from the main body text. 1809 Below we show an example of XML-encoded configuration data using the 1810 routing policy and BGP models to illustrate both how policies are 1811 defined, and also how they can be applied. Note that the XML has 1812 been simplified for readability. [minor] Please describe what the example is about before presenting it: 1814 <config xmlns="urn:ietf:params:xml:ns:netconf:base:1.0"> 1815 <routing-policy 1816 xmlns="urn:ietf:params:xml:ns:yang:ietf-routing-policy"> ... [EoR -27] _______________________________________________ rtgwg mailing list [email protected] https://www.ietf.org/mailman/listinfo/rtgwg
