[aclorch] Validate that provided IN/OUT_PORTS are physical interfaces (…

…sonic-net#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
raphaelt-nvidia · Jan 21, 2020 · 3e53c3e · 3e53c3e
1 parent 770e617
commit 3e53c3e
Show file tree

Hide file tree

Showing 2 changed files with 86 additions and 6 deletions.
diff --git a/orchagent/aclorch.cpp b/orchagent/aclorch.cpp
@@ -206,6 +206,13 @@ bool AclRule::validateAddMatch(string attr_name, string attr_value)
                     SWSS_LOG_ERROR("Failed to locate port %s", alias.c_str());
                     return false;
                 }
+
+                if (port.m_type != Port::PHY)
+                {
+                    SWSS_LOG_ERROR("Cannot bind rule to %s: IN_PORTS can only match physical interfaces", alias.c_str());
+                    return false;
+                }
+
                 m_inPorts.push_back(port.m_port_id);
             }
 
@@ -230,6 +237,13 @@ bool AclRule::validateAddMatch(string attr_name, string attr_value)
                     SWSS_LOG_ERROR("Failed to locate port %s", alias.c_str());
                     return false;
                 }
+
+                if (port.m_type != Port::PHY)
+                {
+                    SWSS_LOG_ERROR("Cannot bind rule to %s: OUT_PORTS can only match physical interfaces", alias.c_str());
+                    return false;
+                }
+
                 m_outPorts.push_back(port.m_port_id);
             }
 

diff --git a/tests/test_acl.py b/tests/test_acl.py
@@ -329,19 +329,85 @@ def test_AclRuleInOutPorts(self, dvs, testlog):
         (status, fvs) = atbl.get(acl_entry[0])
         assert status == False
 
-    def test_AclTableDeletion(self, dvs, testlog):
+    def test_AclRuleInPortsNonExistingInterface(self, dvs, testlog):
+        self.setup_db(dvs)
+
+        # Create ACL rule with a completely wrong interface
+        tbl = swsscommon.Table(self.cdb, "ACL_RULE")
+        fvs = swsscommon.FieldValuePairs([("priority", "55"),
+                                          ("PACKET_ACTION", "FORWARD"),
+                                          ("IN_PORTS", "FOO_BAR_BAZ")])
+        tbl.set("test|foo_bar_baz", fvs)
+        time.sleep(1)
+
+        # Make sure no rules were created
+        atbl = swsscommon.Table(self.adb, "ASIC_STATE:SAI_OBJECT_TYPE_ACL_ENTRY")
+        keys = atbl.getKeys()
+        acl_entry = [k for k in keys if k not in dvs.asicdb.default_acl_entries]
+        assert len(acl_entry) == 0
+
+        # Create ACL rule with a correct interface and a completely wrong interface
+        tbl = swsscommon.Table(self.cdb, "ACL_RULE")
+        fvs = swsscommon.FieldValuePairs([("priority", "55"),
+                                          ("PACKET_ACTION", "FORWARD"),
+                                          ("IN_PORTS", "Ethernet0,FOO_BAR_BAZ")])
+        tbl.set("test|foo_bar_baz", fvs)
+        time.sleep(1)
+
+        # Make sure no rules were created
+        atbl = swsscommon.Table(self.adb, "ASIC_STATE:SAI_OBJECT_TYPE_ACL_ENTRY")
+        keys = atbl.getKeys()
+        acl_entry = [k for k in keys if k not in dvs.asicdb.default_acl_entries]
+        assert len(acl_entry) == 0
 
+        # Delete rule
+        tbl._del("test|foo_bar_baz")
+        time.sleep(1)
+
+    def test_AclRuleOutPortsNonExistingInterface(self, dvs, testlog):
         self.setup_db(dvs)
-        db = swsscommon.DBConnector(4, dvs.redis_sock, 0)
-        adb = swsscommon.DBConnector(1, dvs.redis_sock, 0)
 
-        # get ACL_TABLE in config db
-        tbl = swsscommon.Table(db, "ACL_TABLE")
+        # Create ACL rule with a completely wrong interface
+        tbl = swsscommon.Table(self.cdb, "ACL_RULE")
+        fvs = swsscommon.FieldValuePairs([("priority", "55"),
+                                          ("PACKET_ACTION", "FORWARD"),
+                                          ("OUT_PORTS", "FOO_BAR_BAZ")])
+        tbl.set("test|foo_bar_baz", fvs)
+        time.sleep(1)
+
+        # Make sure no rules were created
+        atbl = swsscommon.Table(self.adb, "ASIC_STATE:SAI_OBJECT_TYPE_ACL_ENTRY")
+        keys = atbl.getKeys()
+        acl_entry = [k for k in keys if k not in dvs.asicdb.default_acl_entries]
+        assert len(acl_entry) == 0
+
+        # Create ACL rule with a correct interface and a completely wrong interface
+        tbl = swsscommon.Table(self.cdb, "ACL_RULE")
+        fvs = swsscommon.FieldValuePairs([("priority", "55"),
+                                          ("PACKET_ACTION", "FORWARD"),
+                                          ("OUT_PORTS", "Ethernet0,FOO_BAR_BAZ")])
+        tbl.set("test|foo_bar_baz", fvs)
+        time.sleep(1)
+
+        # Make sure no rules were created
+        atbl = swsscommon.Table(self.adb, "ASIC_STATE:SAI_OBJECT_TYPE_ACL_ENTRY")
+        keys = atbl.getKeys()
+        acl_entry = [k for k in keys if k not in dvs.asicdb.default_acl_entries]
+        assert len(acl_entry) == 0
+
+        # Delete rule
+        tbl._del("test|foo_bar_baz")
+        time.sleep(1)
+
+    def test_AclTableDeletion(self, dvs, testlog):
+        self.setup_db(dvs)
+
+        tbl = swsscommon.Table(self.cdb, "ACL_TABLE")
         tbl._del("test")
 
         time.sleep(1)
 
-        atbl = swsscommon.Table(adb, "ASIC_STATE:SAI_OBJECT_TYPE_ACL_TABLE")
+        atbl = swsscommon.Table(self.adb, "ASIC_STATE:SAI_OBJECT_TYPE_ACL_TABLE")
         keys = atbl.getKeys()
         # only the default table was left along with DTel tables
         assert len(keys) >= 1