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 ingress_acl and egress_acl attributes to tunnel object #1908

Closed

Conversation

kperumalbfn
Copy link
Contributor

@kperumalbfn kperumalbfn commented Sep 28, 2023

Add SAI_TUNNEL_ATTR_INGRESS_ACL and SAI_TUNNEL_ATTR_EGRESS_ACL attributes to tunnel object.
Add new ACL bind point type SAI_ACL_BIND_POINT_TYPE_TUNNEL

SAI_TUNNEL_ATTR_INGRESS_ACL - ACL table/group lookup after tunnel termination.
SAI_TUNNEL_ATTR_EGRESS_ACL - ACL table/group lookup after encapsulation.

Packet flow scenarios:
https://github.com/r12f/SONiC/blob/user/r12f/ha/doc/smart-switch/high-availability/smart-switch-ha-hld.md

@kperumalbfn
Copy link
Contributor Author

@marian-pritsak @mvenkat12 Could you review this PR

@rck-innovium
Copy link
Contributor

@kperumalbfn 

In order to align with the current SAI model, we should define a new bindpoint type in sai_acl_bind_point_type_t, say SAI_ACL_BIND_POINT_TYPE_TUNNEL.

Only the ACL tables/groups created with this bindpoint type should be allowed to be bound to SAI_TUNNEL_ATTR_[INGRESS/EGRESS]_ACL

Copy link
Contributor

@rck-innovium rck-innovium left a comment

Choose a reason for hiding this comment

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

Please include the required bindpoint type additions.

Signed-off-by: Kumaresh Perumal <kperumal@microsoft.com>
Signed-off-by: Kumaresh Perumal <kperumal@microsoft.com>
@kperumalbfn
Copy link
Contributor Author

@kperumalbfn 

In order to align with the current SAI model, we should define a new bindpoint type in sai_acl_bind_point_type_t, say SAI_ACL_BIND_POINT_TYPE_TUNNEL.

Only the ACL tables/groups created with this bindpoint type should be allowed to be bound to SAI_TUNNEL_ATTR_[INGRESS/EGRESS]_ACL

Thanks @rck-innovium Added the bind point type.

Copy link
Contributor

@srikrishnagopu srikrishnagopu left a comment

Choose a reason for hiding this comment

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

@kperumalbfn Few suggestions/questions:

  • Can we please more description to the PR on the motivation and usecases so that it will be helpful to refer it in the future ?
  • Any packet which goes through tunnel decap on this tunnel object will go through ingress ACL and any packet which goes through the tunnel encap on this tunnel object on this tunnel object will go through egress ACL ?
  • Given that ACL already has the fields to match on outer and inner packet fields, any inputs on why this bindpoint is required ?

Copy link
Collaborator

@kcudnik kcudnik left a comment

Choose a reason for hiding this comment

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

approve just for code syntax ok, for the actual approve, please discuss this on weekly community meeting

@kperumalbfn
Copy link
Contributor Author

@kperumalbfn Few suggestions/questions:

  • Can we please more description to the PR on the motivation and usecases so that it will be helpful to refer it in the future ?

@srikrishnagopu There are use cases in SmartSwitch where the incoming traffic is double VxLAN/NVGRE encap and ACL matches on outer+inner packet fields(before decap or after decap). Few packet flow examples are defined here:

https://github.com/sonic-net/DASH/blob/main/documentation/general/sdn-pipeline-basic-elements.md#packet-transforms

https://github.com/r12f/SONiC/blob/user/r12f/ha/doc/smart-switch/high-availability/smart-switch-ha-hld.md#4221-tunneling-packet-to-local-dpu

  • Any packet which goes through tunnel decap on this tunnel object will go through ingress ACL and any packet which goes through the tunnel encap on this tunnel object on this tunnel object will go through egress ACL ?

Yes, INGRESS_ACL is for ACL match after decap and EGRESS_ACL for packets after encap. Will add more details to the description.

  • Given that ACL already has the fields to match on outer and inner packet fields, any inputs on why this bindpoint is required ?
    When ACLs are attached to the port, there is no easy way to enforce ACL lookups only after decap without adding extra match fields. With extra match fields, it will increase the ACL scale requirements for certain use cases and will be hard for all the vendor ASICs to support the scale. In Smartswitch, there are packet scenarios with double encap to match outer+inner packet fields after tunnel decap.

For eg: ENCAP1|ENCAP2|VM PACKET.

Terminate and Decap ENCAP1
Match on ENCAP2+VM PACKET fields-->action

Copy link
Contributor

@srikrishnagopu srikrishnagopu left a comment

Choose a reason for hiding this comment

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

Thanks a lot @kperumalbfn for the clarification. Please update the PR description.

SAI changes LGTM. We can discuss in the sai meeting if not discussed already.

@kperumalbfn
Copy link
Contributor Author

Will close this PR. To support Smartswitch usecases, we will add a new SAI ACL attribute to match on 'TUNNEL_TERMINATE_FLAG' to support different ACL actions for 'tunnel_terminate_flag=false+packet_fields' and 'tunnel_terminate_flag=true+packet_fields'.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants