Subject: Re: AD Review of draft-ietf-rtgwg-policy-model-27

From: Yingzhen Qu <[email-masked]>
Date: Mon, 7 Jun 2021 11:23:46 -0700

Hi Alvaro,

Thank you for your thorough review, really appreciate.

We’ve published a new version to address your comments. Please see inline below for detailed answers.

Thanks,
Yingzhen

> On May 27, 2021, at 12:34 PM, Alvaro Retana <[email-masked]> wrote:
> 
> 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.
> 
[Yingzhen]: moved the module, also added the complete tree. Here is a snapshot:
   7.  YANG Module and Tree  . . . . . . . . . . . . . . . . . . . .  11
     7.1.  Routing Policy Model Tree . . . . . . . . . . . . . . . .  11
     7.2.  Routing policy model  . . . . . . . . . . . . . . . . . .  12
   8.  Security Considerations . . . . . . . . . . . . . . . . . . .  33
   9.  IANA Considerations . . . . . . . . . . . . . . . . . . . . .  35
> 
> ...
> 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.
> 
> 
[Yingzhen]: The augmentation in I-D.ietf-idr-bgp-model is a real augmentation of the policy model.
We don’t want to create a circular dependency on BGP model, so Appendix A is really an example.
We added some text for clarification.
From section 3:
   o  A structure that allows routing protocol models to add protocol-
      specific policy conditions and actions though YANG augmentations
      is also provided.  There is a complete example of this for BGP
      [RFC4271] policies in the proposed vendor-neutral BGP data model
      [I-D.ietf-idr-bgp-model].  Appendix A provides an example of how
      an augmentation for BGP policies might be accomplished.  Note that
      this section is not normative as the BGP model is still evolving.

> ...
> 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
> 
> 
[Yingzhen]: fixed.

> ...
> 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.
> 
> 
[Yingzhen]: We added a default value to reject-route, also added text to clarify in section 4.2.
              leaf policy-result {
                type policy-result-type;
                default reject-route;
                description
                  "Select the final disposition for the route,
                   either accept or reject.";
              }
> ...
> 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.
> 
[Yingzhen]: added description in the model, consistent with RFC 8349.
              leaf set-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.";
              }
> 
> 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?
> 
[Yingzhen]: added description. Hope this clarifies. 
              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.";
              }
> 
> 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.
> 
[Yingzhen]: We modified the text, hope it’s clear now. Regarding your 
example, your understanding is correct. Since you don’t have actions configured 
in the match-prefix-set and match-neighbor-set, and the default action is to
reject-route, both these policies return False, so your top-level policy won’t 
match anything.

> 
> 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?
> 
[Yingzhen]: Changed to "implementations MUST validate
        to ensure that there is no recursion amongst nested routing policies.”.

> 
> 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
> 
[Yingzhen]: fixed.

> ...
> 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
> 
[Yingzhen]: added required descriptions. Please see the updated version for details.

> 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
> 
> 
[Yingzhen]: added descriptions.

> ...
> 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.
> 
> 
[Yingzhen]: added the complete tree.

> 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.
> 
[Yingzhen]: fixed. There are still a couple fo references to be published as RFCs (e.g. sub-interface module), will need to modify this again later.
> 
> 538	9.1.  Routing policy model
> ...
> 587	       "WG Web:   <http://tools.ietf.org/wg/rtgwg/>
> 
> [major] s/<...>/<https://datatracker.ietf.org/wg/rtgwg/>
> 
[Yingzhen]: fixed
> 
> 588	        WG List:  <Email: [email-masked]>
> 
> [minor] s/Email:/mailto:/g
> 
[Yingzhen]: fixed.

> ...
> 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.
> 
[Yingzhen]: fixed.
> 
> 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.
> 
[Yingzhen]: changed to import only. 
  identity route-level {
    description
      "Base identity for route import level.";
  }

> ...
> 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?
> 
[Yingzhen]: fixed.
   description
      "Type used to specify route disposition in
       a policy chain. This typedef is used in
       the default import and export policy.";
  }

> 
> ...
> 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?
> 
[Yingzhen]: if a metric is shorter, say only 0xff, and a value is set out of range,
the implementation should reject it. It’s set to maximum here for simplicity.

> ...
> 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
> 
[Yingzhen]: fixed.

> [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”.
> 
> 
[Yingzhen]: I modified description of “leaf mode” to match the description of “ip-prefix”.
            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, all prefixes MUST
               be of the indicated type. The device MUST validate
               that all prefixes and reject the configuration if
               there is a discrepancy.";
          }

> 1020	       leaf mask-length-lower {
> 1021	         type uint8;
> 
> [minor] Shouldn't a range be defined here too?
> 
[Yingzhen]: added a range
> 
> 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?
> 
[Yingzhen]: the prefix length defined as part of ip-prefix, so it’s not so easy to
define a “must” statement. It can be done in implementation after parsing ip-prefix.
> 
> [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.

[Yingzhen]: added in description.
leaf mask-length-upper {
      type uint8 {
        range "1..128";
      }
      must "../mask-length-upper >= ../mask-length-lower" {
        error-message "The upper bound MUST NOT be less"
                    + "than lower bound.";
      }
      description
        "Mask length range upper bound. It MUST NOT be less than
         lower bound.";
    }
> 
> 
> 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.
> 
[Yingzhen]: moved the description and added 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?
> 
[Yingzhen]: I changed “match-tag-set” to use “match-set-options-group”, so it could use “all” option.
  grouping tag-set-condition {
    description
      "This grouping provides tag-set conditions.";

    container match-tag-set {
      leaf tag-set {
        type leafref {
          path  "../../../../../../../defined-sets/tag-sets" +
          "/tag-set/name";
          require-instance true;
        }
        description
          "References a defined tag set.";
      }
      uses match-set-options-group;  <---

      description
        "Match a referenced tag set according to the logic defined
         in the match-options-set leaf.";
    }
  }
> 
> ...
> 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?
> 
[Yingzhen]: changed to “MUST NOT”.
        description
          "Reference to a subinterface -- this requires the base
           interface to be specified using the interface leaf in
           this container.  If only a reference to a base interface
           is required, this leaf MUST NOT be set.";
> 
> ...
> 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.
> 
[Yingzhen]: This means the neighbor has to be there. But for prefix, it’s possible to define 
prefix-set without those prefixes really configured.

> ...
> 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?
> 
> 
[Yingzhen]: Type of “address” is “ip-address”, it can be either IPv4 or IPv6. Since it’s a 
leaf-list, it can contain both IPv4 and 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.
> 
[Yingzhen]: clarified above.
> ...
> 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:

[Yingzhen]: added some descriptions, also added IPv6 example.
> 
> 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]

---

From: Alvaro Retana <[email-masked]>
Date: Mon, 7 Jun 2021 15:43:37 -0700

On June 7, 2021 at 2:23:50 PM, Yingzhen Qu wrote:

Yingzhen:

Hi!

...
> below for detailed answers.

I have a couple of minor comments below.   I'm starting the IETF LC.

Thanks!!

Alvaro.

...
> [Yingzhen]: added description in the model, consistent with RFC 8349.

Ahhh, that's where it comes from.  Why not call it "route-preference"?

...
> [Yingzhen]: added description. Hope this clarifies.

Hmmm..so, is that a local thing?  If it is not advertised...

The name "application" is what's throwing me off.

...
> > [nit] s/described in . (Section 4)/as described in Section 4

s/in . (Section 4)/in Section 4

...
> [Yingzhen]: if a metric is shorter, say only 0xff, and a value is set out of
> range, the implementation should reject it. It’s set to maximum here for
> simplicity.

Sure...but that is not what the text says.  The text is only specific
about 0xffffffff.  I think that if you take "(0xffffffff)" then it
would be a general statement.

---

From: Yingzhen Qu <[email-masked]>
Date: Mon, 7 Jun 2021 16:49:34 -0700

Hi Alvaro,

Thanks for the review. Considering these are minor comments, if you don’t mind I’ll make the changes in next revision with other comments received later.

Please see my answers inline.

Thanks,
Yingzhen

> On Jun 7, 2021, at 3:43 PM, Alvaro Retana <[email-masked]> wrote:
> 
> Ahhh, that's where it comes from.  Why not call it "route-preference”?
> 
[Yingzhen]: we can change the leaf to “set-route-preference”. 
> s/in . (Section 4)/in Section 4
[Yingzhen]: will fix this.
> would be a general statement.
[Yingzhen]: will add some description for shorter range.