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

Fix ACL ICMP rules #1471

Merged
merged 2 commits into from
Sep 18, 2019
Merged

Fix ACL ICMP rules #1471

merged 2 commits into from
Sep 18, 2019

Conversation

VladoLavor
Copy link
Collaborator

Fixed in all supported VPP versions:

  • ICMP code range 'last' filled with the correct value for IPv6 rule
  • Fixed dump of ICMP code range and type range values

Signed-off-by: Vladimir Lavor vlavor@cisco.com

Signed-off-by: Vladimir Lavor <vlavor@cisco.com>
@ondrej-fabry
Copy link
Member

Please add VPP integration test case for ACLs that verifies this behaviour.

Copy link
Member

@ondrej-fabry ondrej-fabry left a comment

Choose a reason for hiding this comment

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

VPP integration test cases should be added.

@VladoLavor
Copy link
Collaborator Author

@ondrej-fabry this bug was caught by our robot tests (a test case fixed by @samelias which was bugged before and did not test what it should) so we already have a test entity.
But I agree and I will add this case to integration tests in other PR.

Also, please do not mark commit with request changes if there are no changes really requested, it's confusing.

Signed-off-by: Vladimir Lavor <vlavor@cisco.com>
@ondrej-fabry ondrej-fabry merged commit 70a8145 into ligato:dev Sep 18, 2019
VladoLavor added a commit to VladoLavor/vpp-agent that referenced this pull request Oct 4, 2019
* Fix ACL ICMP rules

Signed-off-by: Vladimir Lavor <vlavor@cisco.com>

* Added integration test case

Signed-off-by: Vladimir Lavor <vlavor@cisco.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants