OCPEDGE-2788: agent: split fencing credentials placement by identification key - #10684
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
@fracappa: This pull request references OCPEDGE-2788 which is a valid 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. |
openshift-ci-robot
added
the
jira/valid-reference
|
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:
📝 WalkthroughWalkthroughChangesFencing credential placement
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@pkg/asset/agent/image/ignition.go`:
- Around line 655-661: Update the credential classification loop in the fencing
credentials processing flow to explicitly handle entries where both HostName and
MACAddress are empty. Surface these malformed credentials through the existing
error or logging mechanism instead of silently dropping them, while preserving
the current hostname and MAC-address collection behavior.
- Around line 675-689: Update the MAC-credential handling in the surrounding
ignition configuration builder to group credentials by the hostDir returned from
findHostDirForMAC before creating Storage.Files entries. Marshal one
FencingCredentialsConfig containing all credentials for each host directory,
then emit exactly one fencing-credentials.yaml path per hostDir while preserving
the existing lookup and error handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 65afbc3e-f790-4bc4-9791-9af12b736083
📒 Files selected for processing (3)
pkg/asset/agent/agentconfig/fencingcredentials_test.gopkg/asset/agent/image/ignition.gopkg/asset/agent/image/ignition_test.go
| for _, cred := range fencingCredentials.Config.Credentials { | ||
| if cred.HostName != "" { | ||
| hostnameCredentials = append(hostnameCredentials, cred) | ||
| } else if cred.MACAddress != "" { | ||
| macCredentials = append(macCredentials, cred) | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Credentials with neither HostName nor MACAddress vanish silently.
If cred.HostName == "" and cred.MACAddress == "", the credential is dropped without error or log — no file is written and nothing signals the misconfiguration to the operator.
🛡️ Proposed fix to surface malformed entries
for _, cred := range fencingCredentials.Config.Credentials {
- if cred.HostName != "" {
+ switch {
+ case cred.HostName != "":
hostnameCredentials = append(hostnameCredentials, cred)
- } else if cred.MACAddress != "" {
+ case cred.MACAddress != "":
macCredentials = append(macCredentials, cred)
+ default:
+ return errors.New("fencing credential must specify either a hostname or a MAC address")
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for _, cred := range fencingCredentials.Config.Credentials { | |
| if cred.HostName != "" { | |
| hostnameCredentials = append(hostnameCredentials, cred) | |
| } else if cred.MACAddress != "" { | |
| macCredentials = append(macCredentials, cred) | |
| } | |
| } | |
| for _, cred := range fencingCredentials.Config.Credentials { | |
| switch { | |
| case cred.HostName != "": | |
| hostnameCredentials = append(hostnameCredentials, cred) | |
| case cred.MACAddress != "": | |
| macCredentials = append(macCredentials, cred) | |
| default: | |
| return errors.New("fencing credential must specify either a hostname or a MAC address") | |
| } | |
| } |
🤖 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 `@pkg/asset/agent/image/ignition.go` around lines 655 - 661, Update the
credential classification loop in the fencing credentials processing flow to
explicitly handle entries where both HostName and MACAddress are empty. Surface
these malformed credentials through the existing error or logging mechanism
instead of silently dropping them, while preserving the current hostname and
MAC-address collection behavior.
There was a problem hiding this comment.
@fracappa the comment seems to be relevant, do you maybe already have somewhere else a validation for that (I assume so)
There was a problem hiding this comment.
@andfasano, yes. We already validate it at installation time, before even reaching asset generation.
We don't need to duplicate it in ignition.go, by the time addFencingCredentials runs, the install-config has already been validated.
fonta-rh
left a comment
fonta-rh
left a comment
There was a problem hiding this comment.
Good work on the credential split — the per-host directory placement is working correctly (verified on a live ABI TNF deployment).
One maintainability issue to address before merge:
findHostDirForMAC duplicates the host directory naming convention from HostConfigFiles() (pkg/asset/agent/image/ignition.go:694)
Both functions independently compute the same logic — host.Hostname if set, else fmt.Sprintf("host-%d", i) (ignition.go:702-705 vs agenthosts.go:343-347). If the naming convention changes in one place, fencing credentials land in a different directory than mac_addresses/role files, silently breaking fencing.
Suggested fix: extract a shared helper like hostDirName(index int, host Host) string and call it from both locations.
| } | ||
|
|
||
| addFencingCredentials(&config, fencingCredentials) | ||
| if err = addFencingCredentials(&config, fencingCredentials, agentHostsAsset); err != nil { |
There was a problem hiding this comment.
Just as a reminder, not specifically for this PR: currently this feature is not supported in the twin asset UnconfiguredIgnition{} - the one that is currently used for the OVE ISO, so probably we'll need to have it in the unconfigured ignition as well (cc @zaneb)
There was a problem hiding this comment.
This looks a lot like configuration, so for the appliance it needs to be handled in the config-iso (not the unconfigured-ignition) and for the OVE ISO it needs to be configured through the Assisted GUI.
| for _, cred := range fencingCredentials.Config.Credentials { | ||
| if cred.HostName != "" { | ||
| hostnameCredentials = append(hostnameCredentials, cred) | ||
| } else if cred.MACAddress != "" { | ||
| macCredentials = append(macCredentials, cred) | ||
| } | ||
| } |
There was a problem hiding this comment.
@fracappa the comment seems to be relevant, do you maybe already have somewhere else a validation for that (I assume so)
| // Hostname-keyed credentials go to /etc/assisted/hostconfig/fencing-credentials.yaml. | ||
| // MAC-keyed credentials (no hostname) go to /etc/assisted/hostconfig/<host-dir>/fencing-credentials.yaml, | ||
| // matched to a host by MAC address. | ||
| func addFencingCredentials(config *igntypes.Config, fencingCredentials *agentconfig.FencingCredentials, agentHosts *agentconfig.AgentHosts) error { |
There was a problem hiding this comment.
I think this part is mixing two different things.
-
I would leave the old
addFencingCredentialsas it is, and simply rename it asaddFencingCredentialsByHostname- leaving unchanged the previous responsibility to create the single top-level/etc/assisted/hostconfig/fencing-credentials.yaml. That part worked fine. -
In the ignition asset, the
addHostConfig()method is currently the location taking care of generating specific configuration files per hosts. Currently it simply inherits and processes all the config files fromagentHosts.HostConfigFiles(). TheAgentHostswas defined as an intermediate asset to abstract the current list of hosts processed (it could be either from agent-config.yaml or nodes-config.yaml).
I think it could be worth adding aFencingCredentialsHostsfield to theAgentHostsstruct and manage it in theHostConfigFilesmethod, separately after the main loop, ie:
...
for i, host := range a.Hosts {
...
}
for i, host := range a.FencingCredentialsHosts {
...
}
Clearly the Generate method should take care of populating the FencingCredentialsHosts when it makes sense. I would not touch/extend the Hosts since that field was meant to handle configs from agent-config.yaml
With this approach, I think that the FC config per host will become totally transparent for the Ignition asset, and more smoothly integrated with the current design: it will become part of the AgentHost, with the notable exception that instead of being populated by from agent-config.yaml in such case the source will be the install-config.yaml FC field.
24585b4 to
4c37224
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/asset/agent/agentconfig/agenthosts.go (1)
356-360: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate host-directory-naming logic.
The "hostname, else
host-%d" fallback is computed twice — once in the mainHostConfigFilesloop and again infindHostDirForMAC. Consider extracting a small helper (e.g.hostDirName(i int, host agent.Host) string) to avoid future divergence between the two.Also applies to: 428-441
🤖 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 `@pkg/asset/agent/agentconfig/agenthosts.go` around lines 356 - 360, The host directory naming rule is duplicated between HostConfigFiles and findHostDirForMAC. Extract a shared helper such as hostDirName that returns host.Hostname when set or the host-%d fallback otherwise, then use it in both loops to keep naming behavior consistent.
🤖 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 `@pkg/asset/agent/agentconfig/agenthosts.go`:
- Around line 356-360: The host directory naming rule is duplicated between
HostConfigFiles and findHostDirForMAC. Extract a shared helper such as
hostDirName that returns host.Hostname when set or the host-%d fallback
otherwise, then use it in both loops to keep naming behavior consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 0b69ede8-e195-42bc-b349-6c256187b24d
📒 Files selected for processing (4)
pkg/asset/agent/agentconfig/agenthosts.gopkg/asset/agent/agentconfig/agenthosts_test.gopkg/asset/agent/image/ignition.gopkg/asset/agent/image/ignition_test.go
7fc0397 to
6599dfa
Compare
|
|
||
| // FencingCredentialHost holds MAC-keyed fencing credentials matched to a host directory. | ||
| type FencingCredentialHost struct { | ||
| DirName string |
There was a problem hiding this comment.
This is not required, see below
| Hosts []agent.Host | ||
| rendezvousIP string | ||
| Hosts []agent.Host | ||
| FencingCredentialsByHost []FencingCredentialHost |
There was a problem hiding this comment.
| FencingCredentialsByHost []FencingCredentialHost | |
| FencingCredentialsHosts []FencingCredentialHost |
There was a problem hiding this comment.
As per the above comments, it looks like this field could be simply a FencingCredentialsHosts []*types.Credential
| return files, nil | ||
| } | ||
|
|
||
| func (a *AgentHosts) populateFencingCredentialHosts(installConfig *agentAsset.OptionalInstallConfig) error { |
There was a problem hiding this comment.
nit: a comment remembering that this method takes care only to store per host FC data only in case the mac-address is configured won't hurt
| // FencingCredentialHost holds MAC-keyed fencing credentials matched to a host directory. | ||
| type FencingCredentialHost struct { | ||
| DirName string | ||
| Credentials []*types.Credential |
There was a problem hiding this comment.
IIUC an host could get just a single credential, so a list here sounds unnecessary, isn't it?
| if cred.HostName != "" || cred.MACAddress == "" { | ||
| continue | ||
| } | ||
| dirName, err := a.findHostDirForMAC(cred.MACAddress) |
There was a problem hiding this comment.
At this level the only code that matters is to append an entry to a.FencingCredentialsHosts only if it was configured by mac, ie:
for _, cred := range installConfig.Config.ControlPlane.Fencing.Credentials {
// filter out hosts without a mac-address configured
if cred.HostName != "" || cred.MACAddress == "" {
continue
}
a.FencingCredentialsHosts = append(a.FencingCredentialsHost, installConfig.Config.ControlPlane.Fencing.Credentials)
}
All the rest is not required.
There was a problem hiding this comment.
Note that in general, for this asset, the concept of "dir" is not known. It only takes care of carrying around hosts list, and provide a file definition for them
| } | ||
| } | ||
|
|
||
| for _, fch := range a.FencingCredentialsByHost { |
There was a problem hiding this comment.
The goal here is to serialize the FC file in one of the eligible hosts-<N> folder. In particular the one whose mac-address(es) matches the current FC one. So it looks like a good spot to reuse findHostDirForMAC.
Anyhow, since the FencingCredentials are defined in the install-config.yaml, and the hosts in the agent-config.yaml, things can become a little bit tricky, since there's no guarantee that an abi host would exist for a given FC entry. So, I would recommend a slightly different logic for the matching:
- Iterate over
files, looking for keys matching<host folder name>/mac_addresses- Check if one the macs matches your current FC mac. If so, get
<host folder name>as your current location to store thefencing-credentials.yamlfor the FC
- Check if one the macs matches your current FC mac. If so, get
- In case of no match, a new host folder will have to be generated - just ensure to not reuse a previously defined host folder name
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.
Inline comments:
In `@pkg/asset/agent/agentconfig/agenthosts_test.go`:
- Around line 1042-1129: Add a table case to
TestHostConfigFiles_FencingCredentials where an existing host has no Interfaces,
roles, or root-device hints and an unmatched fencing credential requires a
generated directory. Assert that the credential is assigned to the next
non-colliding host-<i> directory, covering reservation of implicit host names by
nextAvailableHostDir.
In `@pkg/asset/agent/agentconfig/agenthosts.go`:
- Around line 377-403: Reserve every host-index directory name generated by the
host-processing loop, including hosts that produce no files, before allocating
directories for unmatched fencing credentials. Update the interaction between
the host loop and nextAvailableHostDir so names derived as host-<i> cannot be
reused, while preserving existing file generation and credential allocation
behavior.
- Around line 377-448: Update nextAvailableHostDir and its call in the host
configuration generation flow to accept len(a.Hosts), reserve every implicit
host-<i> name for indices below that count before scanning files, and then
select the next unused directory. In
pkg/asset/agent/agentconfig/agenthosts_test.go lines 1042-1129, add a
TestHostConfigFiles_FencingCredentials case with a host lacking Interfaces,
Role, and RootDeviceHints plus an unmatched fencing credential, asserting the
generated directory does not reuse that host’s implicit host-<i> name.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: d07aed07-aba5-498d-9420-aa7dea27c5d4
📒 Files selected for processing (2)
pkg/asset/agent/agentconfig/agenthosts.gopkg/asset/agent/agentconfig/agenthosts_test.go
525ff92 to
7f46cd5
Compare
Hostname-keyed credentials stay at the top-level /etc/assisted/hostconfig/fencing-credentials.yaml, while MAC-keyed credentials (no hostname) are placed in per-host directories matched by MAC address, e.g. host-0/fencing-credentials.yaml. This allows assisted-service to discover MAC-keyed fencing credentials alongside other host-scoped config files.
7f46cd5 to
efd2285
Compare
andfasano
left a comment
andfasano
left a comment
There was a problem hiding this comment.
@fracappa looks like there was a nice progress, and functionally speaking the code now looks aligned to our latest discussion.
I have just two non-blocking points:
- It looks like the new code in
HostConfigFiles()may be simplified - It looks like this patch could be a very nice fit for a new integration test
/approve
| cred := &a.FencingCredentialsHost[i] | ||
| dirName := findHostDirForMAC(files, cred.MACAddress) | ||
| if dirName == "" { | ||
| dirName = nextAvailableHostDir(files, len(a.Hosts)) |
There was a problem hiding this comment.
non-blocking comment: I think there could be some space for further simplification here - unless I missed something (even though the current code seems to work), by applying the following approach:
- Initalize a a counter
maxNewDirsequals tolen(Hosts) - if dirName is empty, simply set the new dirName to
host-<maxNewDirs+1>and incrementmaxNewDirs, and use it for adding themac_addresses - Marshal the FC struct and add it to
filesdirectly in this loop
That approach may allow to user a simpler logic
openshift-ci
Bot
added
the
approved
3e79ce6 to
f00c23a
Compare
d8eecd2 to
5302df4
Compare
|
/verified by @fracappa |
openshift-ci-robot
added
the
verified
|
@fracappa: This PR has been marked as verified by 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. |
fonta-rh
left a comment
fonta-rh
left a comment
There was a problem hiding this comment.
My concern was addressed in a better way, so nothing more to say from my side, good work!
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andfasano, fonta-rh 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 |
|
Scheduling required tests: Scheduling tests matching the |
|
/test e2e-agent-5control-ipv4 |
dc5aa9c
into
openshift:main
5 participants
Hostname-keyed credentials stay at the top-level
/etc/assisted/hostconfig/fencing-credentials.yaml, while MAC-keyed credentials (no hostname) are placed in per-host directories matched by MAC address, e.g. host-0/fencing-credentials.yaml.
This allows assisted-service to discover MAC-keyed fencing credentials alongside other host-scoped config files.
Summary by CodeRabbit
Bug Fixes
fencing-credentials.yaml.fencing-credentials.yaml, creating newhost-<n>directories when needed.Tests