OSAC-3770: add bmcdiscovery package for Redfish system path discovery #358
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,173 @@ | ||
| /* | ||
| Copyright 2026. | ||
|
|
||
| Licensed under the Apache License, Version 2.0 (the "License"); | ||
| you may not use this file except in compliance with the License. | ||
| You may obtain a copy of the License at | ||
|
|
||
| http://www.apache.org/licenses/LICENSE-2.0 | ||
|
|
||
| Unless required by applicable law or agreed to in writing, software | ||
| distributed under the License is distributed on an "AS IS" BASIS, | ||
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| See the License for the specific language governing permissions and | ||
| limitations under the License. | ||
| */ | ||
|
|
||
| // Package bmcdiscovery discovers BMC addresses for bare-metal hosts. | ||
| // It classifies BMC protocols from interface names, discovers Redfish | ||
| // system paths via MAC-address matching, and constructs validated BMC | ||
| // URLs suitable for Metal3 BareMetalHost spec.bmc.address. | ||
| package bmcdiscovery | ||
|
|
||
| import ( | ||
| "context" | ||
| "errors" | ||
| "fmt" | ||
| "net" | ||
| "regexp" | ||
| "strings" | ||
| ) | ||
|
|
||
| var ( | ||
| ErrNoBMCInterface = errors.New("no matching BMC interface found in device interfaces") | ||
| ErrUnsupportedBMCType = errors.New("unsupported BMC protocol type") | ||
| ErrNoMACMatch = errors.New("no Redfish system found matching boot MAC address") | ||
| ErrInvalidBMCTarget = errors.New("invalid BMC target") | ||
| ) | ||
|
|
||
| // Protocol represents a BMC protocol type classified from a device | ||
| // interface name. | ||
| type Protocol string | ||
|
|
||
| const ( | ||
| ProtocolRedfish Protocol = "redfish" | ||
| ProtocolIPMI Protocol = "ipmi" | ||
| ProtocolILO Protocol = "ilo" | ||
| ProtocolDRAC Protocol = "drac" | ||
| ) | ||
|
|
||
| // DeviceInterface represents a network interface entry from a device's | ||
| // interface list. Only the fields needed for BMC discovery are included. | ||
| type DeviceInterface struct { | ||
|
There was a problem hiding this comment. [low] naming-trajectory DeviceInterface defines own fields rather than reusing bcmclient types. Intentional per PR design, but integration wiring (PR #354) will need a mapping layer. |
||
| // ChildType is the interface type identifier used to distinguish | ||
| // BMC interfaces from other interface types (e.g. data network). | ||
| ChildType string | ||
| // Name encodes the BMC protocol (e.g. "rf0" for Redfish, "ipmi0" | ||
| // for IPMI, "ilo0" for iLO, "drac0" for iDRAC). | ||
| Name string | ||
| // IP is the BMC management interface IP address. | ||
| IP string | ||
| } | ||
|
|
||
| // BMCInfo holds the BMC connection information needed for address | ||
| // construction and Redfish discovery. | ||
| type BMCInfo struct { | ||
| IP string | ||
| Protocol Protocol | ||
| } | ||
|
|
||
| var bmcTypePatterns = map[Protocol]*regexp.Regexp{ | ||
| ProtocolRedfish: regexp.MustCompile(`^rf\d+$`), | ||
| ProtocolIPMI: regexp.MustCompile(`^ipmi\d+$`), | ||
| ProtocolILO: regexp.MustCompile(`^ilo\d+$`), | ||
| ProtocolDRAC: regexp.MustCompile(`^drac\d+$`), | ||
| } | ||
|
|
||
| var redfishCompatiblePrefixes = map[Protocol]string{ | ||
| ProtocolRedfish: "redfish-virtualmedia", | ||
| ProtocolDRAC: "idrac-virtualmedia", | ||
| ProtocolILO: "ilo5-virtualmedia", | ||
| } | ||
|
|
||
| func classifyProtocol(interfaceName string) (Protocol, error) { | ||
| name := strings.ToLower(interfaceName) | ||
| for protocol, pattern := range bmcTypePatterns { | ||
| if pattern.MatchString(name) { | ||
| return protocol, nil | ||
| } | ||
| } | ||
| return "", fmt.Errorf("%w: %q", ErrUnsupportedBMCType, interfaceName) | ||
| } | ||
|
|
||
| func isRedfishCompatible(p Protocol) bool { | ||
|
There was a problem hiding this comment. [low] documentation-comments Doc comments on unexported functions use capitalized/exported-style names (e.g. // IsRedfishCompatible for isRedfishCompatible). Go convention is that doc comments should start with the actual function name. |
||
| _, ok := redfishCompatiblePrefixes[p] | ||
| return ok | ||
| } | ||
|
|
||
|
There was a problem hiding this comment. [medium] logic-error IPv6 IPMI addresses fail validation. formatHost wraps IPv6 in brackets, but splitHostPort returns the bracketed string when no port is present. net.ParseIP rejects bracketed input, so all IPMI addresses with IPv6 BMC IPs fail validation. Suggested fix: In splitHostPort, strip surrounding brackets when net.SplitHostPort returns an error. Add a test for ValidateBMCAddress("ipmi://[2001:db8::1]"). |
||
| // ExtractBMCInfo scans a device's interfaces for the first entry whose | ||
| // ChildType matches childType, then classifies the BMC protocol from | ||
| // the interface name. | ||
| func ExtractBMCInfo(interfaces []DeviceInterface, childType string) (*BMCInfo, error) { | ||
| for _, iface := range interfaces { | ||
| if iface.ChildType != childType { | ||
| continue | ||
|
There was a problem hiding this comment. [medium] edge-case IPv6 BMC addresses produce malformed URIs. BuildStaticAddress and BuildRedfishAddress use raw IP strings without brackets. For IPv6 addresses this produces non-RFC-3986-compliant URIs. ValidateBMCTarget accepts global-unicast IPv6, so these malformed URIs can reach downstream consumers (Metal3/Ironic). Suggested fix: Detect IPv6 addresses (net.ParseIP(ip).To4() == nil) and wrap them in brackets for URI construction. |
||
| } | ||
| protocol, err := classifyProtocol(iface.Name) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return &BMCInfo{ | ||
| IP: iface.IP, | ||
| Protocol: protocol, | ||
|
There was a problem hiding this comment. [medium] edge-case ExtractBMCInfo does not validate that the matched interface has a non-empty IP address. An empty DeviceInterface.IP produces an invalid BMC URL (e.g., 'ipmi://' with no host) that passes ValidateBMCAddress's scheme-only check. The error surfaces as an opaque BMH registration failure rather than a clear validation error at construction time. Suggested fix: Validate iface.IP != "" in ExtractBMCInfo before returning, e.g.: if iface.IP == "" { return nil, fmt.Errorf("%w: interface %q has no IP address", ErrInvalidBMCTarget, iface.Name) } There was a problem hiding this comment. [low] injection-vuln BMCInfo.IP is populated from DeviceInterface.IP without IP address validation. formatHost uses net.ParseIP only for IPv6 bracket wrapping; parse failure falls through to raw string interpolation. See also: [edge-case] finding at this location. |
||
| }, nil | ||
|
There was a problem hiding this comment. [medium] nil-deref Resolve panics with nil discoverer on Redfish-compatible protocols. IPMI path is safe (returns before reaching discoverer call), but Redfish/iLO/iDRAC paths call discoverer.DiscoverSystemPath without nil guard. Suggested fix: Add nil check: if discoverer == nil { return "", fmt.Errorf("discoverer is required for %s protocol", info.Protocol) }. |
||
| } | ||
| return nil, ErrNoBMCInterface | ||
| } | ||
|
|
||
| func formatHost(ip string) string { | ||
| parsed := net.ParseIP(ip) | ||
| if parsed != nil && parsed.To4() == nil { | ||
| return "[" + ip + "]" | ||
|
There was a problem hiding this comment. [low] code-organization formatHost calls net.ParseIP(ip) twice on the same input. Minor inefficiency; parse once and reuse the result. |
||
| } | ||
| return ip | ||
| } | ||
|
|
||
| func buildStaticAddress(bmcIP string) string { | ||
| return fmt.Sprintf("ipmi://%s", formatHost(bmcIP)) | ||
| } | ||
|
|
||
| func buildRedfishAddress(bmcIP string, protocol Protocol, systemPath string) string { | ||
| prefix := redfishCompatiblePrefixes[protocol] | ||
| return fmt.Sprintf("%s+https://%s%s", prefix, formatHost(bmcIP), systemPath) | ||
| } | ||
|
There was a problem hiding this comment. [low] edge-case buildRedfishAddress concatenates systemPath without validating it starts with '/'. A missing leading slash produces a malformed URL. |
||
|
|
||
|
There was a problem hiding this comment. [low] input-validation buildRedfishAddress interpolates systemPath from a remote BMC's ODataID response (untrusted network input) without validating it is a legitimate Redfish system path. ValidateBMCAddress validates scheme/host/port but not the path component. Suggested fix: Consider validating that systemPath starts with /redfish/v1/Systems/ and contains only expected characters. |
||
| // Resolve constructs a validated BMC address from the given BMCInfo. | ||
| // For IPMI, it returns a static URL. For Redfish-compatible protocols | ||
| // (Redfish, iLO, iDRAC), it uses the provided Discoverer to find the | ||
| // system path via MAC-address matching. | ||
| // | ||
| // Callers that have raw device interface data can use ExtractBMCInfo | ||
| // to build the BMCInfo first. | ||
| func Resolve( | ||
| ctx context.Context, | ||
| info *BMCInfo, | ||
| bootMAC string, | ||
| username string, | ||
| password string, | ||
| discoverer Discoverer, | ||
| ) (string, error) { | ||
| if !isRedfishCompatible(info.Protocol) { | ||
|
There was a problem hiding this comment. [low] edge-case Resolve dereferences info.Protocol without a nil check. A nil *BMCInfo causes a panic. As an internal API where *BMCInfo is only constructed via ExtractBMCInfo, a nil argument would be a programming error, but a defensive nil guard is inexpensive. |
||
| address := buildStaticAddress(info.IP) | ||
| if err := ValidateBMCAddress(address); err != nil { | ||
| return "", err | ||
| } | ||
| return address, nil | ||
| } | ||
|
|
||
| if discoverer == nil { | ||
| return "", fmt.Errorf("discoverer is required for %s protocol", info.Protocol) | ||
|
There was a problem hiding this comment. [low] error-handling-idiom Nil-discoverer error uses fmt.Errorf without %w sentinel wrapping, unlike all other error paths in this package. Callers cannot use errors.Is to distinguish this failure. |
||
| } | ||
|
|
||
| systemPath, err := discoverer.DiscoverSystemPath(ctx, info.IP, bootMAC, username, password) | ||
| if err != nil { | ||
| return "", fmt.Errorf("redfish discovery failed for %s: %w", info.IP, err) | ||
| } | ||
|
|
||
| address := buildRedfishAddress(info.IP, info.Protocol, systemPath) | ||
| if err := ValidateBMCAddress(address); err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| return address, nil | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| /* | ||
| Copyright 2026. | ||
|
|
||
| Licensed under the Apache License, Version 2.0 (the "License"); | ||
| you may not use this file except in compliance with the License. | ||
| You may obtain a copy of the License at | ||
|
|
||
| http://www.apache.org/licenses/LICENSE-2.0 | ||
|
|
||
| Unless required by applicable law or agreed to in writing, software | ||
| distributed under the License is distributed on an "AS IS" BASIS, | ||
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| See the License for the specific language governing permissions and | ||
| limitations under the License. | ||
| */ | ||
|
|
||
| package bmcdiscovery | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| . "github.com/onsi/ginkgo/v2" //nolint:revive,staticcheck | ||
| . "github.com/onsi/gomega" //nolint:revive,staticcheck | ||
| ) | ||
|
|
||
| func TestBMCDiscovery(t *testing.T) { | ||
| RegisterFailHandler(Fail) | ||
| RunSpecs(t, "BMCDiscovery Suite") | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[low] api-shape
DeviceInterface struct lacks JSON tags. Sibling packages consistently use JSON tags on structs representing external system data. Whether tags are needed depends on the integration wiring in downstream PR #354.