Skip ProxyJump for baremetal nodes when direct SSH works#91
Open
fonta-rh wants to merge 1 commit into
Open
Conversation
After propagating the laptop SSH key to nodes, test direct SSH from localhost. If it succeeds, write [cluster_nodes:vars] without ProxyJump. Baremetal nodes are typically on the same network as the provisioning host, making the tunnel unnecessary. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
[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
Contributor
WalkthroughThis change modifies the Ansible task file for baremetal inventory updates to test direct SSH connectivity to the first cluster node. Based on the test result, it conditionally configures ProxyJump settings and updates related debug and success messaging. ChangesConditional Direct SSH Access
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Laptop as Local Machine
participant Node as First Baremetal Node
participant Playbook as Inventory Update Task
Playbook->>Laptop: Delegate SSH test to node
Laptop->>Node: Attempt direct SSH connection
Node-->>Laptop: Connection result
Laptop-->>Playbook: Return test result
Playbook->>Playbook: Set direct_node_access fact
alt Direct access succeeded
Playbook->>Playbook: Build ansible_ssh_common_args without ProxyJump
Playbook->>Playbook: Debug/success message: direct SSH
else Direct access failed
Playbook->>Playbook: Build ansible_ssh_common_args with ProxyJump
Playbook->>Playbook: Debug/success message: via ProxyJump
end
Suggested labels: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
coderabbitai
Bot
added
the
ready-for-human-review
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
deploy/openshift-clusters/roles/common/tasks/update-baremetal-inventory.yml (2)
98-104: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSuppressed stderr hides probe failure diagnostics.
2>/dev/nulldiscards ssh's own error output, so whendirect_ssh_test.rc != 0there's no way to distinguish a timeout, refused connection, or auth failure from the registered result. Sincefailed_when: falseis already set, capturing stderr wouldn't fail the play but would aid troubleshooting.🤖 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 `@deploy/openshift-clusters/roles/common/tasks/update-baremetal-inventory.yml` around lines 98 - 104, The direct SSH probe in the update-baremetal-inventory task is suppressing useful failure diagnostics by discarding stderr. Remove the stderr redirection from the shell command so `direct_ssh_test` retains ssh’s error output, while keeping `failed_when: false` and `changed_when: false` unchanged; this will preserve troubleshooting details for connection failures, timeouts, and auth issues when inspecting `direct_ssh_test`.
111-118: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated connectivity messaging across two locations.
The "direct vs ProxyJump" phrasing is repeated (lines 111-118 and 172-176) with slightly different wording. Consider deriving both messages from a single templated variable/fact to avoid drift if one is updated later.
Also applies to: 172-176
🤖 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 `@deploy/openshift-clusters/roles/common/tasks/update-baremetal-inventory.yml` around lines 111 - 118, The connectivity status text is duplicated in the inventory update tasks, so the “direct vs ProxyJump” message can drift between the two debug blocks. Refactor the logic in update-baremetal-inventory.yml to build the message once from a shared templated fact/variable, then reuse that value in both debug statements that reference direct_node_access, provisioning_host_user, and provisioning_host_addr. Keep the phrasing centralized so both locations always render the same connectivity wording.
🤖 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 `@deploy/openshift-clusters/roles/common/tasks/update-baremetal-inventory.yml`:
- Around line 95-105: The direct SSH test in the task that uses
parsed_vm_entries[0].ip needs a guard for an empty baremetal_nodes result before
indexing the first entry. Update the when condition or add a prior check in the
update-baremetal-inventory workflow so the SSH probe only runs when
parsed_vm_entries is defined and has at least one item, and otherwise skip or
fail with a clear message. Keep the fix local to the existing direct_ssh_test
task and its surrounding conditions.
---
Nitpick comments:
In `@deploy/openshift-clusters/roles/common/tasks/update-baremetal-inventory.yml`:
- Around line 98-104: The direct SSH probe in the update-baremetal-inventory
task is suppressing useful failure diagnostics by discarding stderr. Remove the
stderr redirection from the shell command so `direct_ssh_test` retains ssh’s
error output, while keeping `failed_when: false` and `changed_when: false`
unchanged; this will preserve troubleshooting details for connection failures,
timeouts, and auth issues when inspecting `direct_ssh_test`.
- Around line 111-118: The connectivity status text is duplicated in the
inventory update tasks, so the “direct vs ProxyJump” message can drift between
the two debug blocks. Refactor the logic in update-baremetal-inventory.yml to
build the message once from a shared templated fact/variable, then reuse that
value in both debug statements that reference direct_node_access,
provisioning_host_user, and provisioning_host_addr. Keep the phrasing
centralized so both locations always render the same connectivity wording.
🪄 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: d57f29ea-2272-488c-b5aa-8d5cac07e8dd
📒 Files selected for processing (1)
deploy/openshift-clusters/roles/common/tasks/update-baremetal-inventory.yml
Comment on lines
+95
to
+105
| - name: Test direct SSH access to first node from laptop | ||
| delegate_to: localhost | ||
| become: false | ||
| shell: | | ||
| ssh -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null \ | ||
| -o ConnectTimeout=5 -o BatchMode=yes \ | ||
| "core@{{ parsed_vm_entries[0].ip }}" "echo ok" 2>/dev/null | ||
| register: direct_ssh_test | ||
| failed_when: false | ||
| changed_when: false | ||
| when: ssh_key_valid | default(false) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard against empty parsed_vm_entries before indexing [0].
parsed_vm_entries[0].ip will raise a Jinja error if the baremetal_nodes group ends up empty, aborting the whole play with an unclear error instead of a meaningful message.
🛡️ Proposed guard
+- name: Fail if no baremetal nodes found
+ fail:
+ msg: "No entries found in parsed_vm_entries; cannot test direct SSH access."
+ when: parsed_vm_entries | length == 0
+
- name: Test direct SSH access to first node from laptop📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - name: Test direct SSH access to first node from laptop | |
| delegate_to: localhost | |
| become: false | |
| shell: | | |
| ssh -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null \ | |
| -o ConnectTimeout=5 -o BatchMode=yes \ | |
| "core@{{ parsed_vm_entries[0].ip }}" "echo ok" 2>/dev/null | |
| register: direct_ssh_test | |
| failed_when: false | |
| changed_when: false | |
| when: ssh_key_valid | default(false) | |
| - name: Fail if no baremetal nodes found | |
| fail: | |
| msg: "No entries found in parsed_vm_entries; cannot test direct SSH access." | |
| when: parsed_vm_entries | length == 0 | |
| - name: Test direct SSH access to first node from laptop | |
| delegate_to: localhost | |
| become: false | |
| shell: | | |
| ssh -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null \ | |
| -o ConnectTimeout=5 -o BatchMode=yes \ | |
| "core@{{ parsed_vm_entries[0].ip }}" "echo ok" 2>/dev/null | |
| register: direct_ssh_test | |
| failed_when: false | |
| changed_when: false | |
| when: ssh_key_valid | default(false) |
🤖 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 `@deploy/openshift-clusters/roles/common/tasks/update-baremetal-inventory.yml`
around lines 95 - 105, The direct SSH test in the task that uses
parsed_vm_entries[0].ip needs a guard for an empty baremetal_nodes result before
indexing the first entry. Update the when condition or add a prior check in the
update-baremetal-inventory workflow so the SSH probe only runs when
parsed_vm_entries is defined and has at least one item, and otherwise skip or
fail with a clear message. Keep the fix local to the existing direct_ssh_test
task and its surrounding conditions.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
[cluster_nodes:vars]withoutProxyJump— baremetal nodes are typically on the same network as the provisioning hostTest plan
ansible -m ping cluster_nodesworks directly🤖 Generated with Claude Code
Summary by CodeRabbit