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] Validate that provided IN/OUT_PORTS are physical interfaces #1156

Merged
merged 10 commits into from
Jan 17, 2020
Merged

[aclorch] Validate that provided IN/OUT_PORTS are physical interfaces #1156

merged 10 commits into from
Jan 17, 2020

Conversation

daall
Copy link
Contributor

@daall daall commented Dec 18, 2019

  • Add a check during the rule validation step to make sure provided IN/OUT_PORTS rules only include physical interfaces
  • Add vs test cases for invalid IN/OUT_PORTS inputs

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

What I did
Added a safety check to the rule validation step in AclOrch so that rules that include the IN_PORTS or OUT_PORTS field can only include physical interfaces.

Why I did it
The SAI only supports physical ports for this particular ACL field, so it'll throw an error if a user tries to pass in a PortChannel, VLAN, or some other interface in one of the PORTS lists. This prevents that from happening.

How I verified it
I added tests to the virtual switch to confirm that invalid rules are being rejected:

daall@valhalla:~/dev/sonic-swss/tests$ sudo pytest -s -v --dvsname=vs test_acl.py::TestAcl
=============================================================================================== test session starts ===============================================================================================
platform linux2 -- Python 2.7.15+, pytest-3.3.0, py-1.8.0, pluggy-0.6.0 -- /usr/bin/python2
cachedir: .cache
rootdir: /home/daall/dev/sonic-swss/tests, inifile:
collected 25 items                                                                                                                                                                                                

test_acl.py::TestAcl::test_AclTableCreation remove extra link dummy PASSED                                                                                                                                  [  4%]
test_acl.py::TestAcl::test_AclRuleL4SrcPort PASSED                                                                                                                                                          [  8%]
test_acl.py::TestAcl::test_AclRuleInOutPorts PASSED                                                                                                                                                         [ 12%]
test_acl.py::TestAcl::test_AclRuleInPortsNonexistentInterface PASSED                                                                                                                                        [ 16%]
test_acl.py::TestAcl::test_AclRuleOutPortsNonexistentInterface PASSED                                                                                                                                       [ 20%]
test_acl.py::TestAcl::test_AclRuleInPortsInvalidInterface PASSED                                                                                                                                            [ 24%]
test_acl.py::TestAcl::test_AclRuleOutPortsInvalidInterface PASSED                                                                                                                                           [ 28%]
test_acl.py::TestAcl::test_AclTableDeletion PASSED                                                                                                                                                          [ 32%]
test_acl.py::TestAcl::test_V6AclTableCreation PASSED                                                                                                                                                        [ 36%]
test_acl.py::TestAcl::test_V6AclRuleIPv6Any PASSED                                                                                                                                                          [ 40%]
test_acl.py::TestAcl::test_V6AclRuleIPv6AnyDrop PASSED                                                                                                                                                      [ 44%]
test_acl.py::TestAcl::test_V6AclRuleIpProtocol PASSED                                                                                                                                                       [ 48%]
test_acl.py::TestAcl::test_V6AclRuleSrcIPv6 PASSED                                                                                                                                                          [ 52%]
test_acl.py::TestAcl::test_V6AclRuleDstIPv6 PASSED                                                                                                                                                          [ 56%]
test_acl.py::TestAcl::test_V6AclRuleL4SrcPort PASSED                                                                                                                                                        [ 60%]
test_acl.py::TestAcl::test_V6AclRuleL4DstPort PASSED                                                                                                                                                        [ 64%]
test_acl.py::TestAcl::test_V6AclRuleTCPFlags PASSED                                                                                                                                                         [ 68%]
test_acl.py::TestAcl::test_V6AclRuleL4SrcPortRange PASSED                                                                                                                                                   [ 72%]
test_acl.py::TestAcl::test_V6AclRuleL4DstPortRange PASSED                                                                                                                                                   [ 76%]
test_acl.py::TestAcl::test_V6AclTableDeletion PASSED                                                                                                                                                        [ 80%]
test_acl.py::TestAcl::test_InsertAclRuleBetweenPriorities PASSED                                                                                                                                            [ 84%]
test_acl.py::TestAcl::test_RulesWithDiffMaskLengths PASSED                                                                                                                                                  [ 88%]
test_acl.py::TestAcl::test_AclRuleIcmp PASSED                                                                                                                                                               [ 92%]
test_acl.py::TestAcl::test_AclRuleIcmpV6 PASSED                                                                                                                                                             [ 96%]
test_acl.py::TestAcl::test_AclRuleRedirectToNexthop PASSED                                                                                                                                                  [100%]

=========================================================================================== 25 passed in 127.22 seconds ===========================================================================================

Details if related

@daall
[aclorch] Validate that provided IN/OUT_PORTS are physical interfaces
249a34a 
- Add a check during the rule validation step to make sure provided IN/OUT_PORTS rules only include physical interfaces
- Add vs test cases for invalid IN/OUT_PORTS inputs

Signed-off-by: Danny Allen <[email protected]>
@daall daall self-assigned this Dec 18, 2019
qiluo-msft
qiluo-msft reviewed Dec 18, 2019
View reviewed changes
tests/test_acl.py Outdated Show resolved Hide resolved
qiluo-msft
qiluo-msft previously approved these changes Dec 18, 2019
View reviewed changes
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.

with naming suggestion

@daall
Copy link
Contributor Author

daall commented Dec 28, 2019

retest this please

1 similar comment
@daall
Copy link
Contributor Author

daall commented Dec 30, 2019

retest this please

@daall daall requested a review from lguohan December 30, 2019 19:12
yxieca
yxieca previously approved these changes Jan 6, 2020
View reviewed changes
qiluo-msft
qiluo-msft previously approved these changes Jan 6, 2020
View reviewed changes
@daall
Copy link
Contributor Author

daall commented Jan 13, 2020

retest this please

@daall daall dismissed stale reviews from qiluo-msft and yxieca via 96a0ab0 January 14, 2020 17:41
@daall
Copy link
Contributor Author

daall commented Jan 15, 2020

retest this please

@daall
Copy link
Contributor Author

daall commented Jan 16, 2020

retest this please

1 similar comment
@daall
Copy link
Contributor Author

daall commented Jan 16, 2020

retest this please

@daall daall requested a review from yxieca January 17, 2020 02:09
@yxieca yxieca merged commit d507a93 into sonic-net:master Jan 17, 2020
yxieca pushed a commit that referenced this pull request Jan 17, 2020
@daall @yxieca
[aclorch] Validate that provided IN/OUT_PORTS are physical interfaces (…
e237e12 
…#1156)

* [aclorch] Validate that provided IN/OUT_PORTS are physical interfaces
- Add a check during the rule validation step to make sure provided IN/OUT_PORTS rules only include physical interfaces
- Add vs test cases for invalid IN/OUT_PORTS inputs

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

* Clarify test names

* Add extra delay for table teardown

* Check if new tests are causing tests to fail

* Check if new tests are causing tests to fail

* Check if ProducerStateTable is having problems

* Make delete ACL table test more strict

* Remove invalid interface test cases

* Undo change to delete test

* Undo change to delete test
abdosi pushed a commit that referenced this pull request Jan 21, 2020
@daall @abdosi
[aclorch] Validate that provided IN/OUT_PORTS are physical interfaces (…
3e53c3e 
…#1156)

* [aclorch] Validate that provided IN/OUT_PORTS are physical interfaces
- Add a check during the rule validation step to make sure provided IN/OUT_PORTS rules only include physical interfaces
- Add vs test cases for invalid IN/OUT_PORTS inputs

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

* Clarify test names

* Add extra delay for table teardown

* Check if new tests are causing tests to fail

* Check if new tests are causing tests to fail

* Check if ProducerStateTable is having problems

* Make delete ACL table test more strict

* Remove invalid interface test cases

* Undo change to delete test

* Undo change to delete test
lguohan pushed a commit that referenced this pull request Jan 30, 2020
@daall @lguohan
[aclorch] Validate that provided IN/OUT_PORTS are physical interfaces (…
32a9014 
…#1156)

* [aclorch] Validate that provided IN/OUT_PORTS are physical interfaces
- Add a check during the rule validation step to make sure provided IN/OUT_PORTS rules only include physical interfaces
- Add vs test cases for invalid IN/OUT_PORTS inputs

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

* Clarify test names

* Add extra delay for table teardown

* Check if new tests are causing tests to fail

* Check if new tests are causing tests to fail

* Check if ProducerStateTable is having problems

* Make delete ACL table test more strict

* Remove invalid interface test cases

* Undo change to delete test

* Undo change to delete test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yxieca yxieca yxieca approved these changes

@qiluo-msft qiluo-msft qiluo-msft left review comments

@lguohan lguohan Awaiting requested review from lguohan

Assignees

@daall daall

Labels
Enhancement ➕ Included in 201811 Branch Included in 201911 Branch Request for 201811 Branch request_for_201911_branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@daall @qiluo-msft @yxieca @abdosi