Repository: metal3-io/baremetal-operator
Pull request #2418 (2025-04-25 12:35:13)

🐛 Redfish: correctly handle missing system ID
Ironic supports [1] not providing a system ID when the BMC handles
exactly one system (which is a common case for standalone servers).
On the other hand, we see a high number of cases where the address is
provided as redfish://host/redfish/v1/, which currently fails with a
very confusing JSON parsing errors.

This change updates all Redfish drivers to ignore paths that are empty
or contain only /redfish/v1. Refactor the drivers to DRY.

[1] https://review.opendev.org/c/openstack/ironic/+/707208

Signed-off-by: Dmitry Tantsur <dtantsur@protonmail.com>



Patch:
diff --git a/pkg/hardwareutils/bmc/idrac_virtualmedia.go b/pkg/hardwareutils/bmc/idrac_virtualmedia.go
index 1ad56eda..2ad0a0ca 100644
--- a/pkg/hardwareutils/bmc/idrac_virtualmedia.go
+++ b/pkg/hardwareutils/bmc/idrac_virtualmedia.go
@@ -1,7 +1,6 @@
 package bmc
 
 import (
-	"fmt"
 	"net/url"
 )
 
@@ -18,18 +17,17 @@ func init() {
 
 func newRedfishiDracVirtualMediaAccessDetails(parsedURL *url.URL, disableCertificateVerification bool) (AccessDetails, error) {
 	return &redfishiDracVirtualMediaAccessDetails{
-		bmcType:                        parsedURL.Scheme,
-		host:                           parsedURL.Host,
-		path:                           parsedURL.Path,
-		disableCertificateVerification: disableCertificateVerification,
+		redfishAccessDetails{
+			bmcType:                        parsedURL.Scheme,
+			host:                           parsedURL.Host,
+			path:                           parsedURL.Path,
+			disableCertificateVerification: disableCertificateVerification,
+		},
 	}, nil
 }
 
 type redfishiDracVirtualMediaAccessDetails struct {
-	bmcType                        string
-	host                           string
-	path                           string
-	disableCertificateVerification bool
+	redfishAccessDetails
 }
 
 func (a *redfishiDracVirtualMediaAccessDetails) Type() string {
@@ -54,18 +52,7 @@ func (a *redfishiDracVirtualMediaAccessDetails) DisableCertificateVerification()
 // expected to add any other information that might be needed (such as
 // the kernel and ramdisk locations).
 func (a *redfishiDracVirtualMediaAccessDetails) DriverInfo(bmcCreds Credentials) map[string]interface{} {
-	result := map[string]interface{}{
-		"redfish_system_id": a.path,
-		"redfish_username":  bmcCreds.Username,
-		"redfish_password":  bmcCreds.Password,
-		"redfish_address":   getRedfishAddress(a.bmcType, a.host),
-	}
-
-	if a.disableCertificateVerification {
-		result["redfish_verify_ca"] = false
-	}
-
-	return result
+	return a.redfishAccessDetails.DriverInfo(bmcCreds)
 }
 
 // iDrac Virtual Media Overrides
@@ -116,8 +103,5 @@ func (a *redfishiDracVirtualMediaAccessDetails) RequiresProvisioningNetwork() bo
 }
 
 func (a *redfishiDracVirtualMediaAccessDetails) BuildBIOSSettings(firmwareConfig *FirmwareConfig) (settings []map[string]string, err error) {
-	if firmwareConfig != nil {
-		return nil, fmt.Errorf("firmware settings for %s are not supported", a.Driver())
-	}
-	return nil, nil
+	return a.redfishAccessDetails.BuildBIOSSettings(firmwareConfig)
 }
diff --git a/pkg/hardwareutils/bmc/redfish.go b/pkg/hardwareutils/bmc/redfish.go
index f303e747..bcf78cb7 100644
--- a/pkg/hardwareutils/bmc/redfish.go
+++ b/pkg/hardwareutils/bmc/redfish.go
@@ -85,10 +85,13 @@ func getRedfishAddress(bmcType, host string) string {
 // the kernel and ramdisk locations).
 func (a *redfishAccessDetails) DriverInfo(bmcCreds Credentials) map[string]interface{} {
 	result := map[string]interface{}{
-		"redfish_system_id": a.path,
-		"redfish_username":  bmcCreds.Username,
-		"redfish_password":  bmcCreds.Password,
-		"redfish_address":   getRedfishAddress(a.bmcType, a.host),
+		"redfish_username": bmcCreds.Username,
+		"redfish_password": bmcCreds.Password,
+		"redfish_address":  getRedfishAddress(a.bmcType, a.host),
+	}
+	trimmedPath := strings.Trim(a.path, "/")
+	if trimmedPath != "" && trimmedPath != "redfish/v1" {
+		result["redfish_system_id"] = a.path
 	}
 
 	if a.disableCertificateVerification {
diff --git a/pkg/hardwareutils/bmc/redfish_virtualmedia.go b/pkg/hardwareutils/bmc/redfish_virtualmedia.go
index 3e46626e..cd98edff 100644
--- a/pkg/hardwareutils/bmc/redfish_virtualmedia.go
+++ b/pkg/hardwareutils/bmc/redfish_virtualmedia.go
@@ -1,7 +1,6 @@
 package bmc
 
 import (
-	"fmt"
 	"net/url"
 )
 
@@ -13,18 +12,17 @@ func init() {
 
 func newRedfishVirtualMediaAccessDetails(parsedURL *url.URL, disableCertificateVerification bool) (AccessDetails, error) {
 	return &redfishVirtualMediaAccessDetails{
-		bmcType:                        parsedURL.Scheme,
-		host:                           parsedURL.Host,
-		path:                           parsedURL.Path,
-		disableCertificateVerification: disableCertificateVerification,
+		redfishAccessDetails{
+			bmcType:                        parsedURL.Scheme,
+			host:                           parsedURL.Host,
+			path:                           parsedURL.Path,
+			disableCertificateVerification: disableCertificateVerification,
+		},
 	}, nil
 }
 
 type redfishVirtualMediaAccessDetails struct {
-	bmcType                        string
-	host                           string
-	path                           string
-	disableCertificateVerification bool
+	redfishAccessDetails
 }
 
 func (a *redfishVirtualMediaAccessDetails) Type() string {
@@ -53,18 +51,7 @@ func (a *redfishVirtualMediaAccessDetails) DisableCertificateVerification() bool
 // expected to add any other information that might be needed (such as
 // the kernel and ramdisk locations).
 func (a *redfishVirtualMediaAccessDetails) DriverInfo(bmcCreds Credentials) map[string]interface{} {
-	result := map[string]interface{}{
-		"redfish_system_id": a.path,
-		"redfish_username":  bmcCreds.Username,
-		"redfish_password":  bmcCreds.Password,
-		"redfish_address":   getRedfishAddress(a.bmcType, a.host),
-	}
-
-	if a.disableCertificateVerification {
-		result["redfish_verify_ca"] = false
-	}
-
-	return result
+	return a.redfishAccessDetails.DriverInfo(bmcCreds)
 }
 
 func (a *redfishVirtualMediaAccessDetails) BIOSInterface() string {
@@ -108,8 +95,5 @@ func (a *redfishVirtualMediaAccessDetails) RequiresProvisioningNetwork() bool {
 }
 
 func (a *redfishVirtualMediaAccessDetails) BuildBIOSSettings(firmwareConfig *FirmwareConfig) (settings []map[string]string, err error) {
-	if firmwareConfig != nil {
-		return nil, fmt.Errorf("firmware settings for %s are not supported", a.Driver())
-	}
-	return nil, nil
+	return a.redfishAccessDetails.BuildBIOSSettings(firmwareConfig)
 }


Test patch:
diff --git a/pkg/hardwareutils/bmc/access_test.go b/pkg/hardwareutils/bmc/access_test.go
index ccfe231a..e7c3ee84 100644
--- a/pkg/hardwareutils/bmc/access_test.go
+++ b/pkg/hardwareutils/bmc/access_test.go
@@ -685,10 +685,10 @@ func TestDriverInfo(t *testing.T) {
 
 		{
 			Scenario: "Redfish",
-			input:    "redfish://192.168.122.1/foo/bar",
+			input:    "redfish://192.168.122.1/redfish/v1/foo/bar",
 			expects: map[string]interface{}{
 				"redfish_address":   "https://192.168.122.1",
-				"redfish_system_id": "/foo/bar",
+				"redfish_system_id": "/redfish/v1/foo/bar",
 				"redfish_password":  "",
 				"redfish_username":  "",
 				"redfish_verify_ca": false,
@@ -755,12 +755,45 @@ func TestDriverInfo(t *testing.T) {
 			},
 		},
 
+		{
+			Scenario: "Redfish no system ID",
+			input:    "redfish+https://192.168.122.1/",
+			expects: map[string]interface{}{
+				"redfish_address":   "https://192.168.122.1",
+				"redfish_password":  "",
+				"redfish_username":  "",
+				"redfish_verify_ca": false,
+			},
+		},
+
+		{
+			Scenario: "Redfish wrong system ID",
+			input:    "redfish+https://192.168.122.1/redfish/v1/",
+			expects: map[string]interface{}{
+				"redfish_address":   "https://192.168.122.1",
+				"redfish_password":  "",
+				"redfish_username":  "",
+				"redfish_verify_ca": false,
+			},
+		},
+
 		{
 			Scenario: "Redfish virtual media",
-			input:    "redfish-virtualmedia://192.168.122.1/foo/bar",
+			input:    "redfish-virtualmedia://192.168.122.1/redfish/v1/foo/bar",
+			expects: map[string]interface{}{
+				"redfish_address":   "https://192.168.122.1",
+				"redfish_system_id": "/redfish/v1/foo/bar",
+				"redfish_password":  "",
+				"redfish_username":  "",
+				"redfish_verify_ca": false,
+			},
+		},
+
+		{
+			Scenario: "Redfish virtual media wrong system ID",
+			input:    "redfish-virtualmedia://192.168.122.1/redfish/v1/",
 			expects: map[string]interface{}{
 				"redfish_address":   "https://192.168.122.1",
-				"redfish_system_id": "/foo/bar",
 				"redfish_password":  "",
 				"redfish_username":  "",
 				"redfish_verify_ca": false,
@@ -793,10 +826,21 @@ func TestDriverInfo(t *testing.T) {
 
 		{
 			Scenario: "idrac virtual media",
-			input:    "idrac-virtualmedia://192.168.122.1/foo/bar",
+			input:    "idrac-virtualmedia://192.168.122.1/redfish/v1/foo/bar",
+			expects: map[string]interface{}{
+				"redfish_address":   "https://192.168.122.1",
+				"redfish_system_id": "/redfish/v1/foo/bar",
+				"redfish_password":  "",
+				"redfish_username":  "",
+				"redfish_verify_ca": false,
+			},
+		},
+
+		{
+			Scenario: "idrac virtual media wrong system ID",
+			input:    "idrac-virtualmedia://192.168.122.1/redfish/v1/",
 			expects: map[string]interface{}{
 				"redfish_address":   "https://192.168.122.1",
-				"redfish_system_id": "/foo/bar",
 				"redfish_password":  "",
 				"redfish_username":  "",
 				"redfish_verify_ca": false,