Add management cluster upgrade CI step for openshift-dpf#81757
Conversation
CI step for rh-ecosystem-edge/openshift-dpf#237 which adds `make upgrade-management` to the openshift-dpf repo. The deploy-cluster step must deploy a cluster that is not on the latest z-stream, since the upgrade script defaults to upgrading to z+1 of the live cluster version. Assisted-By: Claude <noreply@anthropic.com>
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: omertuc The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
WalkthroughAdds a DPF hypervisor management-cluster upgrade step that connects over SSH, locates the last deployed cluster, runs ChangesDPF management upgrade
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CIJob
participant UpgradeStep
participant Hypervisor
CIJob->>UpgradeStep: execute dpf-hypervisor-upgrade-management
UpgradeStep->>Hypervisor: establish SSH connection
UpgradeStep->>Hypervisor: locate last deployed DPF directory
UpgradeStep->>Hypervisor: run make upgrade-management with KUBECONFIG
Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[REHEARSALNOTIFIER] Note: If this PR includes changes to step registry files (
|
openshift-merge-bot
Bot
added
the
rehearsals-ack
![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
🧹 Nitpick comments (2)
ci-operator/step-registry/dpf/hypervisor/upgrade-management/dpf-hypervisor-upgrade-management-commands.sh (2)
19-19: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPass SSH options as an array and quote configured values.
The unquoted
${SSH_OPTS}and${REMOTE_HOST}expansions are fragile and can split or glob arguments. Use an array such asSSH_OPTS=(...), invoke"${SSH_OPTS[@]}", and quote"root@${REMOTE_HOST}"for bothsshandscp.Also applies to: 23-23, 28-28, 43-43
🤖 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 `@ci-operator/step-registry/dpf/hypervisor/upgrade-management/dpf-hypervisor-upgrade-management-commands.sh` at line 19, Replace the scalar SSH_OPTS with an array of options and invoke it as "${SSH_OPTS[@]}". Quote the remote target as "root@${REMOTE_HOST}" in every ssh and scp invocation, including the commands at the referenced locations, to prevent word splitting and glob expansion.Source: Linters/SAST tools
4-6: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftVerify reuse before carrying forward the duplicated SSH setup.
This new step explicitly duplicates boilerplate from
sanity-existing,network-tests, anddeploy-cluster. Confirm/step-finderwas used and either reuse an existing helper or track the shared extraction as a follow-up; otherwise fixes to SSH and temporary-file handling will continue to drift across steps.As per coding guidelines: always use
/step-finderbefore creating new step-registry components.🤖 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 `@ci-operator/step-registry/dpf/hypervisor/upgrade-management/dpf-hypervisor-upgrade-management-commands.sh` around lines 4 - 6, The new step duplicates SSH and cluster-location setup without verifying whether an existing helper can be reused. Use /step-finder to locate shared step-registry utilities, reuse an appropriate helper if available, and otherwise document the shared extraction as a follow-up while preserving consistent SSH and temporary-file handling across dpf-hypervisor-sanity-existing, network-tests, deploy-cluster, and the new upgrade-management commands.Source: Coding guidelines
🤖 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
`@ci-operator/step-registry/dpf/hypervisor/upgrade-management/dpf-hypervisor-upgrade-management-commands.sh`:
- Line 16: The SSH configuration disables host authentication before remotely
fetching and sourcing content. Update SSH_OPTS to use a trusted mounted
known_hosts file with strict host-key verification, and replace the local source
of last-openshift-dpf-dir.sh with parsing of a single validated path after
securely retrieving it.
- Around line 12-14: Replace the predictable /tmp/id_rsa and
/tmp/last-openshift-dpf-dir.sh paths in the upgrade-management script with files
under a newly created mktemp -d directory. Register an EXIT trap to remove the
directory, and update all references in the credential decoding, chmod,
download, and script execution commands to use the private temporary paths.
---
Nitpick comments:
In
`@ci-operator/step-registry/dpf/hypervisor/upgrade-management/dpf-hypervisor-upgrade-management-commands.sh`:
- Line 19: Replace the scalar SSH_OPTS with an array of options and invoke it as
"${SSH_OPTS[@]}". Quote the remote target as "root@${REMOTE_HOST}" in every ssh
and scp invocation, including the commands at the referenced locations, to
prevent word splitting and glob expansion.
- Around line 4-6: The new step duplicates SSH and cluster-location setup
without verifying whether an existing helper can be reused. Use /step-finder to
locate shared step-registry utilities, reuse an appropriate helper if available,
and otherwise document the shared extraction as a follow-up while preserving
consistent SSH and temporary-file handling across
dpf-hypervisor-sanity-existing, network-tests, deploy-cluster, and the new
upgrade-management commands.
🪄 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: 5584fa09-9783-49c8-bc01-81e712295d31
⛔ Files ignored due to path filters (1)
ci-operator/jobs/rh-ecosystem-edge/openshift-dpf/rh-ecosystem-edge-openshift-dpf-main-presubmits.yamlis excluded by!ci-operator/jobs/**
📒 Files selected for processing (5)
ci-operator/config/rh-ecosystem-edge/openshift-dpf/rh-ecosystem-edge-openshift-dpf-main.yamlci-operator/step-registry/dpf/hypervisor/upgrade-management/OWNERSci-operator/step-registry/dpf/hypervisor/upgrade-management/dpf-hypervisor-upgrade-management-commands.shci-operator/step-registry/dpf/hypervisor/upgrade-management/dpf-hypervisor-upgrade-management-ref.metadata.jsonci-operator/step-registry/dpf/hypervisor/upgrade-management/dpf-hypervisor-upgrade-management-ref.yaml
| cat /var/run/dpf-ci/private-key | base64 -d >/tmp/id_rsa | ||
| echo "" >>/tmp/id_rsa | ||
| chmod 600 /tmp/id_rsa |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Use a private temporary directory for credentials and downloaded scripts.
The predictable /tmp/id_rsa and /tmp/last-openshift-dpf-dir.sh paths can be pre-created as symlinks by another process in the pod, redirecting key writes or the downloaded script. Create a mktemp -d directory, use it for both files, and remove it with an EXIT trap.
Also applies to: 28-37
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 12-12: Writing to or reading from a hardcoded, predictable path under /tmp is vulnerable to symlink and TOCTOU attacks: a local attacker can pre-create the file (or a symlink pointing elsewhere) and hijack or corrupt the contents. Generate a unique, unpredictable temporary file with mktemp instead, e.g. tmpfile="$(mktemp)" (or mktemp -d for directories) and reference "$tmpfile".
Context: /tmp/id_rsa
Note: [CWE-377] Insecure Temporary File.
(predictable-tmp-file-bash)
[warning] 13-13: Writing to or reading from a hardcoded, predictable path under /tmp is vulnerable to symlink and TOCTOU attacks: a local attacker can pre-create the file (or a symlink pointing elsewhere) and hijack or corrupt the contents. Generate a unique, unpredictable temporary file with mktemp instead, e.g. tmpfile="$(mktemp)" (or mktemp -d for directories) and reference "$tmpfile".
Context: /tmp/id_rsa
Note: [CWE-377] Insecure Temporary File.
(predictable-tmp-file-bash)
🤖 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
`@ci-operator/step-registry/dpf/hypervisor/upgrade-management/dpf-hypervisor-upgrade-management-commands.sh`
around lines 12 - 14, Replace the predictable /tmp/id_rsa and
/tmp/last-openshift-dpf-dir.sh paths in the upgrade-management script with files
under a newly created mktemp -d directory. Register an EXIT trap to remove the
directory, and update all references in the credential decoding, chmod,
download, and script execution commands to use the private temporary paths.
Source: Linters/SAST tools
| echo "" >>/tmp/id_rsa | ||
| chmod 600 /tmp/id_rsa | ||
|
|
||
| SSH_OPTS="-i /tmp/id_rsa -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null -o LogLevel=ERROR -o ConnectTimeout=30 -o ServerAliveInterval=10 -o ServerAliveCountMax=3 -o BatchMode=yes" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Pin the hypervisor host key before sourcing remote content.
StrictHostKeyChecking=no with UserKnownHostsFile=/dev/null disables server authentication. Because the script fetches and sources last-openshift-dpf-dir.sh, a wrong host or MITM can execute arbitrary code in the CI pod. Mount a trusted known_hosts/fingerprint and enable strict verification; the local source should also be removed or replaced with parsing of a single validated path.
Also applies to: 35-37
🤖 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
`@ci-operator/step-registry/dpf/hypervisor/upgrade-management/dpf-hypervisor-upgrade-management-commands.sh`
at line 16, The SSH configuration disables host authentication before remotely
fetching and sourcing content. Update SSH_OPTS to use a trusted mounted
known_hosts file with strict host-key verification, and replace the local source
of last-openshift-dpf-dir.sh with parsing of a single validated path after
securely retrieving it.
1 participant
See commit message
Related rh-ecosystem-edge/openshift-dpf#237
Summary by CodeRabbit
Adds an optional OpenShift DPF CI job that upgrades the management cluster from the previous z-stream to the next one. The new step securely connects to the hypervisor, locates the latest deployed cluster, and runs
make upgrade-managementwith configurable target version support. It is registered with required credentials, ownership, resource limits, and a two-hour timeout.