OCPEDGE-2606: e2e: add fencing credentials update and validation tests for dual-replica etcd#31199
OCPEDGE-2606: e2e: add fencing credentials update and validation tests for dual-replica etcd#31199fracappa 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 references OCPEDGE-2606 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. |
WalkthroughThis pull request adds infrastructure and an integration test for rotating BMC (baseboard management controller) fencing credentials during etcd cluster recovery scenarios. It introduces Redfish API helpers for password changes, utilities to locate credentials in Kubernetes secrets, and an orchestrated test that validates credential rotation, fencing tool compatibility, and cluster recovery. ChangesFencing Credential Rotation and Recovery Testing
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 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 |
|
[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/utils/apis/baremetalhost.go`:
- Around line 51-57: The FencingCredentials return builds values from
secret.Data without validating required keys, causing silent empty fields;
update the code around the return of &FencingCredentials (the secret variable
and FencingCredentials construction) to explicitly check that secret.Data
contains non-empty "address", "username", and "password" keys and return a clear
error identifying any missing/empty key(s) instead of returning empty strings;
treat "certificateVerification" as optional (or validate similarly if desired)
and ensure the function returns an error when required secret fields are absent
so callers get immediate, descriptive diagnostics.
- Line 50: The current selector uses strings.Contains(secret.Name, shortName)
which can falsely match names like "master-01" for "master-0"; change the
condition in the selection block (the if using strings.HasPrefix, secret.Name,
shortName, and fencingCredentialsPrefix) to require an exact name match (e.g.
construct expectedName := fencingCredentialsPrefix + shortName and compare
secret.Name == expectedName) or apply a strict regex with explicit boundaries
anchored to the full secret.Name so only the intended host secret is selected.
In `@test/extended/edge_topologies/utils/apis/redfish.go`:
- Around line 17-20: The code currently uses strings.TrimPrefix on address which
silently accepts non-`redfish+` inputs; change the logic to explicitly require
the "redfish+" prefix by checking strings.HasPrefix(address, "redfish+") and
returning a clear error if it's missing, then strip the prefix and parse the
remainder (replace the current use of strings.TrimPrefix and the following
url.Parse call accordingly); ensure the returned error message identifies the
input as non-`redfish+` rather than failing later during url.Parse.
🪄 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: a51572b5-235f-40f7-86f5-bbbacdeee8fe
📒 Files selected for processing (3)
test/extended/edge_topologies/tnf_recovery.gotest/extended/edge_topologies/utils/apis/baremetalhost.gotest/extended/edge_topologies/utils/apis/redfish.go
|
Scheduling required tests: Scheduling tests matching the |
…lica etcd Add a new e2e test that rotates BMC fencing credentials via the Redfish API, runs update-fencing-credentials-sh wiuth the new password, and verifies fencing still works with updated credentials. Introduces Redfish API helpers and fencing credentials discovery in the apis package.
fracappa
force-pushed
the
fca/update-fencing-credentials-test
branch
from
2c5b983 to
c32869d
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/extended/edge_topologies/utils/apis/redfish.go (1)
59-68: 💤 Low valueScheme information is lost; HTTPS is hardcoded.
ParseRedfishAddressparses bothredfish+http://andredfish+https://addresses (with appropriate default ports), but this function only receiveshostandport, then hardcodeshttps://at line 63. If aredfish+http://...address is ever used, the curl call would fail.In practice, BMC Redfish endpoints are virtually always HTTPS, so this is likely acceptable. Consider either documenting this assumption or extending
ParseRedfishAddressto also return the scheme for completeness.🤖 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/utils/apis/redfish.go` around lines 59 - 68, The function ChangeBMCPasswordViaRedfish currently hardcodes https:// when building baseURL; preserve/propagate the scheme returned by ParseRedfishAddress (or extend ParseRedfishAddress to return scheme) and use it when constructing baseURL instead of "https://"; update ChangeBMCPasswordViaRedfish (and its calls) to accept or derive scheme (so baseURL := fmt.Sprintf("%s://%s", scheme, authority)), and ensure findRedfishAccountByUsername is called with that baseURL; alternatively document the HTTPS-only assumption if you choose not to support http.
🤖 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 `@test/extended/edge_topologies/utils/apis/redfish.go`:
- Around line 59-68: The function ChangeBMCPasswordViaRedfish currently
hardcodes https:// when building baseURL; preserve/propagate the scheme returned
by ParseRedfishAddress (or extend ParseRedfishAddress to return scheme) and use
it when constructing baseURL instead of "https://"; update
ChangeBMCPasswordViaRedfish (and its calls) to accept or derive scheme (so
baseURL := fmt.Sprintf("%s://%s", scheme, authority)), and ensure
findRedfishAccountByUsername is called with that baseURL; alternatively document
the HTTPS-only assumption if you choose not to support http.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 56db3364-d4c5-48b9-8382-e2256b844b0a
📒 Files selected for processing (3)
test/extended/edge_topologies/tnf_recovery.gotest/extended/edge_topologies/utils/apis/baremetalhost.gotest/extended/edge_topologies/utils/apis/redfish.go
openshift-ci
Bot
added
the
ready-for-human-review
|
Scheduling required tests: Scheduling tests matching the |
|
/retest-required |
|
@fracappa: The following tests failed, say
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. |
2 participants
Add a new e2e test that rotates BMC fencing credentials via the Redfish API, runs update-fencing-credentials-sh wiuth the new password, and verifies fencing still works with updated credentials. Introduces Redfish API helpers and fencing credentials discovery in the apis package.
Summary by CodeRabbit