diff --git a/acl_loader/main.py b/acl_loader/main.py index 6a5311be5136..91cb750c3318 100644 --- a/acl_loader/main.py +++ b/acl_loader/main.py @@ -559,6 +559,19 @@ def convert_input_interface(self, table_name, rule_idx, rule): return rule_props + def validate_rule_fields(self, rule_props): + protocol = rule_props.get("IP_PROTOCOL") + + if protocol: + if "TCP_FLAGS" in rule_props and protocol != 6: + raise AclLoaderException("IP_PROTOCOL={} is not TCP, but TCP flags were provided".format(protocol)) + + if ("ICMP_TYPE" in rule_props or "ICMP_CODE" in rule_props) and protocol != 1: + raise AclLoaderException("IP_PROTOCOL={} is not ICMP, but ICMP fields were provided".format(protocol)) + + if ("ICMPV6_TYPE" in rule_props or "ICMPV6_CODE" in rule_props) and protocol != 58: + raise AclLoaderException("IP_PROTOCOL={} is not ICMPV6, but ICMPV6 fields were provided".format(protocol)) + def convert_rule_to_db_schema(self, table_name, rule): """ Convert rules format from openconfig ACL to Config DB schema @@ -579,6 +592,8 @@ def convert_rule_to_db_schema(self, table_name, rule): deep_update(rule_props, self.convert_transport(table_name, rule_idx, rule)) deep_update(rule_props, self.convert_input_interface(table_name, rule_idx, rule)) + self.validate_rule_fields(rule_props) + return rule_data def deny_rule(self, table_name): @@ -591,7 +606,7 @@ def deny_rule(self, table_name): rule_data = {(table_name, "DEFAULT_RULE"): rule_props} rule_props["PRIORITY"] = str(self.min_priority) rule_props["PACKET_ACTION"] = "DROP" - if 'v6' in table_name.lower(): + if self.is_table_ipv6(table_name): rule_props["IP_TYPE"] = "IPV6ANY" # ETHERTYPE is not supported for DATAACLV6 else: rule_props["ETHER_TYPE"] = str(self.ethertype_map["ETHERTYPE_IPV4"]) diff --git a/tests/acl_input/acl1.json b/tests/acl_input/acl1.json index adfde60f3a84..4af13400fa5e 100644 --- a/tests/acl_input/acl1.json +++ b/tests/acl_input/acl1.json @@ -193,7 +193,7 @@ } } }, - "DATAACLV6": { + "DATAACL_2": { "acl-entries": { "acl-entry": { "1": { diff --git a/tests/acl_input/icmp_bad_protocol_number.json b/tests/acl_input/icmp_bad_protocol_number.json new file mode 100644 index 000000000000..1bc1d6ef48de --- /dev/null +++ b/tests/acl_input/icmp_bad_protocol_number.json @@ -0,0 +1,37 @@ +{ + "acl": { + "acl-sets": { + "acl-set": { + "DATAACL": { + "acl-entries": { + "acl-entry": { + "1": { + "config": { + "sequence-id": 1 + }, + "actions": { + "config": { + "forwarding-action": "ACCEPT" + } + }, + "ip": { + "config": { + "protocol": "IP_UDP", + "source-ip-address": "20.0.0.2/32", + "destination-ip-address": "30.0.0.3/32" + } + }, + "icmp": { + "config": { + "type": "3", + "code": "0" + } + } + } + } + } + } + } + } + } +} diff --git a/tests/acl_input/icmpv6_bad_protocol_number.json b/tests/acl_input/icmpv6_bad_protocol_number.json new file mode 100644 index 000000000000..a4494ecec540 --- /dev/null +++ b/tests/acl_input/icmpv6_bad_protocol_number.json @@ -0,0 +1,37 @@ +{ + "acl": { + "acl-sets": { + "acl-set": { + "DATAACL_2": { + "acl-entries": { + "acl-entry": { + "1": { + "config": { + "sequence-id": 1 + }, + "actions": { + "config": { + "forwarding-action": "ACCEPT" + } + }, + "ip": { + "config": { + "protocol": "IP_UDP", + "source-ip-address": "20.0.0.2/32", + "destination-ip-address": "30.0.0.3/32" + } + }, + "icmp": { + "config": { + "type": "3", + "code": "0" + } + } + } + } + } + } + } + } + } +} diff --git a/tests/acl_input/tcp_bad_protocol_number.json b/tests/acl_input/tcp_bad_protocol_number.json new file mode 100644 index 000000000000..3510a804641d --- /dev/null +++ b/tests/acl_input/tcp_bad_protocol_number.json @@ -0,0 +1,36 @@ +{ + "acl": { + "acl-sets": { + "acl-set": { + "DATAACL": { + "acl-entries": { + "acl-entry": { + "1": { + "config": { + "sequence-id": 1 + }, + "actions": { + "config": { + "forwarding-action": "ACCEPT" + } + }, + "ip": { + "config": { + "protocol": "IP_UDP", + "source-ip-address": "20.0.0.2/32", + "destination-ip-address": "30.0.0.3/32" + } + }, + "transport": { + "config": { + "tcp-flags": ["TCP_ACK"] + } + } + } + } + } + } + } + } + } +} diff --git a/tests/acl_loader_test.py b/tests/acl_loader_test.py index 9a33819eb2cb..e2ee414a5037 100644 --- a/tests/acl_loader_test.py +++ b/tests/acl_loader_test.py @@ -100,7 +100,7 @@ def test_icmpv6_translation(self, acl_loader): acl_loader.rules_info = {} acl_loader.load_rules_from_file(os.path.join(test_path, 'acl_input/acl1.json')) print(acl_loader.rules_info) - assert acl_loader.rules_info[("DATAACLV6", "RULE_1")] == { + assert acl_loader.rules_info[("DATAACL_2", "RULE_1")] == { "ICMPV6_TYPE": 1, "ICMPV6_CODE": 0, "IP_PROTOCOL": 58, @@ -109,7 +109,7 @@ def test_icmpv6_translation(self, acl_loader): "PACKET_ACTION": "FORWARD", "PRIORITY": "9999" } - assert acl_loader.rules_info[("DATAACLV6", "RULE_100")] == { + assert acl_loader.rules_info[("DATAACL_2", "RULE_100")] == { "ICMPV6_TYPE": 128, "IP_PROTOCOL": 58, "SRC_IPV6": "::1/128", @@ -147,3 +147,19 @@ def test_icmp_code_not_a_number(self, acl_loader): with pytest.raises(ValueError): acl_loader.rules_info = {} acl_loader.load_rules_from_file(os.path.join(test_path, 'acl_input/illegal_icmp_code_nan.json')) + + def test_icmp_fields_with_non_icmp_protocol(self, acl_loader): + acl_loader.rules_info = {} + acl_loader.load_rules_from_file(os.path.join(test_path, 'acl_input/icmp_bad_protocol_number.json')) + assert not acl_loader.rules_info.get("RULE_1") + + def ttest_icmp_fields_with_non_icmpv6_protocol(self, acl_loader): + acl_loader.rules_info = {} + acl_loader.load_rules_from_file(os.path.join(test_path, 'acl_input/icmpv6_bad_protocol_number.json')) + assert not acl_loader.rules_info.get("RULE_1") + + + def test_icmp_fields_with_non_tcp_protocol(self, acl_loader): + acl_loader.rules_info = {} + acl_loader.load_rules_from_file(os.path.join(test_path, 'acl_input/tcp_bad_protocol_number.json')) + assert not acl_loader.rules_info.get("RULE_1") diff --git a/tests/mock_tables/config_db.json b/tests/mock_tables/config_db.json index a8545a5a80de..dc951f977d74 100644 --- a/tests/mock_tables/config_db.json +++ b/tests/mock_tables/config_db.json @@ -400,8 +400,8 @@ "ports@": "PortChannel0002,PortChannel0005,PortChannel0008,PortChannel0011,PortChannel0014,PortChannel0017,PortChannel0020,PortChannel0023,Ethernet64,Ethernet68,Ethernet72,Ethernet76,Ethernet80,Ethernet84,Ethernet88,Ethernet92,Ethernet96,Ethernet100,Ethernet104,Ethernet108,Ethernet112,Ethernet116,Ethernet120,Ethernet124", "type": "L3" }, - "ACL_TABLE|DATAACLV6": { - "policy_desc": "DATAACLV6", + "ACL_TABLE|DATAACL_2": { + "policy_desc": "DATAACL_2", "ports@": "PortChannel0002,PortChannel0005,PortChannel0008,PortChannel0011,PortChannel0014,PortChannel0017,PortChannel0020,PortChannel0023,Ethernet64,Ethernet68,Ethernet72,Ethernet76,Ethernet80,Ethernet84,Ethernet88,Ethernet92,Ethernet96,Ethernet100,Ethernet104,Ethernet108,Ethernet112,Ethernet116,Ethernet120,Ethernet124", "type": "L3V6" },