Create openconfig-bgp-bmp.yang #1343
Conversation
Creating BGP BMP yang OC path definitions
|
No major YANG version changes in commit 3bb5803 |
There was a problem hiding this comment.
Please add a .spec.yml file to ensure our CI tools and website will check and build this model. For example:
https://github.com/openconfig/public/blob/master/release/models/acl/.spec.yml
| reference | ||
| "RFC 7854: BGP Monitoring Protocol"; |
There was a problem hiding this comment.
You can move the RFC reference to the module description:
https://github.com/openconfig/public/pull/1343/files#diff-cfba4613951c6f0bc189f9dcdf9adfa231718e7665adce8dc8a15fb80b2fe01cR23-R25
And change the reference here to the version # of the module
| reference | |
| "RFC 7854: BGP Monitoring Protocol"; | |
| reference | |
| "0.1.0"; |
| import openconfig-bgp { prefix oc-bgp; } | ||
| import openconfig-bgp-types { prefix oc-bgp-types; } | ||
| import openconfig-network-instance { prefix oc-netinst; } | ||
| import openconfig-inet-types { prefix oc-inet; } | ||
| import openconfig-extensions { prefix oc-ext; } | ||
| import openconfig-yang-types { prefix oc-yang; } |
There was a problem hiding this comment.
Run pyang to build the tree and you'll see a series of errors you can correct:
dloher@nettool:~$ python3 -m venv ~/venv
dloher@nettool:~$ source ~/venv/bin/activate
(venv) dloher@nettool:~$ pip install pyang
(venv) dloher@nettool:~$ pyang -f tree -p release/models/*/* > ~/bmp.tree
When the tree looks like what you want, you can add it to the comment in the description for everyone to see for easy review.
Here are the errors so far:
release/models/bgp/openconfig-bgp-bmp.yang:9: warning: imported module "openconfig-bgp" not used
release/models/bgp/openconfig-bgp-bmp.yang:10: warning: imported module "openconfig-bgp-types" not used
release/models/bgp/openconfig-bgp-bmp.yang:97: error: prefix "oc-types" is not defined (reported only once)
release/models/bgp/openconfig-bgp-bmp.yang:281: error: unexpected keyword "type"
release/models/bgp/openconfig-bgp-bmp.yang:330: error: "openconfig-bgp-bmp:stations" in the path for station-names at release/models/bgp/openconfig-bgp-bmp.yang:556 (at release/models/bgp/openconfig-bgp-bmp.yang:328) is not found
release/models/bgp/openconfig-bgp-bmp.yang:343: error: "openconfig-bgp-bmp:stations" in the path for station-names at release/models/bgp/openconfig-bgp-bmp.yang:556 (at release/models/bgp/openconfig-bgp-bmp.yang:341) is not found
release/models/bgp/openconfig-bgp-bmp.yang:381: error: grouping "bmp-afi-safis-top" not found in module "openconfig-bgp-bmp"
release/models/bgp/openconfig-bgp-bmp.yang:446: error: grouping name "bmp-config" is already defined at release/models/bgp/openconfig-bgp-bmp.yang:413
release/models/bgp/openconfig-bgp-bmp.yang:496: error: grouping "bmp-state" not found in module "openconfig-bgp-bmp"
release/models/bgp/openconfig-bgp-bmp.yang:501: error: grouping "bmp-afi-safis-top" not found in module "openconfig-bgp-bmp"
release/models/bgp/openconfig-bgp-bmp.yang:548: error: grouping "bmp-afi-safis-top" not found in module "openconfig-bgp-bmp"
|
|
||
| leaf policy-type { | ||
| type route-monitoring-policy-type; | ||
| default "BOTH"; |
There was a problem hiding this comment.
Recommend removing this default. We should avoid setting defaults unless required by RFC.
| grouping bmp-tcp-keepalive-state { | ||
| description | ||
| "Operational state parameters for BMP TCP keepalive settings"; | ||
|
|
||
| leaf idle-time { | ||
| type oc-types:timeticks64; | ||
| description | ||
| "The time in seconds before sending a TCP keepalive probe | ||
| when the connection is idle"; | ||
| } | ||
|
|
||
| leaf probe-count { | ||
| type uint8; | ||
| description | ||
| "The maximum number of TCP keepalive probes to send before | ||
| declaring the connection dead"; | ||
| } | ||
|
|
||
| leaf probe-interval { | ||
| type oc-types:timeticks64; | ||
| description | ||
| "The time in seconds between TCP keepalive probes"; | ||
| } | ||
| } |
There was a problem hiding this comment.
This grouping is redundant to the -config grouping and should be deleted. OC commonly reuses the config to represent state.
| config false; | ||
| description | ||
| "Operational state parameters for BMP TCP keepalive"; | ||
| uses bmp-tcp-keepalive-state; |
There was a problem hiding this comment.
Let's reuse the config grouping here to follow common OC style
| uses bmp-tcp-keepalive-state; | |
| uses bmp-tcp-keepalive-config; |
|
|
||
| leaf exclude-non-eligible { | ||
| type boolean; | ||
| default false; |
| grouping bmp-route-monitoring-state { | ||
| description | ||
| "Operational state parameters for BMP route monitoring"; | ||
|
|
||
| leaf policy-type { | ||
| type route-monitoring-policy-type; | ||
| description | ||
| "Specifies whether to send pre-policy, post-policy, or both | ||
| types of route monitoring messages"; | ||
| } | ||
|
|
||
| leaf exclude-non-eligible { | ||
| type boolean; | ||
| description | ||
| "When true, exclude routes that are not eligible for | ||
| installation in the routing table from route monitoring | ||
| messages"; | ||
| } | ||
| } |
There was a problem hiding this comment.
as above, this grouping is redundant to -config and can be deleted.
| config false; | ||
| description | ||
| "Operational state parameters for BGP Monitoring Protocol"; | ||
| uses bmp-state; |
There was a problem hiding this comment.
| uses bmp-state; | |
| uses bmp-config; |
| to BMP servers or passively listens for connections"; | ||
| } | ||
|
|
||
| leaf source-address { |
There was a problem hiding this comment.
| leaf source-address { | |
| leaf local-address { |
|
|
||
| leaf enabled { | ||
| type boolean; | ||
| default false; |
| grouping bmp-neighbor-state { | ||
| description | ||
| "Operational state parameters for BMP at the neighbor level"; | ||
|
|
||
| leaf enabled { | ||
| type boolean; | ||
| description | ||
| "When true, BMP monitoring is enabled for this BGP neighbor"; | ||
| } | ||
| } |
| config false; | ||
| description | ||
| "Operational state parameters for BMP at the neighbor level"; | ||
| uses bmp-neighbor-state; |
There was a problem hiding this comment.
| uses bmp-neighbor-state; | |
| uses bmp-neighbor-config; |
There was a problem hiding this comment.
Provided quick first pass - some overall comments
- Suggest looking at and providing reference implementations. There are various parameters not covered in this and would need to expand to accomodate. e.g. https://www.juniper.net/documentation/us/en/software/junos/cli-reference/topics/ref/statement/bmp-edit-routing-options.html
- Consider proper anchor points and NI relationships. Much of the OC modeling is often very loose allowing for combinations that are unrealistic or not supported so best to narrow that in
| "Configuration parameters for BMP TCP keepalive settings"; | ||
|
|
||
| leaf idle-time { | ||
| type oc-types:timeticks64; |
There was a problem hiding this comment.
wrong datatype which is measured as nanoseconds delta
suggest: proper primitive uint type with a units seconds;
| } | ||
|
|
||
| leaf probe-interval { | ||
| type oc-types:timeticks64; |
There was a problem hiding this comment.
wrong datatype which is measured as nanoseconds delta
suggest: proper primitive uint type with a units seconds;
| "List of BMP monitoring stations"; | ||
|
|
||
| leaf name { | ||
| type string; |
There was a problem hiding this comment.
List keys in OC design are a leafref to their child ./config/... node
|
|
||
| leaf port { | ||
| type oc-inet:port-number; | ||
| default 7039; |
There was a problem hiding this comment.
Where is this default coming from? There is no IANA registered port - recommend removing - also sounds like you want this to be mandatory as well then
| } | ||
|
|
||
| leaf uptime { | ||
| type oc-types:timeticks64; |
There was a problem hiding this comment.
wrong datatype which is measured as nanoseconds delta
suggest: proper primitive uint type with a units seconds;
| } | ||
|
|
||
| leaf flap-count { | ||
| type uint32; |
There was a problem hiding this comment.
2 types defined - suggest colocating counters under a proper counters container as you'll see in other domains
| } | ||
|
|
||
| // Augmentations | ||
| augment "/oc-netinst:network-instances/oc-netinst:network-instance/oc-netinst:protocols/oc-netinst:protocol/oc-netinst:bgp/oc-netinst:global" { |
There was a problem hiding this comment.
While obviously tied to BGP, this would be tying it to a BGP protocol instance
Suggest decoupling the BMP global parameters away and also determining the network-instance aspects of stations
| "Type to define the connection mode for BMP sessions"; | ||
| } | ||
|
|
||
| typedef route-monitoring-policy-type { |
There was a problem hiding this comment.
how do we map route-monitoring-policy-type to rib-in and rib-out? In other words, how do we support all 5 RIB views - rib-in-pre-policy, rib-in-post-policy, rib-out-pre-policy, rib-out-post-policy and loc-rib?
| "Type to define the route monitoring policy for BMP"; | ||
| } | ||
|
|
||
| typedef connection-status-type { |
There was a problem hiding this comment.
Can we add other state:
- listening (this would be state if the station is in the passive mode).
- cleanup (this would be when station goes into cleanup phase due to configuration change or socket disconnection)
- holddown
| types of route monitoring messages"; | ||
| } | ||
|
|
||
| leaf exclude-non-eligible { |
There was a problem hiding this comment.
This seems to be applicable for rib-in-post-policy case only. Should there be some conditional statement in this yang model to enforce that?
We need to add below clarification here for this knob:
Routes that have post-policy protocol next-hops that are unresolved or are otherwise unusable are considered to be non-eligible. If using post-policy with exclude-non-eligible, routes that are accepted by import policy but considered non-eligible will be advertised as unreachable.
| } | ||
| } | ||
|
|
||
| grouping bmp-route-monitoring-state { |
There was a problem hiding this comment.
Can we consider another leaf node - "exclude-non-feasible". This is applicable only for rib-in-pre-policy. We should add this condition in yang.
Routes that are received but are hidden are considered to be non-feasible. If using pre-policy exclude-non-feasible, such routes will be advertised by BMP as unreachable. A route could be hidden because one of the following reason:
• suppressed by damping
• cluster loop
• AS confed path loop
• AS path loop
• originator loop
• invalid optional transitive partial attribute
• empty path on external peer
• fails enforce-first-as check
• invalid confed segment
• path received out of the domain
• AS of value zero in AS path
• AS restricted value (4294967295) in AS path
• protocol nexthop is local
• protocol nexthop is not on the interface
• protocol nexthop is IPv6 link local
• protocol nexthop is multicast
• protocol nexthop is martian
• prefix is martian
• protocol nexthop is an unsupported address family
• AS path size overfflow
• Flow-route fails validation
• unsupported label stack or label block
• MPLS not enabled on interface
| } | ||
| } | ||
|
|
||
| container station-groups { |
There was a problem hiding this comment.
What is the use case for this container?
| } | ||
| } | ||
|
|
||
| grouping bmp-config { |
There was a problem hiding this comment.
we need "local-port" as well. This shouldn't be present if the connection mode is active. This should be mandatory for passive connection mode.
We also need station-address and station-port here.
| uses bmp-tcp-keepalive-config; | ||
| } | ||
|
|
||
| grouping bmp-config { |
| description | ||
| "Configuration parameters for a BMP monitoring station"; | ||
|
|
||
| leaf address { |
There was a problem hiding this comment.
"address" is very generic word. Should be renamed to station-address. This can be part of "bmp-config".
| "IP address of the BMP monitoring station"; | ||
| } | ||
|
|
||
| leaf port { |
There was a problem hiding this comment.
"port" is very generic word. Should be renamed to station-port. This can be part of "bmp-config".
| } | ||
| } | ||
|
|
||
| grouping bmp-neighbor-config { |
There was a problem hiding this comment.
we need to be able configure monitoring type (rib-in-pre-policy or/and rib-in-post-policy or/and rib-out-pre-policy or/and rib-out-post-policy) per neighbor or bgp group level.
|
Also want to bring to attention some prior art/IETF modeling as reference/alignment. https://datatracker.ietf.org/doc/html/draft-ietf-grow-bmp-yang-05 |
|
Hi @vvlakshmanamurthy! Looking at the config, |
Creating BGP BMP yang OC path definitions
[Note: Please fill out the following template for your pull request. lines
tagged with "Note" can be removed from the template.]
[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
Platform Implementations
implementation output.
implementation output.
[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].