public icon indicating copy to clipboard operation
public copied to clipboard

enabling support for multiple protocol instances in redistribution

Open rszarecki opened this issue 1 year ago • 12 comments

Enable support for source and desination instance of respective protocol.

  1. DEPRECATE /network-instances/network-instance/tables/ and /network-instances/network-instance/table-connections/. Hence this change is breaking.
  2. add new /network-instances/network-instance/route-redistributions. It is similar in concepts and structure to deprecated table-connections, but:
  • alows to specify source and destination instance of protocol. This brings parity to protocol configuration when it is possible to define multiple instances of the same protocol, differentiated by instance name.
  • has more intuitive container name
  • the 5-tuple KEY for route-redistribution is reference to protocols idenitier and name. This simplify model.
module: openconfig-network-instance
  +--rw network-instances
     +--rw network-instance* [name]
        +--rw route-redistributions
           +--rw route-redistribution* [src-protocol src-name dst-protocol dst-name address-family]
              +--rw src-protocol         -> ../config/src-protocol
              +--rw src-instance-name?   -> ../config/src-instance-name
              +--rw dst-protocol?        -> ../config/dst-protocol
              +--rw dst-instance-name?   -> ../config/dst-instance-name
              +--rw address-family?      -> ../config/address-family
              +--rw config
              |  +--rw src-protocol?                 -> ../../../../protocols/protocol/config/identifier
              |  +--rw src-instance-name?            -> ../../../../protocols/protocol/config/name
              |  +--rw address-family?               -> ../../../../protocols/protocol[identifier=current()/../src-protocol]/config/address-family
              |  +--rw dst-protocol?                 -> ../../../../protocols/protocol/config/identifier
              |  +--rw dst-instance-name?            -> ../../../../protocols/protocol/config/name
              |  +--rw disable-metric-propagation?   boolean
              |  +--rw import-policy*                -> /oc-rpol:routing-policy/policy-definitions/policy-definition/name
              |  +--rw default-import-policy?        default-policy-type
              +--ro state
                 +--ro src-protocol?                 -> ../../../../protocols/protocol/config/identifier
                 +--ro src-instance-name?            -> ../../../../protocols/protocol/config/name
                 +--ro address-family?               -> ../../../../protocols/protocol[identifier=current()/../src-protocol]/config/address-family
                 +--ro dst-protocol?                 -> ../../../../protocols/protocol/config/identifier
                 +--ro dst-instance-name?            -> ../../../../protocols/protocol/config/name
                 +--ro disable-metric-propagation?   boolean
                 +--ro import-policy*                -> /oc-rpol:routing-policy/policy-definitions/policy-definition/name
                 +--ro default-import-policy?        default-policy-type

Cisco XR

router bgp 12345 instance FOO
   net 00.0000.0000.12a5.00
   address-family ipv4 unicast
    metric-style wide
    redistribute isis BAAR level 2
!
router isis BAR
   ...

rszarecki avatar May 25 '24 00:05 rszarecki

Instead of refactoring tables and table-connections, how about deprecating both of those completely and introducing a new tree /network-instances/network-instance/protocol-redistribution

It could look like that you have proposed. I would say since we are using src-instance-name, we no longer need the protocol leaf. A protocols/protocol/config/name will only be one protocol. (no longer a table concept with multiple protocols possible). ie:

  +--rw network-instances
     +--rw network-instance* [name]
        +--rw protocol-redistributions
           +--rw protocol-redistribution* [src-instance-name  dst-instance-name address-family]
              +--rw src-instance-name?   -> ../config/src-instance-name
              +--rw dst-instance-name?   -> ../config/dst-instance-name
              +--rw address-family?      -> ../config/address-family
              +--rw config
              |  +--rw src-instance-name?            -> ../../../../protocols/protocol/config/name
              |  +--rw dst-instance-name?            -> ../../../../protocols/protocol/config/name
              |  +--rw address-family?               -> ../../../../protocols/protocol[identifier=current()/../src-protocol]/config/address-family
              |  [...]
              +--ro state
                 +--ro src-instance-name?            -> ../../../../protocols/protocol/config/name
                 +--ro dst-instance-name?            -> ../../../../protocols/protocol/config/name
                 +--ro address-family?               -> ../../../../protocols/protocol[identifier=current()/../src-protocol]/config/address-family
                 [...]

WDYT?

dplore avatar May 25 '24 00:05 dplore

@dplore

  1. from my point of view, we can deprecate /network-instances/network-instance/tables/ completly.
  2. we need both src/dst-protocol and src/dst-instance-name. This is because instance name is free-text-string and can be same for multiple protocols. For example: ( src-protocol=BGP, src-instance-name="default", dst-protocol=ISIS, src-instance-name="default").
  3. "table-connections" --> "protocol-redistributions" (or route-redistributions); This would be breaking change. I hope if we keep table-connections AND allow inertpetate lack of instance-name as "the only one existing" (see description), we may be backward compatible.

rszarecki avatar May 25 '24 03:05 rszarecki

@dplore

  1. from my point of view, we can deprecate /network-instances/network-instance/tables/ completly.

+1 on deprecating /network-instances/network-instance/tables.

As configuration objects they serve no purpose other than anchoring the endpoints of a table connection, and surely there are simpler ways to accomplish this, especially in light of trying to redistribute routes of a particular protocol instance.

If the configuration of these tables is supposed to be added automatically by the system (and protected from delete), what happens when there is a replace-from-root type operation?

If the tables were state only I would have less objection, but that breaks their utility as a leaf-ref target and takes us back to square one of just using a simpler way to define a table connection.

nokia1adam avatar May 30 '24 01:05 nokia1adam

In this proposal, table-connections KEYs are leaf-ref to '/protocol/protocols/' directly. Not to fables as it was previously.

rszarecki avatar Jun 01 '24 00:06 rszarecki

/gcbrun

wenovus avatar Jun 25 '24 21:06 wenovus

No major YANG version changes in commit d74af9c79860d2d95b7179ab21496e46409a25a5

OpenConfigBot avatar Jun 25 '24 21:06 OpenConfigBot

I refactored it quite heavly, following off-line discussion. Please see PR description.

rszarecki avatar Jul 18 '24 15:07 rszarecki

This does support the operational use case to redistribute routes between two network instances, which the current model does not (at least, not as far I can determine). So I am in favor of making this change.

I'd like to ask the community for more feedback. Adding this to the November 2024 openconfig community meeting.

dplore avatar Oct 15 '24 16:10 dplore

/gcbrun

dplore avatar Oct 15 '24 17:10 dplore

This does support the operational use case to redistribute routes between two network instances, which the current model does not (at least, not as far I can determine). So I am in favor of making this change.

I'd like to ask the community for more feedback. Adding this to the November 2024 openconfig community meeting.

Correction: This does support the operational use case to redistribute routes between two protocol instances (of potentially the same protocol like BGP) within the same network-instance.

However main use case is to support cases when protocol instance name is different than "DEFAULT".

rszarecki avatar Oct 15 '24 19:10 rszarecki

This was presented at the Nov 7, 2024 OC Community meeting to create more visibility. We'd like to see comments from the community on this proposal.

dplore avatar Nov 08 '24 17:11 dplore

/gcbrun

dplore avatar Nov 08 '24 17:11 dplore