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] Improve input validation for acl_loader #1479

Merged
merged 2 commits into from
Mar 5, 2021

Conversation

daall
Copy link
Contributor

@daall daall commented Mar 4, 2021

Signed-off-by: Danny Allen [email protected]

What I did

I fixed two issues:

  1. The IPv6 block rule was only looking at the table name, not the table type, when determining what type of default deny rule to add. This isn't very robust.

  2. Individual fields are validated, but there are some invalid combinations like mixing certain protocol numbers like TCP (6) with fields that don't make sense for TCP (like ICMP type/code fields).

How I did it

  1. Used the new is_table_ipv6 method to verify that the table is a V6 dataacl table.
  2. Added a post-check once all qualifiers are validated to ensure that they make sense when put together.

How to verify it

Added new test cases, verified that the existing ACL and CACL tests have no regression.

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)

acl_loader/main.py Outdated Show resolved Hide resolved
Copy link
Contributor

@qiluo-msft qiluo-msft left a comment

Choose a reason for hiding this comment

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

As comment

@daall
Copy link
Contributor Author

daall commented Mar 5, 2021

retest this please

@daall daall merged commit f78e7ce into sonic-net:master Mar 5, 2021
@daall daall deleted the acl_loader_sanity_check branch March 5, 2021 02:57
@abdosi
Copy link
Contributor

abdosi commented Mar 5, 2021

@daall please create PR for 201911.

sonic-util test is very different between master and 201911

@qiluo-msft

@daall
Copy link
Contributor Author

daall commented Mar 5, 2021

already done: #1481

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.

3 participants