Skip to content

fixed test_null_route_helper: changed a default ACL rule #4163

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

Closed
antonptashnik wants to merge 1 commit into from
Closed

fixed test_null_route_helper: changed a default ACL rule #4163

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions tests/acl/null_route/test_null_route_helper.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,11 @@ def apply_pre_defined_rules(rand_selected_dut, create_acl_table):
rand_selected_dut.shell("acl-loader update full " + ACL_JSON_FILE_DEST)
# Wait 5 seconds for ACL rule creation
time.sleep(5)
# remove default DROP rule to make ACCEPT by default
Copy link
Copy Markdown

@SavchukRomanLv SavchukRomanLv Sep 2, 2021

Choose a reason for hiding this comment

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

Would it be better to change default rule action from DROP to ACCEPT in case if adding this to config and than deleting?

Copy link
Copy Markdown
Contributor Author

@antonptashnik antonptashnik Sep 2, 2021

Choose a reason for hiding this comment

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

not sure how to do that. The applied config file does not contain default rule, it seems to be added implicitly at the moment when we apply the config

Copy link
Copy Markdown
Collaborator

@bingwang-ms bingwang-ms Sep 3, 2021

Choose a reason for hiding this comment

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

Thanks for the update. Actually, the default DROP rule is expected. It's added automatically in INGRESS ACL. To defy that behavior, we will add a default FORWARD rule in the pre-loaded ACL table.
https://github.com/Azure/sonic-mgmt/blob/34b3766a4259853a71bde90a94ecbed5921e758b/tests/acl/null_route/acl.json#L115-L128

Copy link
Copy Markdown
Collaborator

@bingwang-ms bingwang-ms Sep 3, 2021

Choose a reason for hiding this comment

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

Could you please provide more details about the test failure? I can do a debug locally. I didn't see any failure from the last update. Thanks so much~

Copy link
Copy Markdown
Contributor Author

@antonptashnik antonptashnik Sep 6, 2021

Choose a reason for hiding this comment

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

@bingwang-ms the test fails with the first dataset
("1.2.3.4", "", FORWARD), # Should be forwared in default

It is expected to be forwarded but seems like default DROP applies and a packet is dropped.

Copy link
Copy Markdown

@SavchukRomanLv SavchukRomanLv Sep 27, 2021

Choose a reason for hiding this comment

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

@bingwang-ms could you please take a look?

Copy link
Copy Markdown
Collaborator

@bingwang-ms bingwang-ms Sep 28, 2021

Choose a reason for hiding this comment

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

@bingwang-ms could you please take a look?

Sorry for missing the comment before.
The packet with src_ip 1.2.3.4 is forwarded because we have this rule in acl.json

"9998": {
                "actions": {
                  "config": {
                    "forwarding-action": "ACCEPT"
                  }
                },
                "config": {
                  "sequence-id": 9998
                },
                "ip": {
                  "config": {
                    "source-ip-address": "0.0.0.0/0",
                    "destination-ip-address": "0.0.0.0/0"
                  }
                }
              }
            }
          },

So there is no need to delete the default DROP rule since the above rule has a higher priority.

Copy link
Copy Markdown
Contributor Author

@antonptashnik antonptashnik Oct 7, 2021

Choose a reason for hiding this comment

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

seems you're right, the mentioned rule should match before a default drop rule. Thanks !

rand_selected_dut.shell('sonic-db-cli CONFIG_DB del "ACL_RULE|{}|DEFAULT_RULE"'.format(ACL_TABLE_NAME_V4))
rand_selected_dut.shell('sonic-db-cli CONFIG_DB del "ACL_RULE|{}|DEFAULT_RULE"'.format(ACL_TABLE_NAME_V6))
time.sleep(5)

yield
# Clear ACL rules
rand_selected_dut.shell('sonic-db-cli CONFIG_DB keys "ACL_RULE|{}*" | xargs sonic-db-cli CONFIG_DB del'.format(ACL_TABLE_NAME_V4))
Expand Down