Merge ~troyanov/maas:backport-redfish-fix-3.2 into maas:3.2

[Anton Troyanov]

fix(redfish): backport redfish fixes to 3.2

fix: LP:1977864

(cherry picked from commit 2a809b6738e209e55b27c597f8182afbf8241662)

fix: LP:1977866

(cherry picked from commit c0bf73e7678b79b7de50c3bd3223c54198d0c5e0)

fix: LP:1977942

(cherry picked from commit b67298258b44c20ab0b95bb89eb53e08299c4ca8)

fix: LP:1977951

(cherry picked from commit 51eb53d51c86d1551feaf6ace317d043ee0deba5)

[MAAS Lander (maas-lander)]

UNIT TESTS
-b backport-redfish-fix-3.2 lp:~troyanov/maas/+git/maas into -b 3.2 lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: d1da01228135186e3b7b673aa9d338d7655404ac

review: Approve

[Alberto Donato (ack)]

+1, LGTM.

A few nitpicks. As this is a backport, maybe they could be addressed as a separate branch (mainly for master, optionally backported).

review: Approve

[Björn Tillenius (bjornt)]

Let's land this as it is, and do the fixes in a separate branch.

Please fix the commit message, though, so that it has all the cherry-picked hashes in it.

review: Approve

diff --git a/src/metadataserver/builtin_scripts/commissioning_scripts/bmc_config.py b/src/metadataserver/builtin_scripts/commissioning_scripts/bmc_config.py
index d75b0b5..0f1859b 100755
--- a/src/metadataserver/builtin_scripts/commissioning_scripts/bmc_config.py
+++ b/src/metadataserver/builtin_scripts/commissioning_scripts/bmc_config.py
@@ -78,6 +78,7 @@ from subprocess import (
 )
 import sys
 import time
+import traceback
 import urllib
 import urllib.request
 
@@ -888,14 +889,27 @@ class Redfish(IPMIBase):
     """Handle detection and configuration of Redfish device."""
 
     power_type = "redfish"
-    username = None
-    password = None
 
-    _bmc_ip = None
+    def __init__(
+        self,
+        username=None,
+        password=None,
+        ipmi_privilege_level="",
+        **kwargs,
+    ):
+        self.username = username
+        self.password = password
+        self._privilege_level = ipmi_privilege_level
+        self._bmc_config = {}
 
+    _bmc_ip = None
     _redfish_ip = None
     _redfish_port = None
 
+    class ConfigurationError(Exception):
+        def __init__(self, msg):
+            super().__init__(msg)
+
     def __str__(self):
         return "Redfish"
 
@@ -953,27 +967,32 @@ class Redfish(IPMIBase):
             }
         )
 
-    def _configure_network(self, iface, data):
+    def _configure_network(
+        self, iface, data, config_path="/etc/netplan/99-redfish.yaml"
+    ):
         # Host IP Assignment Type:
         #   Possible values: Unknown, Static, DHCP, AutoConfigure, HostSelected
         netplan_config = None
         assignment_type = get_smbios_value(data, "Host IP Assignment Type")
         if assignment_type in ("Unknown", "AutoConfigure", "HostSelected"):
-            raise ValueError("Unsupported Host IP Assignment Type")
+            raise self.ConfigurationError(
+                "Unsupported Host IP Assignment Type"
+            )
 
         if assignment_type == "Static":
             ipv4_addr = get_smbios_value(data, "IPv4 Address")
             ipv4_mask = get_smbios_value(data, "IPv4 Mask")
             if not all((ipv4_addr, ipv4_mask)):
-                raise ValueError("Missing Host IPv4 information")
+                raise self.ConfigurationError("Missing Host IPv4 information")
 
-            ipv4 = ipaddress.IPv4Network((ipv4_addr, ipv4_mask)).with_prefixlen
+            prefix_length = ipaddress.IPv4Network((0, ipv4_mask)).prefixlen
+            ipv4 = f"{ipv4_addr}/{prefix_length}"
             netplan_config = self._generate_netplan_config(iface, False, ipv4)
 
         if assignment_type == "DHCP":
             netplan_config = self._generate_netplan_config(iface, True)
 
-        with open("/etc/netplan/99-redfish.yaml", "w") as netplan:
+        with open(config_path, "w") as netplan:
             netplan.write(netplan_config)
 
         # TODO: How to determine if netplan was applied?
@@ -982,39 +1001,44 @@ class Redfish(IPMIBase):
     def _detect(self):
         data = self._get_smbios_data()
         if data is None:
-            raise ValueError("Missing SMBIOS data")
+            raise self.ConfigurationError("Missing SMBIOS data")
 
         vendor_id = get_smbios_value(data, "idVendor")[2:]
         product_id = get_smbios_value(data, "idProduct")[2:]
 
         if not all((vendor_id, product_id)):
-            raise ValueError("Missing idVendor and idProduct information")
+            raise self.ConfigurationError(
+                "Missing idVendor and idProduct information"
+            )
 
         iface = None
         with open(os.environ["MAAS_RESOURCES_FILE"]) as fd:
-            iface = get_network_interface(fd.read(), vendor_id, product_id)
+            data = json.load(fd)
+            iface = get_network_interface(data, vendor_id, product_id)
             if iface is None:
-                raise ValueError("Missing Redfish Host Interface")
+                raise self.ConfigurationError("Missing Redfish Host Interface")
 
         self._configure_network(iface, data)
 
         self.add_bmc_user()
 
-        self.redfish_ip = get_smbios_value(
+        self._redfish_ip = get_smbios_value(
             data, "IPv4 Redfish Service Address"
         )
-        self.redfish_port = get_smbios_value(data, "Redfish Service Port")
+        self._redfish_port = get_smbios_value(data, "Redfish Service Port")
 
-        if not all((self.redfish_ip, self.redfish_port)):
-            raise ValueError("Missing Redfish Service information.")
+        if not all((self._redfish_ip, self._redfish_port)):
+            raise self.ConfigurationError(
+                "Missing Redfish Service information."
+            )
 
     def detected(self):
         try:
             self._detect()
             return self.get_bmc_ip() is not None
         # XXX: we should know which exceptions to expect, so we can handle them
-        except ValueError as err:
-            print(f"ERROR: {err}")
+        except self.ConfigurationError as err:
+            print(f"ERROR: {err}\n{traceback.format_exc()}")
             return False
 
     def _get_bmc_ip(self):
diff --git a/src/metadataserver/builtin_scripts/commissioning_scripts/tests/test_bmc_config.py b/src/metadataserver/builtin_scripts/commissioning_scripts/tests/test_bmc_config.py
index 2aaa3c0..63c72b0 100644
--- a/src/metadataserver/builtin_scripts/commissioning_scripts/tests/test_bmc_config.py
+++ b/src/metadataserver/builtin_scripts/commissioning_scripts/tests/test_bmc_config.py
@@ -8,6 +8,7 @@ import os
 import random
 import re
 from subprocess import CalledProcessError, DEVNULL, TimeoutExpired
+import tempfile
 import textwrap
 from unittest.mock import call, MagicMock
 import urllib
@@ -1417,6 +1418,93 @@ class TestRedfish(MAASTestCase):
 
         self.assertEqual("127.0.0.1", self.redfish.get_bmc_ip())
 
+    def test_configure_network(self):
+        data = textwrap.dedent(
+            """\
+            Device Type: USB
+            idVendor: 0x046b
+            idProduct: 0xffb0
+            Protocol ID: 04 (Redfish over IP)
+            Service UUID: a0635470-debf-0010-9c04-d85ed302b24a
+            Host IP Assignment Type: Static
+            Host IP Address Format: IPv4
+            IPv4 Address: 169.254.95.120
+            IPv4 Mask: 255.255.0.0
+            Redfish Service IP Discovery Type: Static
+            Redfish Service IP Address Format: IPv4
+            IPv4 Redfish Service Address: 169.254.95.118
+            IPv4 Redfish Service Mask: 255.255.0.0
+            Redfish Service Port: 443
+            Redfish Service Vlan: 0
+            Redfish Service Hostname:"""
+        )
+
+        expected_config = textwrap.dedent(
+            """
+            network:
+              version: 2
+              ethernets:
+                eth0:
+                    addresses: [169.254.95.120/16]
+                    dhcp4: false
+            """
+        )
+
+        with tempfile.NamedTemporaryFile(
+            prefix="redfish-netplan", mode="w+"
+        ) as netconfig:
+            mock_check_output = self.patch(bmc_config, "check_output")
+            self.redfish._configure_network("eth0", data, netconfig.name)
+            self.assertEqual(
+                yaml.safe_dump((yaml.safe_load(expected_config))),
+                netconfig.read(),
+            )
+
+            mock_check_output.assert_called_once_with(
+                ["netplan", "apply"], timeout=bmc_config.COMMAND_TIMEOUT
+            )
+
+    def test_add_bmc_user(self):
+        username = factory.make_name("username")
+        password = factory.make_name("password")
+        self.redfish = bmc_config.Redfish(username, password)
+        mock_bmc_set = self.patch(self.redfish, "_bmc_set")
+        mock_bmc_set_keys = self.patch(self.redfish, "_bmc_set_keys")
+        self.redfish._bmc_config = {
+            "User1": {},
+            "User2": {
+                "Username": "",
+                "Password": factory.make_name("password"),
+                "Enable_User": "Yes",
+                "Lan_Privilege_Limit": "Administrator",
+                "Lan_Enable_IPMI_Msgs": "Yes",
+            },
+        }
+        self.redfish._privilege_level = "OPERATOR"
+
+        self.redfish.add_bmc_user()
+
+        self.assertEqual(username, self.redfish.username)
+        self.assertEqual(password, self.redfish.password)
+        # Verify bmc_set is only called for values that have changed
+        mock_bmc_set.assert_has_calls(
+            (
+                call("User2", "Username", username),
+                call("User2", "Password", password),
+                call("User2", "Lan_Privilege_Limit", "Operator"),
+            )
+        )
+
+        mock_bmc_set_keys.assert_called_once_with(
+            "User2",
+            [
+                "Lan_Enable_Link_Auth",
+                "SOL_Payload_Access",
+                "Serial_Enable_Link_Auth",
+            ],
+            "Yes",
+        )
+
 
 class TestGetIPMILocateOutput(MAASTestCase):
     def test_get_ipmi_locate_output(self):