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

Add policy-forwarding network-instance attachment point #1224

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

dplore
Copy link
Member

@dplore dplore commented Nov 14, 2024

Change Scope

Today policy-forwarding policies can only be attached to interfaces. This change adds network-instance as an attachment point for policy-forwarding rules. This causes packets in the specified network-instance to be evaluated against the specified policy-forwarding rules and actions.

This change is backwards compatible.

Tree diff view

We are at:
/network-instances/network-instance/policy-forwarding/

         |  +--rw interfaces
         |  |  +--rw interface* [interface-id]
         |  |     +--rw interface-id     -> ../config/interface-id
         |  |     +--rw config
         |  |     |  +--rw interface-id?                 oc-if:interface-id
         |  |     |  +--rw apply-forwarding-policy?      -> ../../../../policies/policy/config/policy-id
         |  |     |  +--rw apply-vrf-selection-policy?   -> ../../../../policies/policy/config/policy-id
         |  |     +--ro state
         |  |     |  +--ro interface-id?                 oc-if:interface-id
         |  |     |  +--ro apply-forwarding-policy?      -> ../../../../policies/policy/config/policy-id
         |  |     |  +--ro apply-vrf-selection-policy?   -> ../../../../policies/policy/config/policy-id
         |  |     +--rw interface-ref
         |  |        +--rw config
         |  |        |  +--rw interface?      -> /oc-if:interfaces/interface/name
         |  |        |  +--rw subinterface?   -> /oc-if:interfaces/interface[oc-if:name=current()/../interface]/subinterfaces/subinterface/index
         |  |        +--ro state
         |  |           +--ro interface?      -> /oc-if:interfaces/interface/name
         |  |           +--ro subinterface?   -> /oc-if:interfaces/interface[oc-if:name=current()/../interface]/subinterfaces/subinterface/index
+        |  +--rw network-instances
+        |  |  +--rw network-instance* [name]
+        |  |     +--rw name      -> ../config/name
+        |  |     +--rw config
+        |  |     |  +--rw name?                         -> /network-instances/network-instance/config/name
+        |  |     |  +--rw apply-forwarding-policy?      -> ../../../../policies/policy/config/policy-id
+        |  |     |  +--rw apply-vrf-selection-policy?   -> ../../../../policies/policy/config/policy-id
+        |  |     +--ro state
+        |  |        +--ro name?                         -> /network-instances/network-instance/config/name
+        |  |        +--ro apply-forwarding-policy?      -> ../../../../policies/policy/config/policy-id
+        |  |        +--ro apply-vrf-selection-policy?   -> ../../../../policies/policy/config/policy-id
         |  +--rw path-selection-groups
         |     +--rw path-selection-group* [group-id]
         |        +--rw group-id    -> ../config/group-id
         |        +--rw config
         |        |  +--rw group-id?   string
         |        |  +--rw mpls-lsp*   -> ../../../../../mpls/lsps/constrained-path/tunnels/tunnel/config/name
         |        +--ro state
         |           +--ro group-id?   string
         |           +--ro mpls-lsp*   -> ../../../../../mpls/lsps/constrained-path/tunnels/tunnel/config/name


### Platform Implementations

Cisco IOS XR

vrf-policy
vrf default address-family ipv4 policy type pbr input GRE-DECAP-PBR

Arista EOS

ip decap-group SRTE-GRE-DECAP
tunnel type gre
tunnel decap-ip 10.5.32.0

@OpenConfigBot
Copy link

OpenConfigBot commented Nov 14, 2024

No major YANG version changes in commit d203259

@dplore dplore marked this pull request as ready for review December 7, 2024 01:41
@dplore dplore requested a review from a team as a code owner December 7, 2024 01:42
@dplore
Copy link
Member Author

dplore commented Dec 11, 2024

Last call for comments. This will merge on Jan 6th, 2025

Comment on lines +105 to +113
description
"The policy to be applied on the network-instance. Packets ingress
on the referenced interface should be compared to the match
criteria within the specified policy, and in the case that
these criteria are met, the forwarding actions specified
applied. These policies should be applied following quality of
service classification, and ACL actions if such entities are
referenced by the corresponding interface.";
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This description still talks about the "referenced interface".

Copy link

@LimeHat LimeHat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to see a precise definition of how matching is supposed to work.

In addition, I think there's a formatting issue with the references in the first comment, can it be fixed?


When attached to a network-instance, the policy-forwarding rules
should be evaluated against packets in the specified
network-instances.";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar comment to what I left in the previous PR #1150: what are the "packets in the specified network-instances"?

  • does that mean that the policy applied to all interfaces associated with the network-instance on ingress (e.g. this change introduces only a new way of configuring the existing functionality), or
  • are there any additional expectations w.r.t what packets should be matched (e.g packets redirected from another network-instance via a policy)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The operational use case motivating this PR is for packets ingress to the device which arrive on interfaces (including virtual interfaces) which are mapped to a network-instance.

I am not aware of an operational use case that we have for packets re-directed from another network-instance.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Policy forwarding entries are already defined within a network instance, so what does the enclosing network instance mean?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The operational use case motivating this PR is for packets ingress to the device which arrive on interfaces (including virtual interfaces) which are mapped to a network-instance.

Thus, in terms of functionality, this configuration will be fully equivalent to applying a policy to all interfaces associated with the network-instance? We just adding a "simpler" way to configure this?

Can this be explicitly stated in the description? Thanks

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is correct. I will make this explicit in the description

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Policy forwarding entries are already defined within a network instance, so what does the enclosing network instance mean?

It is true, but without any "apply-policy" configuration, it is ambiguous where the policy is applied? Further, since OC has a way to apply-policy to interfaces, if none are specified, what should the behavior be?

I'd prefer an explicit leaf for this.

I will change the name/description from a leaf-ref to network-instance instead to a boolean like 'all-interfaces'. In our use case, we only want to apply the policy to the same network-instance the policy is defined in, and not some different network-instance.

@nandanarista
Copy link

Hi Darren, please note that EOS decap groups cannot be attached to a network instances, so not sure why we think that EOS decap group implements this proposal.
https://www.arista.com/en/um-eos/eos-decap-groups

Copy link
Contributor

@robshakir robshakir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tend to agree with the other reviewers here -- given that we are already in the context of a network-instance when we are applying a policy this looks to be:

  • syntatic sugar to apply to "all interfaces",
  • probably something that can be applied programmatically at the cost of adding some more content to the configuration request.

It looks like @nandanarista also highlighted this isn't supported in EOS. Can we look at whether we need to make this enhancement?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: last-call
Development

Successfully merging this pull request may close these issues.

7 participants