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.

Sign up for GitHub

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

Jump to bottom

[aclorch]: add support for acl rule to match out port #810

Merged
merged 7 commits into from
Jan 29, 2020
Merged

[aclorch]: add support for acl rule to match out port #810

merged 7 commits into from
Jan 29, 2020

Conversation

shine4chen
Copy link
Contributor

@shine4chen shine4chen commented Mar 8, 2019 •
edited
Loading

What I did
ACL rule can match out port. Out port could be port intf , lag or vlan.

Why I did it
mclag need acl rule can bind to LAG and acl rule can match out port.

How I verified it
test it on nephos lab

Details if related

@shine4chen shine4chen changed the title Acl enhancement [aclorch]: add support for acl rule to match out port Mar 11, 2019
zhenggen-xu
zhenggen-xu reviewed Apr 4, 2019
View reviewed changes
orchagent/aclorch.cpp Outdated Show resolved Hide resolved
lguohan
lguohan reviewed Apr 25, 2019
View reviewed changes
orchagent/aclorch.cpp Outdated Show resolved Hide resolved
lguohan
lguohan reviewed Apr 25, 2019
View reviewed changes
orchagent/aclorch.cpp Outdated
}
else if (port.m_type == Port::LAG)
{
m_outPorts.push_back(port.m_lag_id);
Copy link
Contributor

Choose a reason for hiding this comment

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

does not match SAI spec.

https://github.com/opencomputeproject/SAI/blob/master/inc/saiacl.h#L1260

same as above.

Copy link

Choose a reason for hiding this comment

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

Thinks, we will delete LAG port.

Do some enhancement to ACLorch for mclag
5079d97 
* Aclorch can be the consumer of the APP_ACL_TABLE_NAME and
  APP_ACL_RULE_TABLE_NAME tables in APP_DB
* ACL rule can match out port

Signed-off-by: shine.chen <shine.chen@nephosinc.com>
@leoli-nps
[vstest] add the corresponding vstest for pr#810
a418c6a 
Signed-off-by: leo.li <leo.li@nephosinc.com>
@leoli-nps
Copy link
Contributor

The corresponding VS test case has been added, please review.

change APP_ACL_TABLE_NAME to APP_ACL_TABLE_TABLE_NAME
fa71011 
Signed-off-by: shine.chen <shine.chen@nephosinc.com>
@stcheng
Copy link
Contributor

stcheng commented Jul 23, 2019

could you resolve the conflicts?

merge to master branch and resolve the conflict
2b0305e 
Signed-off-by: shine.chen <shine.chen@nephosinc.com>
@shine4chen
Copy link
Contributor Author

@stcheng we have fixed the conflicts.

stcheng
stcheng reviewed Jul 24, 2019
View reviewed changes
orchagent/aclorch.cpp Outdated Show resolved Hide resolved
@lguohan
Copy link
Contributor

lguohan commented Aug 1, 2019

where is the mclag design spec? since out_ports match are not supported in various asic vendor. we need a more generic approach to address this need.

@lguohan lguohan requested a review from rlhui August 1, 2019 17:54
@shine4chen
Copy link
Contributor Author

shine4chen commented Aug 2, 2019 •
edited
Loading

@lguohan we have discuss it with BRCM folks in sonic-mclag-subgroup. There is a workaround in SAI layer by combination of ingress acl and egress acl. With this workaround most vendors can support out-ports.

The following is the isolation logic description copied from MCLAG-HLD

  • In ASIC, ACL rule is used to isolate peer link from MC-LAG port. The rule is when the traffic is received from peer link and the output port is MC-LAG member port, the traffic must be dropped. For the chips whose ACL rule can't support out-port, there is a workaround in SAI layer by combination of ingress acl and egress acl. An alternative approach is to use isolation group. Isolation group still have some weakness, First isolation group can't support tunnel-port and orchagent doesn't support it currently. secondly isolation group may not be supported by all ASIC vendors. Using ACL is a more generic way to support the isolation function. We can refine this function to use isolation group later if it’s required.

@shine4chen shine4chen mentioned this pull request Aug 30, 2019
Merged
@lguohan
Copy link
Contributor

lguohan commented Sep 18, 2019

unless Broadcom SAI as well as other vendor SAI has implemented outPorts, merging this PR will immediately break the image.

@lguohan
Copy link
Contributor

lguohan commented Sep 18, 2019

isolation group is much more generic and different ASIC can choose different way to implement. Not all ASIC verdors have the same workaround as broadcom. I do not see a reason why we cannot add LAG into isolation group, it is a simple metadata change.

@shine4chen
Copy link
Contributor Author

@lguohan Thanks for the feedback. The following is my answer for your concern.

  1. Mclag feature is not enabled by default. So even for the vendor who doesn't work around acl-outport it wouldn't break the image by default. If vendor like to enable the mclag , of course it need to fix up.
  2. In the existing master branch , saiisolationgroup.h isn't included and orchagent support is missing too. So needless to mention SAI support. Nephos team plans to include mclag feature on 201910 release. As time is limited could community accept this PR now? We can refine the isolation code on our next release.

@adyeung
Copy link

adyeung commented Sep 20, 2019

BRCM will implement an isolation group solution for ingress filtering instead of outPorts, pkts will be dropped at the ingress instead of queueing for egress drop. Design proposal along with other MCLAG improvements will be shared in the upcoming BRCM enhancement HLD.

adyeung
adyeung approved these changes Sep 20, 2019
View reviewed changes
Copy link

@adyeung adyeung left a comment

Choose a reason for hiding this comment

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

BRCM team has completed the review, we have no further request or comment to add, please proceed to the next steps.

@shine4chen
Copy link
Contributor Author

retest this please

1 similar comment
@shine4chen
Copy link
Contributor Author

retest this please

@shine4chen
Merge pull request #3 from Azure/master
b7d6d64 
merge azure/sonic-swss to aclorch branch
define ACL-table-MCLAG for MCLAG feature
3ed8258 
Signed-off-by: shine.chen <shine.chen@mediatek.com>
@rlhui rlhui requested a review from prsunny November 16, 2019 00:50
@shine4chen
Copy link
Contributor Author

retest this please

3 similar comments
@shine4chen
Copy link
Contributor Author

retest this please

@shine4chen
Copy link
Contributor Author

retest this please

@shine4chen
Copy link
Contributor Author

retest this please

@lguohan lguohan merged commit be867dc into sonic-net:master Jan 29, 2020
lguohan pushed a commit that referenced this pull request Jan 30, 2020
@shine4chen @lguohan
[aclorch]: add support for acl rule to match out port (#810)
c6a8a04 
ACL rule can match out port. Out port could be port intf , lag or vlan.

Signed-off-by: shine.chen <shine.chen@nephosinc.com>
@davidfordouce davidfordouce mentioned this pull request Sep 21, 2020
Open
EdenGri pushed a commit to EdenGri/sonic-swss that referenced this pull request Feb 28, 2022
@dzhangalibaba
[MultiDB] sonic-utilities - replace redis-cli/redis-dump with sonic-d…
66e9dfb 
…b-cli/sonic-db-dump (sonic-net#810)

* [MultiDB] sonic-utilities - replace redis-cli/redis-dump with sonic-db-cli/sonic-db-dump
* only accept upper and underscore to prevent injection
* quotation on db_name
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rlhui rlhui rlhui approved these changes

@jianjundong jianjundong jianjundong left review comments

@zhenggen-xu zhenggen-xu zhenggen-xu left review comments

@lguohan lguohan lguohan left review comments

@adyeung adyeung adyeung approved these changes

@prsunny prsunny prsunny approved these changes

@stcheng stcheng stcheng approved these changes

Assignees

@rlhui rlhui

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@shine4chen @leoli-nps @stcheng @lguohan @adyeung @zhenggen-xu @prsunny @jianjundong @rlhui