Re: Benjamin Kaduk's No Objection on draft-ietf-rtgwg-policy-model-30: (with COMMENT)

[Yingzhen Qu <[e-mail]>]

Hi Benjamin,

Thank you for your review and comments. Please see my answers inline.

Thanks,
Yingzhen

> On Aug 9, 2021, at 4:44 PM, Benjamin Kaduk via Datatracker <[e-mail]> wrote:
> 
> Benjamin Kaduk has entered the following ballot position for
> draft-ietf-rtgwg-policy-model-30: No Objection
> 
> When responding, please keep the subject line intact and reply to all
> email addresses included in the To and CC lines. (Feel free to cut this
> introductory paragraph, however.)
> 
> 
> Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
> for more information about DISCUSS and COMMENT positions.
> 
> 
> The document, along with other ballot positions, can be found here:
> https://datatracker.ietf.org/doc/draft-ietf-rtgwg-policy-model/
> 
> 
> 
> ----------------------------------------------------------------------
> COMMENT:
> ----------------------------------------------------------------------
> 
> Section 4.2
> 
>   Comparison conditions may similarly use options to change how route
>   attributes should be tested, e.g., for equality or inequality,
>   against a given value.
> 
> I'm not sure if this is stale text, or if still current, which
> operations it refers to (e.g., where is an "inequality" option
> offered?).  Perhaps it is referring to the potential behavior of future
> augmentations?
> 
[Yingzhen]: You’re right, this is referring to the potential behavior of comparison, just like “match-set-options”.

> Section 4.4
> 
>   Note that the called policy may itself call other policies (subject
>   to implementation limitations).  The model does not prescribe a
>   nesting depth because this varies among implementations.  For
>   example, an implementation may only support a single level of
>   subroutine recursion.  [...]
> 
> Is there a useful way to programmatically determine the nesting limit of
> a given implementation, or is this more of just a "read the
> documentation" thing?

[Yingzhen]: this is implementation specific. Usually it’s not a good idea to nest many levels anyway since it’s error prone.

> 
> Section 5
> 
>   Whether or not the route's pre-policy attributes are used for testing
>   policy statement conditions is dependent on the implementation
>   specific value of the match-modified-attributes leaf.  If match-
>   modified-attributes is false and actions modify route attributes,
>   these modifications are not used for policy statement conditions.
>   Conversely, if match-modified-attributes is true and actions modify
>   the policy application-specific attributes, the attributes as
>   modified by the policy are used for policy condition statements.
> 
> Are these "actions" in question only for the current policy statement,
> all policy statements in the current policy definition, or all policy
> definitions?  (I see on second read that this is an "ro" node at
> effectively global scope, and so likely to be at the broadest "all
> policy statements" scope, but I would still welcome some clarification.

[Yingzhen]: It is meant for all policy statements. 
> 
> Section 7.2
> 
>      enum  add-metric {
>        description
>          "Add the specified value to the existing metric.
>           If the result would overflow the maximum metric
>           (0xffffffff), set the metric to the maximum.";
>      }
> 
> Is the parenthetical always fully generic?  I seem to recall that, e.g.,
> the Babel YANG model uses a 16-bit field for the metric.
> 
[Yingzhen]: Protocols have different metric lengths. Here the maximum length is used for simplicity, and the implementation should use the right length.

>    container apply-policy {
> 
> It slightly surprised me that the import-policy and export-policy lists
> both required an instance, but I am rather distanced from what is
> actually done in practice, and it doesn't seem too hard to put in a
> "reject everything" policy if needed.  OTOH, there are *also* separate
> default-{import,export} policy leaves, that do have a default reject, so
> the need to explicitly set at least one policy in both direction may be
> overkill.

[Yingzhen]: “require-instance” here means the policy-definition being referred to needs to exist, but there is no “min-elements” required for the leaf-list, so it can be empty. Hope this clarifies.
> 
>          leaf-list tag-value {
>            type tag-type;
>            description
>              "Value of the tag set member.";
> 
> The tag-type is a union of uint32 and hex-string; does the notion of
> "equivalence" for matching tags do any type conversion between those?
> If not, a warning that the value set here must be of the correct type to
> match the received (or generated?) route might be in order.

[Yingzhen]: This is a common practice in YANG. A union is chosen because there are cases one of the types in the union is used. Implementations are expected to figure out the right type. So I don’t think a warning is necessary.
> 
>              container match-interface {
>                leaf interface {
>                  type leafref {
>                    path "/if:interfaces/if:interface/if:name";
> 
> Is it always the case that the interface over which we receive a route
> and the interface that traffic on that route is sent will be the same?
> Or do we have to worry about potential skew between the two ways that
> the interface comes into play?

[Yingzhen]: It’s not always true that the interface received a route is the same as traffic received. Routing policy or policy based routing can be tricky and has to be designed carefully to avoid creating loops.
> 
>              container match-neighbor-set {
> 
> I don't understand why there's no "match-set-options" analogue for
> match-neighbor set.  Does the neighbor set use "ANY" matching semantics,
> then?
> 
[Yingzhen]: This is actually debatable and was brought up before. The authors decided to not have an option, and the match “ALL” option obviously doesn’t make sense for neighbors.

>              leaf set-route-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.";
> 
> Thank you for including the last sentence!
> 
> Section 8
> 
> We should probably mention the apply-policy grouping as being
> security-relevant (and how).

[Yingzhen]: We leave this to the model using the grouping.
> 
> Section 11.1
> 
> Since RFCs 6241 and 8040 are only referenced from the security
> considerations boilerplate (as examples of how YANG modules can be
> used), they may not need to be classified as normative.
> 
[Yingzhen]: we’re following other published IETF models. Unless you have a strong opinion, I’m going to leave as it is.

> Appendix A,B
> 
> I didn't do much of a review here, since I'd need to understand
> draft-ietf-idr-bgp-model in order to have very much useful to say, and
> any such comments would accordingly be better directed at that draft.
> 
> NITS
> 
> Section 4.4
> 
> As an editorial matter, I'd consider adding a statement along the lines
> of "when call-policy elements are present, a given policy statement
> matches if all called policies return accept-route and all the other
> conditions in the policy statement also match".  There don't seem to be
> any other ways to read the current text that are reasonable, but it took
> me a bit of thinking to work it out.

[Yingzhen]: I’ll leave this to RFC editor.
> 
> Section 7.2
> 
>  identity isis-level-1-2 {
>    base route-level;
>    description
>      "Identity for IS-IS Level 1 and Level 2 area importation. It
>       is only applicable to routes imported into the IS-IS
>       protocol.";
> 
> The "importation into both" phrasing from ospf-normal-nssa reads much
> more clearly to me than what's here.

[Yingzhen]: how about I change it to “Identity for ISIS importation into both Level 1 and Level 2 areas.”? Will change it in the next version.
> 
>          container prefixes {
>            description
>              "Container for the list of prefixes in a policy
>               prefix list. Since individual prefixes do not have
>               unique actions, the order in which the prefix in
>               prefix-list are matched has no impact on the outcome
>               and is left to the implementation. A given prefix-set
>               condition is satisfied if the input prefix matches
>               any of the prefixes in the prefix-set.";
> 
> The last sentence seems like it could be removed, since the
> match-set-options leaf is what actually specifies the matching behavior
> (and is not always "any").

[Yingzhen]: I think this is fine. For prefix-set, "match-set-options-restricted-group” is used.
> 
>              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.";
> 
> This seems to make more sense if I apply s/implicitly/explicitly/.

[Yingzhen]: This is intentional. Application-tags don’t get advertised automatically/implicitly.
> 
> Section 8
> 
>      /routing-policy/defined-sets/prefix-sets -- Knowledge of these
>      data nodes can be used to ascertain which local prefixes are
>      susceptible to a Denial-of-Service (DoS) attack.
> 
>      /routing-policy/defined-sets/prefix-sets -- Knowledge of these
>      data nodes can be used to ascertain local neighbors against whom
>      to mount a Denial-of-Service (DoS) attack.
> 
> The second one reads like it should be "neighbor-set" rather than
> "prefix-sets".
> 
>      /routing-policy/policy-definitions/policy-definition /statements/
> 
> Spurious space.
> 
[Yingzhen]: Thanks for catching these. I’ll fix them in the next version.
> 
>