openconfig / public

Repository for publishing OpenConfig models, documentation, and other material for the community.
Apache License 2.0
879 stars 643 forks source link

YANG attribute type change (e.g IP to (IP & intf-name)) for new features #368

Closed venkatmahalingam closed 1 day ago

venkatmahalingam commented 4 years ago

Hello OC community,

For BGP unnumbered use-case, we would like to change the type of the below attribute to include interface name as well along with IP, please see below for more information, this is similar to the commit (https://github.com/openconfig/public/commit/ad2c88a2bf938a8129a271621f0b23d2d2905ee3) where the type has been changed from uint16 to uint8, but when we referred https://tools.ietf.org/html/rfc7950#section-11 & https://tools.ietf.org/html/rfc6020#section-10 in both RFCs, it is mentioned that revisions of the yang should not change the semantics, syntax of the type,

   RFC-6020 section#10 - A "type" statement may be replaced with another "type" statement
      that does not change the syntax or semantics of the type.  For
      example, an inline type definition may be replaced with a typedef,
      but an int8 type cannot be replaced by an int16, since the syntax
      would change.

wondering if this needs to be followed in OC YANGs or we're good as long as the new change is backward compatible, please advise.

typedef bgp-neighbor {
      type union {
          type oc-inet:ip-address;
          type leafref {
             path "/oc-if:interfaces/oc-if:interface/oc-if:name";
          }
       }
      description
          "BGP neighbor can be either IP or interface name";
  }

/network-instances/network-instance/protocols/protocol/bgp/neighbors/neighbor/config/neighbor-address {
     deviate replace {
          type bgp-neighbor;
      }
}
missaesasaya commented 2 years ago

Any comments on this issue? Or any other way to configure BGP unnumbered?

robshakir commented 2 years ago

I think this requires a backwards incompatible change to be made to the models (whilst the data instances should be compatible, code generation that is strongly typed will require a change thus the change isn't invisible to users). Rather than using a deviation, I'd propose that we convert this to a union as suggested.

One question - in the scenario that you are configuring an interface (with no address) as suggested, what happens if there is >1 device available via that interface? Could this potentially be better to support via something akin to dynamic-neighbor-prefixes? If we did this, you could configure an interface as essentially a "listen", and then have all of the individual neighbours have their own state in the neighbors list, this sounds a potentially cleaner change. You may need to still change the type of the neighbor address, since it may need to be a zoned IP address, but this will be a backwards compatible change.

jsterne commented 2 years ago

Even if this is converted to a union, i.e. changed from this:

    leaf neighbor-address {
        type oc-inet:ip-address;
    }

to this:

    leaf neighbor-address {
        union {
            type oc-inet:ip-address;
            type leafref {
                path "/oc-if:interfaces/oc-if:interface/oc-if:name";
            }
        }
    }

isn't that potentially a non-backwards-compatible change ?

In NETCONF the encoding of the address would look the same, but is there potentially some impact in gNMI with certain encodings (i.e. would the protobuf identifier potentially be different for the ip-address part) ?

robshakir commented 2 years ago

Yes, this is a backwards-incompatible change from the perspective of OpenConfig - since when a strongly typed language generates code for this binding then there is the possibility of a different type being required (depending on implementation).

The change that would be backwards compatible would be a change of something from oc-inet:ip-address to oc-inet:ip-address-zoned since this doesn't change the type (both string) and only relaxes the constraints on the leaf.

jsterne commented 2 years ago

OK thx. That's in sync with the backwards compatibility rules in RFC7950 (although this particular example of going from 'type x' to a union that includes 'type x' often causes confusion because it seems like a simple superset).

github-actions[bot] commented 1 month ago

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.