DPF don't hard-code IP address in kubeconfig - #83183
Conversation
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
WalkthroughThe change adds a DPF hypervisor cluster-access step. The step deploys a remote proxy, copies its kubeconfig, writes shared proxy configuration, and replaces the previous kubeconfig-loading step in two conformance workflows. Image-registry setup loads the shared configuration when present. ChangesDPF cluster access
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ConformanceWorkflow
participant Hypervisor
participant SharedDirectory
participant ImageRegistrySetup
ConformanceWorkflow->>Hypervisor: Run dpf-hypervisor-setup-cluster-access
Hypervisor-->>ConformanceWorkflow: Provide proxy kubeconfig
ConformanceWorkflow->>SharedDirectory: Store kubeconfig and proxy-conf.sh
ImageRegistrySetup->>SharedDirectory: Load proxy-conf.sh when present
ImageRegistrySetup->>ImageRegistrySetup: Configure the image registry
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[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 |
openshift-merge-bot
Bot
added
the
rehearsals-ack
|
[REHEARSALNOTIFIER] Note: If this PR includes changes to step registry files (
|
![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
`@ci-operator/step-registry/dpf/hypervisor/setup-cluster-access/dpf-hypervisor-setup-cluster-access-commands.sh`:
- Around line 19-21: Replace the source call following the scp of
last-openshift-dpf-dir.sh with safe parsing of only the expected
LAST_OPENSHIFT_DPF assignment. Validate the extracted value against the
permitted path format, then export LAST_OPENSHIFT_DPF for subsequent commands;
never execute or source any content from the hypervisor-fetched file.
- Around line 12-15: Update the SSH configuration in the setup-cluster-access
command flow to require host-key verification for the credentialed connection:
remove the insecure StrictHostKeyChecking=no and UserKnownHostsFile=/dev/null
options, mount or reference the trusted host-key credential, and configure SSH
to use it with StrictHostKeyChecking=yes. Apply the same verified options to the
SSH connectivity test and subsequent SSH operations.
🪄 Autofix
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: c7baf5c1-0db1-41b5-95ff-eda607beb381
📒 Files selected for processing (7)
ci-operator/config/rh-ecosystem-edge/openshift-dpf/rh-ecosystem-edge-openshift-dpf-main.yamlci-operator/config/rh-ecosystem-edge/openshift-dpf/rh-ecosystem-edge-openshift-dpf-release-4.22.yamlci-operator/step-registry/dpf/hypervisor/setup-cluster-access/OWNERSci-operator/step-registry/dpf/hypervisor/setup-cluster-access/dpf-hypervisor-setup-cluster-access-commands.shci-operator/step-registry/dpf/hypervisor/setup-cluster-access/dpf-hypervisor-setup-cluster-access-ref.metadata.jsonci-operator/step-registry/dpf/hypervisor/setup-cluster-access/dpf-hypervisor-setup-cluster-access-ref.yamlci-operator/step-registry/dpf/setup-image-registry/dpf-setup-image-registry-commands.sh
| 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" | ||
|
|
||
| echo "Testing SSH to ${REMOTE_HOST}..." | ||
| ssh ${SSH_OPTS} root@${REMOTE_HOST} echo 'SSH OK' |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Require host-key verification for the credentialed SSH connection.
StrictHostKeyChecking=no and UserKnownHostsFile=/dev/null accept any host while the step uses the mounted private key. A man-in-the-middle endpoint can alter the SSH session and the files copied from the hypervisor. Mount a trusted host key through the step credentials and use StrictHostKeyChecking=yes.
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 15-15: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 15-15: Double quote to prevent globbing and word splitting.
(SC2086)
🤖 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/setup-cluster-access/dpf-hypervisor-setup-cluster-access-commands.sh`
around lines 12 - 15, Update the SSH configuration in the setup-cluster-access
command flow to require host-key verification for the credentialed connection:
remove the insecure StrictHostKeyChecking=no and UserKnownHostsFile=/dev/null
options, mount or reference the trusted host-key credential, and configure SSH
to use it with StrictHostKeyChecking=yes. Apply the same verified options to the
SSH connectivity test and subsequent SSH operations.
| scp ${SSH_OPTS} root@${REMOTE_HOST}:${REMOTE_LAST_OPENSHIFT_DPF_DIR_LOCATION} /tmp | ||
| set -a | ||
| source /tmp/last-openshift-dpf-dir.sh |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not source shell code fetched from the hypervisor.
last-openshift-dpf-dir.sh is copied from the remote host and then executed in the CI container. A compromised hypervisor can run commands in the CI container and access the mounted private key or ${SHARED_DIR}. Parse and validate only the expected LAST_OPENSHIFT_DPF value instead of sourcing the file.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 20-20: 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/last-openshift-dpf-dir.sh
Note: [CWE-377] Insecure Temporary File.
(predictable-tmp-file-bash)
🪛 Shellcheck (0.11.0)
[info] 19-19: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 19-19: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 19-19: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 21-21: Not following: /tmp/last-openshift-dpf-dir.sh was not specified as input (see shellcheck -x).
(SC1091)
🤖 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/setup-cluster-access/dpf-hypervisor-setup-cluster-access-commands.sh`
around lines 19 - 21, Replace the source call following the scp of
last-openshift-dpf-dir.sh with safe parsing of only the expected
LAST_OPENSHIFT_DPF assignment. Validate the extracted value against the
permitted path format, then export LAST_OPENSHIFT_DPF for subsequent commands;
never execute or source any content from the hypervisor-fetched file.
The existing kubeconfig step hard-codes the hypervisor IP address in the kubeconfig file, which is not ideal. This change introduces a new step that sets up cluster access by deploying a proxy on the hypervisor, which should handle DNS resolution and remove the need for hard-coded IP addresses in the kubeconfig. The new step is called `dpf-hypervisor-setup-cluster-access` and replaces the previous `dpf-hypervisor-load-kubeconfig` step in the test configurations. Most of the work is done by the make deploy-proxy command introduced in rh-ecosystem-edge/openshift-dpf#287
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
@omertuc: |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
ci-operator/step-registry/dpf/hypervisor/setup-cluster-access/dpf-hypervisor-setup-cluster-access-commands.sh (2)
19-21:⚠️ Potential issue | 🟠 MajorDo not source shell code fetched from the hypervisor.
last-openshift-dpf-dir.shis copied from the remote host and then executed in the CI container. A compromised hypervisor can run commands in the CI container and access the mounted private key or${SHARED_DIR}. Parse and validate onlyLAST_OPENSHIFT_DPF, then export it for the latersshandscpcommands.
12-15:⚠️ Potential issue | 🟠 MajorRequire host-key verification for the credentialed SSH connection.
StrictHostKeyChecking=noandUserKnownHostsFile=/dev/nullstill accept any server identity. This removes host authentication for everysshandscpcall that reusesSSH_OPTS. Configure a trusted known-hosts file and useStrictHostKeyChecking=yes.🤖 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/setup-cluster-access/dpf-hypervisor-setup-cluster-access-commands.sh` around lines 12 - 15, Update the SSH_OPTS definition used by the credentialed SSH and scp calls to reference a trusted known-hosts file and set StrictHostKeyChecking=yes; remove the insecure UserKnownHostsFile=/dev/null setting while preserving the remaining connection options.
🤖 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
`@ci-operator/step-registry/dpf/hypervisor/setup-cluster-access/dpf-hypervisor-setup-cluster-access-commands.sh`:
- Around line 12-15: Update the SSH_OPTS definition used by the credentialed SSH
and scp calls to reference a trusted known-hosts file and set
StrictHostKeyChecking=yes; remove the insecure UserKnownHostsFile=/dev/null
setting while preserving the remaining connection options.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 0d7de312-f3ed-472a-b64d-3672afa42631
📒 Files selected for processing (7)
ci-operator/config/rh-ecosystem-edge/openshift-dpf/rh-ecosystem-edge-openshift-dpf-main.yamlci-operator/config/rh-ecosystem-edge/openshift-dpf/rh-ecosystem-edge-openshift-dpf-release-4.22.yamlci-operator/step-registry/dpf/hypervisor/setup-cluster-access/OWNERSci-operator/step-registry/dpf/hypervisor/setup-cluster-access/dpf-hypervisor-setup-cluster-access-commands.shci-operator/step-registry/dpf/hypervisor/setup-cluster-access/dpf-hypervisor-setup-cluster-access-ref.metadata.jsonci-operator/step-registry/dpf/hypervisor/setup-cluster-access/dpf-hypervisor-setup-cluster-access-ref.yamlci-operator/step-registry/dpf/setup-image-registry/dpf-setup-image-registry-commands.sh
🚧 Files skipped from review as they are similar to previous changes (5)
- ci-operator/step-registry/dpf/hypervisor/setup-cluster-access/dpf-hypervisor-setup-cluster-access-ref.yaml
- ci-operator/step-registry/dpf/hypervisor/setup-cluster-access/dpf-hypervisor-setup-cluster-access-ref.metadata.json
- ci-operator/step-registry/dpf/hypervisor/setup-cluster-access/OWNERS
- ci-operator/config/rh-ecosystem-edge/openshift-dpf/rh-ecosystem-edge-openshift-dpf-release-4.22.yaml
- ci-operator/config/rh-ecosystem-edge/openshift-dpf/rh-ecosystem-edge-openshift-dpf-main.yaml
1 participant
The existing kubeconfig step hard-codes the hypervisor IP address in the kubeconfig file, which is not ideal. This change introduces a new step that sets up cluster access by deploying a proxy on the hypervisor, which should handle DNS resolution and remove the need for hard-coded IP addresses in the kubeconfig.
The new step is called
dpf-hypervisor-setup-cluster-accessand replaces the previousdpf-hypervisor-load-kubeconfigstep in the test configurations.Most of the work is done by the make deploy-proxy command introduced in rh-ecosystem-edge/openshift-dpf#287
Summary by CodeRabbit
dpf-hypervisor-setup-cluster-accessin OpenShift DPF conformance workflows.make deploy-proxy, enables DNS resolution, and removes the hard-coded hypervisor IP from the kubeconfig.