Skip to content

[acl-loader] Identity ICMP v4/v6 based on IP_PROTOCOL for custom ACL table types#2994

Merged
lizhijianrd merged 2 commits intosonic-net:masterfrom
lizhijianrd:acl-loader-custom-icmpv6
Sep 23, 2023
Merged

[acl-loader] Identity ICMP v4/v6 based on IP_PROTOCOL for custom ACL table types#2994
lizhijianrd merged 2 commits intosonic-net:masterfrom
lizhijianrd:acl-loader-custom-icmpv6

Conversation

@lizhijianrd
Copy link
Contributor

@lizhijianrd lizhijianrd commented Sep 21, 2023 •
edited
Loading

What I did

When adding ICMPv6 ACL rules in custom ACL table type, current acl-loader will incorrectly treat the ACL table as IPv4. I open this PR to fix this bug and let acl-loader identify ICMP v4 or v6 based on IP_PROTOCOL.

Also fixed some typo in UT of acl-loader to avoid confusion.

How I did it

In function convert_icmp, add one step to identify the rule is v4 or v6 based on IP_PROTOCOL.

How to verify it

Verified by UT:

Name                                                   Stmts   Miss Branch BrPart  Cover
----------------------------------------------------------------------------------------
acl_loader/__init__.py                                     0      0      0      0   100%
acl_loader/main.py                                       691    154    320     50    74%
...
=============================================== short test summary info ================================================
FAILED tests/disk_check_test.py::TestDiskCheck::test_readonly - assert 1 == 0
FAILED tests/drops_group_test.py::TestDropCounters::test_show_counts - AssertionError: assert ('    IFACE    STATE   ...
FAILED tests/drops_group_test.py::TestDropCounters::test_show_counts_with_group - AssertionError: assert ('\n'\n '   ...
FAILED tests/drops_group_test.py::TestDropCounters::test_show_counts_with_type - AssertionError: assert ('    IFACE  ...
========================== 4 failed, 2722 passed, 3 skipped, 17 warnings in 666.50s (0:11:06) ==========================

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)

@lizhijianrd lizhijianrd changed the title [acl-loader] Identity ICMP/ICMPv6 based on IP_PROTOCOL for custom ACL table types [acl-loader] Identity ICMP v4/v6 based on IP_PROTOCOL for custom ACL table types Sep 21, 2023
Blueve
Blueve reviewed Sep 22, 2023
View reviewed changes
Blueve
Blueve reviewed Sep 22, 2023
View reviewed changes
@bingwang-ms
Copy link
Contributor

bingwang-ms commented Sep 22, 2023

LGTM. Thanks for the fix!

@lizhijianrd lizhijianrd merged commit d82a490 into sonic-net:master Sep 23, 2023
@lizhijianrd lizhijianrd deleted the acl-loader-custom-icmpv6 branch September 25, 2023 03:56
@lizhijianrd
Copy link
Contributor Author

lizhijianrd commented Sep 25, 2023 •
edited
Loading

Hi @StormLiangMS, can you please help to backport this PR to 202305 branch? [ADO: 25230030]

@lizhijianrd
Copy link
Contributor Author

lizhijianrd commented Sep 26, 2023

Hi @yxieca, can you please help to backport this PR to 202205? [ADO: 25230030]

@yxieca
Copy link
Contributor

yxieca commented Sep 26, 2023

@lizhijianrd, this change cannot be cherry-picked cleanly. Please raise PR.

lizhijianrd added a commit to lizhijianrd/sonic-utilities that referenced this pull request Sep 29, 2023
@lizhijianrd
[acl-loader] Identity ICMP v4/v6 based on IP_PROTOCOL for custom ACL …
90e9794 
…table types (sonic-net#2994)

What is the motivation for this PR?
When adding ICMPv6 ACL rules in custom ACL table type, current acl-loader will incorrectly treat the ACL table as IPv4. I open this PR to fix this bug and let acl-loader identify ICMP v4 or v6 based on IP_PROTOCOL.
Also fixed some typo in UT of acl-loader to avoid confusion.

How did you do it?
In function convert_icmp, add one step to identify the rule is v4 or v6 based on IP_PROTOCOL.

How did you verify/test it?
Verified by UT.

Signed-off-by: Zhijian Li <zhijianli@microsoft.com>
@lizhijianrd lizhijianrd mentioned this pull request Sep 29, 2023
Merged
@lizhijianrd
Copy link
Contributor Author

lizhijianrd commented Sep 29, 2023

Hi @yxieca, here is the backport PR: #3003 .

BTW, do I need to open a PR to update the commit_id in sonic-buildimage repo? Or you will handle that?

@yxieca
Copy link
Contributor

yxieca commented Sep 29, 2023

Hi @yxieca, here is the backport PR: #3003 .

BTW, do I need to open a PR to update the commit_id in sonic-buildimage repo? Or you will handle that?

Thanks @lizhijianrd, automation will handle the submodule advance.

yxieca pushed a commit that referenced this pull request Sep 29, 2023
@lizhijianrd
[202205][acl-loader] Identity ICMP v4/v6 based on IP_PROTOCOL for cus…
ff8a064 
…tom ACL table types (#3003)

* [acl-loader] Identity ICMP v4/v6 based on IP_PROTOCOL for custom ACL table types (#2994)

What is the motivation for this PR?
When adding ICMPv6 ACL rules in custom ACL table type, current acl-loader will incorrectly treat the ACL table as IPv4. I open this PR to fix this bug and let acl-loader identify ICMP v4 or v6 based on IP_PROTOCOL.
Also fixed some typo in UT of acl-loader to avoid confusion.

How did you do it?
In function convert_icmp, add one step to identify the rule is v4 or v6 based on IP_PROTOCOL.

How did you verify/test it?
Verified by UT.

Signed-off-by: Zhijian Li <zhijianli@microsoft.com>

* update

---------

Signed-off-by: Zhijian Li <zhijianli@microsoft.com>
JunhongMao pushed a commit to JunhongMao/sonic-utilities that referenced this pull request Oct 4, 2023
@lizhijianrd @JunhongMao
[acl-loader] Identity ICMP v4/v6 based on IP_PROTOCOL for custom ACL …
dfa21e0 
…table types (sonic-net#2994)

What is the motivation for this PR?
When adding ICMPv6 ACL rules in custom ACL table type, current acl-loader will incorrectly treat the ACL table as IPv4. I open this PR to fix this bug and let acl-loader identify ICMP v4 or v6 based on IP_PROTOCOL.
Also fixed some typo in UT of acl-loader to avoid confusion.

How did you do it?
In function convert_icmp, add one step to identify the rule is v4 or v6 based on IP_PROTOCOL.

How did you verify/test it?
Verified by UT.

Signed-off-by: Zhijian Li <zhijianli@microsoft.com>
StormLiangMS pushed a commit that referenced this pull request Oct 7, 2023
@lizhijianrd @StormLiangMS
[acl-loader] Identity ICMP v4/v6 based on IP_PROTOCOL for custom ACL …
7fdf144 
…table types (#2994)

What is the motivation for this PR?
When adding ICMPv6 ACL rules in custom ACL table type, current acl-loader will incorrectly treat the ACL table as IPv4. I open this PR to fix this bug and let acl-loader identify ICMP v4 or v6 based on IP_PROTOCOL.
Also fixed some typo in UT of acl-loader to avoid confusion.

How did you do it?
In function convert_icmp, add one step to identify the rule is v4 or v6 based on IP_PROTOCOL.

How did you verify/test it?
Verified by UT.

Signed-off-by: Zhijian Li <zhijianli@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Blueve Blueve Blueve approved these changes

@bingwang-ms bingwang-ms bingwang-ms approved these changes

Assignees

No one assigned

Labels

Cherry Pick Conflict_202205 Included in 202205 Branch Included in 202305 Branch Request for 202205 Branch Request for 202305 Branch

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@lizhijianrd @bingwang-ms @yxieca @Blueve @StormLiangMS