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

[aclorch] Remove L4 port range support limitation on egress ACL table and add new SWSS virtual test. #741

Merged
merged 8 commits into from
Jan 29, 2019

Conversation

keboliu
Copy link
Collaborator

@keboliu keboliu commented Jan 9, 2019

What I did

  1. Remove the restriction of only support L4 port ACL range in ingress table
    In function AclTable::create() there is code preventing support of ACL range in egress ACL table, should be removed.

  2. New SWSS virtual test Added to cover the egress ACL support
    Dedicated test cases for egress ACL table creation/deletion and various egress ACL rule verification

more detail info can be found in this PR #sonic-net/SONiC#322 which describe the change for the bug fix
Why I did it
To have an integrated ACL feature support.

How I verified it
Run sonic-mgmt ACL test case to make sure exiting function not break, manually test the egress ACL function to make sure it works.

Details if related

@keboliu
Copy link
Collaborator Author

keboliu commented Jan 9, 2019

retest this please

@stcheng
Copy link
Contributor

stcheng commented Jan 9, 2019

Does 'stage' goes with table or rule? According to the test, 'stage' is a new attribute under table, but in the code, stage is a variable under Rule class. I'm a little bit confused on this.

@keboliu
Copy link
Collaborator Author

keboliu commented Jan 9, 2019

@stcheng "stage" is not a new attribute to the table, it already there, since if we don't explicitly set the stage, by default the table will be an ingress table, if we want to create an egress table, we have to explicitly set the stage to egress.

regarding the new attribute to the AclRuleMirror class, this new attribute "m_tableStage" is derived from the table stage which associated with this mirror rule, with this new member, the AclRuleMirror can know it needs to set an ingress mirror action or an egress mirror action to SAI, because in SAI level there is different mirror action for ingress and egress.

Copy link
Contributor

@stcheng stcheng left a comment

Choose a reason for hiding this comment

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

sorry for blocking this pull request.
I have following suggestions and concerns:

  1. could you separate the item 1 - remove the restriction of supporting port range, into a separate pull request? we could merge that independently

  2. for supporting the SAI_ACL_ENTRY_ATTR_ACTION_MIRROR_INGRESS/SAI_ACL_ENTRY_ATTR_ACTION_MIRROR_EGRESS, I have this concern. In your pull request, the entry's action is tied to the table's stage which limits the flexibility of setting different stages for table and entry.

there could be scenarios that we try to match a specific rule on the ingress packets and would like to mirror them on the egress pipeline to check which port and what format will the packets be; we could also have a rule matching egress packets but we want to check what the packets look like before they enter the pipeline.

I would suggest to have a separate attribute for mirror ACL entries to indicate whether the mirror happens on ingress or egress, and we could have a default value set as ingress if we want.

@keboliu
Copy link
Collaborator Author

keboliu commented Jan 17, 2019

@stcheng Thanks for your comments. For the second Item, I doubt whether the case you described is achievable. I consulted internally and the comments is that egress mirror performed at egress ACL rule, ingress mirror performed at ingress ACL rule, so would you please also double check the feasibility?

@keboliu
Copy link
Collaborator Author

keboliu commented Jan 19, 2019

@stcheng any update on Item 2?

@liat-grozovik
Copy link
Collaborator

@keboliu in some ASIC vendor ingress mirroring can be on egress ACL. this is why @stcheng asked not to limit the solution for it.
Now the question is how we can know which vendor supports this option or not. Please check for each what action are supported and allow it. Please verify the get API for each egress/ingress if it supported the relevant action from SAI_ACL_TABLE_ATTR_ACL_ACTION_TYPE_LIST.
for example: SAI_ACL_ACTION_TYPE_MIRROR_EGRESS and SAI_ACL_ACTION_TYPE_MIRROR_INGRESS

@stcheng
Copy link
Contributor

stcheng commented Jan 23, 2019

Thanks @liat-grozovik for clarifying this. @keboliu could you first generate a separate pull request for the first item?

@keboliu
Copy link
Collaborator Author

keboliu commented Jan 24, 2019

@stcheng @liat-grozovik thanks for the clarification, I'll create a separate PR for Item1 first.

@keboliu
Copy link
Collaborator Author

keboliu commented Jan 25, 2019

@stcheng I have removed the code change related to Item2, only have Item1 left in this PR, please help to review.

Copy link
Contributor

@stcheng stcheng left a comment

Choose a reason for hiding this comment

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

Thanks. The pull request looks good.
Could you change this pull request's title as well?

@keboliu keboliu changed the title [aclorch] Egress acl support bug fix and new SWSS virtual test. [aclorch] Remove L4 port range support limitation on egress ACL table and add new SWSS virtual test. Jan 29, 2019
@liat-grozovik liat-grozovik merged commit 5779c1a into sonic-net:master Jan 29, 2019
yxieca pushed a commit that referenced this pull request Mar 19, 2019
… and add new SWSS virtual test. (#741)

* fix bug for egress acl support and add vs test cases
@keboliu keboliu deleted the egress-acl branch March 23, 2019 02:18
EdenGri pushed a commit to EdenGri/sonic-swss that referenced this pull request Feb 28, 2022
oleksandrivantsiv pushed a commit to oleksandrivantsiv/sonic-swss that referenced this pull request Mar 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants