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

[caclmgrd]: Add infrastructure to support adding feature specific acls #11367

Merged
merged 5 commits into from
Jul 13, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
11 changes: 11 additions & 0 deletions src/sonic-host-services/scripts/caclmgrd
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ class ControlPlaneAclManager(daemon_base.DaemonBase):
Attributes:
config_db: Handle to Config Redis database via SwSS SDK
"""
FEATURE_TABLE = "FEATURE"
ACL_TABLE = "ACL_TABLE"
ACL_RULE = "ACL_RULE"
DEVICE_METADATA_TABLE = "DEVICE_METADATA"
Expand Down Expand Up @@ -117,6 +118,10 @@ class ControlPlaneAclManager(daemon_base.DaemonBase):
self.namespace_docker_mgmt_ip = {}
self.namespace_docker_mgmt_ipv6 = {}

# Get all features that are present {feature_name : True/False}
self.feature_present = {}
self.update_feature_present()

metadata = self.config_db_map[DEFAULT_NAMESPACE].get_table(self.DEVICE_METADATA_TABLE)
if 'subtype' in metadata['localhost'] and metadata['localhost']['subtype'] == 'DualToR':
self.DualToR = True
Expand Down Expand Up @@ -201,6 +206,12 @@ class ControlPlaneAclManager(daemon_base.DaemonBase):
tcp_flags_str = tcp_flags_str[:-1]
return tcp_flags_str

def update_feature_present(self):
Copy link
Collaborator

@qiluo-msft qiluo-msft Jul 8, 2022

Choose a reason for hiding this comment

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

update_feature_present

Could you add unit tests to cover new code? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added unit-test and mock data for the unit-test.

feature_tb_info = self.config_db_map[DEFAULT_NAMESPACE].get_table(self.FEATURE_TABLE)
if feature_tb_info:
for k, v in feature_tb_info.items():
self.feature_present[k] = True
Copy link
Collaborator

@qiluo-msft qiluo-msft Jul 8, 2022

Choose a reason for hiding this comment

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

feature_present

Seems this is a generic oneshot cache. Suggest leverage common library like cached_property. ref: https://www.geeksforgeeks.org/python-functools-cached_property/ #WontFix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you for the pointer. Currently, we are updating this feature_present map with the features that are present only once during initialization so it is not repeatedlyinvoked.
Also, if we use cached_property, we will invoke the function once, which could be in per-thread context, which might cause issues. So not using cached_property in this case.


def generate_block_ip2me_traffic_iptables_commands(self, namespace):
INTERFACE_TABLE_NAME_LIST = [
"LOOPBACK_INTERFACE",
Expand Down
50 changes: 50 additions & 0 deletions src/sonic-host-services/tests/caclmgrd/caclmgrd_feature_test.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
import os
import sys
import swsscommon

from parameterized import parameterized
from sonic_py_common.general import load_module_from_source
from unittest import TestCase, mock
from pyfakefs.fake_filesystem_unittest import patchfs

from .test_bfd_vectors import CACLMGRD_BFD_TEST_VECTOR
from tests.common.mock_configdb import MockConfigDb
from unittest.mock import MagicMock, patch

DBCONFIG_PATH = '/var/run/redis/sonic-db/database_config.json'

class TestFeature(TestCase):
"""
Test caclmgrd feature present
"""
def setUp(self):
swsscommon.swsscommon.ConfigDBConnector = MockConfigDb
test_path = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
modules_path = os.path.dirname(test_path)
scripts_path = os.path.join(modules_path, "scripts")
sys.path.insert(0, modules_path)
caclmgrd_path = os.path.join(scripts_path, 'caclmgrd')
self.caclmgrd = load_module_from_source('caclmgrd', caclmgrd_path)

@parameterized.expand(CACLMGRD_BFD_TEST_VECTOR)
@patchfs
def test_feature_present(self, test_name, test_data, fs):
if not os.path.exists(DBCONFIG_PATH):
fs.create_file(DBCONFIG_PATH) # fake database_config.json

MockConfigDb.set_config_db(test_data["config_db"])

with mock.patch("caclmgrd.subprocess") as mocked_subprocess:
popen_mock = mock.Mock()
popen_attrs = test_data["popen_attributes"]
popen_mock.configure_mock(**popen_attrs)
mocked_subprocess.Popen.return_value = popen_mock
mocked_subprocess.PIPE = -1

call_rc = test_data["call_rc"]
mocked_subprocess.call.return_value = call_rc

caclmgrd_daemon = self.caclmgrd.ControlPlaneAclManager("caclmgrd")
caclmgrd_daemon.update_feature_present()
self.assertTrue("bgp" in caclmgrd_daemon.feature_present)
self.assertEqual(caclmgrd_daemon.feature_present["bgp"], True)
6 changes: 6 additions & 0 deletions src/sonic-host-services/tests/caclmgrd/test_bfd_vectors.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,12 @@
"type": "ToRRouter",
}
},
"FEATURE": {
"bgp": {
"auto_restart": "enabled",
"state": "enabled",
}
},
},
"expected_subprocess_calls": [
call("iptables -I INPUT 2 -p udp -m multiport --dports 3784,4784 -j ACCEPT", shell=True, universal_newlines=True, stdout=subprocess.PIPE),
Expand Down
24 changes: 24 additions & 0 deletions src/sonic-host-services/tests/caclmgrd/test_dhcp_vectors.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
"type": "ToRRouter",
}
},
"FEATURE": {
},
},
"mux_update": [
("Ethernet4", {"state": "active"}),
Expand Down Expand Up @@ -42,6 +44,8 @@
"type": "ToRRouter",
}
},
"FEATURE": {
},
},
"mux_update": [
("Ethernet4", {"state": "active"}),
Expand All @@ -67,6 +71,8 @@
"type": "ToRRouter",
}
},
"FEATURE": {
},
},
"mux_update": [
("Ethernet4", {"state": "active"}),
Expand All @@ -93,6 +99,8 @@
"type": "ToRRouter",
}
},
"FEATURE": {
},
},
"mux_update": [
("Ethernet4", {"state": "active"}),
Expand All @@ -117,6 +125,8 @@
"type": "ToRRouter",
}
},
"FEATURE": {
},
},
"mux_update": [
("Ethernet4", {"state": "standby"}),
Expand All @@ -143,6 +153,8 @@
"type": "ToRRouter",
}
},
"FEATURE": {
},
},
"mux_update": [
("Ethernet4", {"state": "standby"}),
Expand All @@ -167,6 +179,8 @@
"type": "ToRRouter",
}
},
"FEATURE": {
},
},
"mux_update": [
("Ethernet4", {"state": "standby"}),
Expand Down Expand Up @@ -195,6 +209,8 @@
"type": "ToRRouter",
}
},
"FEATURE": {
},
},
"mux_update": [
("Ethernet4", {"state": "standby"}),
Expand All @@ -220,6 +236,8 @@
"type": "ToRRouter",
}
},
"FEATURE": {
},
},
"mux_update": [
("Ethernet4", {"state": "unknown"}),
Expand Down Expand Up @@ -248,6 +266,8 @@
"type": "ToRRouter",
}
},
"FEATURE": {
},
},
"mux_update": [
("Ethernet4", {"state": "unknown"}),
Expand All @@ -273,6 +293,8 @@
"type": "ToRRouter",
}
},
"FEATURE": {
},
},
"mux_update": [
("Ethernet4", {"state": "unknown"}),
Expand All @@ -299,6 +321,8 @@
"type": "ToRRouter",
}
},
"FEATURE": {
},
},
"mux_update": [
("Ethernet4", {"state": "unknown"}),
Expand Down