NO-JIRA: e2e: extracting fencing credentials rotaion into standalone test#31340
NO-JIRA: e2e: extracting fencing credentials rotaion into standalone test#31340fracappa wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a new dual-replica fencing-credentials e2e test that rotates BMC credentials, refreshes fencing configuration, and checks Pacemaker and etcd health. It also removes the older recovery spec that covered the same credential-rotation flow. ChangesDual-replica fencing credential tests
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@test/extended/edge_topologies/tnf_fencing_credentials.go`:
- Around line 140-157: Track whether the initial BMC password change in the
fencing credentials test actually succeeded, and only attempt to restore from
newPassword when that state is true. In the DeferCleanup block in
tnf_fencing_credentials.go, use a success flag tied to the earlier password
change and gate both the changeBMCPassword(originalPassword) restore call and
the scriptPassword fallback on that flag; if the initial change failed, skip the
restore path that would re-run update-fencing-credentials.sh with the wrong
password. Reference the cleanup logic around DeferCleanup, changeBMCPassword,
and update-fencing-credentials.sh so the BMC password and fencing credentials
stay in sync.
- Around line 44-49: The node selection in the `Test...` setup is unsafe because
it indexes `nodes.Items` without confirming there are exactly two entries at the
point of use. Update the local logic around `utils.GetNodes`, `rand.Intn`,
`peerNode`, and `targetNode` to assert the returned list has exactly two nodes
before any indexing, and fail fast if not; also avoid relying on a separate
earlier fetch for this invariant.
- Line 113: The temporary BMC password generation in the tnffencing credential
flow uses k8srand.String, which is not cryptographically secure for a real
credential. Update the password creation in the TNF fencing test helper to use a
crypto/rand-based helper (for example, a new secureRandomString function) and
handle any generation error before proceeding. Remove the k8srand import if it
is no longer needed, and keep the change localized around the newPassword
assignment and the related credential setup path.
🪄 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: 385123d5-07f8-4f3c-9b0a-6ab7d9ee56f1
📒 Files selected for processing (2)
test/extended/edge_topologies/tnf_fencing_credentials.gotest/extended/edge_topologies/tnf_recovery.go
💤 Files with no reviewable changes (1)
- test/extended/edge_topologies/tnf_recovery.go
|
Scheduling required tests: Scheduling tests matching the |
fracappa
force-pushed
the
fca/fencing-credentials-rotation-test
branch
from
87866c4 to
134d234
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/extended/edge_topologies/tnf_fencing_credentials.go (1)
151-193: 🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
bmcPasswordChangedis declared but never set totrue, so the BMC password is never restored.
bmcPasswordChangedis initialized tofalseat Line 151 and is never assignedtrueafter the successful password change at Lines 192-193. Consequently the cleanup block always takes theelsebranch (Lines 161-163) and skips restoring the BMC password. After a successful rotation the BMC is left with the random test password (newPassword) permanently, while the fencing script at Line 173 rewrites credentials withscriptPassword = originalPassword(since the flag stays false) — leaving the BMC and the fencing secret out of sync. This is the exact failure mode the gating flag was meant to prevent.🛠️ Proposed fix
g.By(fmt.Sprintf("Changing BMC password on %s", bmcNode.Name)) err = changeBMCPassword(originalPassword, newPassword) o.Expect(err).ToNot(o.HaveOccurred(), "expected to change BMC password") + bmcPasswordChanged = true🤖 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 `@test/extended/edge_topologies/tnf_fencing_credentials.go` around lines 151 - 193, The cleanup gate in the BMC password rotation flow is never flipped, so the restore path is skipped. In tnf_fencing_credentials.go, update the logic around changeBMCPassword and the DeferCleanup closure so bmcPasswordChanged is set to true only after the password change succeeds, allowing the cleanup block to restore the original password and keep the fencing credentials in sync. Use the existing symbols bmcPasswordChanged, changeBMCPassword, and the DeferCleanup restoration block to place the fix without relying on line numbers.
🧹 Nitpick comments (1)
test/extended/edge_topologies/tnf_fencing_credentials.go (1)
37-38: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
defer g.GinkgoRecover()in theDescribebody is a no-op.The
Describeclosure runs during tree construction, not during spec execution, so this deferredGinkgoRecoverfires immediately after the container function returns and never guards a running spec.GinkgoRecoveris only needed inside goroutines spawned by a spec. Safe to remove.🤖 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 `@test/extended/edge_topologies/tnf_fencing_credentials.go` around lines 37 - 38, The deferred ginkgo recovery in the Describe container is ineffective because the closure runs during tree construction, not spec execution. Remove the defer ginkgo.GinkgoRecover() from the g.Describe block in the fencing credentials test, since GinkgoRecover is only needed inside goroutines spawned from actual specs.
🤖 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.
Duplicate comments:
In `@test/extended/edge_topologies/tnf_fencing_credentials.go`:
- Around line 151-193: The cleanup gate in the BMC password rotation flow is
never flipped, so the restore path is skipped. In tnf_fencing_credentials.go,
update the logic around changeBMCPassword and the DeferCleanup closure so
bmcPasswordChanged is set to true only after the password change succeeds,
allowing the cleanup block to restore the original password and keep the fencing
credentials in sync. Use the existing symbols bmcPasswordChanged,
changeBMCPassword, and the DeferCleanup restoration block to place the fix
without relying on line numbers.
---
Nitpick comments:
In `@test/extended/edge_topologies/tnf_fencing_credentials.go`:
- Around line 37-38: The deferred ginkgo recovery in the Describe container is
ineffective because the closure runs during tree construction, not spec
execution. Remove the defer ginkgo.GinkgoRecover() from the g.Describe block in
the fencing credentials test, since GinkgoRecover is only needed inside
goroutines spawned from actual specs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 10fd6f7a-ddb7-4451-9c95-2beed53891a6
📒 Files selected for processing (2)
test/extended/edge_topologies/tnf_fencing_credentials.gotest/extended/edge_topologies/tnf_recovery.go
💤 Files with no reviewable changes (1)
- test/extended/edge_topologies/tnf_recovery.go
|
Scheduling required tests: Scheduling tests matching the |
|
@coderabbitai approve this PR |
|
/verified by @fracappa |
|
✅ Action performedComments resolved and changes approved. |
openshift-ci
Bot
added
the
ready-for-human-review
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. |
| g.By(fmt.Sprintf("Changing BMC password on %s", bmcNode.Name)) | ||
| err = changeBMCPassword(originalPassword, newPassword) | ||
| o.Expect(err).ToNot(o.HaveOccurred(), "expected to change BMC password") | ||
|
|
There was a problem hiding this comment.
I think you need to add a line here
bmcPasswordChanged = true
The flag is never flipped. Consequences:
- changeBMCPassword(originalPassword, newPassword) succeeds → BMC now expects newPassword
- DeferCleanup runs → sees bmcPasswordChanged == false → skips changeBMCPassword(newPassword, originalPassword)
- Runs the script with originalPassword → pacemaker and k8s secret reset to originalPassword
- BMC expects newPassword, pacemaker sends originalPassword → fencing broken
There was a problem hiding this comment.
Absolutely, pushing the fix
openshift-ci-robot
removed
the
verified
fracappa
force-pushed
the
fca/fencing-credentials-rotation-test
branch
from
503eb46 to
261c116
Compare
|
@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. |
|
Scheduling required tests: Scheduling tests matching the |
3 participants
Summary by CodeRabbit