Skip to content

Add prefix-set description and prefix entry sequence/action - #1524

Open
Verma-Anukul wants to merge 1 commit into
openconfig:masterfrom
Verma-Anukul:prefix-list
Open

Add prefix-set description and prefix entry sequence/action#1524
Verma-Anukul wants to merge 1 commit into
openconfig:masterfrom
Verma-Anukul:prefix-list

Conversation

@Verma-Anukul

Copy link
Copy Markdown
Contributor

Extend openconfig-routing-policy for BGP-style prefix lists: optional prefix-set description and per-entry sequence-number and permit/deny action while retaining ip-prefix and masklength-range as list keys.

[Note: Please fill out the following template for your pull request. Replace
all the text in [] with your own content.]

[Note: Before this PR can be reviewed please agree to the CLA covering this
repo. Please also review the contribution guide -
https://github.com/openconfig/public/blob/master/doc/contributions-guide.md]

Change Scope

  • [Please briefly describe the change that is being made to the models.]
  • [Please indicate whether this change is backwards compatible.]

Platform Implementations

[Note: Please provide at least two references to implementations which are relevant to the model changes proposed. Each implementation should be from separate organizations.].

[Note: If the feature being proposed is new - and something that is being
proposed as an enhancement to device functionality, it is sufficient to have
reviewers from the producers of two different implementations].

Tree View

  • [Please provide a view of the tree being modified. It's preferred if a diff format is used for ease of review.]
  • [Here are recommended steps to generate the tree view as a diff]
git checkout mychangebranch
pyang -f tree -p release/models/*/* > ~/new-tree.txt 
git checkout master
git pull
pyang -f tree -p release/models/*/* > ~/old-tree.txt
diff -bU 100 ~/old-tree.txt ~/new-tree.txt   | less

[Next, cut and paste the relevant portion of the tree with enough context for reviewers to quickly understand the change.]

 module: openconfig-interfaces
   +--rw interfaces
      +--rw interface* [name]
         +--ro state
         |  +--ro counters
         |  |  +--ro in-octets?               oc-yang:counter64
         |  |  +--ro in-pkts?                 oc-yang:counter64
         |  |  +--ro in-unicast-pkts?         oc-yang:counter64
         |  |  +--ro in-broadcast-pkts?       oc-yang:counter64
         |  |  +--ro in-multicast-pkts?       oc-yang:counter64
         |  |  +--ro in-errors?               oc-yang:counter64
         |  |  +--ro in-discards?             oc-yang:counter64
         |  |  +--ro out-octets?              oc-yang:counter64
         |  |  +--ro out-pkts?                oc-yang:counter64
         |  |  +--ro out-unicast-pkts?        oc-yang:counter64
         |  |  +--ro out-broadcast-pkts?      oc-yang:counter64
         |  |  +--ro out-multicast-pkts?      oc-yang:counter64
         |  |  +--ro out-discards?            oc-yang:counter64
         |  |  +--ro out-errors?              oc-yang:counter64
         |  |  +--ro last-clear?              oc-types:timeticks64
         |  |  +--ro in-unknown-protos?       oc-yang:counter64
         |  |  +--ro in-fcs-errors?           oc-yang:counter64
+        |  |  x--ro carrier-transitions?     oc-yang:counter64
-        |  |  +--ro carrier-transitions?     oc-yang:counter64
+        |  |  +--ro interface-transitions?   oc-yang:counter64
+        |  |  +--ro link-transitions?        oc-yang:counter64
         |  |  +--ro resets?                oc-yang:counter64

Extend openconfig-routing-policy for BGP-style prefix lists: optional
prefix-set description and per-entry sequence-number and permit/deny
action while retaining ip-prefix and masklength-range as list keys.

Signed-off-by: Verma-Anukul <anukulverma2013@gmail.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the openconfig-routing-policy YANG module to version 3.6.0, adding a description leaf to prefix sets, and introducing sequence numbers and permit/deny actions to prefix entries to support ordered BGP-style prefix lists. The review feedback highlights potential ambiguities in the evaluation semantics of these new fields. Specifically, it recommends clarifying how the action leaf affects the evaluation of match-prefix-set conditions (especially for DENY actions) and defining the expected behavior when optional sequence-number values are omitted.

Comment on lines +391 to +396
leaf action {
type prefix-set-prefix-action-type;
description
"Action applied to routes matching this prefix entry within
the prefix set.";
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Adding action (PERMIT/DENY) and sequence-number to prefix-set entries introduces a major ambiguity in how match-prefix-set conditions are evaluated.

Historically, a prefix-set was a simple unordered set of prefixes used for matching (i.e., match-prefix-set returns true if the route's prefix matches any prefix in the set). With the introduction of action (PERMIT/DENY) and sequence-number (ordering):

  1. Evaluation Semantics: How does match-prefix-set behave when a route matches a DENY entry? Does the match-prefix-set condition return false (not matched), or does it stop evaluation of the entire policy statement?
  2. Interaction with Match Options: How does this interact with match-set-options (e.g., INVERT)?
  3. Default Behavior: What is the behavior if a route does not match any entry in the prefix-set? (Usually, standard prefix lists have an implicit deny at the end, but in OpenConfig, a non-match in a condition simply means the condition is not met, and evaluation proceeds to the next statement or policy).

To ensure consistent implementation across different platforms, the description of the action leaf (or the module's general description) must explicitly define the evaluation semantics of these new fields within the context of routing policy conditions.

    leaf action {
      type prefix-set-prefix-action-type;
      description
        "Action applied to routes matching this prefix entry within
         the prefix set. If the action is PERMIT, a match on this entry
         causes the match-prefix-set condition to evaluate to true. If
         the action is DENY, a match on this entry causes the
         match-prefix-set condition to evaluate to false, terminating
         further evaluation of the prefix-set.";
    }

Comment on lines +378 to +389
leaf sequence-number {
type uint32 {
range "1..4294967295";
}
description
"Sequence number for the prefix entry within the prefix set.
Prefix list implementations evaluate entries in ascending
sequence order. The list key for prefix entries remains
ip-prefix and masklength-range; sequence-number is an
additional attribute used by implementations that require
explicit ordering or permit/deny semantics per entry.";
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Since sequence-number is an optional leaf, the model should clarify the expected behavior when it is omitted for some or all entries in a prefix-set.

For example:

  • Are entries without a sequence number evaluated in a system-defined order, or is their evaluation order undefined?
  • Does the system automatically assign sequence numbers to entries that lack them?
  • Is it invalid to mix entries with and without sequence numbers in the same prefix-set?

Specifying this behavior in the description will prevent vendor-specific discrepancies.

    leaf sequence-number {
      type uint32 {
        range "1..4294967295";
      }
      description
        "Sequence number for the prefix entry within the prefix set.
         Prefix list implementations evaluate entries in ascending
         sequence order. If sequence numbers are omitted, the evaluation
         order of those entries is system-dependent. The list key for
         prefix entries remains ip-prefix and masklength-range;
         sequence-number is an additional attribute used by
         implementations that require explicit ordering or permit/deny
         semantics per entry.";
    }

@akalluru1

Copy link
Copy Markdown
Contributor

Please update the PR description to accurately reflect the YANG paths & tree output that this PR is adding.

prefix-set & prefix-list are two different constructs with different objectives. It is not correct to retrofit prefix-list attributes into prefix-set model.

Below description explains that it is a set of prefixes that can be referenced in a routing-policy match conditions:

/routing-policy/defined-sets/prefix-sets/prefix-set/config/
name
description:
name / label of the prefix set -- this is used to reference the set in match conditions

nodetype: leaf (rw)

type: string

sequence-number implies an order on which prefixes need to be parsed & it doesn't make sense to have it as part of prefix-set which doesn't have an order.

action leaf doesn't fit with prefix-set object too as the prefix-sets are not referenced in any OC model, apart from policy-defintion, directly to permit/deny prefixes.

Also, the prefix-set action action interacts with /routing-policy/policy-definitions/policy-defintion/statements/statement/actions/config/policy-result is ambiguous.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

2 participants