[macsecorch]: Fix MACsec SC creating before MACsec port enabling (#2087)

* Update MACsec SC attributes after MACsec SC creating

Signed-off-by: Ze Gan <[email protected]>

* Add unittest

Signed-off-by: Ze Gan <[email protected]>

* teardown unittest

Signed-off-by: Ze Gan <[email protected]>

* polish name

Signed-off-by: Ze Gan <[email protected]>

Author: Pterosaur

orchagent/macsecorch.cpp

@@ -1183,6 +1183,18 @@ bool MACsecOrch::updateMACsecPort(MACsecPort &macsec_port, const TaskArgs &port_
     if (get_value(port_attr, "enable_encrypt", alpha_boolean))
     {
         macsec_port.m_enable_encrypt = alpha_boolean.operator bool();
+        if (!updateMACsecSCs(
+                macsec_port,
+                [&macsec_port, this](MACsecOrch::MACsecSC &macsec_sc)
+                {
+                    sai_attribute_t attr;
+                    attr.id = SAI_MACSEC_SC_ATTR_ENCRYPTION_ENABLE;
+                    attr.value.booldata = macsec_port.m_enable_encrypt;
+                    return this->updateMACsecAttr(SAI_OBJECT_TYPE_MACSEC_SC, macsec_sc.m_sc_id, attr);
+                }))
+        {
+            return false;
+        }
     }
     if (get_value(port_attr, "send_sci", alpha_boolean))
     {
@@ -1212,42 +1224,74 @@ bool MACsecOrch::updateMACsecPort(MACsecPort &macsec_port, const TaskArgs &port_
             SWSS_LOG_WARN("Unknown Cipher Suite %s", cipher_suite.c_str());
             return false;
         }
+        if (!updateMACsecSCs(
+                macsec_port,
+                [&macsec_port, this](MACsecOrch::MACsecSC &macsec_sc)
+                {
+                    sai_attribute_t attr;
+                    attr.id = SAI_MACSEC_SC_ATTR_MACSEC_CIPHER_SUITE;
+                    attr.value.s32 = macsec_port.m_cipher_suite;
+                    return this->updateMACsecAttr(SAI_OBJECT_TYPE_MACSEC_SC, macsec_sc.m_sc_id, attr);
+                }))
+        {
+            return false;
+        }
     }
     swss::AlphaBoolean enable = false;
     if (get_value(port_attr, "enable", enable) && enable.operator bool() != macsec_port.m_enable)
     {
-        std::vector<MACsecOrch::MACsecSC *> macsec_scs;
         macsec_port.m_enable = enable.operator bool();
-        for (auto &sc : macsec_port.m_egress_scs)
+        if (!updateMACsecSCs(
+                macsec_port,
+                [&macsec_port, &recover, this](MACsecOrch::MACsecSC &macsec_sc)
+                {
+                    // Change the ACL entry action from packet action to MACsec flow
+                    if (macsec_port.m_enable)
+                    {
+                        if (!this->setMACsecFlowActive(macsec_sc.m_entry_id, macsec_sc.m_flow_id, true))
+                        {
+                            SWSS_LOG_WARN("Cannot change the ACL entry action from packet action to MACsec flow");
+                            return false;
+                        }
+                        auto entry_id = macsec_sc.m_entry_id;
+                        auto flow_id = macsec_sc.m_flow_id;
+                        recover.add_action([this, entry_id, flow_id]()
+                                           { this->setMACsecFlowActive(entry_id, flow_id, false); });
+                    }
+                    else
+                    {
+                        this->setMACsecFlowActive(macsec_sc.m_entry_id, macsec_sc.m_flow_id, false);
+                    }
+                    return true;
+                }))
         {
-            macsec_scs.push_back(&sc.second);
+            return false;
         }
-        for (auto &sc : macsec_port.m_ingress_scs)
+    }
+
+    recover.clear();
+    return true;
+}
+
+bool MACsecOrch::updateMACsecSCs(MACsecPort &macsec_port, std::function<bool(MACsecOrch::MACsecSC &)> action)
+{
+    SWSS_LOG_ENTER();
+
+    for (auto &sc : macsec_port.m_egress_scs)
+    {
+        if (!action(sc.second))
         {
-            macsec_scs.push_back(&sc.second);
+            return false;
         }
-        for (auto &macsec_sc : macsec_scs)
+    }
+    for (auto &sc : macsec_port.m_ingress_scs)
+    {
+        if (!action(sc.second))
         {
-            // Change the ACL entry action from packet action to MACsec flow
-            if (macsec_port.m_enable)
-            {
-                if (!setMACsecFlowActive(macsec_sc->m_entry_id, macsec_sc->m_flow_id, true))
-                {
-                    SWSS_LOG_WARN("Cannot change the ACL entry action from packet action to MACsec flow");
-                    return false;
-                }
-                auto entry_id = macsec_sc->m_entry_id;
-                auto flow_id = macsec_sc->m_flow_id;
-                recover.add_action([this, entry_id, flow_id]() { this->setMACsecFlowActive(entry_id, flow_id, false); });
-            }
-            else
-            {
-                setMACsecFlowActive(macsec_sc->m_entry_id, macsec_sc->m_flow_id, false);
-            }
+            return false;
         }
     }
 
-    recover.clear();
     return true;
 }
 
@@ -1721,6 +1765,42 @@ bool MACsecOrch::deleteMACsecSC(sai_object_id_t sc_id)
     return true;
 }
 
+bool MACsecOrch::updateMACsecAttr(sai_object_type_t object_type, sai_object_id_t object_id, const sai_attribute_t &attr)
+{
+    SWSS_LOG_ENTER();
+
+    sai_status_t status = SAI_STATUS_SUCCESS;
+
+    if (object_type == SAI_OBJECT_TYPE_MACSEC_PORT)
+    {
+        status = sai_macsec_api->set_macsec_port_attribute(object_id, &attr);
+    }
+    else if (object_type == SAI_OBJECT_TYPE_MACSEC_SC)
+    {
+        status = sai_macsec_api->set_macsec_sc_attribute(object_id, &attr);
+    }
+    else if (object_type == SAI_OBJECT_TYPE_MACSEC_SA)
+    {
+        status = sai_macsec_api->set_macsec_sa_attribute(object_id, &attr);
+    }
+    else
+    {
+        SWSS_LOG_ERROR("Wrong type %s", sai_serialize_object_type(object_type).c_str());
+        return false;
+    }
+
+    if (status != SAI_STATUS_SUCCESS)
+    {
+        task_process_status handle_status = handleSaiSetStatus(SAI_API_MACSEC, status);
+        if (handle_status != task_success)
+        {
+            return parseHandleSaiStatusFailure(handle_status);
+        }
+    }
+
+    return true;
+}
+
 task_process_status MACsecOrch::createMACsecSA(
     const std::string &port_sci_an,
     const TaskArgs &sa_attr,

orchagent/macsecorch.h

@@ -132,6 +132,7 @@ class MACsecOrch : public Orch
         sai_object_id_t switch_id,
         sai_macsec_direction_t direction);
     bool updateMACsecPort(MACsecPort &macsec_port, const TaskArgs & port_attr);
+    bool updateMACsecSCs(MACsecPort &macsec_port, std::function<bool(MACsecOrch::MACsecSC &)> action);
     bool deleteMACsecPort(
         const MACsecPort &macsec_port,
         const std::string &port_name,
@@ -179,6 +180,8 @@ class MACsecOrch : public Orch
         sai_macsec_direction_t direction);
     bool deleteMACsecSC(sai_object_id_t sc_id);
 
+    bool updateMACsecAttr(sai_object_type_t object_type, sai_object_id_t object_id, const sai_attribute_t &attr);
+
     /* MACsec SA */
     task_process_status createMACsecSA(
         const std::string &port_sci_an,

tests/test_macsec.py

@@ -699,6 +699,54 @@ def test_macsec_term_orch(self, dvs: conftest.DockerVirtualSwitch, testlog):
             1)
         assert(not inspector.get_macsec_port(macsec_port))
 
+    def test_macsec_attribute_change(self, dvs: conftest.DockerVirtualSwitch, testlog):
+        port_name = "Ethernet0"
+        local_mac_address = "00-15-5D-78-FF-C1"
+        peer_mac_address = "00-15-5D-78-FF-C2"
+        macsec_port_identifier = 1
+        macsec_port = "macsec_eth1"
+        sak = "0" * 32
+        auth_key = "0" * 32
+        packet_number = 1
+        ssci = 1
+        salt = "0" * 24
+
+        wpa = WPASupplicantMock(dvs)
+        inspector = MACsecInspector(dvs)
+
+        self.init_macsec(
+            wpa,
+            port_name,
+            local_mac_address,
+            macsec_port_identifier)
+        wpa.set_macsec_control(port_name, True)
+        wpa.config_macsec_port(port_name, {"enable_encrypt": False})
+        wpa.config_macsec_port(port_name, {"cipher_suite": "GCM-AES-256"})
+        self.establish_macsec(
+            wpa,
+            port_name,
+            local_mac_address,
+            peer_mac_address,
+            macsec_port_identifier,
+            0,
+            sak,
+            packet_number,
+            auth_key,
+            ssci,
+            salt)
+        macsec_info = inspector.get_macsec_port(macsec_port)
+        assert("encrypt off" in macsec_info)
+        assert("GCM-AES-256" in macsec_info)
+        self.deinit_macsec(
+            wpa,
+            inspector,
+            port_name,
+            macsec_port,
+            local_mac_address,
+            peer_mac_address,
+            macsec_port_identifier,
+            0)
+
 
 # Add Dummy always-pass test at end as workaroud
 # for issue when Flaky fail on final test it invokes module tear-down