Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

network-instance protocol identifier/name effects on other domains #287

Closed
exa-arrcus opened this issue Sep 10, 2019 · 5 comments
Closed
Labels

Comments

@exa-arrcus
Copy link

The protocol list defined at /network-instances/network-instance/protocols/protocol has 2 keys:

  • An identifier to represent the protocol - oc-pol-types:INSTALL_PROTOCOL_TYPE (e.g. ISIS, BGP, STATIC, etc..)
  • A name to represent a unique "name" for the protocol instance which could be a separate process name, etc..

Now, the model allows for and if an implementation permits that > 1 protocol instance can run within the same network-instance.

e.g.

  • /network-instances/network-instance[name='default']/protocols/protocol[identifier='oc-pol-types:ISIS' and name='p1']
  • /network-instances/network-instance[name='default']/protocols/protocol[identifier='oc-pol-types:ISIS' and name='p2']

This might mean that you have 2 processes of ISIS running in the same NI, each completely separate from each other.

Now, when we look to things such as:

  • table-connections
  • RPOL match conditions install-protocol-eq

There is no level of granularity down to the protocol name but only the INSTALL_PROTOCOL_TYPE. If NI protocols support multiple "instances" then other domains need to be adjusted to be able to handle this precision as matching on the install protocol only does not suffice

@aashaikh
Copy link
Contributor

aashaikh commented Oct 1, 2019

Thanks for pointing this out -- it's indeed a gap in the current model. After discussion in the group, we'll need to think a bit more about the impact of this change to model compatibility (it did not seem to be a common use case).

@missaesasaya
Copy link
Contributor

Hi,

I also ran into the same issue. For example, if you take Cisco redistribute command for OSPF:

https://developer.cisco.com/docs/openconfig-yang-release-9-2x/#ocni-bgp/configuring-the-redistribution-of-ospf-routes21

router bgp 100
router-id 1.1.1.1
address-family ipv4 unicast
redistribute ospf 10 route-map policy1

The table-connections equivalent does not mention bgp AS (100) or the OSPF ID (10). In my current project, we only have one instance of BGP, but there are more instances of OSPF. It would be nice if we could have the name key in tables and table-connections to be able to reflect this.

@rszarecki
Copy link
Contributor

rszarecki commented May 24, 2024

@dplore @aashaikh

I see 2 issues with current "table-connections" redistribution model, if we assume existence of multiple instances of given protocol.

  1. Two instances of src-protocol may have routes to same destination prefix but with differnet next-hop and other attributes. Both best and active under ECMP hence eligable to be redistributed. Similary operator may wich to redistribute route only if it comes form certain instance. The current model leave decission which one to redistribute to policy. The policy .../conditions/match-protocol-instance/config/protocol-name allows to select instance of src-protocol. But existence of policy .../conditions/match-protocol-instance/config/protocol-identifier in policy makes this bit redundant and ambigous. Not intuaitive for sure.
  2. There could be 2 or more instanced of destination protocol. In current model it is not possible to which instance route shall be inserted.

Finally entire container /network-instances/network-instance/tables/ provide zero value.

  • It is list of composit key w/o any value assotiated.
  • The entries are auto-created wne protocol is configured, and destroyed when protocol is disabled/removed.
  • it define one table for all (multiple) instances of fiven protocol (idenitifier).

Here is quick proposal:

  1. change /network-instances/network-instance/tables/ so it become read-only and make tables per protocol instance.
  +--rw network-instances
     +--rw network-instance* [name]
        +--rw tables
           +--rw table* [protocol instance-name address-family]
              +--rw protocol          -> ../state/protocol
              +--rw address-family    -> ../state/address-family
              +--rw instance-name     -> ../state/instance-name
              +--ro state
                 +--ro protocol?         -> ../../../../protocols/protocol/config/identifier
                 +--ro instance-name?    -> ../../../../protocols/protocol/config/name
                 +--ro address-family?   identityref

I still do not see much value, but ...
2. Extent table-connections to include dst-instance-name and src-instance-name. While doing so define config leafs of keys references to .../protocols/protocol/config rather then to .../tables/table/config as it is today

  +--rw network-instances
     +--rw network-instance* [name]
        +--rw table-connections
           +--rw table-connection* [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
              |  [...]
              +--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
                 [...]

I personally do not like string "table-connections". Network engineers would rather understand "redistribition", "route exporting". But "table-connections" sounds odd and ambigous with forwarding table chaining. I keep "table-connections" because this make change non-breaking.

@dplore
Copy link
Member

dplore commented May 25, 2024

See #1114 for suggested update

Copy link

This issue is stale because it has been open 180 days with no activity. If you wish to keep this issue active, please remove the stale label or add a comment, otherwise will be closed in 14 days.

@github-actions github-actions bot added the Stale label Nov 21, 2024
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Dec 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants