public Add `gre` container under next-hops aft entry state. Add `src-ip`, `dst-ip` and `ttl` under `gre` aft entry state for telemetry.

(M) aft/openconfig-aft-common.yang
(M) aft/openconfig-aft-ethernet.yang
(M) aft/openconfig-aft-ipv4.yang
(M) aft/openconfig-aft-ipv6.yang
(M) aft/openconfig-aft-mpls.yang
(M) aft/openconfig-aft-pf.yang
(M) aft/openconfig-aft-state-synced.yang
(M) aft/openconfig-aft.yang

Change Scope

Add gre container under next-hops aft entry state.
Add src-ip, dst-ip and ttl under gre aft entry state for telemetry.
This change is backwards compatible

Platform Implementations

Arista documentation

Relevant pyang output:

module: openconfig-network-instance
  +--rw network-instances
     +--rw network-instance* [name]
        +--ro afts
        |  +--ro next-hops
        |     +--ro next-hop* [index]
        |        +--ro index            -> ../state/index
        |        +--ro state
        |        |  +--ro index?                       uint64
        |        |  +--ro programmed-index?            uint64
        |        |  +--ro ip-address?                  oc-inet:ip-address
        |        |  +--ro mac-address?                 oc-yang:mac-address
        |        |  +--ro pop-top-label?               boolean
        |        |  +--ro pushed-mpls-label-stack*     oc-mplst:mpls-label
        |        |  +--ro encapsulate-header?          oc-aftt:encapsulation-header-type
        |        |  +--ro decapsulate-header?          oc-aftt:encapsulation-header-type
        |        |  +--ro origin-protocol?             identityref
        |        |  +--ro lsp-name?                    string
        |        |  +--ro counters
        |        |  |  +--ro packets-forwarded?   oc-yang:counter64
        |        |  |  +--ro octets-forwarded?    oc-yang:counter64
        |        |  +--ro vni-label?                   oc-evpn-types:evi-id
        |        |  +--ro tunnel-src-ip-address?       oc-inet:ip-address
        |        |  +--ro oc-aftni:network-instance?   oc-ni:network-instance-ref
        |        +--ro ip-in-ip
        |        |  +--ro state
        |        |     +--ro src-ip?   oc-inet:ip-address
        |        |     +--ro dst-ip?   oc-inet:ip-address
        |        +--ro gre
        |        |  +--ro state
        |        |     +--ro src-ip?   oc-inet:ip-address
        |        |     +--ro dst-ip?   oc-inet:ip-address
        |        |     +--ro ttl?      uint8
        |        +--ro interface-ref
        |           +--ro state
        |              +--ro interface?      -> /oc-if:interfaces/interface/name
        |              +--ro subinterface?   -> ../interface]/subinterfaces/subinterface/index

Jan 26 '24 14:01 romeyod

No major YANG version changes in commit 28166f84ac78f607c130accb013e50e6e8011486

Jan 26 '24 14:01 OpenConfigBot

Doesn't need to be handled in this PR -- but in reviewing this I had a thought. If we were doing MPLS over GRE then we'll specify something in the MPLS-related leaves and then subsequently in the GRE container. encapsulate-header today is a scalar leaf, so what would its value be if we were doing MPLS over GRE?

There are a few options:

make encapsulate-header just the outer-most header.
deprecate encapsulate-header and imply the headers from the specified leaves -- this one probably doesn't work because we then don't know what order they are applied in.
have containers that are specific to combinatorial encapsulations -- e.g., mpls-over-gre which has the MPLS and GRE leaves in it. This denormalises the model a bit and duplicates definitions.
have a new encapsulate-stack leaf-list which specifies the order that the encapsulations are being applied -- i.e., I can specify ["GRE", "MPLS"] which indicates that it's GRE+IPv[46] and then MPLS.

WDYT?

Jan 26 '24 16:01 robshakir

https://github.com/openconfig/public/blob/master/release/models/aft/openconfig-aft-common.yang#L272

Description of tunnel-src-ip-address is confusing. It can be used to populate gre tunnel src address. can we update the description to make it specific to EVPN/VXLAN?

Jan 29 '24 06:01 krishna-juniper

Doesn't need to be handled in this PR -- but in reviewing this I had a thought. If we were doing MPLS over GRE then we'll specify something in the MPLS-related leaves and then subsequently in the GRE container. encapsulate-header today is a scalar leaf, so what would its value be if we were doing MPLS over GRE?

There are a few options:

make encapsulate-header just the outer-most header.

deprecate encapsulate-header and imply the headers from the specified leaves -- this one probably doesn't work because we then don't know what order they are applied in.

have containers that are specific to combinatorial encapsulations -- e.g., mpls-over-gre which has the MPLS and GRE leaves in it. This denormalises the model a bit and duplicates definitions.

have a new encapsulate-stack leaf-list which specifies the order that the encapsulations are being applied -- i.e., I can specify ["GRE", "MPLS"] which indicates that it's GRE+IPv[46] and then MPLS.

WDYT?

IMHO a new encapsulate-stack leaf-list seems like the best path forward at first glance.

Jan 29 '24 15:01 romeyod

https://github.com/openconfig/public/blob/master/release/models/aft/openconfig-aft-common.yang#L272

Description of tunnel-src-ip-address is confusing. It can be used to populate gre tunnel src address. can we update the description to make it specific to EVPN/VXLAN?

Makes sense to update the documentation. Not sure if we should couple that with this PR. Maybe file a issue and @robshakir and company to start a separate discussion.

Jan 29 '24 15:01 romeyod

Merging into main branch for pyangbind check fix.

Feb 05 '24 17:02 wenovus

w.r.t

https://github.com/openconfig/public/blob/master/release/models/aft/openconfig-aft-common.yang#L272 Description of tunnel-src-ip-address is confusing. It can be used to populate gre tunnel src address. can we update the description to make it specific to EVPN/VXLAN?

Makes sense to update the documentation. Not sure if we should couple that with this PR. Maybe file a issue and @robshakir and company to start a separate discussion.

let's continue this discussion here: https://github.com/openconfig/public/issues/1040

Feb 05 '24 21:02 robshakir

/gcbrun

Mar 07 '24 22:03 dplore

/gcbrun

Mar 07 '24 22:03 dplore

Doesn't need to be handled in this PR -- but in reviewing this I had a thought. If we were doing MPLS over GRE then we'll specify something in the MPLS-related leaves and then subsequently in the GRE container. encapsulate-header today is a scalar leaf, so what would its value be if we were doing MPLS over GRE? There are a few options:

make encapsulate-header just the outer-most header.

deprecate encapsulate-header and imply the headers from the specified leaves -- this one probably doesn't work because we then don't know what order they are applied in.

have containers that are specific to combinatorial encapsulations -- e.g., mpls-over-gre which has the MPLS and GRE leaves in it. This denormalises the model a bit and duplicates definitions.

have a new encapsulate-stack leaf-list which specifies the order that the encapsulations are being applied -- i.e., I can specify ["GRE", "MPLS"] which indicates that it's GRE+IPv[46] and then MPLS.

WDYT?

IMHO a new encapsulate-stack leaf-list seems like the best path forward at first glance.

Agreed, ideally we have some option that allows exposing any number of nested encapsulations. I like this option because it keeps the afts table flat.

Do you mind refactoring this PR to implement leaf-list for the list of encaps? Something like:

module: openconfig-network-instance
  +--rw network-instances
     +--rw network-instance* [name]
        +--ro afts
        |  +--ro next-hops
        |     +--ro next-hop* [index]
        |        x--ro ip-in-ip     (deprecated)
        |        |  x--ro state     (deprecated)
        |        |     x--ro src-ip?   oc-inet:ip-address  (deprecated)
        |        |     x--ro dst-ip?   oc-inet:ip-address  (deprecated)
        |        +--ro encapsulate
        |        |  +--ro state
        |        |     +--ro stack*?   oc-types:encap-types   
        |        |     +--ro src-ip?   oc-inet:ip-address
        |        |     +--ro dst-ip?   oc-inet:ip-address
        |        |     +--ro ttl?      uint8
        |        +--ro interface-ref
        |           +--ro state
        |              +--ro interface?      -> /oc-if:interfaces/interface/name
        |              +--ro subinterface?   -> ../interface]/subinterfaces/subinterface/index**

Mar 07 '24 23:03 dplore

Doesn't need to be handled in this PR -- but in reviewing this I had a thought. If we were doing MPLS over GRE then we'll specify something in the MPLS-related leaves and then subsequently in the GRE container. encapsulate-header today is a scalar leaf, so what would its value be if we were doing MPLS over GRE? There are a few options:

make encapsulate-header just the outer-most header.

deprecate encapsulate-header and imply the headers from the specified leaves -- this one probably doesn't work because we then don't know what order they are applied in.

have containers that are specific to combinatorial encapsulations -- e.g., mpls-over-gre which has the MPLS and GRE leaves in it. This denormalises the model a bit and duplicates definitions.

have a new encapsulate-stack leaf-list which specifies the order that the encapsulations are being applied -- i.e., I can specify ["GRE", "MPLS"] which indicates that it's GRE+IPv[46] and then MPLS.

WDYT?

IMHO a new encapsulate-stack leaf-list seems like the best path forward at first glance.

Agreed, ideally we have some option that allows exposing any number of nested encapsulations. I like this option because it keeps the afts table flat.

Do you mind refactoring this PR to implement leaf-list for the list of encaps? Something like:
module: openconfig-network-instance
  +--rw network-instances
     +--rw network-instance* [name]
        +--ro afts
        |  +--ro next-hops
        |     +--ro next-hop* [index]
        |        x--ro ip-in-ip     (deprecated)
        |        |  x--ro state     (deprecated)
        |        |     x--ro src-ip?   oc-inet:ip-address  (deprecated)
        |        |     x--ro dst-ip?   oc-inet:ip-address  (deprecated)
        |        +--ro encapsulate
        |        |  +--ro state
        |        |     +--ro stack*?   oc-types:encap-types   
        |        |     +--ro src-ip?   oc-inet:ip-address
        |        |     +--ro dst-ip?   oc-inet:ip-address
        |        |     +--ro ttl?      uint8
        |        +--ro interface-ref
        |           +--ro state
        |              +--ro interface?      -> /oc-if:interfaces/interface/name
        |              +--ro subinterface?   -> ../interface]/subinterfaces/subinterface/index**

This will need to a LIST of the entire encap header state since src-ip, dst-ip, ttl, tos (in the future) could be part unique per encap, right? Something like:

|        +--ro encapsulate
|        |  +--ro state
|        |     +--ro stack*?   oc-types:encap-header  
|        |      |     +--ro encap?   oc-types:encap-type
|        |      |     +--ro src-ip?   oc-inet:ip-address
|        |      |     +--ro dst-ip?   oc-inet:ip-address
|        |      |     +--ro ttl?         uint8
|        |      |     +--ro tos?       uint8

@robshakir @dplore let me know your thoughts on this.

Not sure if we can make each of the "encap-header" definition different per encap type i.e. aft-common-entry-nexthop-gre-state aft-common-entry-nexthop-ip-in-ip-state etc and they are still be part of "encap-header" List. This will make the documentation per "encap-header" definition easier as well.

Mar 08 '24 14:03 romeyod

Doesn't need to be handled in this PR -- but in reviewing this I had a thought. If we were doing MPLS over GRE then we'll specify something in the MPLS-related leaves and then subsequently in the GRE container. encapsulate-header today is a scalar leaf, so what would its value be if we were doing MPLS over GRE? There are a few options:

make encapsulate-header just the outer-most header.

deprecate encapsulate-header and imply the headers from the specified leaves -- this one probably doesn't work because we then don't know what order they are applied in.

have containers that are specific to combinatorial encapsulations -- e.g., mpls-over-gre which has the MPLS and GRE leaves in it. This denormalises the model a bit and duplicates definitions.

have a new encapsulate-stack leaf-list which specifies the order that the encapsulations are being applied -- i.e., I can specify ["GRE", "MPLS"] which indicates that it's GRE+IPv[46] and then MPLS.

WDYT?

IMHO a new encapsulate-stack leaf-list seems like the best path forward at first glance.

Agreed, ideally we have some option that allows exposing any number of nested encapsulations. I like this option because it keeps the afts table flat. Do you mind refactoring this PR to implement leaf-list for the list of encaps? Something like:
module: openconfig-network-instance
  +--rw network-instances
     +--rw network-instance* [name]
        +--ro afts
        |  +--ro next-hops
        |     +--ro next-hop* [index]
        |        x--ro ip-in-ip     (deprecated)
        |        |  x--ro state     (deprecated)
        |        |     x--ro src-ip?   oc-inet:ip-address  (deprecated)
        |        |     x--ro dst-ip?   oc-inet:ip-address  (deprecated)
        |        +--ro encapsulate
        |        |  +--ro state
        |        |     +--ro stack*?   oc-types:encap-types   
        |        |     +--ro src-ip?   oc-inet:ip-address
        |        |     +--ro dst-ip?   oc-inet:ip-address
        |        |     +--ro ttl?      uint8
        |        +--ro interface-ref
        |           +--ro state
        |              +--ro interface?      -> /oc-if:interfaces/interface/name
        |              +--ro subinterface?   -> ../interface]/subinterfaces/subinterface/index**
This will need to a LIST of the entire encap header state since src-ip, dst-ip, ttl, tos (in the future) could be part unique per encap, right? Something like:
|        +--ro encapsulate
|        |  +--ro state
|        |     +--ro stack*?   oc-types:encap-header  
|        |      |     +--ro encap?   oc-types:encap-type
|        |      |     +--ro src-ip?   oc-inet:ip-address
|        |      |     +--ro dst-ip?   oc-inet:ip-address
|        |      |     +--ro ttl?         uint8
|        |      |     +--ro tos?       uint8
@robshakir @dplore let me know your thoughts on this.

Not sure if we can make each of the "encap-header" definition different per encap type i.e. aft-common-entry-nexthop-gre-state aft-common-entry-nexthop-ip-in-ip-state etc and they are still be part of "encap-header" List. This will make the documentation per "encap-header" definition easier as well.

So we need to have a leaf-list of encap-headers that has a typedef/container with all the fields per encapsulation like src-ip, dst-ip, ttl and tos as well as the encapsulation header type. This way will be more future safe and we can support any encapsulation combination including theoretically GRE over GRE and more.

Apr 19 '24 13:04 romeyod

How about defining containers for each encap with it's own set of custom state leaves? Since encaps can be quite different, I am not sure it is feasible to have a list of encaps, but rather models containers for each encap with typed leaves.

  +--rw network-instances
     +--rw network-instance* [name]
        +--ro afts
        |  +--ro next-hops
        |     +--ro next-hop* [index]
        |        +--ro ip-in-ip   
        |        |  +--ro state   
        |        |     +--ro <state leafs added here>
        |        +--ro <my other encap container>
        |        |  +--ro state   
        |        |     +--ro <my other encap state leafs added here>

Apr 20 '24 00:04 dplore

How about defining containers for each encap with it's own set of custom state leaves? Since encaps can be quite different, I am not sure it is feasible to have a list of encaps, but rather models containers for each encap with typed leaves.
  +--rw network-instances
     +--rw network-instance* [name]
        +--ro afts
        |  +--ro next-hops
        |     +--ro next-hop* [index]
        |        +--ro ip-in-ip   
        |        |  +--ro state   
        |        |     +--ro <state leafs added here>
        |        +--ro <my other encap container>
        |        |  +--ro state   
        |        |     +--ro <my other encap state leafs added here>

@dplore Maybe I am being a bit dense here. Can you please simplify your suggestion for me?

The original intent of the PR was to add gre container under next-hops aft entry state. Add src-ip, dst-ip and ttl under gre aft entry state for telemetry. This was being achieved by using uses but is there a different way to typed leaves each of them?

The only reason the List/stack on encap can into the discussion was as a suggestion/food for thought by @robshakir and we were planning to take it up later.

I know this PR has dragged on for a while so I am just TLDRing it for any other reader/party that comes across it :)

Apr 23 '24 19:04 romeyod

https://github.com/openconfig/public/blob/master/release/models/aft/openconfig-aft-common.yang#L272 Description of tunnel-src-ip-address is confusing. It can be used to populate gre tunnel src address. can we update the description to make it specific to EVPN/VXLAN?

Makes sense to update the documentation. Not sure if we should couple that with this PR. Maybe file a issue and @robshakir and company to start a separate discussion.

Please add this description change. It should be in this PR because this PR introduces a src-ip leaf which could be confused with the existing tunnel-src-ip-address. Clarifying that tunnel-src-ip-address is not used for gre in the formal description is good. This will cause the git history to also point to this PR for context.

Apr 24 '24 21:04 dplore

How about defining containers for each encap with it's own set of custom state leaves? Since encaps can be quite different, I am not sure it is feasible to have a list of encaps, but rather models containers for each encap with typed leaves.
  +--rw network-instances
     +--rw network-instance* [name]
        +--ro afts
        |  +--ro next-hops
        |     +--ro next-hop* [index]
        |        +--ro ip-in-ip   
        |        |  +--ro state   
        |        |     +--ro <state leafs added here>
        |        +--ro <my other encap container>
        |        |  +--ro state   
        |        |     +--ro <my other encap state leafs added here>
@dplore Maybe I am being a bit dense here. Can you please simplify your suggestion for me?

The original intent of the PR was to add gre container under next-hops aft entry state. Add src-ip, dst-ip and ttl under gre aft entry state for telemetry. This was being achieved by using uses but is there a different way to typed leaves each of them?

The only reason the List/stack on encap can into the discussion was as a suggestion/food for thought by @robshakir and we were planning to take it up later.

I know this PR has dragged on for a while so I am just TLDRing it for any other reader/party that comes across it :)

Ah yes, let's consider this comment thread out of scope for this PR. The current PR scope for GRE only is ok as is and does not need to consider refactoring how any/all tunnel encapsulations are modeled in OC.

Apr 24 '24 21:04 dplore

Please add a description change for tunnel-src-ip-address as per the issue:

https://github.com/openconfig/public/blob/master/release/models/aft/openconfig-aft-common.yang#L272

I think it is appropriate for this description change to be in this PR because this PR introduces a new src-ip leaf which could be confused with the existing tunnel-src-ip-address. Clarifying that tunnel-src-ip-address is only used for vxlan in the formal description is good.

The git history will point to this PR and our mention of #1040 for additional context.

Other than this, LGTM. This will be reviewed at the Apr 30, 2024 OC Operators meeting

Thansk @dplore. I have updated the PR with the requested changes. Let me know if any other action is needed.

Apr 25 '24 18:04 romeyod

/gcbrun

Apr 25 '24 19:04 dplore

One more comment on the description for src-ip and I think it's ready to review at the April 30th OC operators meeting.

Thanks @dplore Committed the suggestion. I am assuming the Changes requested blocking item will be resolved after the OC operators meeting

Apr 25 '24 21:04 romeyod

/gcbrun

Apr 25 '24 22:04 dplore

Reviewed in Apr 30 OC operators meeting without objection.

Apr 30 '24 16:04 dplore