OCPBUGS-97919: fix: use typed credentials key to support MAC-based fencing in ABI flow - #10477
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@fracappa: This pull request explicitly references no jira issue. 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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughFencing credential parsing now prefers hostname keys and falls back to MAC keys, while skipping entries without either. Host configuration generation, credential lookup, model serialization, and install-config generation support MAC-based credentials. ChangesFencing credential keying and host matching
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant LoadHostConfigs
participant loadFencingCredentials
participant hostConfig
participant InstallConfigBuilder
participant InstallConfig
LoadHostConfigs->>loadFencingCredentials: Load hostname or MAC keyed credentials
loadFencingCredentials-->>LoadHostConfigs: Return credential map
LoadHostConfigs->>hostConfig: Create hostname or MAC configuration
hostConfig->>hostConfig: Select hostname key or matching inventory MAC
hostConfig-->>InstallConfigBuilder: Return fencing credentials
InstallConfigBuilder->>InstallConfig: Emit Hostname or MacAddress credential
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (12 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
openshift-ci
Bot
added
the
size/L
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #10477 +/- ##
==========================================
+ Coverage 44.47% 44.52% +0.04%
==========================================
Files 423 423
Lines 73355 73437 +82
==========================================
+ Hits 32628 32701 +73
- Misses 37809 37819 +10
+ Partials 2918 2917 -1
🚀 New features to boost your workflow:
|
fonta-rh
left a comment
fonta-rh
left a comment
There was a problem hiding this comment.
Review: PR #10477 — Typed credentials key for MAC-based fencing in ABI flow
Reviewed the bug report, the fix, cross-repo compatibility (installer contract, CEO impact), and test coverage. The PR is correct — all three bugs from PR #10385 are real and properly fixed:
- Untyped string keys → Fixed by
credentialKeystruct with explicitkeyTypediscriminator. The oldmap[string]approach was the root cause —LoadHostConfigscouldn't distinguish hostname keys from MAC keys. - Bogus hostname configs from MAC keys → Fixed by skipping MAC-keyed entries in the hostname-config creation loop (line 361). Old behavior created phantom
hostConfig{hostname: "aa:bb:cc:dd:ee:01"}entries that triggered misleading warnings andmissingHostfailures. - Wrong directory for fencing file → Fixed by
fencingConfigDirfield. MAC-based configs correctly resolve to the parent directory containingfencing-credentials.yamlinstead of the per-host subdirectory.
The integration test (LoadHostConfigs with MAC-only fencing credentials) is the key addition — it covers the full pipeline that #10385 missed, and would have caught all three bugs.
Cross-repo compatibility verified: installer's yaml:"macaddress,omitempty" matches assisted-service's yaml:"macaddress". Both sides normalize MACs to lowercase. No CEO impact (ABI-only path).
One minor issue
host_config.go:284 — log.Infof("Adding fencing credentials for hostname %s", config.hostname) will log an empty hostname for MAC-based configs. Pre-fix, this line was unreachable for MAC configs (credentials returned nil). Post-fix, MAC configs correctly return credentials, making this line reachable with config.hostname == "".
Suggested fix:
if config.hostname != "" {
log.Infof("Adding fencing credentials for hostname %s", config.hostname)
} else {
log.Infof("Adding fencing credentials via MAC address match")
}Not blocking — the fix is correct as-is. This is a log clarity improvement for operators debugging fencing credential application.
e072531 to
ce5d969
Compare
|
/retest |
| if config.hostname != "" { | ||
| log.Infof("Adding fencing credentials for hostname %s", config.hostname) | ||
| } else { | ||
| log.Infof("Adding fencing credentials via MAC address match") |
There was a problem hiding this comment.
nit
| log.Infof("Adding fencing credentials via MAC address match") | |
| log.Info("Adding fencing credentials via MAC address match") |
91aec49 to
ff9d121
Compare
|
/retest-required |
andfasano
left a comment
andfasano
left a comment
There was a problem hiding this comment.
Please create an OCPBUGS jira card to properly track this issue
| // fencingConfigDir is the parent directory containing fencing-credentials.yaml. | ||
| // Set on MAC-based configs so FencingCredentials() can find the file, | ||
| // since configDir points to the per-host subdirectory. | ||
| fencingConfigDir string |
There was a problem hiding this comment.
Can you please elaborate better the requirements for this change? It's not clear its relationship with the main issue described (credentials key), and why it didn't work as per the previous implementation. The other changes look good, but this one looks a little bit strange, given that as per the above comments configDir contains the fencing-credentials.yaml
There was a problem hiding this comment.
@andfasano the requirements for this change come from introducing the possibility of keying the fencing-credentials by MAC address as a fallback when the user doesn't know any host-name at installation time yet.
Think of the directory layout, as something like this:
/etc/assisted/hostconfig/
├── fencing-credentials.yaml
├── host-0/
│ ├── mac_addresses
│ ├── role
│ └── root-device-hints.yaml
└── host-1/
├── mac_addresses
├── role
└── root-device-hints.yaml
For hostname-based configs, configDir is set to the parent dir (/hostconfig/), so FencingCredentials() can directly do filepath.Join(configDir, "fencing-credentials.yaml") and find the file.
For MAC-based configs, configDir is set to the per-host subdirectory (/hostconfig/host-X/) because that's where role and root-device-hints.yaml live. But fencing-credentials.yaml doesn't live there, it's one level up in the parent dir.
Without fencingConfigDir, when a MAC-based config tried to load fencing credentials, it would look for /hostconfig/host-0/fencing-credentials.yaml, which doesn't exist. The file is at /hostconfig/fencing-credentials.yaml.
There was a problem hiding this comment.
(wrote a long reply but lost thanks to gh, rewriting it from scratch... :/)
Thanks @fracappa I wasn't aware that github.com/openshift/installer/pull/10513 was already landed.
Previously we agreed with @fonta-rh to extend the current logic to support a host-config by hostname (limited to FC case, but preparing the ground for future improvements).
The agent-installer-controller 2-steps pattern (load & apply) was extended in a way that:
- In the first step the file system is explored looking for any host config artefact (LoadHostConfigs). They per-host config is keyed by:
- by mac - in this case it's assumed a specific folder containing a
mac_addressessfile is present, to match it. - by hostname - right now only the presence of the single
fencing-credentials.yamlis considered. To properly handle the single file definition layout, the path stored here is the parent one.
In both the cases, a new hostConfig struct was added to the list, to be processed (and filtered) later in the second step, ApplyHostConfigs.
- In the second step the outer loop is given by the list of hosts currently registered in AS, and we're looking for those
hostConfigin our local list matching the AS host (by mac or by hostname). So they are filtered in findHostConfigs.
Then every applyXXX method has its own logic to load the host config bit and apply it.
The previous patch #10385 seems to have somehow broken the first step assumption, trying to force a per-mac loading in a portion of code dedicated to handle hostnames only. Here the hostname may contain now a mac-address, which is clearly not in sync with agreed logic (and also meaning of the field).
So rather than patching the original #10385, I think it could be worth reviewing the whole approach in a way that is more in line with the original logic. I'd be happy to setup an offline discussion to explore the details, but at first glance I think that for those cases where the fencing credentials are defined by mac then it would be a lot simpler to modify the installer code and generate a small hostXXX-fencing-credentials.yaml file per host stored in the host specific folder (as for role and rdh), leaving the generic (parent-level) fencing-credentials.yaml to store just those ones defined by hostname (and probably the FencingCredentials() apply should be adapted to consider both the cases).
There was a problem hiding this comment.
Hello @andfasano
This late confusion on me, although I didn't review assisted-service#10385 I should've flagged the deviation on openshift/installer#10513, which I did review. Sorry about that.
So now I'll work with @fracappa to create a pair of PRs that moves this to the originally intended shape: fencing creds created in the installer directly in the mac-based location (host config dir).
How does this look?
/etc/assisted/hostconfig/
├── fencing-credentials.yaml ← hostname-keyed credentials only
├── host-0/
│ ├── mac_addresses
│ ├── role
│ ├── root-device-hints.yaml
│ └── fencing-credentials.yaml ← this host's MAC-keyed credential
└── host-1/
├── mac_addresses
├── role
├── root-device-hints.yaml
└── fencing-credentials.yaml ← this host's MAC-keyed credential
Each per-host file uses the same credentials array format from PR #9946, with a single entry:
credentials:
- macaddress: "aa:bb:cc:dd:ee:ff"
address: "redfish+https://192.168.111.1:8000/redfish/v1/Systems/1"
username: admin
password: password
certificateVerification: Disabled
MAC configs read from their configDir (host-N/). Hostname configs read from their configDir (parent). The configDir invariant from assisted-service#8457 holds for both.
We keep the precedence rule (from installer #10513): when a credential has both hostname and macaddress, hostname wins — it goes in the shared file only.
Now for this very PR on assisted-service, we can simplify the code to something like this:
loadFencingCredentials goes back to returning map[string]*models.FencingCredentialsParams (plain string keys). It still needs to parse the macaddress YAML field (new since #8457), but the key is just whichever identifier is present — hostname or MAC address — as a plain string. No typed key needed because hostname-keyed and MAC-keyed entries never coexist in the same file anymore. So like you said, we change FencingCredentials() to handle both cases:
func (hc hostConfig) FencingCredentials() (*models.FencingCredentialsParams, error) {
if hc.hostname == "" && len(hc.macAddresses) == 0 {
return nil, nil
}
creds, err := loadFencingCredentials(filepath.Join(hc.configDir, "fencing-credentials.yaml"))
if err != nil {
return nil, err
}
if creds == nil {
return nil, nil
}
if hc.hostname != "" {
creds, err := loadFencingCredentials(filepath.Join(hc.configDir, "fencing-credentials.yaml"))
if err != nil {
return nil, err
}
if creds == nil {
return nil, nil
}
if hc.hostname != "" {
return creds[hc.hostname], nil
}
for _, mac := range hc.macAddresses {
if c, ok := creds[mac]; ok {
return c, nil
}
}
return nil, nil
}
What do you think @fracappa @andfasano ?
There was a problem hiding this comment.
In line of principle the approach sounds fine, feel free to ping me when you'll need another round of review. Personally I'd consider a configuration bug when the FC are defined for the same host for both the hostname/mac - it seems something that could easily verified in the installer statically (remember that when you reach this point is pretty late in the chain, the user already created and distributed the ISO).
fracappa
changed the title
openshift-ci-robot
added
the
jira/invalid-bug
|
@fracappa: This pull request references Jira Issue OCPBUGS-97919, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
/jira refresh |
|
@fracappa: This pull request references Jira Issue OCPBUGS-97919, which is invalid:
Comment 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. |
|
/jira refresh |
openshift-ci-robot
added
jira/severity-important
|
@fracappa: This pull request references Jira Issue OCPBUGS-97919, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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. |
003e490 to
a38d871
Compare
|
@fracappa: This pull request references Jira Issue OCPBUGS-97919, which is valid. 3 validation(s) were run on this bug
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/agentbasedinstaller/host_config.go (1)
346-351: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMAC-keyed entries in parent-level
fencing-credentials.yamlare not filtered or tested. The code and tests both rely on the convention that the parent file only contains hostname-keyed entries, without enforcing or testing this assumption.
cmd/agentbasedinstaller/host_config.go#L346-L351: The loop createshostConfigentries withhostnameset to every key infencingCreds, including MAC address keys. If the parent file contains MAC-keyed entries, this produces semantically incorrect configs withhostnameset to a MAC string. Filter MAC-keyed entries (e.g., by tracking key origin inloadFencingCredentials) or document the convention that the parent file must only contain hostname-keyed entries.cmd/agentbasedinstaller/host_config_test.go#L781-L806: The test writesfencing-credentials.yamlto the per-host directory (hostDir), butLoadHostConfigsreads from the parent directory (tempDir). The test passes because the parent file is missing, not because MAC keys are filtered. Add a test that writes MAC-keyed entries to the parent directory and verifies no hostname-based configs are created.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/agentbasedinstaller/host_config.go` around lines 346 - 351, Filter MAC-keyed entries before the hostConfig creation loop in loadFencingCredentials/LoadHostConfigs so only hostname-keyed parent credentials produce hostname-based configs; update cmd/agentbasedinstaller/host_config.go:346-351 accordingly. In cmd/agentbasedinstaller/host_config_test.go:781-806, write MAC-keyed credentials to the parent directory and assert that no hostname-based configs are created.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cmd/agentbasedinstaller/host_config.go`:
- Around line 346-351: Filter MAC-keyed entries before the hostConfig creation
loop in loadFencingCredentials/LoadHostConfigs so only hostname-keyed parent
credentials produce hostname-based configs; update
cmd/agentbasedinstaller/host_config.go:346-351 accordingly. In
cmd/agentbasedinstaller/host_config_test.go:781-806, write MAC-keyed credentials
to the parent directory and assert that no hostname-based configs are created.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3ed6c062-e8a1-4dc3-9d5b-49f7d43e1df7
📒 Files selected for processing (2)
cmd/agentbasedinstaller/host_config.gocmd/agentbasedinstaller/host_config_test.go
|
/retest-required |
a38d871 to
60438d1
Compare
openshift-ci
Bot
added
the
api-review
8b9b8e2 to
5055be9
Compare
|
/retest-required |
1 similar comment
|
/retest-required |
|
At least for the part related to ABI, the changes look fine and in line also with the topic and changes discussed in openshift/installer#10684. Tip: not sure if you've already documented (internally) how the FC are currently managed (by hostname / by mac), in case it would be useful to have something for future troubleshooting. (just noted you'll have to fix the verify-generated-code) |
…pipeline Add mac_address field to fencing-credentials-params so MAC-keyed credentials survive the API→DB→install-config round-trip. When handleFencing() builds the install-config, it emits macAddress instead of hostname when the credential carries a MAC, letting the installer's gatherFencingCredentials() create MAC-hashed secrets. Simplify host_config.go: remove credentialKey struct and fencingConfigDir field, since the installer now places MAC-keyed credentials in per-host subdirectories where configDir already points.
26d79a1 to
f0bb093
Compare
openshift-ci
Bot
added
size/XL
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: CrystalChun, fracappa 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 |
openshift-ci
Bot
added
the
approved
|
/override ci/prow/edge-e2e-ai-operator-disconnected-capi |
|
@gamli75: Overrode contexts on behalf of gamli75: ci/prow/edge-e2e-ai-operator-disconnected-capi 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 kubernetes-sigs/prow repository. |
|
/test e2e-agent-compact-ipv4 edge-e2e-metal-assisted-5-0 edge-e2e-ai-operator-ztp |
|
/override ci/prow/edge-e2e-ai-operator-disconnected-capi |
|
@CrystalChun: Overrode contexts on behalf of CrystalChun: ci/prow/edge-e2e-ai-operator-disconnected-capi 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 kubernetes-sigs/prow repository. |
|
@fracappa: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
@fracappa: Jira Issue OCPBUGS-97919: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-97919 has been moved to the MODIFIED state. 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. |
|
Fix included in release 5.0.0-0.nightly-2026-07-31-015042 |
8 participants
MAC-only fencing credentials were broken in the ABI path because
loadFencingCredentialskeyed all entries as plain strings, causingLoadHostConfigsto createhostname-basedconfigs with MAC addresses as hostnames. IntroducecredentialKeystruct to distinguish hostname from MAC keys, skiphostname-configcreation for MAC entries, andadd
fencingConfigDirso MAC-matched configs find the credentials file in the parent directory.This is a follow-up for PR #10385
List all the issues related to this PR
What environments does this code impact?
How was this code tested?
Checklist
docs, README, etc)Reviewers Checklist
Summary by CodeRabbit
mac_addressfield in fencing credential parameters (including Swagger schema updates).