From efd51388536a4caff7ed7e85fcfb4a3e80e124e3 Mon Sep 17 00:00:00 2001
From: Chris Peterson <chris.peterson@canonical.com>
Date: Thu, 15 Feb 2024 15:12:20 -0800
Subject: [PATCH] network: fix typo in BondConfig to_config method

A mismatch between the key names in BondConfig's to_config method
and NetworkDev's netdev_info function was causing subiquity to
crash when creating a bond with a valid transmit hash policy and
then later trying to edit it (LP: #2051586).

The correct key name set by the to_config method should be
"transmit-hash-policy" since this later gets passed to netplan
and neither "xmit-hash-policy" nor "xmit_hash_policy" is a valid
key name in pure netplan config.
---
 examples/answers/bond.yaml                 | 12 ++++++
 subiquitycore/models/network.py            |  2 +-
 subiquitycore/models/tests/test_network.py | 46 +++++++++++++++++++---
 3 files changed, 54 insertions(+), 6 deletions(-)

diff --git a/examples/answers/bond.yaml b/examples/answers/bond.yaml
index dd085d4a..aa95a360 100644
--- a/examples/answers/bond.yaml
+++ b/examples/answers/bond.yaml
@@ -9,6 +9,18 @@ Keyboard:
   layout: us
 Network:
   actions:
+    # LP: #2051586 the UI would crash when editing a 802.3ad bond
+    - action: create-bond
+      data:
+        name: bond10
+        devices:
+          - [interface index 0]
+        mode: 802.3ad
+    - obj: [interface name bond10]
+      action: EDIT_BOND
+    - obj: [interface name bond10]
+      action: DELETE
+
     - action: create-bond
       data:
         name: bond10
diff --git a/subiquitycore/models/network.py b/subiquitycore/models/network.py
index d5ce0cb3..d49bd4de 100644
--- a/subiquitycore/models/network.py
+++ b/subiquitycore/models/network.py
@@ -234,7 +234,7 @@ class NetworkDev:
             bond = BondConfig(
                 interfaces=self.config["interfaces"],
                 mode=params["mode"],
-                xmit_hash_policy=params.get("xmit-hash-policy"),
+                xmit_hash_policy=params.get("transmit-hash-policy"),
                 lacp_rate=params.get("lacp-rate"),
             )
         vlan: Optional[VLANConfig] = None
diff --git a/subiquitycore/models/tests/test_network.py b/subiquitycore/models/tests/test_network.py
index e03333f6..6c12d9d4 100644
--- a/subiquitycore/models/tests/test_network.py
+++ b/subiquitycore/models/tests/test_network.py
@@ -15,8 +15,9 @@
 
 from unittest.mock import Mock
 
-from subiquitycore.models.network import NetworkDev
+from subiquitycore.models.network import BondConfig, BondParameters, NetworkDev
 from subiquitycore.tests import SubiTestCase
+from subiquitycore.tests.parameterized import parameterized
 
 
 class TestRouteManagement(SubiTestCase):
@@ -44,6 +45,9 @@ class TestRouteManagement(SubiTestCase):
 
 
 class TestNetworkDev(SubiTestCase):
+    def setUp(self):
+        self.model = Mock(get_all_netdevs=Mock(return_value=[]))
+
     def test_netdev_info_eth_inexistent(self):
         # LP: #2012659 - just after physically removing an Ethernet interface
         # from the system, Subiquity tries to collect information via
@@ -51,16 +55,48 @@ class TestNetworkDev(SubiTestCase):
         # reset to None when the interface got removed.
         # In other private reports, the same issue would occur with Wi-Fi
         # interfaces.
-        model = Mock(get_all_netdevs=Mock(return_value=[]))
-        nd = NetworkDev(model, "testdev0", "eth")
+        nd = NetworkDev(self.model, "testdev0", "eth")
         info = nd.netdev_info()
         self.assertFalse(info.is_connected)
 
     def test_netdev_info_wlan_inexistent(self):
         # Just like test_netdev_info_eth_inexistent but with Wi-Fi interfaces
         # which suffer the same issue.
-        model = Mock(get_all_netdevs=Mock(return_value=[]))
-        nd = NetworkDev(model, "testdev0", "wlan")
+        nd = NetworkDev(self.model, "testdev0", "wlan")
         info = nd.netdev_info()
         self.assertIsNone(info.wlan.scan_state)
         self.assertEqual(info.wlan.visible_ssids, [])
+
+    def test_netdev_info_bond_extract(self):
+        nd = NetworkDev(self.model, "testdev0", "bond")
+        bond = BondConfig(["interface"], "802.3ad", "layer3+4", "slow")
+        nd.config = bond.to_config()
+        info = nd.netdev_info()
+        self.assertEqual(info.bond, bond)
+
+
+class TestBondConfig(SubiTestCase):
+    @parameterized.expand(
+        [
+            (mode, ["interface"], mode, "transmit", "lacp")
+            for mode in BondParameters.modes
+        ]
+    )
+    def test_to_config(self, name, interfaces, mode, transmit, lacp):
+        bond = BondConfig(interfaces, mode, transmit, lacp)
+        config = bond.to_config()
+        params = config["parameters"]
+        self.assertEqual(config["interfaces"], interfaces)
+        self.assertEqual(params["mode"], mode)
+        if mode in BondParameters.supports_xmit_hash_policy:
+            self.assertIn(
+                "transmit-hash-policy", params
+            )  # redundant but helpful error message
+            self.assertEqual(params["transmit-hash-policy"], transmit)
+        else:
+            self.assertNotIn("transmit-hash-policy", params)
+        if mode in BondParameters.supports_lacp_rate:
+            self.assertIn("lacp-rate", params)  # redundant but helpful error message
+            self.assertEqual(params["lacp-rate"], lacp)
+        else:
+            self.assertNotIn("lacp-rate", params)