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

[acl-loader]: do not add default deny rule for egress acl #1531

Merged
merged 6 commits into from
Mar 29, 2021

Conversation

lguohan
Copy link
Contributor

@lguohan lguohan commented Mar 28, 2021

Signed-off-by: Guohan Lu [email protected]

What I did

do not add default deny rule for egress acl

How I did it

How to verify it

tbd

Previous command output (if the output of a command-line utility has changed)

New command output (if the output of a command-line utility has changed)

@lguohan lguohan requested a review from daall March 28, 2021 21:21
@lgtm-com
Copy link

lgtm-com bot commented Mar 28, 2021

This pull request introduces 1 alert when merging 1b952e7 into 4d89510 - view on LGTM.com

new alerts:

  • 1 for Syntax error

Signed-off-by: Guohan Lu <[email protected]>
acl_loader/main.py Outdated Show resolved Hide resolved
Signed-off-by: Guohan Lu <[email protected]>
Signed-off-by: Guohan Lu <[email protected]>
Signed-off-by: Guohan Lu <[email protected]>
Signed-off-by: Guohan Lu <[email protected]>
@lguohan lguohan marked this pull request as ready for review March 29, 2021 04:10
@lguohan lguohan merged commit 28b64ec into sonic-net:master Mar 29, 2021
@lguohan lguohan deleted the egracl branch March 29, 2021 05:20
lguohan added a commit that referenced this pull request Mar 29, 2021
lguohan added a commit that referenced this pull request Mar 29, 2021
@SavchukRomanLv
Copy link

@lguohan, @qiluo-msft can you please provide info why these changes happen (HLD, some other doc), as after this test_unmatched_blocked (which expects that all unmatched traffic for egress will be automatically blocked) started failing
Thank you in advance!

@qiluo-msft
Copy link
Contributor

@SavchukRomanLv Sorry for the inconvenience. We have use case that one physical port belongs to 2 vlans and there are multiple egress ACL tables bound to the same port, actually each ACL table only have ACL rules matching one vlan ID.

If all unmatched traffic for egress will be automatically dropped, the first table will drop remaining traffic on one vlan and all traffic on the other vlan. To quickly fix this issue, we expected user provide explicit drop rules at the end of each ACL table, and we removed the default drop rule.

Please be aware that we translate the egress ACL table bound to vlan to egress ACL table bound to each port in that vlan in currently implementation.

@stepanblyschak
Copy link
Contributor

@qiluo-msft @lguohan is there a plan to change the sonic-mgmt test expectations?

stepanblyschak pushed a commit to stepanblyschak/sonic-utilities that referenced this pull request Apr 28, 2022
* fc80eeb 2021-03-28 | [acl-loader]: do not add default deny rule for egress acl (sonic-net#1531) (HEAD, origin/201911) [lguohan]

Signed-off-by: Guohan Lu <[email protected]>
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