OSAC-3770: add bmcdiscovery package for Redfish system path discovery - #358
Conversation
|
@mennyaboush: This pull request references OSAC-3770 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.1.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
mennyaboush
changed the title
|
🤖 Finished Review · ✅ Success · Started 5:04 PM UTC · Completed 5:25 PM UTC Commit: |
ReviewFindingsMedium
Low
Next steps:
Previous runReviewFindingsMedium
Low
Previous run (2)ReviewFindingsLow
Previous run (3)ReviewFindingsMedium
Low
Next steps:
Previous run (4)ReviewFindingsMedium
Low
Labels: PR adds new Go package for BMC discovery in bare-metal-fulfillment-operator Next steps:
Previous run (5)ReviewFindingsMedium
Low
Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Auto-dismissed: only Prow labels gate merging
|
🤖 Review · Commit: |
|
🤖 Finished Review · ✅ Success · Started 11:27 AM UTC · Completed 11:46 AM UTC Commit: |
mennyaboush
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
go
| port := parsed.Port() | ||
|
|
||
| if port != "" && port != "443" { | ||
| return fmt.Errorf("%w: non-standard Redfish port %s (expected 443)", ErrInvalidBMCTarget, port) |
There was a problem hiding this comment.
is it really an issue?
There was a problem hiding this comment.
I validate that staff as all the other validation in this pr as we described at the design.
I understand why you think its redundant... as you said its trusted server.
to summaries the design notes in there is the requirements:
- Allowed URL schemes — https, ipmi, redfish-virtualmedia+https, idrac-virtualmedia+https, ilo5-virtualmedia+https
- Rejected targets — loopback (127.0.0.0/8, ::1), link-local (169.254.0.0/16, fe80::/10), cloud metadata (169.254.169.254)
- Rejected ports — only 443 (Redfish) and 623 (IPMI)
- No caching on failure — return actionable error, don't cache
the combination of that requirement demand the validation we can see in that package.
There was a problem hiding this comment.
I mean, an alternative port is not something suspicious, contrary to a malformed URL... unless there's something I miss
| // ValidateBMCTarget checks that a BMC IP address is safe to connect to. | ||
| // Rejects loopback, link-local, and cloud metadata addresses. | ||
| func ValidateBMCTarget(bmcIP string) error { | ||
| ip := net.ParseIP(bmcIP) |
There was a problem hiding this comment.
not sure if we need to validate that much, the data comes from a trusted source set by the cloud provider admin, it's not a user input. I agree that we need to make sure that the bmc target is compatible with metal3 though.
| limitations under the License. | ||
| */ | ||
|
|
||
| // Package bmcdiscovery extracts BMC connection information from device |
There was a problem hiding this comment.
add that this discovery is aimed at being used by metal3's BMH
52beb3d to
9690fba
Compare
|
🤖 Finished Review · ✅ Success · Started 12:31 PM UTC · Completed 12:47 PM UTC Commit: |
fullsend-ai-review
Bot
added
requires-manual-review
Add internal/bmcdiscovery/ to bare-metal-fulfillment-operator, implementing Priority 2 of the BMC address discovery strategy from the OSAC-1339 design. The package discovers BMC addresses for Metal3 BareMetalHost CRs. It classifies BMC protocols from interface names, discovers Redfish system paths via MAC-address matching using gofish, and validates BMC URL schemes against Metal3/BMO supported protocols. API: - Resolve(*BMCInfo, ...) — main entry point, takes pre-built BMCInfo - ExtractBMCInfo(interfaces, childType) — optional helper to extract BMCInfo from a device interface list, parameterized by childType so any inventory backend can use it - Discoverer interface — abstracts the Redfish client for testability Validated against a real Dell PowerEdge R450 iDRAC. Signed-off-by: Menny Aboush <maboush@redhat.com> Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: MENNY ABOUSH <maboush@maboush-thinkpadt14gen5.raanaii.csb>
9690fba to
71c85d6
Compare
|
🤖 Review · ❌ Terminated · Started 2:42 PM UTC · Ended 3:02 PM UTC Commit: |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adriengentil, mennyaboush The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| } | ||
| 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) }
| 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.
| queryErrors = append(queryErrors, fmt.Errorf("system %s: %w", system.ODataID, err)) | ||
| continue | ||
| } | ||
|
|
There was a problem hiding this comment.
[low] edge-case
MAC comparison uses strings.EqualFold (handles case) but not format normalization. Different MAC separators (colon vs dash vs period) would cause match failure. DMTF Redfish mandates colons, but some vendor BMC firmwares may deviate.
| 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.
| continue | ||
| } | ||
|
|
||
| for _, eth := range ethInterfaces { |
There was a problem hiding this comment.
[low] injection-vuln
DiscoverSystemPath returns system.ODataID from the BMC Redfish response without path validation. The threat model is weak (the operator already sends credentials to the BMC), but validating the path starts with /redfish/v1/Systems/ is cheap hardening.
| } | ||
| return &BMCInfo{ | ||
| IP: iface.IP, | ||
| Protocol: protocol, |
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.
|
|
||
| // 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] 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.
| } | ||
|
|
||
| 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.
|
|
||
| // 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.
fullsend-ai-review
Bot
removed
the
requires-manual-review
|
🤖 Finished Review · ✅ Success · Started 2:42 PM UTC · Completed 3:02 PM UTC Commit: |
Auto-dismissed: only Prow labels gate merging
36851ee
4 participants
Summary
internal/bmcdiscovery/package tobare-metal-fulfillment-operatorimplementing Priority 2 of the BMC address discovery strategy from the OSAC-1339 BCM backend designrf0→Redfish,ipmi0→IPMI,ilo0→iLO,drac0→iDRAC), validates BMC targets (loopback/link-local/port rejection), and discovers Redfish system paths via MAC-address matchinggithub.com/stmcginnis/gofishv0.24.0 as a new dependency for Redfish API interaction behind aDiscovererinterface for testabilityDesign
This is a standalone package with no dependencies on
bcmclientorinventorytypes — it accepts[]DeviceInterface(three string fields) and returns a validated BMC URL. TheDiscovererinterface abstracts the gofish Redfish client so callers can inject mocks. The package will be consumed byinternal/inventory/bcm.go'sAssignHostwhen the upstream PRs (#228, #353, #354) merge.Validation
osac-1339/reference/bmc-discovery-real-validation.md(not in this PR — workspace-local)Test plan
make lintpasses with 0 issuesmake testpasses (all existing tests unaffected)Resolve()flowGofishDiscovereragainst real Dell iDRAC with MAC matching across 6 NICs (embedded + PCIe slot)internal/inventory/bcm.goAssignHostneedsDepends on: #228 (bcmclient), #353 (inventory adapter), #354 (wiring) — this PR can merge independently but the package is consumed after those land.
Jira: OSAC-3770