Add script null_route_helper #1718

bingwang-ms · 2021-07-19T03:24:29Z

This is a backport of #1737

Signed-off-by: bingwang [email protected]

What I did

This PR introduced a new helper script null_route_helper.

null_route_helper is a utility for blocking and unblocking traffic from given source ip_prefix on ACL tables.
The block operation will insert a DENY rule at the top of the table. The unblock operation will remove an existing DENY rule that has been created by the block operation (i.e. it does NOT insert an ALLOW rule, only removes DENY rules).
Since SONiC supports multi ACL rules share the same priority, all ACL rules created by null_route_helper will use the highest priority(9999).

Example:

Block traffic from 10.2.3.4/32:
./null_route_helper acl_table_name block 10.2.3.4/32

Unblock all traffic from 10.2.3.4/32:
./null_route_helper acl_table_name unblock 10.2.3.4/32

How I did it

The feature is implemented with applying ACL rules.

How to verify it

Verified with both unit test and traffic test

tests/null_route_helper_test.py::test_ip_validation PASSED                                                                                                                                      [ 11%]
tests/null_route_helper_test.py::test_ip_ver PASSED                                                                                                                                             [ 22%]
tests/null_route_helper_test.py::test_check_table_existence PASSED                                                                                                                              [ 33%]
tests/null_route_helper_test.py::test_build_rule PASSED                                                                                                                                         [ 44%]
tests/null_route_helper_test.py::test_get_rule PASSED                                                                                                                                           [ 55%]
tests/null_route_helper_test.py::test_run_when_table_absent PASSED                                                                                                                              [ 66%]
tests/null_route_helper_test.py::test_run_with_invalid_ip PASSED                                                                                                                                [ 77%]
tests/null_route_helper_test.py::test_block PASSED                                                                                                                                              [ 88%]
tests/null_route_helper_test.py::test_unblock PASSED                                                                                                                                            [100%]

Name                                          Stmts   Miss Branch BrPart  Cover
-------------------------------------------------------------------------------
scripts/null_route_helper                        96      4     24      1    96%

The coverage is not 100 since below line can't be covered

if __name__ == "__main__":
    cli()

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)

Signed-off-by: bingwang <[email protected]>

bingwang-ms · 2021-07-19T07:03:09Z

/azp run

azure-pipelines · 2021-07-19T07:03:20Z

Azure Pipelines successfully started running 1 pipeline(s).

prsunny

Please update command reference guide.

prsunny · 2021-07-20T00:21:13Z

scripts/null_route_helper

+    return block_rules
+
+
+def validate_input(ip_address):


Can you reuse anything from utils?

The validate_input in this PR will add prefix len for IP addresses that don't explicitly set prefix. For example, 1.2.3.4 is changed to 1.2.3.4/32. The common utils may not do this.

prsunny · 2021-07-20T00:22:37Z

scripts/null_route_helper

+    rule_value = list(rule.values())[0] if rule else None
+    if action == ACTION_ALLOW:
+        if not rule:
+            return


I think here we should be adding a rule even if it doesn't exist, may be for managing priorities. Why is it skipped here?

Because it will be allowed anyway?

So no need to add a rule.

I think here we should be adding a rule even if it doesn't exist, may be for managing priorities. Why is it skipped here?

I skip here since there will be a default ALLOW rule in the pre-created ACL table. Is it necessary to add an ALLOW rule for a certain prefix?

my concern is, lets say user specifies "unblock 1.2.3.4/32" and if there is some rule in between that says 1.2.3.0/24 drop. In this case if we don't explicitly add the "allow" rule, it will get dropped.

That's a good catch. We discuss about it offline, and confirm that all prefix len are 32 (For IPv4) or 128 (IPv6). Based on this acknowledge, it shouldn't be a issue. I will update the validate_input interface to ensure that. Thanks

qiluo-msft · 2021-07-22T06:04:36Z

scripts/null_route_helper

@@ -0,0 +1,216 @@
+#!/usr/bin/env python3


python3

We need this feature on 201911 branch, so please make sure it could run on python2. Seems we could not cherry-pick directly. #WontFix

If you tested the same script with both python2/3, you can change it to

#!/usr/bin/env python

#!/usr/bin/env python doesn't work on 202012 image since it will point to python2, and several packages are missing. I think the only way to solve the issue is to create another PR for 201911 or earlier image. It's not a clean cherry-pick.

sonic-utilities can no longer be cherry-picked to 201911. We faced it few times and suggest to create a new PR to 201911

qiluo-msft · 2021-07-22T06:19:59Z

scripts/null_route_helper

+./null_route_helper acl_table_name block 10.2.3.4/32
+
+Unblock all traffic from 10.2.3.4/32:
+./null_route_helper acl_table_name unblock 10.2.3.4/32


unblock

./null_route_helper acl_table_name unblock 10.2.3.4/32

There is no action to list all the exising rules. So if a user want to unblock all, it's inconvenient #Closed

It's a good suggestion. Thanks.
I was thinking which one is better, a list command or a unblock all command? Any suggestions?

list command will be useful in many cases, including

unblock all

verify existing rules

I added a new interface list. Thanks

Add the list command in the usage comment?

qiluo-msft · 2021-07-22T06:20:57Z

scripts/null_route_helper

+import click
+import ipaddress
+
+from swsssdk import ConfigDBConnector


swsssdk

from swsssdk import ConfigDBConnector

Please use swsscommon on master branch. #Closed

Updated. Thanks

qiluo-msft · 2021-07-22T07:26:53Z

scripts/null_route_helper

+    """
+    Check the existence of pre-created ACL tables
+    """
+    target_table = configdb.get_entry(CONFIG_DB_ACL_TABLE_TABLE, table_name)


table_name

target_table = configdb.get_entry(CONFIG_DB_ACL_TABLE_TABLE, table_name)

Both arguments are table names, confusing. #Closed

Updated. Thanks

qiluo-msft · 2021-07-22T07:27:42Z

scripts/null_route_helper

+    return ipaddress.ip_network(ip_prefix, False).version
+
+
+def check_table_existence(configdb, table_name):


check_table_existence

def check_table_existence(configdb, table_name):

-> get_config_db_acl_table_subtable or something similiar? #Closed

Updated. Thanks

qiluo-msft · 2021-07-22T07:30:28Z

scripts/null_route_helper

+
+
+def ip_ver(ip_prefix):
+    return ipaddress.ip_network(ip_prefix, False).version


ip_network

If you keep this variable, you can reuse it in future, such as check prefix length. #Closed

I don't like to add a global variable to save the ip version. Any better ideas?

I don't like to add a global variable to save the return value. Any better ideas?

I don't like global var either. To clarify, I mean the object of ip_network is very useful, and you could deduce many attributes from it such as prefix length, not only the version.

qiluo-msft · 2021-07-22T07:31:18Z

scripts/null_route_helper

+        "PACKET_ACTION": "DROP"
+    }
+    if ip_ver(src_ip) == 4:
+        rule['ETHER_TYPE'] = str(0x0800)


0x0800

rule['ETHER_TYPE'] = str(0x0800)

Explain magic number? #Closed

Updated. Thanks

qiluo-msft · 2021-07-22T07:47:09Z

scripts/null_route_helper

+    For 'DENY', an 'DROP' rule for given ip_prefix will be added if not existed
+    For 'ALLOW', we will try to remove the existing 'DENY' rule, and nothing is changed if not existed
+    """
+    check_table_existence(configdb, acl_table_name)


check_table_existence

check_table_existence(configdb, acl_table_name)

If you store the return value, will you optimize below code? #WontFix

qiluo-msft · 2021-07-22T07:56:09Z

scripts/null_route_helper

+    if action == ACTION_ALLOW:
+        if not rule:
+            return
+        configdb.mod_entry(CONFIG_DB_ACL_RULE_TABLE, rule_key, None)


mod_entry

Do you actually delete the rule? add some comment to help reading. #Closed

Yes. I checked the code and confirm the entry will be deleted. Also verified in test.
https://github.com/Azure/sonic-swss-common/blob/bf8c832cf1c7a6e72d7b1c843888ffb4a27088c8/common/configdb.cpp#L96

Signed-off-by: bingwang <[email protected]>

qiluo-msft · 2021-07-22T07:57:31Z

scripts/null_route_helper

+            configdb.set_entry(CONFIG_DB_ACL_RULE_TABLE, new_rule_key, new_rule_value)
+
+
+def helper(table_name, ip_prefix, action):


helper

def helper(table_name, ip_prefix, action):

Use a better name? #Closed

Updated. Thanks

Signed-off-by: bingwang <[email protected]>

bingwang-ms · 2021-07-30T09:40:59Z

Hi @qiluo-msft @prsunny . All comments were addressed. And the unit test code was also checked in. Please help to review. Thanks

Signed-off-by: bingwang <[email protected]>

qiluo-msft · 2021-07-30T15:25:47Z

scripts/null_route_helper

+Example:
+
+Block traffic from 10.2.3.4/32:
+./null_route_helper block acl_table_name 10.2.3.4/32


/32

we can remove the prefix completely from CLI

Thanks, I will update.

Signed-off-by: bingwang <[email protected]>

qiluo-msft

Block. This PR should go master first, and then 201911 and later branches.

bingwang-ms · 2021-08-02T03:06:31Z

Block. This PR should go master first, and then 201911 and later branches.

OK. I will hold on merging this PR and create a same PR to master branch.

bingwang-ms · 2021-08-02T03:49:19Z

Same PR created for master branch #1737

To include following changes: * d84a8cc 2021-08-05 | [fast-reboot] revert the change of disabling counter polling before fast-reboot (sonic-net#1744) (HEAD -> 202012, github/202012) [Ying Xie] * e900bc5 2021-08-04 | Add script null_route_helper (sonic-net#1718) [bingwang-ms] * 85f14e1 2021-08-02 | disk_check updates: (sonic-net#1736) [Renuka Manavalan] * d68ac1c 2021-05-27 | [console][show] Force refresh all lines status during show line (sonic-net#1641) [Blueve] * a0e417f 2021-04-25 | [console] Display success message after line cleared (sonic-net#1579) [Blueve] * 0c6bb27 2021-04-07 | [console] Include Flow Control status in show line result (sonic-net#1549) [Blueve]

Add script null_route_heler

19baad0

Signed-off-by: bingwang <[email protected]>

bingwang-ms mentioned this pull request Jul 19, 2021

Add test script for null_route_helper sonic-net/sonic-mgmt#3812

Merged

4 tasks

bingwang-ms requested a review from qiluo-msft July 19, 2021 07:08

prsunny changed the title ~~Add script null_route_heler~~ Add script null_route_helper Jul 19, 2021

prsunny reviewed Jul 20, 2021

View reviewed changes

qiluo-msft reviewed Jul 22, 2021

View reviewed changes

Fix aclshow_test

45896c3

Signed-off-by: bingwang <[email protected]>

qiluo-msft reviewed Jul 22, 2021

View reviewed changes

bingwang-ms force-pushed the null_route_helper branch from 7d82d49 to 45896c3 Compare July 22, 2021 07:57

Address comments

047a5b4

Signed-off-by: bingwang <[email protected]>

bingwang-ms added 2 commits July 30, 2021 03:08

Limit input ip prefix len to 32 or 128

4df2bce

Signed-off-by: bingwang <[email protected]>

Update usage

574be48

Signed-off-by: bingwang <[email protected]>

qiluo-msft approved these changes Jul 30, 2021

View reviewed changes

qiluo-msft reviewed Jul 30, 2021

View reviewed changes

prsunny approved these changes Jul 30, 2021

View reviewed changes

Remove 32 from comments

330fac7

Signed-off-by: bingwang <[email protected]>

qiluo-msft approved these changes Aug 2, 2021

View reviewed changes

qiluo-msft requested changes Aug 2, 2021

View reviewed changes

qiluo-msft approved these changes Aug 3, 2021

View reviewed changes

qiluo-msft added Request for 201911 Branch and removed Request for 201911 Branch labels Aug 3, 2021

bingwang-ms merged commit e900bc5 into sonic-net:202012 Aug 4, 2021

antonptashnik mentioned this pull request Sep 2, 2021

fixed test_null_route_helper: changed a default ACL rule sonic-net/sonic-mgmt#4163

Closed

4 tasks

		return ipaddress.ip_network(ip_prefix, False).version


		def check_table_existence(configdb, table_name):



		def ip_ver(ip_prefix):
		return ipaddress.ip_network(ip_prefix, False).version

		configdb.set_entry(CONFIG_DB_ACL_RULE_TABLE, new_rule_key, new_rule_value)


		def helper(table_name, ip_prefix, action):

Add script null_route_helper #1718

Add script null_route_helper #1718

Conversation

bingwang-ms commented Jul 19, 2021 • edited by qiluo-msft Loading

What I did

How I did it

How to verify it

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)

bingwang-ms commented Jul 19, 2021

azure-pipelines bot commented Jul 19, 2021

prsunny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Jul 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Jul 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Jul 22, 2021 • edited Loading

Choose a reason for hiding this comment

./null_route_helper acl_table_name unblock 10.2.3.4/32

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Jul 22, 2021 • edited Loading

Choose a reason for hiding this comment

from swsssdk import ConfigDBConnector

Choose a reason for hiding this comment

qiluo-msft Jul 22, 2021 • edited Loading

Choose a reason for hiding this comment

target_table = configdb.get_entry(CONFIG_DB_ACL_TABLE_TABLE, table_name)

Choose a reason for hiding this comment

qiluo-msft Jul 22, 2021 • edited Loading

Choose a reason for hiding this comment

def check_table_existence(configdb, table_name):

Choose a reason for hiding this comment

qiluo-msft Jul 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Jul 22, 2021 • edited Loading

Choose a reason for hiding this comment

rule['ETHER_TYPE'] = str(0x0800)

Choose a reason for hiding this comment

qiluo-msft Jul 22, 2021 • edited Loading

Choose a reason for hiding this comment

check_table_existence(configdb, acl_table_name)

qiluo-msft Jul 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Jul 22, 2021 • edited Loading

Choose a reason for hiding this comment

def helper(table_name, ip_prefix, action):

Choose a reason for hiding this comment

bingwang-ms commented Jul 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft left a comment

Choose a reason for hiding this comment

bingwang-ms commented Aug 2, 2021

bingwang-ms commented Aug 2, 2021

bingwang-ms commented Jul 19, 2021 •

edited by qiluo-msft

Loading

qiluo-msft Jul 23, 2021 •

edited

Loading

qiluo-msft Jul 22, 2021 •

edited

Loading

qiluo-msft Jul 22, 2021 •

edited

Loading

qiluo-msft Jul 22, 2021 •

edited

Loading

qiluo-msft Jul 22, 2021 •

edited

Loading

qiluo-msft Jul 22, 2021 •

edited

Loading

qiluo-msft Jul 22, 2021 •

edited

Loading

qiluo-msft Jul 22, 2021 •

edited

Loading

qiluo-msft Jul 22, 2021 •

edited

Loading

qiluo-msft Jul 22, 2021 •

edited

Loading

qiluo-msft Jul 22, 2021 •

edited

Loading