OCPEDGE-2116: feat: support MAC-address based fencing credentials lookup#1600
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@fracappa: This pull request references OCPEDGE-2116 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 story to target the "5.0.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. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
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:
WalkthroughAdds a dynamic Kubernetes client, replaces per-node fencing secret lookups with a bulk GetFencingSecrets resolver that matches secrets by address/hostname and BMH references, updates ConfigureFencing to accept a dynamic client, wires the client through runners, updates RBAC, and adds tests for multi-node resolution. ChangesBulk Fencing Credential Resolution
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
Skipping CI for Draft Pull Request. |
fracappa
force-pushed
the
fca/support-mac-based-fencing-credentials
branch
from
cd4d1fd to
90ccef0
Compare
fracappa
force-pushed
the
fca/support-mac-based-fencing-credentials
branch
3 times, most recently
from
e043c1d to
417dce3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/tnf/pkg/tools/mac.go`:
- Around line 130-166: The loop over bmhList currently returns an error as soon
as it finds a matching BareMetalHost (consumerRef name == machineName) that has
no MACs; change the logic so that when len(macs) == 0 you continue the loop
instead of returning (i.e., replace the early return in the block checking "if
len(macs) == 0 { ... }" with continue) so other matching BMHs are checked; keep
the final return after the loop that errors when no matching BMHs with MACs were
found (preserve the existing error message).
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1e802cb5-28b6-4f07-929a-e66973b06ec5
📒 Files selected for processing (9)
bindata/tnfdeployment/clusterrole.yamlpkg/tnf/fencing/runner.gopkg/tnf/pkg/pcs/fencing.gopkg/tnf/pkg/tools/mac.gopkg/tnf/pkg/tools/mac_test.gopkg/tnf/pkg/tools/secrets.gopkg/tnf/pkg/tools/secrets_test.gopkg/tnf/setup/runner.gopkg/tnf/update-setup/runner.go
fracappa
force-pushed
the
fca/support-mac-based-fencing-credentials
branch
2 times, most recently
from
eb87e9a to
7c7bcba
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/tnf/pkg/tools/secrets_test.go (1)
90-217: ⚡ Quick winAdd a mixed mapping test (partial Machine CRs + node-annotation fallback).
Please add a case where one node resolves via Machine
status.nodeRefand another resolves only viamachine.openshift.io/machineannotation. That guards the intended “all available signals” behavior from regressing.🤖 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/tnf/pkg/tools/secrets_test.go` around lines 90 - 217, Add a new test case to the existing tests slice that verifies mixed resolution: one node should be matched to a secret via a Machine CR exposing status.nodeRef (create a Machine object whose status.nodeRef.Name equals the node name using the helper newMachine or a variant that sets status.nodeRef) and the other node should be matched only via the node annotation "machine.openshift.io/machine" (add the annotation to the Node object). Use newFencingSecret for secrets and newBMHWithConsumer to link BMHs to machines, set nodeNames to include both nodes, and set wantMap to assert each node maps to the correct secret; set wantErr=false. Ensure the test includes bmhs, machines, nodes, and secrets entries to exercise both code paths (Machine.status.nodeRef and node annotation fallback).
🤖 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/tnf/pkg/tools/secrets.go`:
- Around line 185-188: The current logic replaces machineToNode with
annotation-derived map only when buildMachineToNodeMap(ctx, dyClient) returns
empty; instead, call both buildMachineToNodeMap(ctx, dyClient) and
buildMachineToNodeMapFromAnnotations(ctx, kubeClient, nodeNames) and merge them
so any missing node entries in the Machine-CR map are filled from the
annotations. Update the code around machineToNode so that you iterate over the
annotation map and add entries only for keys not present in the Machine-CR map
(preserving Machine-CR values when present) to produce a combined mapping.
- Around line 253-255: The code currently splits annotation into parts using
strings.SplitN(annotation, "/", 2) and only checks parts length and empty name;
update the logic to also validate the annotation namespace (parts[0]) before
using the machine name so you don't accept annotations from other namespaces. In
the block around parts := strings.SplitN(annotation, "/", 2) (where you
currently check len(parts) != 2 || parts[1] == ""), add a check that parts[0]
equals the expected namespace string (e.g., "machine.openshift.io") and continue
if it does not match, ensuring any subsequent use of parts[1] (the machine name)
only occurs when the namespace is verified.
---
Nitpick comments:
In `@pkg/tnf/pkg/tools/secrets_test.go`:
- Around line 90-217: Add a new test case to the existing tests slice that
verifies mixed resolution: one node should be matched to a secret via a Machine
CR exposing status.nodeRef (create a Machine object whose status.nodeRef.Name
equals the node name using the helper newMachine or a variant that sets
status.nodeRef) and the other node should be matched only via the node
annotation "machine.openshift.io/machine" (add the annotation to the Node
object). Use newFencingSecret for secrets and newBMHWithConsumer to link BMHs to
machines, set nodeNames to include both nodes, and set wantMap to assert each
node maps to the correct secret; set wantErr=false. Ensure the test includes
bmhs, machines, nodes, and secrets entries to exercise both code paths
(Machine.status.nodeRef and node annotation fallback).
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 711be4e4-7a94-453a-9b86-a5c15ee74d6f
📒 Files selected for processing (3)
pkg/tnf/pkg/pcs/fencing.gopkg/tnf/pkg/tools/secrets.gopkg/tnf/pkg/tools/secrets_test.go
fracappa
force-pushed
the
fca/support-mac-based-fencing-credentials
branch
from
3ed6430 to
fd1dfe2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/tnf/pkg/tools/secrets.go`:
- Around line 185-190: The variable machineToNode may be nil if
buildMachineToNodeMap(ctx, dyClient) returns nil, causing a panic when assigning
entries from annotationMachineToNode; before the merge loop (which references
machineToNode and annotationMachineToNode), ensure machineToNode is initialized
(e.g., if machineToNode == nil then allocate a new map[string]string) so that
the subsequent loop that writes into machineToNode (the for loop using
machineName, nodeName) is safe.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f4544ae5-31cd-4131-b069-adf783a51b33
📒 Files selected for processing (7)
bindata/tnfdeployment/clusterrole.yamlpkg/tnf/fencing/runner.gopkg/tnf/pkg/pcs/fencing.gopkg/tnf/pkg/tools/secrets.gopkg/tnf/pkg/tools/secrets_test.gopkg/tnf/setup/runner.gopkg/tnf/update-setup/runner.go
fracappa
force-pushed
the
fca/support-mac-based-fencing-credentials
branch
3 times, most recently
from
f407e2a to
b93b47a
Compare
|
Test suggestion: cover the
A Not blocking — the pattern is standard k8s and unlikely to regress accidentally. But it documents the contract and catches future refactors that might accidentally change the error routing. |
When the installer creates fencing credentials using MAC addresses instead of hostname, the secrets are named with a SHA256 hash of the normalized MAC (e.g. fencing-credentials-11aa22bb). If both hostname and MAC address are not provided (or if provided wrongly) the system falls back to match fencing-credentials and Nodes by UUID, using Redfish.
fracappa
force-pushed
the
fca/support-mac-based-fencing-credentials
branch
from
0a60beb to
636bb69
Compare
|
/lgtm |
|
/lgtm |
|
@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. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
openshift-ci
Bot
added
the
approved
|
/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. |
4 participants
When the installer creates fencing credentials using MAC addresses instead of hostnames, the secrets are named with a SHA256 hash of the normalized MAC (e.g. fencing-credentials-{hash}).
This PR adds a multi-phase fencing secret resolution to GetFencingSecrets:
fencing-credentials-{nodeName}directlytnf.openshift.io/mac-addresses), hash each, tryfencing-credentials-{hash}by namenode.status.nodeInfo.systemUUIDThe auth job (per-node) discovers all non-loopback MAC addresses via nsenter and annotates the node, so they are available when the setup/fencing/update-setup jobs resolve secrets.
Also removes unused
machinesandbaremetalhostsRBAC rules from the TNF clusterrole and addspatchon nodes for the annotation.Summary by CodeRabbit
New Features
Chores
Tests