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 <[email protected]> > 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. > > _______________________________________________ rtgwg mailing list [email protected] https://www.ietf.org/mailman/listinfo/rtgwg
